Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EmailTask: accept msg_plain, email_to_cc and email_to_bcc #4157

Merged
merged 5 commits into from
Feb 24, 2021

Conversation

joelluijmes
Copy link
Contributor

Summary

Expends possibilities of EmailTask by accepting the following parameters: msg_plain, email_to_cc and email_to_bcc

  • msg_plain: allows to add plain text version of email
  • email_to_cc: additional addresses to put in CC
  • email_to_bcc: additional addresses to put in BCC

Changes

Additional parameters.

Importance

More flexibility :)

Checklist

This PR:

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

Merging #4157 (1adf73f) into master (5d086e3) will decrease coverage by 0.01%.
The diff coverage is n/a.

Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Hey @joelluijmes, this is a nice upgrade for this task! Can you confirm that switching to this different email API isn't going to break anything for existing users?

src/prefect/tasks/notifications/email_task.py Outdated Show resolved Hide resolved
@joelluijmes
Copy link
Contributor Author

Hey @joelluijmes, this is a nice upgrade for this task! Can you confirm that switching to this different email API isn't going to break anything for existing users?

Yes should be backwards compatible. As in, didn’t rename the param or changed the order :)

@zanieb
Copy link
Contributor

zanieb commented Feb 23, 2021

Hey @joelluijmes, this is a nice upgrade for this task! Can you confirm that switching to this different email API isn't going to break anything for existing users?

Yes should be backwards compatible. As in, didn’t rename the param or changed the order :)

My concern is that the appearance of people's emails will change since the content is being specified differently now and I don't want to break anyone's flows using email tasks.

@joelluijmes
Copy link
Contributor Author

My concern is that the appearance of people's emails will change since the content is being specified differently now and I don't want to break anyone's flows using email tasks.

Understandably, but fortunately this doesn't change anything. Only specifiying the msg (add_alternative) with html yields a html email 👍

zanieb
zanieb previously approved these changes Feb 24, 2021
Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

If you merge master in you'll fix the codecov error. You'll need to run black on whatever file it's complaining about. Otherwise this LGTM, I like the rephrased comment 👍

@jcrist jcrist merged commit 2b6fee3 into PrefectHQ:master Feb 24, 2021
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.

3 participants