-
Merge branch '18193-developers-can-merge' into 'master' · 9ca633ebRémy Coutable authored
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 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 (thanks, @mvestergaard!) ; I'm wrapping it up. ## What are the relevant issue numbers? #18193 Closes #967 ## Screenshots ![1](/uploads/c636e88ba38628211754e7cf122b0dc4/1.png) ![2](/uploads/5ed1e7917e2f36853a479faa565b022a/2.png) ![3](/uploads/0d202ba42e8dc6aade7bc6ac8db41ee6/3.png) ## TODO - [ ] #18193 !4892 Add "developers can merge" as an option for protected branches - [x] Review existing code - [x] Fix build - [x] Implementation / refactoring - [x] Clean up `GitAccess` - [x] Clean up `protected_branches.js.coffee` - [x] 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 - [x] Try detecting merge commits and allowing those - [x] A push with regular commits _and_ merge commits should fail - [x] Figure out a solution - [x] Extensive tests for `MergeCommitCheck` - [x] Add tests - [x] Untested parts of original MR - [x] 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 - [x] Wait for replies on my proposal - [x] Pick a strategy - [x] Implementation - [x] Save `in_progress_merge_commit_sha` - [x] Check `in_progress_merge_commit_sha` - [x] Clear `in_progress_merge_commit_sha` - [x] Test / refactor - [x] Merge conflicts - [x] Verify workflows - [x] Developer with 'developer can merge' on: - [x] Can merge an MR from the Web UI - [x] Error message for conflicts in the Web UI - [x] Cannot merge an MR from the command line (HTTP) - [x] Cannot merge an MR from the command line (SSH) - [x] Cannot modify the branch otherwise - [x] Developer with 'developer can merge' off: - [x] Cannot merge an MR from the Web UI - [x] Error message for conflicts in the Web UI - [x] Cannot merge an MR from the command line (HTTP) - [x] Cannot merge an MR from the command line (SSH) - [x] Cannot modify the branch otherwise - [x] New projects created could have have "Developers can merge" turned on automatically for the default branch - [x] CHANGELOG - [x] Fix build - [x] Wait for [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/42624e3d53754064186d4ae9048e310d1d3eed0b/builds) to pass - [x] Screenshots - [x] Assign to endboss - [x] Respond to @dbalexandre's comments - [x] Duplicated line, this is equals to line 26. - [x] We aren't using any of these helpers in this migration, we can remove the include. - [x] What do you think to add a default value for this column to avoid the Three-state Boolean Problem? - [x] group all checks under Gitlab::Checks - [x] You have a default value for developers_can_merge column, but your migration doesn't add it. - [x] What do you think to rename Partially protected to anything else? - [x] Fix conflicts - [x] Make sure [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/b1cfd42f20a78fd7f844288954e97cff32962e20/builds) passes - [ ] Wait for merge See merge request !4892
9ca633eb
Name |
Last commit
|
Last update |
---|---|---|
.. | ||
banzai | Loading commit data... | |
ci | Loading commit data... | |
container_registry | Loading commit data... | |
gitlab | Loading commit data... | |
json_web_token | Loading commit data... | |
disable_email_interceptor_spec.rb | Loading commit data... | |
extracts_path_spec.rb | Loading commit data... | |
file_size_validator_spec.rb | Loading commit data... | |
git_ref_validator_spec.rb | Loading commit data... | |
gitlab_spec.rb | Loading commit data... | |
repository_cache_spec.rb | Loading commit data... |