Page MenuHomePhabricator

[services] Tunnelbroker - Updating notifications libraries with the config
ClosedPublic

Authored by max on Nov 16 2022, 3:22 AM.
Tags
None
Referenced Files
F3517444: D5642.id18470.diff
Sun, Dec 22, 5:28 PM
Unknown Object (File)
Thu, Nov 28, 12:11 AM
Unknown Object (File)
Tue, Nov 26, 5:32 AM
Unknown Object (File)
Tue, Nov 26, 5:32 AM
Unknown Object (File)
Tue, Nov 26, 5:32 AM
Unknown Object (File)
Sun, Nov 24, 11:52 AM
Unknown Object (File)
Oct 25 2024, 5:18 PM
Unknown Object (File)
Oct 25 2024, 5:18 PM

Details

Summary

This diff introduces the following changes to the internal notifications library:

  • Removing the old FFI return type (we are using it to call the Rust from C++, but now it doesn't needed as we are calling Rust from Rust) and using the Rust native Result instead with the Ok and our custom error types ApnsErrors and FcmErrors because we want to distinguish token-related errors and request the new token from the client later.
  • Using the static CONFIG parameters from the D5641 instead of passing config parameters on every notification-sending call.

Linear task: ENG-1891

Test Plan
  1. Service is successfully built.
  2. Parameters from the config file successfully passed to the functions.
  3. Returning OK or correct error type in case of the token expiration/or wrong.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

max held this revision as a draft.

Removing deprecated comments.

max edited the test plan for this revision. (Show Details)
max added reviewers: tomek, varun.
max edited the summary of this revision. (Show Details)
max edited the summary of this revision. (Show Details)
max published this revision for review.Nov 16 2022, 5:35 AM

Adding @tomek here as this diff is a follow-up from the abandoned D5329 and D5330 that are passed the first pass.

services/tunnelbroker/src/notifications/apns.rs
20–24 ↗(On Diff #18470)

Similar to https://phab.comm.dev/D5640#167758, I don't think the business logic should really be aware if it's in a sandbox.

We should just be configuring the service to use a sandbox endpoint. If you want to move this to future work, that's fine.

max added inline comments.
services/tunnelbroker/src/notifications/apns.rs
20–24 ↗(On Diff #18470)

Similar to https://phab.comm.dev/D5640#167758, I don't think the business logic should really be aware if it's in a sandbox.

We should just be configuring the service to use a sandbox endpoint. If you want to move this to future work, that's fine.

Yes, I agree that this should be removed in the future. This approach comes from the old blob and backup services.

tomek added inline comments.
services/tunnelbroker/src/notifications/apns.rs
8 ↗(On Diff #18470)

It's better to use singular name. It's not a collection of errors, just their type.

18–19 ↗(On Diff #18470)

Why is error handling changed? ? vs .expect

20 ↗(On Diff #18470)

Can we import the CONFIG so that we don't have tu use super in code?

20–24 ↗(On Diff #18470)

Do we have a task for it?

services/tunnelbroker/src/notifications/fcm.rs
7 ↗(On Diff #18470)
This revision is now accepted and ready to land.Nov 18 2022, 3:23 AM
max marked 6 inline comments as done.

Updating on comments.

services/tunnelbroker/src/notifications/apns.rs
8 ↗(On Diff #18470)

It's better to use singular name. It's not a collection of errors, just their type.

Yes, makes sense. I've changed it.

18–19 ↗(On Diff #18470)

Why is error handling changed? ? vs .expect

We are not using anyhow for errors here, that's why we can't use ?.

20 ↗(On Diff #18470)

Can we import the CONFIG so that we don't have tu use super in code?

Yes, it was changed.

20–24 ↗(On Diff #18470)

Do we have a task for it?

Yep, ENG-2296 covers this.