diff options
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/assets/javascripts/project_new.js.coffee | 19 | ||||
-rw-r--r-- | app/controllers/projects_controller.rb | 2 | ||||
-rw-r--r-- | app/models/merge_request.rb | 17 | ||||
-rw-r--r-- | app/views/projects/_merge_request_settings.html.haml | 11 | ||||
-rw-r--r-- | app/views/projects/edit.html.haml | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/widget/_open.html.haml | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/widget/open/_accept.html.haml | 27 | ||||
-rw-r--r-- | app/views/projects/merge_requests/widget/open/_build_failed.html.haml | 6 | ||||
-rw-r--r-- | db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb | 15 | ||||
-rw-r--r-- | db/schema.rb | 35 | ||||
-rw-r--r-- | doc/workflow/merge_requests.md | 11 | ||||
-rw-r--r-- | doc/workflow/merge_requests/only_allow_merge_if_build_succeeds.png | bin | 0 -> 17552 bytes | |||
-rw-r--r-- | lib/api/merge_requests.rb | 5 | ||||
-rw-r--r-- | spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb | 105 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 153 | ||||
-rw-r--r-- | spec/requests/api/merge_requests_spec.rb | 9 |
17 files changed, 378 insertions, 42 deletions
diff --git a/CHANGELOG b/CHANGELOG index b00c149a753..176fc16943f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -31,6 +31,7 @@ v 8.9.0 (unreleased) - Add rake task 'gitlab:db:configure' for conditionally seeding or migrating the database - Changed the Slack build message to use the singular duration if necessary (Aran Koning) - Links from a wiki page to other wiki pages should be rewritten as expected + - Add option to project to only allow merge requests to be merged if the build succeeds (Rui Santos) - Fix issues filter when ordering by milestone - Todos will display target state if issuable target is 'Closed' or 'Merged' - Fix bug when sorting issues by milestone due date and filtering by two or more labels diff --git a/app/assets/javascripts/project_new.js.coffee b/app/assets/javascripts/project_new.js.coffee index 63dee4ed5d7..e48343a19b5 100644 --- a/app/assets/javascripts/project_new.js.coffee +++ b/app/assets/javascripts/project_new.js.coffee @@ -7,12 +7,17 @@ class @ProjectNew @toggleSettingsOnclick() - toggleSettings: -> - checked = $("#project_builds_enabled").prop("checked") - if checked - $('.builds-feature').show() - else - $('.builds-feature').hide() + toggleSettings: => + @_showOrHide('#project_builds_enabled', '.builds-feature') + @_showOrHide('#project_merge_requests_enabled', '.merge-requests-feature') toggleSettingsOnclick: -> - $("#project_builds_enabled").on 'click', @toggleSettings + $('#project_builds_enabled, #project_merge_requests_enabled').on 'click', @toggleSettings + + _showOrHide: (checkElement, container) -> + $container = $(container) + + if $(checkElement).prop('checked') + $container.show() + else + $container.hide() diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 3af62c7696c..a6479c42d94 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -234,7 +234,7 @@ class ProjectsController < Projects::ApplicationController :issues_tracker_id, :default_branch, :wiki_enabled, :visibility_level, :import_url, :last_activity_at, :namespace_id, :avatar, :builds_enabled, :build_allow_git_fetch, :build_timeout_in_minutes, :build_coverage_regex, - :public_builds, + :public_builds, :only_allow_merge_if_build_succeeds ) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b0ed8182855..29f36570912 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -260,13 +260,22 @@ class MergeRequest < ActiveRecord::Base end def mergeable? - return false unless open? && !work_in_progress? && !broken? + return false unless mergeable_state? check_if_can_be_merged can_be_merged? end + def mergeable_state? + return false unless open? + return false if work_in_progress? + return false if broken? + return false unless mergeable_ci_state? + + true + end + def gitlab_merge_status if work_in_progress? "work_in_progress" @@ -481,6 +490,12 @@ class MergeRequest < ActiveRecord::Base ::Gitlab::GitAccess.new(user, project).can_push_to_branch?(target_branch) end + def mergeable_ci_state? + return true unless project.only_allow_merge_if_build_succeeds? + + !pipeline || pipeline.success? + end + def state_human_name if merged? "Merged" diff --git a/app/views/projects/_merge_request_settings.html.haml b/app/views/projects/_merge_request_settings.html.haml new file mode 100644 index 00000000000..da522b53417 --- /dev/null +++ b/app/views/projects/_merge_request_settings.html.haml @@ -0,0 +1,11 @@ +%fieldset.builds-feature + %h5.prepend-top-0 + Merge Requests + .form-group + .checkbox + = f.label :only_allow_merge_if_build_succeeds do + = f.check_box :only_allow_merge_if_build_succeeds + %strong Only allow merge requests to be merged if the build succeeds + .help-block + Builds need to be configured to enable this feature. + = link_to icon('question-circle'), help_page_path('workflow', 'merge_requests#only-allow-merge-requests-to-be-merged-if-the-build-succeeds') diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 18b125ff9d4..8449fe1e4e0 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -84,6 +84,8 @@ %br %span.descr Enable Container Registry for this repository %hr + = render 'merge_request_settings', f: f + %hr = render 'builds_settings', f: f %hr %fieldset.features.append-bottom-default diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index 13359abede7..0e0af57d76e 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/app/views/projects/merge_requests/widget/_open.html.haml @@ -17,6 +17,8 @@ = render 'projects/merge_requests/widget/open/merge_when_build_succeeds' - elsif !@merge_request.can_be_merged_by?(current_user) = render 'projects/merge_requests/widget/open/not_allowed' + - elsif !@merge_request.mergeable_ci_state? && @pipeline && @pipeline.failed? + = render 'projects/merge_requests/widget/open/build_failed' - elsif @merge_request.can_be_merged? = render 'projects/merge_requests/widget/open/accept' diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml index 60d7d6ff1f5..941513febbd 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -10,19 +10,20 @@ %span.btn-group = button_tag class: "btn btn-create js-merge-button merge_when_build_succeeds" do Merge When Build Succeeds - = button_tag class: "btn btn-success dropdown-toggle", 'data-toggle' => 'dropdown' do - %span.caret - %span.sr-only - Select Merge Moment - %ul.js-merge-dropdown.dropdown-menu.dropdown-menu-right{ role: 'menu' } - %li - = link_to "#", class: "merge_when_build_succeeds" do - = icon('check fw') - Merge When Build Succeeds - %li - = link_to "#", class: "accept_merge_request" do - = icon('warning fw') - Merge Immediately + - unless @project.only_allow_merge_if_build_succeeds? + = button_tag class: "btn btn-success dropdown-toggle", 'data-toggle' => 'dropdown' do + %span.caret + %span.sr-only + Select Merge Moment + %ul.js-merge-dropdown.dropdown-menu.dropdown-menu-right{ role: 'menu' } + %li + = link_to "#", class: "merge_when_build_succeeds" do + = icon('check fw') + Merge When Build Succeeds + %li + = link_to "#", class: "accept_merge_request" do + = icon('warning fw') + Merge Immediately - else = f.button class: "btn btn-create btn-grouped js-merge-button accept_merge_request #{status_class}" do Accept Merge Request diff --git a/app/views/projects/merge_requests/widget/open/_build_failed.html.haml b/app/views/projects/merge_requests/widget/open/_build_failed.html.haml new file mode 100644 index 00000000000..14f51af5360 --- /dev/null +++ b/app/views/projects/merge_requests/widget/open/_build_failed.html.haml @@ -0,0 +1,6 @@ +%h4 + = icon('exclamation-triangle') + The build for this merge request failed + +%p + Please retry the build or push a new commit to fix the failure. diff --git a/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb b/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb new file mode 100644 index 00000000000..69d64ccd006 --- /dev/null +++ b/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb @@ -0,0 +1,15 @@ +class AddOnlyAllowMergeIfBuildSucceedsToProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + def up + add_column_with_default(:projects, + :only_allow_merge_if_build_succeeds, + :boolean, + default: false) + end + + def down + remove_column(:projects, :only_allow_merge_if_build_succeeds) + end +end diff --git a/db/schema.rb b/db/schema.rb index b7adf48fdb4..09ff52ccc8e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -747,38 +747,39 @@ ActiveRecord::Schema.define(version: 20160608155312) do t.datetime "created_at" t.datetime "updated_at" t.integer "creator_id" - t.boolean "issues_enabled", default: true, null: false - t.boolean "merge_requests_enabled", default: true, null: false - t.boolean "wiki_enabled", default: true, null: false + t.boolean "issues_enabled", default: true, null: false + t.boolean "merge_requests_enabled", default: true, null: false + t.boolean "wiki_enabled", default: true, null: false t.integer "namespace_id" - t.string "issues_tracker", default: "gitlab", null: false + t.string "issues_tracker", default: "gitlab", null: false t.string "issues_tracker_id" - t.boolean "snippets_enabled", default: true, null: false + t.boolean "snippets_enabled", default: true, null: false t.datetime "last_activity_at" t.string "import_url" - t.integer "visibility_level", default: 0, null: false - t.boolean "archived", default: false, null: false + t.integer "visibility_level", default: 0, null: false + t.boolean "archived", default: false, null: false t.string "avatar" t.string "import_status" - t.float "repository_size", default: 0.0 - t.integer "star_count", default: 0, null: false + t.float "repository_size", default: 0.0 + t.integer "star_count", default: 0, null: false t.string "import_type" t.string "import_source" - t.integer "commit_count", default: 0 + t.integer "commit_count", default: 0 t.text "import_error" t.integer "ci_id" - t.boolean "builds_enabled", default: true, null: false - t.boolean "shared_runners_enabled", default: true, null: false + t.boolean "builds_enabled", default: true, null: false + t.boolean "shared_runners_enabled", default: true, null: false t.string "runners_token" t.string "build_coverage_regex" - t.boolean "build_allow_git_fetch", default: true, null: false - t.integer "build_timeout", default: 3600, null: false - t.boolean "pending_delete", default: false - t.boolean "public_builds", default: true, null: false - t.integer "pushes_since_gc", default: 0 + t.boolean "build_allow_git_fetch", default: true, null: false + t.integer "build_timeout", default: 3600, null: false + t.boolean "pending_delete", default: false + t.boolean "public_builds", default: true, null: false + t.integer "pushes_since_gc", default: 0 t.boolean "last_repository_check_failed" t.datetime "last_repository_check_at" t.boolean "container_registry_enabled" + t.boolean "only_allow_merge_if_build_succeeds", default: false, null: false end add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree diff --git a/doc/workflow/merge_requests.md b/doc/workflow/merge_requests.md index 1b5718c91c1..d2ec56e6504 100644 --- a/doc/workflow/merge_requests.md +++ b/doc/workflow/merge_requests.md @@ -2,6 +2,17 @@ Merge requests allow you to exchange changes you made to source code +## Only allow merge requests to be merged if the build succeeds + +You can prevent merge requests from being merged if their build did not succeed +in the project settings page. + +![only_allow_merge_if_build_succeeds](merge_requests/only_allow_merge_if_build_succeeds.png) + +Navigate to project settings page and select the `Only allow merge requests to be merged if the build succeeds` check box. + +Please note that you need to have builds configured to enable this feature. + ## Checkout merge requests locally Locate the section for your GitLab remote in the `.git/config` file. It looks like this: diff --git a/doc/workflow/merge_requests/only_allow_merge_if_build_succeeds.png b/doc/workflow/merge_requests/only_allow_merge_if_build_succeeds.png Binary files differnew file mode 100644 index 00000000000..18bebf5fe92 --- /dev/null +++ b/doc/workflow/merge_requests/only_allow_merge_if_build_succeeds.png diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 43221d5622a..24df3e397e0 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -228,11 +228,10 @@ module API # Merge request can not be merged # because user dont have permissions to push into target branch unauthorized! unless merge_request.can_be_merged_by?(current_user) - not_allowed! if !merge_request.open? || merge_request.work_in_progress? - merge_request.check_if_can_be_merged + not_allowed! unless merge_request.mergeable_state? - render_api_error!('Branch cannot be merged', 406) unless merge_request.can_be_merged? + render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable? if params[:sha] && merge_request.source_sha != params[:sha] render_api_error!("SHA does not match HEAD of source branch: #{merge_request.source_sha}", 409) diff --git a/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb b/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb new file mode 100644 index 00000000000..65e9185ec24 --- /dev/null +++ b/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb @@ -0,0 +1,105 @@ +require 'spec_helper' + +feature 'Only allow merge requests to be merged if the build succeeds', feature: true do + let(:project) { create(:project, :public) } + let(:merge_request) { create(:merge_request_with_diffs, source_project: project) } + + before do + login_as merge_request.author + + project.team << [merge_request.author, :master] + end + + context 'project does not have CI enabled' do + it 'allows MR to be merged' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' + end + end + + context 'when project has CI enabled' do + let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) } + + context 'when merge requests can only be merged if the build succeeds' do + before do + project.update_attribute(:only_allow_merge_if_build_succeeds, true) + end + + context 'when CI is running' do + before { pipeline.update_column(:status, :running) } + + it 'does not allow to merge immediately' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Merge When Build Succeeds' + expect(page).not_to have_button 'Select Merge Moment' + end + end + + context 'when CI failed' do + before { pipeline.update_column(:status, :failed) } + + it 'does not allow MR to be merged' do + visit_merge_request(merge_request) + + expect(page).not_to have_button 'Accept Merge Request' + expect(page).to have_content('Please retry the build or push a new commit to fix the failure.') + end + end + + context 'when CI succeeded' do + before { pipeline.update_column(:status, :success) } + + it 'allows MR to be merged' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' + end + end + end + + context 'when merge requests can be merged when the build failed' do + before do + project.update_attribute(:only_allow_merge_if_build_succeeds, false) + end + + context 'when CI is running' do + before { pipeline.update_column(:status, :running) } + + it 'allows MR to be merged immediately', js: true do + visit_merge_request(merge_request) + + expect(page).to have_button 'Merge When Build Succeeds' + + click_button 'Select Merge Moment' + expect(page).to have_content 'Merge Immediately' + end + end + + context 'when CI failed' do + before { pipeline.update_column(:status, :failed) } + + it 'allows MR to be merged' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' + end + end + + context 'when CI succeeded' do + before { pipeline.update_column(:status, :success) } + + it 'allows MR to be merged' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' + end + end + end + end + + def visit_merge_request(merge_request) + visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request) + end +end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 1b7cbc3efda..3b199f4d98d 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -455,4 +455,157 @@ describe MergeRequest, models: true do expect(user2.assigned_open_merge_request_count).to eq(1) end end + + describe '#check_if_can_be_merged' do + let(:project) { create(:project, only_allow_merge_if_build_succeeds: true) } + + subject { create(:merge_request, source_project: project, merge_status: :unchecked) } + + context 'when it is not broken and has no conflicts' do + it 'is marked as mergeable' do + allow(subject).to receive(:broken?) { false } + allow(project).to receive_message_chain(:repository, :can_be_merged?) { true } + + expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged') + end + end + + context 'when broken' do + before { allow(subject).to receive(:broken?) { true } } + + it 'becomes unmergeable' do + expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') + end + end + + context 'when it has conflicts' do + before do + allow(subject).to receive(:broken?) { false } + allow(project).to receive_message_chain(:repository, :can_be_merged?) { false } + end + + it 'becomes unmergeable' do + expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') + end + end + end + + describe '#mergeable?' do + let(:project) { create(:project) } + + subject { create(:merge_request, source_project: project) } + + it 'returns false if #mergeable_state? is false' do + expect(subject).to receive(:mergeable_state?) { false } + + expect(subject.mergeable?).to be_falsey + end + + it 'return true if #mergeable_state? is true and the MR #can_be_merged? is true' do + allow(subject).to receive(:mergeable_state?) { true } + expect(subject).to receive(:check_if_can_be_merged) + expect(subject).to receive(:can_be_merged?) { true } + + expect(subject.mergeable?).to be_truthy + end + end + + describe '#mergeable_state?' do + let(:project) { create(:project) } + + subject { create(:merge_request, source_project: project) } + + it 'checks if merge request can be merged' do + allow(subject).to receive(:mergeable_ci_state?) { true } + expect(subject).to receive(:check_if_can_be_merged) + + subject.mergeable? + end + + context 'when not open' do + before { subject.close } + + it 'returns false' do + expect(subject.mergeable_state?).to be_falsey + end + end + + context 'when working in progress' do + before { subject.title = 'WIP MR' } + + it 'returns false' do + expect(subject.mergeable_state?).to be_falsey + end + end + + context 'when broken' do + before { allow(subject).to receive(:broken?) { true } } + + it 'returns false' do + expect(subject.mergeable_state?).to be_falsey + end + end + + context 'when failed' do + before { allow(subject).to receive(:broken?) { false } } + + context 'when project settings restrict to merge only if build succeeds and build failed' do + before do + project.only_allow_merge_if_build_succeeds = true + allow(subject).to receive(:mergeable_ci_state?) { false } + end + + it 'returns false' do + expect(subject.mergeable_state?).to be_falsey + end + end + end + end + + describe '#mergeable_ci_state?' do + let(:project) { create(:empty_project, only_allow_merge_if_build_succeeds: true) } + let(:pipeline) { create(:ci_empty_pipeline) } + + subject { build(:merge_request, target_project: project) } + + context 'when it is only allowed to merge when build is green' do + context 'and a failed pipeline is associated' do + before do + pipeline.statuses << create(:commit_status, status: 'failed', project: project) + allow(subject).to receive(:pipeline) { pipeline } + end + + it { expect(subject.mergeable_ci_state?).to be_falsey } + end + + context 'when no pipeline is associated' do + before do + allow(subject).to receive(:pipeline) { nil } + end + + it { expect(subject.mergeable_ci_state?).to be_truthy } + end + end + + context 'when merges are not restricted to green builds' do + subject { build(:merge_request, target_project: build(:empty_project, only_allow_merge_if_build_succeeds: false)) } + + context 'and a failed pipeline is associated' do + before do + pipeline.statuses << create(:commit_status, status: 'failed', project: project) + allow(subject).to receive(:pipeline) { pipeline } + end + + it { expect(subject.mergeable_ci_state?).to be_truthy } + end + + context 'when no pipeline is associated' do + before do + allow(subject).to receive(:pipeline) { nil } + end + + it { expect(subject.mergeable_ci_state?).to be_truthy } + end + end + end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 9da69a913a8..1356f87b0e9 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -419,6 +419,15 @@ describe API::API, api: true do expect(json_response['message']).to eq('405 Method Not Allowed') end + it 'returns 405 if the build failed for a merge request that requires success' do + allow_any_instance_of(MergeRequest).to receive(:mergeable_ci_state?).and_return(false) + + put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user) + + expect(response.status).to eq(405) + expect(json_response['message']).to eq('405 Method Not Allowed') + end + it "should return 401 if user has no permissions to merge" do user2 = create(:user) project.team << [user2, :reporter] |