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

Admin Merge broken on 1.18.4 #23000

Closed
gempir opened this issue Feb 20, 2023 · 3 comments · Fixed by #23010
Closed

Admin Merge broken on 1.18.4 #23000

gempir opened this issue Feb 20, 2023 · 3 comments · Fixed by #23010
Assignees
@wolfogre
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug
Milestone
1.18.5

Comments

@gempir
Copy link
Contributor

gempir commented Feb 20, 2023

Description

I have a branch with a Branch Protection (at least 1 approval and a check that has to run)

image

The UI tells me I still can merge it anyway since i'm Admin. But when I try to merge it

The error message

This pull request is not ready to be merged, check review status and status checks.
Appears

Could also reproduce on try.gitea.io with [Powered by Gitea](https://gitea.io/) Version: 1.19.0+dev-521-g2b3f12f6f Page: 99ms Template: 11ms

Steps To Reproduce

  • Be admin in repository
  • Setup branch protection for any branch, require 1 approval
  • Create a pull request pointing at the protected branch
  • Try to merge without having 1 approval (with the red admin merge button)

Gitea Version

1.18.4

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

image

Git Version

No response

Operating System

No response

How are you running Gitea?

Irrelevant

Database

None
@wolfogre wolfogre added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Feb 20, 2023
@wolfogre wolfogre self-assigned this Feb 20, 2023
@yardenshoham yardenshoham added this to the 1.18.5 milestone Feb 20, 2023
@wolfogre wolfogre mentioned this issue Feb 20, 2023
Merged
@kolaente
Copy link
Member

Noticed that as well.

@lunny
Copy link
Member

lunny commented Feb 20, 2023

@kolaente Please confirm #23010 fix this problem.

@parnic-sks
Copy link

parnic-sks commented Feb 20, 2023 •
edited
Loading

This same issue occurs when trying to schedule an "auto-merge after checks succeed", even as a non-admin.

edit: logged this as #23022 since it seems slightly different and might not have the same fix.

@parnic-sks parnic-sks mentioned this issue Feb 20, 2023
Closed
@wolfogre wolfogre mentioned this issue Feb 21, 2023
Merged
jolheiser pushed a commit that referenced this issue Feb 21, 2023
@wolfogre
Add force_merge to merge request and fix checking mergable (#23010)
c8c2a31 
Fix #23000.
jolheiser pushed a commit that referenced this issue Feb 21, 2023
@wolfogre
Add force_merge to merge request and fix checking mergable (#23010) (#…
90982bf 
…23032)

Backport #23010.

Fix #23000.

The bug was introduced in #22633, and it seems that it has been noticed:
#22633 (comment) .

However, #22633 did nothing wrong, the logic should be "check if they is
admin only when `force` is true".

So we should provide the `ForceMerge` when merging from UI.

After this, an admin can also send a normal merge request with
`ForceMerge` false. So it fixes a potential bug: if the admin doesn't
want to do a force merge, they just see the green "Merge" button and
click it. At the same time, the status of the PR changed, and it
shouldn't be merged now, so the admin could send an unexpected force
merge.

In addition, I updated `ForceMerge *bool` to `ForceMerge bool`, I don't
see the reason to use a pointer.

And fixed the logic of CheckPullMergable to handle auto merge and force
merge correctly.
@gempir gempir mentioned this issue Mar 1, 2023
Closed
@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.
Assignees

@wolfogre wolfogre

Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug
Projects
None yet
Milestone
1.18.5
Development

Successfully merging a pull request may close this issue.

Add force_merge to merge request and fix checking mergable
6 participants
@lunny @wolfogre @gempir @kolaente @yardenshoham @parnic-sks