Allow developers to merge into a protected branch without having push access
What does this MR do?
Adds a "Developers can merge" checkbox to protected branches much like the "Developers can push" checkbox. When the checkbox is enabled, a developer can merge MRs into that protected branch from the Web UI and from the command-line (any push that is entirely composed of merge commits is allowed).
Are there points in the code the reviewer needs to double check?
- This MR refactors the
GitAccess
module, moving parts of it toUserAccess
and the newChangeAccessCheck
. - This MR refactors
GitAccessSpec
, which generates a "matrix" of tests. - The main logic "developers can merge" should be straightforward enough.
- The commits are fairly atomic, and the commit messages are descriptive regarding the motivations behind every change.
Why was this MR needed?
A significant portion of this feature was implemented in !4220 (closed) (thanks, @mvestergaard!) ; I'm wrapping it up.
What are the relevant issue numbers?
#18193 (closed) Closes #967 (closed)
Screenshots
TODO
-
#18193 (closed) !4892 (merged) Add "developers can merge" as an option for protected branches -
Review existing code -
Fix build -
Implementation / refactoring -
Clean up GitAccess
-
Clean up protected_branches.js.coffee
-
Figure out authorization issue - If we try to merge code into a protected branch for a user who doesn't have access to that branch, an auth check will fail
- We need to get around this, somehow
-
Try detecting merge commits and allowing those
-
A push with regular commits and merge commits should fail -
Figure out a solution -
Extensive tests for MergeCommitCheck
-
-
-
Add tests -
Untested parts of original MR
-
-
Improve the checks in /allowed
-
@dzaporozhets's proposal:
- commits in push == commits in merge request
- branch to push == target branch of merge request
- merge request has required amount of approves (ee only)
- merge commit in push == merge commit we created when merged via UI
- save merge commit sha in database and compare with
newrev
- my proposal
- /allowed finds all open merge requests with the appropriate target branch
- For each MR, compare the commit SHAs in the MR to the commit SHAs in the change set
- If we have a match, compare the diff of the matching MR to the diff of the change set
- If we still have a match, the merge is legit
-
Wait for replies on my proposal -
Pick a strategy -
Implementation -
Save in_progress_merge_commit_sha
-
Check in_progress_merge_commit_sha
-
Clear in_progress_merge_commit_sha
-
-
Test / refactor
-
@dzaporozhets's proposal:
-
Merge conflicts -
Verify workflows -
Developer with 'developer can merge' on: -
Can merge an MR from the Web UI -
Error message for conflicts in the Web UI -
Cannot merge an MR from the command line (HTTP) -
Cannot merge an MR from the command line (SSH) -
Cannot modify the branch otherwise
-
-
Developer with 'developer can merge' off: -
Cannot merge an MR from the Web UI -
Error message for conflicts in the Web UI -
Cannot merge an MR from the command line (HTTP) -
Cannot merge an MR from the command line (SSH) -
Cannot modify the branch otherwise
-
-
New projects created could have have "Developers can merge" turned on automatically for the default branch
-
-
CHANGELOG -
Fix build -
Wait for build to pass -
Screenshots -
Assign to endboss -
Respond to @dbalexandre's comments -
Duplicated line, this is equals to line 26. -
We aren't using any of these helpers in this migration, we can remove the include. -
What do you think to add a default value for this column to avoid the Three-state Boolean Problem? -
group all checks under Gitlab::Checks -
You have a default value for developers_can_merge column, but your migration doesn't add it. -
What do you think to rename Partially protected to anything else?
-
-
Fix conflicts -
Make sure build passes -
Wait for merge
-