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
Merged
merged 46 commits into from
Jul 16, 2025
Merged

Send email on Workflow Run Success/Failure #34982

merged 46 commits into from
Jul 16, 2025

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
Closed
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
@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.

@wxiaoguang wxiaoguang added reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Jul 15, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 15, 2025 •
edited
Loading

Will explain more "bad smells" in old code that I have made changes to:

  1. It shouldn't duplicate log.Error when using ServerError which already logs the err
  2. It shouldn't show 500 error page when the user input is invalid.
  3. It shouldn't set selected active for a dropdown item, selected and active have different meanings
  4. It shouldn't duplicate {{if eq ...}}selected{{end}} when using dropdown, the input value is the selected one and the framework already handles it
  5. It shouldn't duplicate rel="noopener noreferer", it only makes the translation more difficult (the affect has been explained above)

Actually I can tolerate the nits in code (not serious bugs) if the PR was reviewed and merged by others, or I missed some of them. While if I am reviewing or writing code, I will always do my best to avoid the bad smells.

@lunny lunny merged commit 0d00ec7 into go-gitea:main Jul 16, 2025
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 16, 2025
@ankurk91
Copy link

@lunny
this will land in 1.25?😬 please

zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 16, 2025
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
e67766c 
* giteaofficial/main:
  Send email on Workflow Run Success/Failure (go-gitea#34982)
  [skip ci] Updated translations via Crowdin
  Replace `poetry` with `uv` (go-gitea#35084)
  nix flake update (go-gitea#35085)
  Use monospace font in PR command line instructions (go-gitea#35074)
  Add gitignore rules to exclude LLM instruction files (go-gitea#35076)
  [skip ci] Updated translations via Crowdin
  Fix form property assignment edge case (go-gitea#35073)
  Improve submodule relative path handling (go-gitea#35056)
  Fixed all grammatical errors in locale_en-US.ini (go-gitea#35053)
  UI: add hover background to table rows in user and repo admin page (go-gitea#35072)
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 16, 2025 •
edited
Loading

Yes, it is in 1.25, great thanks to @NorthRealm and reviewers 🎉

You can use it by the nightly build:

@ghnp5
Copy link

ghnp5 commented Jul 16, 2025

Thank you so much for this!! 🥳🎉🧡

@NorthRealm NorthRealm deleted the patch4 branch July 16, 2025 08:11
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

@hiifong hiifong hiifong 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 type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.25.0
Development

Successfully merging this pull request may close these issues.

Actions - send Email on Success/Failure
10 participants
@NorthRealm @lunny @wxiaoguang @Naxdy @jonnytest1 @ankurk91 @ghnp5 @ChristopherHX @hiifong @GiteaBot