[services] Tunnelbroker - Add `init` function to the Rust notification library
AbandonedPublic
Actions

Authored by • max on Oct 10 2022, 7:15 AM.

Details

Reviewers

• jon
marcin
michal
tomek

Summary

This diff introduces the addition of the init and a global configuration struct to use in sending notifications functions to avoid duplication of providing the same parameters in the functions.
Instead of providing a certificate, fcm key for every function call we can initialize it once the service starts.

The usage changes are introduced in the next D5330.

Related Linear task: ENG-1737

Test Plan

Service successfully built.

Diff Detail

Repository

rCOMM Comm

Branch

notiflib-add-init

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Oct 10 2022, 7:15 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptOct 10 2022, 7:15 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

Harbormaster completed remote builds in B12697: Diff 17449.Oct 10 2022, 7:25 AM

• max edited the summary of this revision. (Show Details)Oct 10 2022, 7:27 AM

• max edited the test plan for this revision. (Show Details)

• max added reviewers: marcin, michal.

• max mentioned this in D5330: [services] Tunnelbroker - Update on using of `init` in Rust notifications library.

• max published this revision for review.Oct 10 2022, 7:29 AM

• max added a child revision: D5330: [services] Tunnelbroker - Update on using of `init` in Rust notifications library.

• jon added inline comments.Oct 10 2022, 1:34 PM

services/tunnelbroker/rust-lib/src/lib.rs
71 ↗	(On Diff #17449)	I would prefer `is_sandbox()` to just denote that the value should be just a bool instead of some discrete type or value.
73 ↗	(On Diff #17449)	avoid vanilla panics
services/tunnelbroker/rust-lib/src/notifications/config.rs
1 ↗	(On Diff #17449)	Even though this will likely be exposed as `notifications::Config`, I would really like to avoid super generic labels like `Config`. I think it should be `NotificationConfig`. Just so that future code can look like: use comm::notifications::NotificationConfig use comm::blob::BlobConfig use comm::tunnelbroker::TunnelbrokerConfig
7 ↗	(On Diff #17449)	NIT: new line

• max planned changes to this revision.Oct 11 2022, 12:02 PM

Changes to use derive(Default), sandbox changed to is_sandbox, removing of unwrap().

• max marked 4 inline comments as done.Oct 12 2022, 1:35 PM

• max added inline comments.

services/tunnelbroker/rust-lib/src/lib.rs
71 ↗	(On Diff #17449)	I would prefer `is_sandbox()` to just denote that the value should be just a bool instead of some discrete type or value. Makes sense to change! Done.
73 ↗	(On Diff #17449)	avoid vanilla panics I agree with this, done.
services/tunnelbroker/rust-lib/src/notifications/config.rs
1 ↗	(On Diff #17449)	Even though this will likely be exposed as `notifications::Config`, I would really like to avoid super generic labels like `Config`. I think it should be `NotificationConfig`. Just so that future code can look like: use comm::notifications::NotificationConfig use comm::blob::BlobConfig use comm::tunnelbroker::TunnelbrokerConfig I do not insist on it, but what is the reason to add the mod name as a prefix? It's exposed only using the mod name like `notifications::Config`. I see the cons are that in this case, we are repeating ourselves and the code readability getting worse.
7 ↗	(On Diff #17449)	NIT: new line Implementation of the `Default` removed in a favor of `derive(Default)` macro. But, thanks for catching it anyway!

Harbormaster completed remote builds in B12760: Diff 17528.Oct 12 2022, 1:36 PM

• jon accepted this revision.Oct 12 2022, 2:47 PM

• jon added inline comments.

services/tunnelbroker/rust-lib/src/notifications/config.rs
1 ↗	(On Diff #17449)	if we are expected to use the namespace `notifications::Config`, then I think this is okay. I agree that `notifications::NotificationConfig` isn't much better

This revision is now accepted and ready to land.Oct 12 2022, 2:47 PM

• max marked 5 inline comments as done.Oct 13 2022, 4:51 AM

• max added inline comments.

services/tunnelbroker/rust-lib/src/notifications/config.rs
1 ↗	(On Diff #17449)	if we are expected to use the namespace `notifications::Config`, then I think this is okay. Yes, this is the only way we will use it. Thanks for your comments @jon ;)

Adding @tomek as a blocking here.

• max added a reviewer: tomek.Oct 13 2022, 4:52 AM

This revision now requires review to proceed.Oct 13 2022, 4:52 AM

tomek requested changes to this revision.Oct 13 2022, 7:37 AM

tomek added inline comments.

services/tunnelbroker/rust-lib/src/lib.rs
29	I'm not a fan of init methods - it's a lot better if we had a constructor (like) method that creates and returns an instance with all the fields set. But I guess it might be more challenging when we use cxx.
66	When this method will be called?
76–80	Can we write to all these fields at once (assign to `*config`)?

This revision now requires changes to proceed.Oct 13 2022, 7:37 AM

• max planned changes to this revision.Oct 17 2022, 5:12 AM

• max mentioned this in D5641: [services] Tunnelbroker - Notifications config initialization.Nov 16 2022, 4:51 AM

This diff is abandoned in a favor of D5641 and D5642.

services/tunnelbroker/rust-lib/src/lib.rs
29	I'm not a fan of init methods - it's a lot better if we had a constructor (like) method that creates and returns an instance with all the fields set. But I guess it might be more challenging when we use cxx. I think we can omit the calling of init completely. I've pushed up a new diffs D5641 and D5642 instead of this one to use a lazy static along with the parameters from the config file. Making a constructor and passing the instance as an 'argument drilling' is not a Rust way, in some cases, we even can't do this like in a Tonic handler: we can't pass arguments to the handler because of the traits for the struct implementation. For example to use the database calls inside the Tonic handler implementation we should create a database instance as static because we can't pass the client instance into the handler. So argument drilling and singletons are not a Rust way. I've investigated how the database and cloud SDKs are written and they are using static to hold the connection pools, clients, etc.