Skip to content

Send email on Workflow Run Success/Failure #34982

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

Send email on Workflow Run Success/Failure #34982

wants to merge 41 commits into from
+364 −41

Conversation

NorthRealm
Copy link
Contributor

@NorthRealm NorthRealm commented Jul 7, 2025 •
edited
Loading

Closes #23725

1
2

/claim #23725

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 7, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Jul 7, 2025
@NorthRealm
Copy link
Contributor Author

NorthRealm commented Jul 7, 2025 •
edited
Loading

Note: For code reusability, there's a refactor/renaming in mail.go, mail_issue_common.go and mail_test.go in c8b64c7, ed7b2bc

@NorthRealm
Copy link
Contributor Author

1

lunny
lunny reviewed Jul 7, 2025
View reviewed changes
lunny
lunny reviewed Jul 7, 2025
View reviewed changes
@lunny lunny added this to the 1.25.0 milestone Jul 7, 2025
@lunny
Copy link
Member

lunny commented Jul 7, 2025

replace #33601

wxiaoguang
wxiaoguang reviewed Jul 8, 2025
View reviewed changes
@wxiaoguang

This comment was marked as resolved.

Sign in to view
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 8, 2025 •
edited
Loading

outdated

And by reading the history discussions:

I think the concern is not addressed? should we send the email on "workflow job" level, or "workflow run" level? (I am not certain about the details, just FYI)

It looks right, the concerns in "Actions - send Email on Success/Failure #33601" should all be addressed. 🙏

@wxiaoguang wxiaoguang mentioned this pull request Jul 8, 2025
Open
wxiaoguang
wxiaoguang reviewed Jul 8, 2025
View reviewed changes
wxiaoguang
wxiaoguang reviewed Jul 8, 2025
View reviewed changes
wxiaoguang
wxiaoguang reviewed Jul 8, 2025
View reviewed changes
wxiaoguang
wxiaoguang reviewed Jul 8, 2025
View reviewed changes
lunny
lunny reviewed Jul 12, 2025
View reviewed changes
@NorthRealm
Copy link
Contributor Author

Any further review please?

@lunny
Copy link
Member

lunny commented Jul 13, 2025

Any further review please?

I will complete my review by Monday.

ChristopherHX
ChristopherHX approved these changes Jul 13, 2025
View reviewed changes
Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

Works great on my end

unrelated: only configureing gitea for me with enabled mails, could be better documented that the email service needs to be enabled additionally to the mailer itself

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 13, 2025
ChristopherHX
ChristopherHX reviewed Jul 13, 2025
View reviewed changes
@NorthRealm @ChristopherHX
UPDATE
628ace2 
Co-authored-by: ChristopherHX <[email protected]>
Signed-off-by: NorthRealm <[email protected]>
wxiaoguang
wxiaoguang reviewed Jul 14, 2025
View reviewed changes
wxiaoguang
wxiaoguang reviewed Jul 14, 2025
View reviewed changes
@NorthRealm NorthRealm requested a review from lunny July 14, 2025 14:57
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 14, 2025
wxiaoguang
wxiaoguang reviewed Jul 15, 2025
View reviewed changes
@NorthRealm
Copy link
Contributor Author

🤦‍♂️ 1

wxiaoguang
wxiaoguang reviewed Jul 15, 2025
View reviewed changes
@wxiaoguang
Copy link
Contributor

🤦‍♂️ 1

You can first understand what they do and what they affect before copying them.

@NorthRealm
Copy link
Contributor Author

NorthRealm commented Jul 15, 2025 •
edited
Loading

🤦‍♂️ 1

You can first understand what they do and what they affect before copying them.

I haven't seen anywhere crappy so I would like to hear your thoughts.
I refer a lot from elsewhere in the whole code base, and I see it potentially reusable/re-applicable if it's been there.
Other than that I'm fine.
Related #28145

@wxiaoguang
Copy link
Contributor

I haven't seen anywhere crappy so I would like to hear your thoughts.
I refer a lot from elsewhere in the whole code base, and I see it potentially reusable/re-applicable if it's been there.
Other than that I'm fine.
Related #28145

  • _blank already means noopener
  • <meta name="referrer" content="no-referrer"> already means noreferrer

@wxiaoguang
Copy link
Contributor

I refer a lot from elsewhere in the whole code base, and I see it potentially reusable/re-applicable if it's been there.

Actually we should never trust existing code, there are bugs and bad-smells. If you just copy-paste existing code, then there will be more bugs and bad-smells.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristopherHX ChristopherHX ChristopherHX approved these changes

@lunny lunny lunny approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

Assignees
No one assigned
Labels
🙋 Bounty claim lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation
Projects
None yet
Milestone
1.25.0
Development

Successfully merging this pull request may close these issues.

Actions - send Email on Success/Failure
7 participants
@NorthRealm @lunny @wxiaoguang @Naxdy @jonnytest1 @ChristopherHX @GiteaBot