diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2019-10-07 12:05:55 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2019-10-07 12:05:55 +0300 |
commit | ab2382923e7a864a3fa27fdf8eeb9b21893b9147 (patch) | |
tree | 89a0d33672657f2c793b0966e1d82041f36c76c9 | |
parent | 3900b8a0062113e11e600e4226426e4163ac5e2c (diff) |
Add latest changes from gitlab-org/gitlab@master
-rw-r--r-- | .rubocop.yml | 1 | ||||
-rw-r--r-- | app/models/merge_request.rb | 4 | ||||
-rw-r--r-- | app/services/merge_requests/refresh_service.rb | 26 | ||||
-rw-r--r-- | changelogs/unreleased/16388-abort-mwps-on-impossible-ff-merge.yml | 5 | ||||
-rw-r--r-- | doc/development/README.md | 1 | ||||
-rw-r--r-- | doc/development/automatic_ce_ee_merge.md | 243 | ||||
-rw-r--r-- | doc/workflow/gitlab_flow.md | 1 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 34 | ||||
-rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 124 |
9 files changed, 193 insertions, 246 deletions
diff --git a/.rubocop.yml b/.rubocop.yml index 45464ca3318..835c321c943 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -51,7 +51,6 @@ Style/FrozenStringLiteralComment: - 'danger/**/*' - 'db/**/*' - 'ee/db/**/*' - - 'ee/spec/**/*' - 'ee/lib/tasks/**/*' - 'lib/tasks/**/*' - 'qa/**/*' diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 63133ca285b..2b6934d4c83 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -196,6 +196,10 @@ class MergeRequest < ApplicationRecord scope :by_target_branch, ->(branch_name) { where(target_branch: branch_name) } scope :preload_source_project, -> { preload(:source_project) } + scope :with_open_merge_when_pipeline_succeeds, -> do + with_state(:opened).where(merge_when_pipeline_succeeds: true) + end + after_save :keep_around_commit alias_attribute :project, :target_project diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index edcfc3bf33f..b32499629ff 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -25,6 +25,7 @@ module MergeRequests outdate_suggestions refresh_pipelines_on_merge_requests abort_auto_merges + abort_ff_merge_requests_with_when_pipeline_succeeds mark_pending_todos_done cache_merge_requests_closing_issues @@ -148,6 +149,31 @@ module MergeRequests end end + def abort_ff_merge_requests_with_when_pipeline_succeeds + return unless @project.ff_merge_must_be_possible? + + requests_with_auto_merge_enabled_to(@push.branch_name).each do |merge_request| + next unless merge_request.should_be_rebased? + + abort_auto_merge_with_todo(merge_request, 'target branch was updated') + end + end + + def abort_auto_merge_with_todo(merge_request, reason) + response = abort_auto_merge(merge_request, reason) + response = ServiceResponse.new(response) + return unless response.success? + + todo_service.merge_request_became_unmergeable(merge_request) + end + + def requests_with_auto_merge_enabled_to(target_branch) + @project + .merge_requests + .by_target_branch(target_branch) + .with_open_merge_when_pipeline_succeeds + end + def mark_pending_todos_done merge_requests_for_source_branch.each do |merge_request| todo_service.merge_request_push(merge_request, @current_user) diff --git a/changelogs/unreleased/16388-abort-mwps-on-impossible-ff-merge.yml b/changelogs/unreleased/16388-abort-mwps-on-impossible-ff-merge.yml new file mode 100644 index 00000000000..4823c75bf2d --- /dev/null +++ b/changelogs/unreleased/16388-abort-mwps-on-impossible-ff-merge.yml @@ -0,0 +1,5 @@ +--- +title: Abort Merge When Pipeline Succeeds when Fast Forward merge is impossible +merge_request: 17886 +author: +type: fixed diff --git a/doc/development/README.md b/doc/development/README.md index 3aa65180fc9..6f712fcf0f4 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -19,7 +19,6 @@ description: 'Learn how to contribute to GitLab.' - [Code review guidelines](code_review.md) for reviewing code and having code reviewed - [Database review guidelines](database_review.md) for reviewing database-related changes and complex SQL queries - [Pipelines for the GitLab project](pipelines.md) -- [Automatic CE->EE merge](automatic_ce_ee_merge.md) - [Guidelines for implementing Enterprise Edition features](ee_features.md) - [Security process for developers](https://gitlab.com/gitlab-org/release/docs/blob/master/general/security/developer.md#security-releases-critical-non-critical-as-a-developer) - [Requesting access to Chatops on GitLab.com](chatops_on_gitlabcom.md#requesting-access) (for GitLabbers) diff --git a/doc/development/automatic_ce_ee_merge.md b/doc/development/automatic_ce_ee_merge.md deleted file mode 100644 index 80a85e144a4..00000000000 --- a/doc/development/automatic_ce_ee_merge.md +++ /dev/null @@ -1,243 +0,0 @@ -# Automatic CE->EE merge - -Commits pushed to CE `master` are automatically merged into EE `master` roughly -every 5 minutes. Changes are merged using the `recursive=ours` merge strategy in -the context of EE. This means that any merge conflicts are resolved by taking -the EE changes and discarding the CE changes. This removes the need for -resolving conflicts or reverting changes, at the cost of **absolutely -requiring** EE merge requests to be created whenever a CE merge request causes -merge conflicts. Failing to do so can result in changes not making their way -into EE. - -## Always create an EE merge request if there are conflicts - -In CI there is a job called `ee_compat_check`, which checks if a CE MR causes -merge conflicts with EE. If this job reports conflicts, you **must** create an -EE merge request. If you are an external contributor you can ask the reviewer to -do this for you. - -## Always merge EE merge requests before their CE counterparts - -**In order to avoid conflicts in the CE->EE merge, you should always merge the -EE version of your CE merge request first, if present.** - -Failing to do so will lead to CE changes being discarded when merging into EE, -if they cause merge conflicts. - -## Avoiding CE->EE merge conflicts beforehand - -To avoid the conflicts beforehand, check out the -[Guidelines for implementing Enterprise Edition features](ee_features.md). - -In any case, the CI `ee_compat_check` job will tell you if you need to open an -EE version of your CE merge request. - -### Conflicts detection in CE merge requests - -For each commit (except on `master`), the `ee_compat_check` CI job tries to -detect if the current branch's changes will conflict during the CE->EE merge. - -The job reports what files are conflicting and how to set up a merge request -against EE. - -#### How the job works - -1. Generates the diff between your branch and current CE `master` -1. Tries to apply it to current EE `master` -1. If it applies cleanly, the job succeeds, otherwise... -1. Detects a branch with the `ee-` prefix or `-ee` suffix in EE -1. If it exists, generate the diff between this branch and current EE `master` -1. Tries to apply it to current EE `master` -1. If it applies cleanly, the job succeeds - -In the case where the job fails, it means you should create an `ee-<ce_branch>` -or `<ce_branch>-ee` branch, push it to EE and open a merge request against EE -`master`. -At this point if you retry the failing job in your CE merge request, it should -now pass. - -Notes: - -- This task is not a silver-bullet, its current goal is to bring awareness to - developers that their work needs to be ported to EE. -- Community contributors shouldn't be required to submit merge requests against - EE, but reviewers should take actions by either creating such EE merge request - or asking a GitLab developer to do it **before the merge request is merged**. -- If you branch is too far behind `master`, the job will fail. In that case you - should rebase your branch upon latest `master`. -- Code reviews for merge requests often consist of multiple iterations of - feedback and fixes. There is no need to update your EE MR after each - iteration. Instead, create an EE MR as soon as you see the - `ee_compat_check` job failing. After you receive the final approval - from a Maintainer (but **before the CE MR is merged**) update the EE MR. - This helps to identify significant conflicts sooner, but also reduces the - number of times you have to resolve conflicts. -- Please remember to - [always have your EE merge request merged before the CE version](#always-merge-ee-merge-requests-before-their-ce-counterparts). -- You can use [`git rerere`](https://git-scm.com/docs/git-rerere) - to avoid resolving the same conflicts multiple times. - -### Cherry-picking from CE to EE - -For avoiding merge conflicts, we use a method of creating equivalent branches -for CE and EE. If the `ee-compat-check` job fails, this process is required. - -This method only requires that you have cloned both CE and EE into your computer. -If you don't have them yet, please go ahead and clone them: - -- Clone CE repo: `git clone git@gitlab.com:gitlab-org/gitlab-foss.git` -- Clone EE repo: `git clone git@gitlab.com:gitlab-org/gitlab.git` - -And the only additional setup we need is to add CE as remote of EE and vice-versa: - -- Open two terminal windows, one in CE, and another one in EE: - - In EE: `git remote add ce git@gitlab.com:gitlab-org/gitlab-foss.git` - - In CE: `git remote add ee git@gitlab.com:gitlab-org/gitlab.git` - -That's all setup we need, so that we can cherry-pick a commit from CE to EE, and -from EE to CE. - -Now, every time you create an MR for CE and EE: - -1. Open two terminal windows, one in CE, and another one in EE -1. In the CE terminal: - 1. Create the CE branch, e.g., `branch-example` - 1. Make your changes and push a commit (commit A) - 1. Create the CE merge request in GitLab -1. In the EE terminal: - 1. Create the EE-equivalent branch ending with `-ee`, e.g., - `git checkout -b branch-example-ee` - 1. Fetch the CE branch: `git fetch ce branch-example` - 1. Cherry-pick the commit A: `git cherry-pick commit-A-SHA` - 1. If Git prompts you to fix the conflicts, do a `git status` - to check which files contain conflicts, fix them, save the files - 1. Add the changes with `git add .` but **DO NOT commit** them - 1. Continue cherry-picking: `git cherry-pick --continue` - 1. Push to EE: `git push origin branch-example-ee` -1. Create the EE-equivalent MR and link to the CE MR from the - description `Ports [CE-MR-LINK] to EE` -1. Once all the jobs are passing in both CE and EE, you've addressed the - feedback from your own team, and got them approved, the merge requests can be merged. -1. When both MRs are ready, the EE merge request will be merged first, and the - CE-equivalent will be merged next. - -**Important notes:** - -- The commit SHA can be easily found from the GitLab UI. From a merge request, - open the tab **Commits** and click the copy icon to copy the commit SHA. -- To cherry-pick a **commit range**, such as (A > B > C > D) use: - - ```shell - git cherry-pick "oldest-commit-SHA^..newest-commit-SHA" - ``` - - For example, suppose the commit A is the oldest, and its SHA is `4f5e4018c09ed797fdf446b3752f82e46f5af502`, - and the commit D is the newest, and its SHA is `80e1c9e56783bd57bd7129828ec20b252ebc0538`. - The cherry-pick command will be: - - ```shell - git cherry-pick "4f5e4018c09ed797fdf446b3752f82e46f5af502^..80e1c9e56783bd57bd7129828ec20b252ebc0538" - ``` - -- To cherry-pick a **merge commit**, use the flag `-m 1`. For example, suppose that the - merge commit SHA is `138f5e2f20289bb376caffa0303adb0cac859ce1`: - - ```shell - git cherry-pick -m 1 138f5e2f20289bb376caffa0303adb0cac859ce1 - ``` - -- To cherry-pick multiple commits, such as B and D in a range (A > B > C > D), use: - - ```shell - git cherry-pick commit-B-SHA commit-D-SHA - ``` - - For example, suppose commit B SHA = `4f5e4018c09ed797fdf446b3752f82e46f5af502`, - and the commit D SHA = `80e1c9e56783bd57bd7129828ec20b252ebc0538`. - The cherry-pick command will be: - - ```shell - git cherry-pick 4f5e4018c09ed797fdf446b3752f82e46f5af502 80e1c9e56783bd57bd7129828ec20b252ebc0538 - ``` - - This case is particularly useful when you have a merge commit in a sequence of - commits and you want to cherry-pick all but the merge commit. - -- If you push more commits to the CE branch, you can safely repeat the procedure - to cherry-pick them to the EE-equivalent branch. You can do that as many times as - necessary, using the same CE and EE branches. -- If you submitted the merge request to the CE repo and the `ee-compat-check` job passed, - you are not required to submit the EE-equivalent MR, but it's still recommended. If the - job failed, you are required to submit the EE MR so that you can fix the conflicts in EE - before merging your changes into CE. - -## How we run the Automatic CE->EE merge at GitLab - -At GitLab, we use the [Merge Train](https://gitlab.com/gitlab-org/merge-train) -project to keep our [GitLab EE](https://gitlab.com/gitlab-org/gitlab) -repository updated with commits from -[GitLab CE](https://gitlab.com/gitlab-org/gitlab-foss). - -We have a mirror of the [Merge Train](https://gitlab.com/gitlab-org/merge-train) -project [configured](https://ops.gitlab.net/gitlab-org/merge-train) to run an -automatic CE->EE merge job every twenty minutes as a scheduled CI job. The -[configured](https://ops.gitlab.net/gitlab-org/merge-train) Merge Train project -is only accessible to authorized GitLab staff. - -## FAQ - -### How does automatic merging work? - -The automatic merging is performed using a project called [Merge -Train](https://gitlab.com/gitlab-org/merge-train/). This project will clone CE -and EE master, and merge CE master into EE master using `git merge ---strategy=recursive --strategy-option=ours`. This process runs multiple times -per hour. - -For more information on the exact implementation you can refer to the source -code. - -### Why merge automatically? - -As we work towards continuous deployments and a single repository for both CE -and EE, we need to first make sure that all CE changes make their way into EE as -fast as possible. Past experiences and data have shown that periodic CE to EE -merge requests do not scale, and often take a very long time to complete. For -example, [in this -comment](https://gitlab.com/gitlab-org/release/framework/issues/49#note_114614619) -we determined that the average time to close an upstream merge request is around -5 hours, with peaks up to several days. Periodic merge requests are also -frustrating to work with, because they often include many changes unrelated to -your own changes. - -To resolve these problems, we now merge changes using the `ours` strategy to -automatically resolve merge conflicts. This removes the need for resolving -conflicts in a periodic merge request, and allows us to merge changes from CE -into EE much faster. - -### My CE merge request caused conflicts after it was merged. What do I do? - -If you notice this, you should set up an EE merge request that resolves these -conflicts as **soon as possible**. Failing to do so can lead to your changes not -being available in EE, which may break tests. This in turn would prevent us from -being able to deploy. - -### Won't this setup be risky? - -No, not if there is an EE merge request for every CE merge request that causes -conflicts _and_ that EE merge request is merged first. In the past we may have -been a bit more relaxed when it comes to enforcing EE merge requests, but to -enable automatic merging we have to start requiring such merge requests even for -the smallest conflicts. - -### Some files I work with often conflict, how can I best deal with this? - -If you find you keep running into merge conflicts, consider refactoring the file -so that the EE specific changes are not intertwined with CE code. For Ruby code -you can do this by moving the EE code to a separate module, which can then be -injected into the appropriate classes or modules. See [Guidelines for -implementing Enterprise Edition features](ee_features.md) for more information. - ---- - -[Return to Development documentation](README.md) diff --git a/doc/workflow/gitlab_flow.md b/doc/workflow/gitlab_flow.md index 136e05281a6..e3568d6489d 100644 --- a/doc/workflow/gitlab_flow.md +++ b/doc/workflow/gitlab_flow.md @@ -168,7 +168,6 @@ This branch is the place for any work related to this change. NOTE: **Note:** The name of a branch might be dictated by organizational standards. -For example, in GitLab, any branches in GitLab EE that are equivalent to branches in GitLab CE [must end in `-ee`](../development/automatic_ce_ee_merge.md#cherry-picking-from-ce-to-ee). When you are done or want to discuss the code, open a merge request. A merge request is an online place to discuss the change and review the code. diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 65cc1a4bd6b..58a6c62cdcf 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -3257,4 +3257,38 @@ describe MergeRequest do end end end + + describe '.with_open_merge_when_pipeline_succeeds' do + let!(:project) { create(:project) } + let!(:fork) { fork_project(project) } + let!(:merge_request1) do + create(:merge_request, + :merge_when_pipeline_succeeds, + target_project: project, + target_branch: 'master', + source_project: project, + source_branch: 'feature-1') + end + + let!(:merge_request2) do + create(:merge_request, + :merge_when_pipeline_succeeds, + target_project: project, + target_branch: 'master', + source_project: fork, + source_branch: 'fork-feature-1') + end + + let!(:merge_request4) do + create(:merge_request, + target_project: project, + target_branch: 'master', + source_project: fork, + source_branch: 'fork-feature-2') + end + + let(:query) { described_class.with_open_merge_when_pipeline_succeeds } + + it { expect(query).to contain_exactly(merge_request1, merge_request2) } + end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 6ba67c7165c..2dc932c9f2c 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe MergeRequests::RefreshService do include ProjectForksHelper + include ProjectHelpers let(:project) { create(:project, :repository) } let(:user) { create(:user) } @@ -756,4 +757,127 @@ describe MergeRequests::RefreshService do end end end + + describe '#abort_ff_merge_requests_with_when_pipeline_succeeds' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:source_project) { project } + let_it_be(:target_project) { project } + let_it_be(:author) { create_user_from_membership(target_project, :developer) } + let_it_be(:user) { create(:user) } + + let_it_be(:forked_project) do + fork_project(target_project, author, repository: true) + end + + let_it_be(:merge_request) do + create(:merge_request, + author: author, + source_project: source_project, + source_branch: 'feature', + target_branch: 'master', + target_project: target_project, + auto_merge_enabled: true, + auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS, + merge_user: user) + end + + let_it_be(:newrev) do + target_project + .repository + .create_file(user, 'test1.txt', 'Test data', + message: 'Test commit', branch_name: 'master') + end + + let_it_be(:oldrev) do + target_project + .repository + .commit(newrev) + .parent_id + end + + let(:refresh_service) { service.new(project, user) } + + before do + target_project.merge_method = merge_method + target_project.save! + + refresh_service.execute(oldrev, newrev, 'refs/heads/master') + merge_request.reload + end + + let(:aborted_message) do + /aborted the automatic merge because target branch was updated/ + end + + shared_examples 'aborted MWPS' do + it 'aborts auto_merge' do + expect(merge_request.auto_merge_enabled?).to be_falsey + expect(merge_request.notes.last.note).to match(aborted_message) + end + + it 'removes merge_user' do + expect(merge_request.merge_user).to be_nil + end + + it 'does not add todos for merge user' do + expect(user.todos.for_target(merge_request)).to be_empty + end + + it 'adds todos for merge author' do + expect(author.todos.for_target(merge_request)).to be_present.and be_all(&:pending?) + end + end + + context 'when Project#merge_method is set to FF' do + let(:merge_method) { :ff } + + it_behaves_like 'aborted MWPS' + + context 'with forked project' do + let(:source_project) { forked_project } + + it_behaves_like 'aborted MWPS' + end + end + + context 'when Project#merge_method is set to rebase_merge' do + let(:merge_method) { :rebase_merge } + + it_behaves_like 'aborted MWPS' + + context 'with forked project' do + let(:source_project) { forked_project } + + it_behaves_like 'aborted MWPS' + end + end + + context 'when Project#merge_method is set to merge' do + let(:merge_method) { :merge } + + shared_examples 'maintained MWPS' do + it 'does not cancel auto merge' do + expect(merge_request.auto_merge_enabled?).to be_truthy + expect(merge_request.notes).to be_empty + end + + it 'does not change merge_user' do + expect(merge_request.merge_user).to eq(user) + end + + it 'does not add todos' do + expect(author.todos.for_target(merge_request)).to be_empty + expect(user.todos.for_target(merge_request)).to be_empty + end + end + + it_behaves_like 'maintained MWPS' + + context 'with forked project' do + let(:source_project) { forked_project } + + it_behaves_like 'maintained MWPS' + end + end + end end |