GitLab Next

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

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