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
GitAccessmodule, moving parts of it toUserAccessand 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
- Clean up
- 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
- Save
- 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
- Developer with 'developer can merge' on:
- 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


