-
Notifications
You must be signed in to change notification settings - Fork 108
feat(email): write live email-sending config to redis #2574
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philbooth A couple things at your discretion, otherwise LGTM!
switch (command) { | ||
case 'read': | ||
case 'write': | ||
case 'revert': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a default or initial set of configs? If so, should we have the concept of reset to defaults? (Probably out of scope for this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, the default config is undefined
.
|
||
async function read () { | ||
const current = await redis.get(KEYS.current) | ||
if (current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we log that there is no config currently?
return cp.execAsync('node scripts/email-config revert', { cwd }) | ||
}) | ||
|
||
it('read does not print', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, this is expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the intention is to be UNIX/command-line friendly. So the only thing that goes to stdout
is the config, if there is any. Otherwise it risks having people accidentally pipe status messages to some other file/command.
scripts/email-config.js
Outdated
// Parse then stringify for validation | ||
const current = JSON.stringify(JSON.parse(await stdin())) | ||
const previous = await redis.get(KEYS.current) | ||
await redis.set(KEYS.current, current) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have some extra validation here before setting? Possibly to ensure that service has at least a percentage or that it is either socketlabs, ses, or sendgrid?
Fixes #2497 (I think).
Adds a script for managing the Redis-stored live email config. The script supports commands for reading and writing the config, reverting the last changes and checking the resolved state for a given email address.
The script is also used to implement integration tests for the read code, so that we can have confidence in the end-to-end process of writing then reading.
And there's tests for the script itself too of course. Those are pretty slow (because exec), so I'm not running them as part of a standard
npm t
. They're included in the travis build though and you can run them locally withnpm run test-scripts
if you need to.@mozilla/fxa-devs r?