Skip to content

Conversation

@marcinapostoluk
Copy link
Contributor

  • Separating out error email so that it can be specified independently via UI. This is so that successful reports can be sent to target audience while errors to notebook maintainer
  • If no error email is specified errors won't be sent to the regular email any more. The assumption here is that notebook maintainer will monitor it manually. This is a change of behavior as after an upgrade to the new version emails will no longer be sent on failure unless changes the config explicitly. If strong objections I can default it to use regular email although this disallows configuring a case where we do want regular emails sent but not the error emails.
  • Storing regular and error emails together in each notebook result - in case one wants to re-run we need to have them both as the result of the re-run can be either a success or a failure

@jonbannister
Copy link
Collaborator

If no error email is specified errors won't be sent to the regular email any more. The assumption here is that notebook maintainer will monitor it manually.

I don't think this is a safe assumption. Plenty of internal users only notice that their scheduled report is failing when they get the email (many don't check the webapp at all).

@marcinapostoluk marcinapostoluk linked an issue May 15, 2023 that may be closed by this pull request
@marcinapostoluk
Copy link
Contributor Author

If no error email is specified errors won't be sent to the regular email any more. The assumption here is that notebook maintainer will monitor it manually.

I don't think this is a safe assumption. Plenty of internal users only notice that their scheduled report is failing when they get the email (many don't check the webapp at all).

Sure, no problem, changed to default to regular email now if error email not specified

@jonbannister
Copy link
Collaborator

jonbannister commented May 17, 2023

  • Please add to the CHANGELOG

Copy link
Collaborator

@jonbannister jonbannister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@marcinapostoluk marcinapostoluk merged commit 018e535 into master May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Different email address for failed and succeeded reports

3 participants