[services] Tunnelbroker - Update on using of `init` in Rust notifications library
AbandonedPublic
Actions

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

Details

Reviewers

• jon
marcin
michal
tomek

Summary

This diff introduces changes for using the init and a global configuration struct from D5329.
Instead of providing a certificate, fcm key for every function call we can initialize it once the service starts.

The function and struct are introduced in D5329.

Related Linear task: ENG-1737

Test Plan

Service is successfully built, and notifications are successfully sent.

Diff Detail

Repository

rCOMM Comm

Branch

notiflib-update-useof-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 B12698: Diff 17450.Oct 10 2022, 7:26 AM

• max mentioned this in D5329: [services] Tunnelbroker - Add `init` function to the Rust notification library.Oct 10 2022, 7:27 AM

• max edited the summary of this revision. (Show Details)

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

• max added reviewers: marcin, michal.

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

• max added a parent revision: D5329: [services] Tunnelbroker - Add `init` function to the Rust notification library.

• jon requested changes to this revision.Oct 10 2022, 1:24 PM

• jon added inline comments.

services/tunnelbroker/rust-lib/src/lib.rs
80–91	we only need to read it once, and we should avoid vanilla panics

This revision now requires changes to proceed.Oct 10 2022, 1:24 PM

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

Fixing multiple reads.

• max marked an inline comment as done.Oct 12 2022, 2:42 PM

• max added inline comments.

services/tunnelbroker/rust-lib/src/lib.rs
80–91	we only need to read it once, and we should avoid vanilla panics Makes sense, fixed. Thanks, @jon !

LGTM, feel like @tomek should take a look

• jon added a reviewer: tomek.Oct 12 2022, 2:45 PM

Harbormaster completed remote builds in B12765: Diff 17534.Oct 12 2022, 2:52 PM

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

tomek added inline comments.

services/tunnelbroker/rust-lib/src/lib.rs
79–93 ↗	(On Diff #17534)	Using global variables isn't a good idea. We should use more object-oriented approach, where we have a struct with config fields and methods that use these fields. Is it possible to have something like this?

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

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

• max marked an inline comment as done.

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

services/tunnelbroker/rust-lib/src/lib.rs
79–93 ↗	(On Diff #17534)	Using global variables isn't a good idea. We should use more object-oriented approach, where we have a struct with config fields and methods that use these fields. Is it possible to have something like this? This diff is abandoned in a favor of D5641 and D5642. The comment in D5329 is related to this question. The approach was changed to use a lazy static in D5641.