Page MenuHomePhabricator

[reports-service] Introduce emails config
ClosedPublic

Authored by bartek on Aug 28 2023, 6:27 AM.
Tags
None
Referenced Files
F3366513: D8978.id30718.diff
Mon, Nov 25, 11:48 AM
Unknown Object (File)
Sun, Oct 27, 4:46 PM
Unknown Object (File)
Sep 27 2024, 10:27 PM
Unknown Object (File)
Sep 27 2024, 10:27 PM
Unknown Object (File)
Sep 27 2024, 10:27 PM
Unknown Object (File)
Sep 27 2024, 10:26 PM
Unknown Object (File)
Sep 27 2024, 10:19 PM
Unknown Object (File)
Sep 3 2024, 12:07 PM
Subscribers

Details

Summary

We're going to use Postmark to send report emails. This diff introduces a simple config file format:

{
  "postmarkToken": "api key here",
  "senderEmail": "comm@example.com",
  "mailingGroups": {
    "inconsistencyReports": "mailgroup1@example.com",
    "mediaReports": "mailgroup2@example.com",
    "errorReports": "mailgroup3@example.com"
  }
}

The config will be loaded from CLI args or file - implemented in the next diff

Depends on D8865

Test Plan

Unit test, also tested with the next diff

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Aug 28 2023, 6:45 AM

This and the next diffs seem a bit complicated for the email configuration. I'm going to accept them because this is probably more of a preference but what do you think about:

  1. Moving the "token replacement from env", from FromStr to the parse method introduces later in the stack. I don't I have ever seen a non-pure from_str so that feels weird to me
  2. Instead of having a #[clap(ignore)] OnceCell, add a email config to the reports service (probably as an Option<>), and just fill it once during init
This revision is now accepted and ready to land.Aug 29 2023, 8:45 AM

This and the next diffs seem a bit complicated for the email configuration.

Agree, I have this feeling too

  1. Moving the "token replacement from env", from FromStr to the parse method introduces later in the stack. I don't I have ever seen a non-pure from_str so that feels weird to me

This is doable I think

  1. Instead of having a #[clap(ignore)] OnceCell, add a email config to the reports service (probably as an Option<>), and just fill it once during init

This one is more tricky when it comes to encapsulation - In current configuration, temporary values (raw CLI args etc) and parse functions are only accessible inside config modules. When I moved config init to the service, these temporary values would have to be accessible there (and therefore across the whole app) which I was trying to avoid.
Perhaps having a custom clap parser (or other kind of extension) would better fit here but I couldn't figure it out quickly and didn't want to spend more time on this. I can create a follow-up if you feel strongly

Would something like this work?

// Config files

pub struct AppConfig {
 <...>

 #[command(flatten)]
 email_args: EmailArgs // `EmailArgs` is the same as in the next diff
}

impl AppConfig {
 <...>
 
 /// Note: this can potentially read from file system
 /// so you should cache it
 pub fn load_email_config(&self) -> Option<EmailConfig> {
   // Basically the `EmailArgs::parse` method + env var handling form `FromStr`
 }

 // Remove `emails_enabled()` 
}

// Service init files

pub struct ReportsService {
  <...>

  email_config: Option<EmailConfig>,
}

// in http server init
let service = ReportsService::new(db, blob_client, cfg.load_email_config());

I think it fits the encapsulation requirements? If you like it or some parts of it you can update the diff, if not we can leave it as-is (I don't think we need to make a followup task in this case).

  • Decided to follow review suggestions and refactor the code
  • Got rid of the POSTMARK_TOKEN env var - it's not needed as it's going to be passed along with the rest of the config in terraform (for local dev it's more convenient too)
  • Regarding above, replaced option with string
This revision was automatically updated to reflect the committed changes.