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.

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

Do not send notifications for draft releases #21451

Merged
merged 12 commits into from
Oct 17, 2022
Merged

Do not send notifications for draft releases #21451

merged 12 commits into from
Oct 17, 2022

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Oct 14, 2022

Fixes #21448

@KN4CK3R KN4CK3R added this to the 1.18.0 milestone Oct 14, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 14, 2022
@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 Oct 14, 2022
@zeripath
Copy link
Contributor

Hmm... Doesn't this mean that we won't ever get a CreateRelease for a release that is marked as Draft but then is not marked as Draft?

@Gusted
Copy link
Contributor

Gusted commented Oct 15, 2022

Hmm... Doesn't this mean that we won't ever get a CreateRelease for a release that is marked as Draft but then is not marked as Draft?

Draft --> Non-draft (notification issued)
Non-Draft --> Draft (notification not issued)

@zeripath
Copy link
Contributor

Hmm... Doesn't this mean that we won't ever get a CreateRelease for a release that is marked as Draft but then is not marked as Draft?

Draft --> Non-draft (notification issued) Non-Draft --> Draft (notification not issued)

You won't get the create release...

@Gusted
Copy link
Contributor

Gusted commented Oct 15, 2022

You won't get the create release...

No? Because the release has been updated... Do you want to have an create release event when draft --> non-draft?

@codecov-commenter
Copy link

Codecov Report

Merging #21451 (03371c4) into main (11d3677) will increase coverage by 47.61%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           main   #21451       +/-   ##
=========================================
+ Coverage      0   47.61%   +47.61%     
=========================================
  Files         0     1023     +1023     
  Lines         0   139349   +139349     
=========================================
+ Hits          0    66347    +66347     
- Misses        0    64996    +64996     
- Partials      0     8006     +8006
Impacted Files Coverage Δ
services/release/release.go 46.09% <100.00%> (ø)
modules/log/file.go 68.78% <0.00%> (ø)
modules/migration/review.go 100.00% <0.00%> (ø)
modules/doctor/storage.go 18.18% <0.00%> (ø)
models/user/redirect.go 81.81% <0.00%> (ø)
routers/web/base.go 16.52% <0.00%> (ø)
models/packages/package_blob.go 62.50% <0.00%> (ø)
routers/web/explore/topic.go 81.81% <0.00%> (ø)
modules/markup/markdown/meta.go 70.42% <0.00%> (ø)
services/forms/user_form_auth_openid.go 0.00% <0.00%> (ø)
... and 1014 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@techknowlogick techknowlogick merged commit a37e8b2 into go-gitea:main Oct 17, 2022
@lafriks
Copy link
Member

lafriks commented Oct 17, 2022

Imho this behavior is broken, while I understand the logic, if you are wanting only create events you will never get them if draft is created and then updated to normal release. Even if I get the update event I can't know it was previously draft...

If we do not want release to send events for draft we should send create event for when status is changed from draft to normal and delete when from normal to draft imho

@lafriks
Copy link
Member

lafriks commented Oct 17, 2022

Also imho this is breaking change now and I do not think it should be backported

@techknowlogick
Copy link
Member

Good points from @lafriks , perhaps we should revert this as we are about to create an RC so we can discuss it more

zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 18, 2022
@zjjhot
Merge remote-tracking branch 'upstream/main'
b55a86c 
* upstream/main: (32 commits)
  inline gitpod image (go-gitea#21494)
  [skip ci] Updated translations via Crowdin
  Do not send notifications for draft releases (go-gitea#21451)
  Update reverse-proxies.zh-cn.md (go-gitea#21484)
  Docs: Update the feature comparison to other Git Hosting Services (go-gitea#20933)
  Add some api integration tests (go-gitea#18872)
  probe if sha before exec git (go-gitea#21467)
  Fix incorrect notification commit url (go-gitea#21479)
  Localize all timestamps (go-gitea#21440)
  [skip ci] Updated translations via Crowdin
  Add system setting table with cache and also add cache supports for user setting (go-gitea#18058)
  Return 404 when user is not found on avatar (go-gitea#21476)
  Enforce grouped NuGet search results (go-gitea#21442)
  Display total commit count in hook message (go-gitea#21400)
  Refactor GetNextResourceIndex to make it work properly with transaction (go-gitea#21469)
  Simplify fmt-check (go-gitea#21458)
  update current stable version
  1.17.3 changelog
  [skip ci] Updated translations via Crowdin
  Fix mermaid-related bugs (go-gitea#21431)
  ...
@zeripath
Copy link
Contributor

This is precisely what I was questioning about above.

We need still need to emit the create event - at least when the draft is finally undrafted (but then you're likely going to need to add a field on the release)

@KN4CK3R KN4CK3R deleted the fix-21448 branch October 19, 2022 06:11
@Gusted
Copy link
Contributor

Gusted commented Oct 20, 2022

That would make sense to do that... bummer it got merged already.

@lunny
Copy link
Member

lunny commented Oct 24, 2022

Please send backport

techknowlogick pushed a commit to techknowlogick/gitea that referenced this pull request Oct 25, 2022
@KN4CK3R @techknowlogick
Revert "Do not send notifications for draft releases (go-gitea#21451)"
61b9772 
This reverts commit a37e8b2.
@techknowlogick techknowlogick mentioned this pull request Oct 25, 2022
Merged
6543 pushed a commit that referenced this pull request Oct 25, 2022
@techknowlogick
Revert "Do not send notifications for draft releases (#21451)" (#21594)
3f7cab4 
This reverts commit a37e8b2 / #21451

Temporarily revert this PR to be able to continue discussion, and
potentially get it into 1.19.0
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 26, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
c6d481e 
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Revert "Do not send notifications for draft releases (go-gitea#21451)" (go-gitea#21594)
  Change `commits-table` column width (go-gitea#21564)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@Gusted Gusted Gusted approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.18.0
Development

Successfully merging this pull request may close these issues.

draft releases: update & delete events being sent while creation dont