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 32 commits into
base: main
Choose a base branch
from
Open

Send email on Workflow Run Success/Failure #34982

wants to merge 32 commits into from
+422 −36

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
@NorthRealm NorthRealm mentioned this pull request Jul 8, 2025
Closed
@NorthRealm NorthRealm mentioned this pull request Jul 9, 2025
Merged
@Naxdy
Copy link
Contributor

Naxdy commented Jul 9, 2025

GitLab is capable of notifying you when you've "fixed" a pipeline, i.e. if a particular workflow succeeded that previously failed on the same branch.

I think it would make sense to implement that if possible (outside of EmailNotificationsAndYourOwn), as opposed to simply notifying on any workflow success.

@NorthRealm
Copy link
Contributor Author

@Naxdy Currently, Gitea just mixes all job attempts in a single run, not as GitHub explicitly stating re-run attempts creating a new run ID. Not possible without extra work. Is it actually desired? Personally not adding that here in this PR. afaik GitHub has no notifying successful retries after failure, unless enable successful run notification.

@Naxdy
Copy link
Contributor

Naxdy commented Jul 9, 2025

Thanks for the explanation. And yeah, as of right now, only GitLab has this feature. Perhaps I'll take a crack at it after your PR is merged 🤔

wxiaoguang added a commit that referenced this pull request Jul 11, 2025
@NorthRealm @wxiaoguang
Add Notifications section in User Settings (#35008)
56eccb4 
Related: #34982

---------

Signed-off-by: NorthRealm <155140859+NorthRealm@users.noreply.github.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@NorthRealm
Copy link
Contributor Author

NorthRealm commented Jul 11, 2025 •
edited
Loading

@wxiaoguang @ChristopherHX @Naxdy

1

@lunny
Copy link
Member

lunny commented Jul 11, 2025

Is the new table NotificationSettings necessary? Could we reuse UserSetting table?

@NorthRealm
Copy link
Contributor Author

@lunny The use of UserSetting table is not clear imv. Looks like it has quite limited use.

@wxiaoguang
Copy link
Contributor

I also think it should reuse the existing setting system, instead of introducing a new system.

And the existing system is indeed not clear, it needs refactoring and improvements.

@NorthRealm
Copy link
Contributor Author

@lunny @wxiaoguang

@jonnytest1
Copy link

Thanks for the explanation. And yeah, as of right now, only GitLab has this feature. Perhaps I'll take a crack at it after your PR is merged 🤔

probably just add a "previousRunFailed" property to the event then it should be easy to make conditions for it based on user configuration

lunny
lunny reviewed Jul 12, 2025
View reviewed changes
services/mailer/mail_workflow_run.go

recipients := make([]*user_model.User, 0)

if !sender.IsGiteaActions() && !sender.IsGhost() && sender.IsMailable() {
Copy link
Member

Choose a reason for hiding this comment

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

Why checking sender's preference rather than receivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lunny Currently in the code the sender is the receiver. Only the sender goes receiver list.
#34982 (comment)
#34982 (comment)

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

@lunny lunny lunny left review comments

@wxiaoguang wxiaoguang wxiaoguang left review comments

@ChristopherHX ChristopherHX ChristopherHX left review comments

Assignees
No one assigned
Labels
🙋 Bounty claim lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. 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