FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

chore: Add Mailgun send email action by lahirumaramba · Pull Request #1210 · firebase/firebase-admin-node · GitHub

/ firebase-admin-node Public

chore: Add Mailgun send email action - #1210

Merged
lahirumaramba merged 8 commits into
masterfrom
lm-mailgun-gha
Mar 31, 2021
ConversationCommits (8)ChecksFiles changed
Merged

chore: Add Mailgun send email action#1210
lahirumaramba merged 8 commits into
masterfrom
lm-mailgun-gha

Conversation

lahirumaramba commented Mar 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Member
  • Add GitHub Action to send email using mailgun.js npm module.
  • Update nightly workflow to use the send email action.
  • Add a failing integration test to trigger the email notification workflow (this test will be removed once we confirm the email notifications be working as expected).

lahirumaramba added the release:stage Stage a release candidate label Mar 30, 2021

lahirumaramba commented Mar 30, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Note: release:stage is added to trigger the integration tests. Integration tests should fail with the added test in this PR. This new test will trigger the send email workflow once merged to master.

lahirumaramba commented Mar 30, 2021
View reviewed changes
Comment thread .github/actions/send-email/index.js Outdated
});

let from = core.getInput('from');
const cc = core.getInput('cc');

lahirumaramba Mar 30, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We probably do not need cc filed. I am open to remove it and keep this action as simple as possible if we need.

hiranya911 suggested changes Mar 30, 2021
View reviewed changes

hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks pretty good. A few thoughts for improvement.

Comment thread .github/actions/send-email/index.js Outdated
Comment thread .github/actions/send-email/index.js Outdated
Comment thread .github/workflows/nightly.yml Outdated

lahirumaramba commented Mar 31, 2021

Copy link
Copy Markdown
Member Author

Thanks, @hiranya911!
Used a bit of a lazy approach to validate all the required fields. Kept cc, text, html, as optional. If html is provided and not text the API extracts the text content from the html. Let me know what you think.

hiranya911 suggested changes Mar 31, 2021
View reviewed changes

hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just a couple of readability nits. And remember to update the binary every time you push a code change.

Comment thread .github/actions/send-email/index.js Outdated

function validate(config) {
for (param in config) {
if (['cc', 'text', 'html'].includes(param)) {

hiranya911 Mar 31, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality 
const optionalFields = ['cc', 'text', 'html'];

Comment thread .github/actions/send-email/index.js Outdated
}
}

function validateParameter(value, name) {

hiranya911 Mar 31, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

validateRequiredParameter

lahirumaramba commented Mar 31, 2021

Copy link
Copy Markdown
Member Author

Thanks @hiranya911! Updated the binary.

This was referenced Jun 20, 2024
Open
Open
Open
Open
davidhin mentioned this pull request Jul 6, 2024
Open
This was referenced Jul 17, 2024
Open
Open
This was referenced Jul 27, 2024
Open
Open
Open
Open
This was referenced Aug 10, 2024
Open
Open
This was referenced Aug 17, 2024
Open
Open
Open
Open
Open
This was referenced Aug 28, 2024
Open
Open
This was referenced Sep 5, 2024
Open
Open
This was referenced Sep 24, 2024
Open
Open
Open
Open
Open
This was referenced Oct 26, 2024
Open
Open
davidhin mentioned this pull request Nov 9, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
hiranya911 hiranya911 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

lahirumaramba

Labels

release:stage Stage a release candidate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL