Skip to content

Allow developers to merge into a protected branch without having push access

Timothy Andrew requested to merge 18193-developers-can-merge into master

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 to UserAccess and the new ChangeAccessCheck.
  • 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

1 2 3

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
    • 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

Merge request reports