-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
feat: Clarify approved vs mergeable for gitlab #3830
base: main
Are you sure you want to change the base?
feat: Clarify approved vs mergeable for gitlab #3830
Conversation
Discussed this at office hours today, and @GenPage asked me to add a comment about the impact of this change. If merged, this would be a breaking change for gitlab users who use the |
13a257b
to
3d99f8d
Compare
e61cf94
to
7650ac8
Compare
@lukemassa, could you sign the commits and fix the conflicts to merge this? Thanks. |
what
approved
mean "Approved by a user other than the author" for gitlabwhy
Per #2605, gitlab does not respect the
approved
the way users would expect, i.e. has someone clicked the "approved" button. This is how the other VCSs interpretapproved
.The current implementation for gitlab is closer to "are approval rules passing". That's what I thought "approved" meant when I implemented #3744 (since I'm most familiar with gitlab).
However, looking closer, it turns out:
mr.ApprovalsBeforeMerge <= 0
)Thus there was this inconsistency, plus it meant that
approved
was strictly weaker thanmergeable
for gitlab.The theory here I think is that it is nice to have one knob for users to turn that means "a user needs to approve this" vs "a user needs to approve this if the project thinks a user needs to approve this".
I personally will only use
mergeable
in my setup, since I have tight control over all the repos, but without a change like this, there's no way to implement #2605 , and force workflows to have approvals from the "atlantis side".tests
Before:
apply_requirements
[]
[approved]
[mergeable]
[approved, mergeable]
After:
apply_requirements
[]
[approved]
[mergeable]
[approved, mergeable]
As you can see from the above, the functional change here is, for projects that do not have an MR Approval Rule, setting
approved
requires approval.references
reverts #3744
closes: #2605