diff options
28 files changed, 399 insertions, 105 deletions
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 2f4c74eb2dd..46448c71b9d 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.56.0 +0.57.0 @@ -400,7 +400,7 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 0.58.0', require: 'gitaly' +gem 'gitaly-proto', '~> 0.59.0', require: 'gitaly' gem 'toml-rb', '~> 0.3.15', require: false diff --git a/Gemfile.lock b/Gemfile.lock index fdb81b101f5..48e46e3fab0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -276,7 +276,7 @@ GEM po_to_json (>= 1.0.0) rails (>= 3.2.0) gherkin-ruby (0.3.2) - gitaly-proto (0.58.0) + gitaly-proto (0.59.0) google-protobuf (~> 3.1) grpc (~> 1.0) github-linguist (4.7.6) @@ -1037,7 +1037,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.2.0) - gitaly-proto (~> 0.58.0) + gitaly-proto (~> 0.59.0) github-linguist (~> 4.7.0) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-markup (~> 1.6.2) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 30d5d7a653b..8d83554d813 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -608,7 +608,7 @@ } .dropdown-content { - max-height: 215px; + max-height: $dropdown-max-height; overflow-y: auto; } @@ -1030,3 +1030,28 @@ header.header-content .dropdown-menu.projects-dropdown-menu { } } } + +.dropdown-content-faded-mask { + position: relative; + + .dropdown-list { + max-height: $dropdown-max-height; + overflow-y: auto; + position: relative; + } + + &::after { + height: $dropdown-fade-mask-height; + width: 100%; + position: absolute; + bottom: 0; + background: linear-gradient(to top, $white-light 0, rgba($white-light, 0)); + transition: opacity $fade-mask-transition-duration $fade-mask-transition-curve; + content: ''; + pointer-events: none; + } + + &.fade-out::after { + opacity: 0; + } +} diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index cb2a237f574..6525b39d55c 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -337,6 +337,7 @@ $regular_font: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Oxygen-San * Dropdowns */ $dropdown-width: 300px; +$dropdown-max-height: 215px; $dropdown-vertical-offset: 4px; $dropdown-link-color: #555; $dropdown-link-hover-bg: $row-hover; @@ -353,6 +354,7 @@ $dropdown-loading-bg: rgba(#fff, .6); $dropdown-chevron-size: 10px; $dropdown-toggle-active-border-color: darken($border-color, 14%); $dropdown-item-hover-bg: $gray-darker; +$dropdown-fade-mask-height: 32px; /* * Filtered Search @@ -564,6 +566,8 @@ $label-border-radius: 100px; * Animation */ $fade-in-duration: 200ms; +$fade-mask-transition-duration: .1s; +$fade-mask-transition-curve: ease-in-out; /* * Lint diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index abe4e5245b1..37acd1c9787 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -283,15 +283,15 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo @merge_request.update(merge_error: nil) if params[:merge_when_pipeline_succeeds].present? - return :failed unless @merge_request.head_pipeline + return :failed unless @merge_request.actual_head_pipeline - if @merge_request.head_pipeline.active? + if @merge_request.actual_head_pipeline.active? ::MergeRequests::MergeWhenPipelineSucceedsService .new(@project, current_user, merge_params) .execute(@merge_request) :merge_when_pipeline_succeeds - elsif @merge_request.head_pipeline.success? + elsif @merge_request.actual_head_pipeline.success? # This can be triggered when a user clicks the auto merge button while # the tests finish at about the same time @merge_request.merge_async(current_user.id, params) diff --git a/app/helpers/button_helper.rb b/app/helpers/button_helper.rb index d06cf2de2c3..3605d6a3c95 100644 --- a/app/helpers/button_helper.rb +++ b/app/helpers/button_helper.rb @@ -84,7 +84,7 @@ module ButtonHelper button_content << content_tag(:span, description, class: 'dropdown-menu-inner-content') if description content_tag (href ? :a : :span), - button_content, + (href ? button_content : title), class: "#{title.downcase}-selector", href: (href if href) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index bbc01e9677c..f2d639a3382 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -145,6 +145,13 @@ class MergeRequest < ActiveRecord::Base '!' end + # Use this method whenever you need to make sure the head_pipeline is synced with the + # branch head commit, for example checking if a merge request can be merged. + # For more information check: https://gitlab.com/gitlab-org/gitlab-ce/issues/40004 + def actual_head_pipeline + head_pipeline&.sha == diff_head_sha ? head_pipeline : nil + end + # Pattern used to extract `!123` merge request references from text # # This pattern supports cross-project references. @@ -822,8 +829,9 @@ class MergeRequest < ActiveRecord::Base def mergeable_ci_state? return true unless project.only_allow_merge_if_pipeline_succeeds? + return true unless head_pipeline - !head_pipeline || head_pipeline.success? || head_pipeline.skipped? + actual_head_pipeline&.success? || actual_head_pipeline&.skipped? end def environments_for(current_user) @@ -997,7 +1005,7 @@ class MergeRequest < ActiveRecord::Base return true if autocomplete_precheck return false unless mergeable?(skip_ci_check: true) - return false if head_pipeline && !(head_pipeline.success? || head_pipeline.active?) + return false if actual_head_pipeline && !(actual_head_pipeline.success? || actual_head_pipeline.active?) return false if last_diff_sha != diff_head_sha true diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index a25882cbb62..ab4c87c0169 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -163,7 +163,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end def pipeline - @pipeline ||= head_pipeline + @pipeline ||= actual_head_pipeline end def issues_sentence(project, issues) diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_entity.rb index b53a49fe59e..eece9445dca 100644 --- a/app/serializers/merge_request_entity.rb +++ b/app/serializers/merge_request_entity.rb @@ -33,7 +33,7 @@ class MergeRequestEntity < IssuableEntity end expose :merge_commit_message - expose :head_pipeline, with: PipelineDetailsEntity, as: :pipeline + expose :actual_head_pipeline, with: PipelineDetailsEntity, as: :pipeline # Booleans expose :merge_ongoing?, as: :merge_ongoing diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 7b9ea223d26..1e5f2ed4dd2 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -29,7 +29,7 @@ module Ci .new(pipeline, command, SEQUENCE) sequence.build! do |pipeline, sequence| - update_merge_requests_head_pipeline if pipeline.persisted? + schedule_head_pipeline_update if sequence.complete? cancel_pending_pipelines if project.auto_cancel_pending_pipelines? @@ -38,15 +38,18 @@ module Ci pipeline.process! end end + + pipeline end private - def update_merge_requests_head_pipeline - return unless pipeline.latest? + def commit + @commit ||= project.commit(origin_sha || origin_ref) + end - MergeRequest.where(source_project: @pipeline.project, source_branch: @pipeline.ref) - .update_all(head_pipeline_id: @pipeline.id) + def sha + commit.try(:id) end def cancel_pending_pipelines @@ -69,5 +72,15 @@ module Ci @pipeline_created_counter ||= Gitlab::Metrics .counter(:pipelines_created_total, "Counter of pipelines created") end + + def schedule_head_pipeline_update + related_merge_requests.each do |merge_request| + UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id) + end + end + + def related_merge_requests + MergeRequest.where(source_project: pipeline.project, source_branch: pipeline.ref) + end end end diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index bf3d4855122..434dda89db0 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -76,6 +76,7 @@ module MergeRequests end merge_request.mark_as_unchecked + UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id) end end diff --git a/app/views/shared/_clone_panel.html.haml b/app/views/shared/_clone_panel.html.haml index fba08092351..1cba4fc6c41 100644 --- a/app/views/shared/_clone_panel.html.haml +++ b/app/views/shared/_clone_panel.html.haml @@ -3,7 +3,7 @@ .git-clone-holder.input-group .input-group-btn - if allowed_protocols_present? - .clone-dropdown-btn.btn.btn-static + .clone-dropdown-btn.btn %span = enabled_project_button(project, enabled_protocol) - else diff --git a/app/workers/update_head_pipeline_for_merge_request_worker.rb b/app/workers/update_head_pipeline_for_merge_request_worker.rb new file mode 100644 index 00000000000..0a2e9b63578 --- /dev/null +++ b/app/workers/update_head_pipeline_for_merge_request_worker.rb @@ -0,0 +1,15 @@ +class UpdateHeadPipelineForMergeRequestWorker + include ApplicationWorker + + sidekiq_options queue: 'pipeline_default' + + def perform(merge_request_id) + merge_request = MergeRequest.find(merge_request_id) + pipeline = Ci::Pipeline.where(project: merge_request.source_project, ref: merge_request.source_branch).last + + return unless pipeline && pipeline.latest? + raise ArgumentError, 'merge request sha does not equal pipeline sha' if merge_request.diff_head_sha != pipeline.sha + + merge_request.update_attribute(:head_pipeline_id, pipeline.id) + end +end diff --git a/changelogs/unreleased/37354-pipelines-update.yml b/changelogs/unreleased/37354-pipelines-update.yml new file mode 100644 index 00000000000..2b6ddfe95ed --- /dev/null +++ b/changelogs/unreleased/37354-pipelines-update.yml @@ -0,0 +1,5 @@ +--- +title: Make sure head pippeline always corresponds with the head sha of an MR +merge_request: +author: +type: fixed diff --git a/doc/user/permissions.md b/doc/user/permissions.md index b9532bf897f..4fa83388d0c 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -23,25 +23,26 @@ The following table depicts the various user permission levels in a project. |---------------------------------------|---------|------------|-------------|----------|--------| | Create new issue | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | | Create confidential issue | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | -| View confidential issues | (✓) [^2] | ✓ | ✓ | ✓ | ✓ | +| View confidential issues | (✓) [^2] | ✓ | ✓ | ✓ | ✓ | | Leave comments | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | | Lock discussions (issues and merge requests) | | | | ✓ | ✓ | | See a list of jobs | ✓ [^3] | ✓ | ✓ | ✓ | ✓ | -| See a job log | ✓ [^3] | ✓ | ✓ | ✓ | ✓ | +| See a job log | ✓ [^3] | ✓ | ✓ | ✓ | ✓ | | Download and browse job artifacts | ✓ [^3] | ✓ | ✓ | ✓ | ✓ | | View wiki pages | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | | Pull project code | [^1] | ✓ | ✓ | ✓ | ✓ | | Download project | [^1] | ✓ | ✓ | ✓ | ✓ | +| Assign issues and merge requests | | ✓ | ✓ | ✓ | ✓ | +| Label issues and merge requests | | ✓ | ✓ | ✓ | ✓ | | Create code snippets | | ✓ | ✓ | ✓ | ✓ | | Manage issue tracker | | ✓ | ✓ | ✓ | ✓ | | Manage labels | | ✓ | ✓ | ✓ | ✓ | | See a commit status | | ✓ | ✓ | ✓ | ✓ | | See a container registry | | ✓ | ✓ | ✓ | ✓ | | See environments | | ✓ | ✓ | ✓ | ✓ | +| See a list of merge requests | | ✓ | ✓ | ✓ | ✓ | | Create new environments | | | ✓ | ✓ | ✓ | -| Use environment terminals | | | | ✓ | ✓ | | Stop environments | | | ✓ | ✓ | ✓ | -| See a list of merge requests | | ✓ | ✓ | ✓ | ✓ | | Manage/Accept merge requests | | | ✓ | ✓ | ✓ | | Create new merge request | | | ✓ | ✓ | ✓ | | Create new branches | | | ✓ | ✓ | ✓ | @@ -55,10 +56,11 @@ The following table depicts the various user permission levels in a project. | Update a container registry | | | ✓ | ✓ | ✓ | | Remove a container registry image | | | ✓ | ✓ | ✓ | | Create/edit/delete project milestones | | | ✓ | ✓ | ✓ | +| Use environment terminals | | | | ✓ | ✓ | | Add new team members | | | | ✓ | ✓ | | Push to protected branches | | | | ✓ | ✓ | | Enable/disable branch protection | | | | ✓ | ✓ | -| Turn on/off protected branch push for devs| | | | ✓ | ✓ | +| Turn on/off protected branch push for devs| | | | ✓ | ✓ | | Enable/disable tag protections | | | | ✓ | ✓ | | Rewrite/remove Git tags | | | | ✓ | ✓ | | Edit project | | | | ✓ | ✓ | @@ -69,14 +71,15 @@ The following table depicts the various user permission levels in a project. | Manage variables | | | | ✓ | ✓ | | Manage pages | | | | ✓ | ✓ | | Manage pages domains and certificates | | | | ✓ | ✓ | +| Manage clusters | | | | ✓ | ✓ | +| Edit comments (posted by any user) | | | | ✓ | ✓ | | Switch visibility level | | | | | ✓ | | Transfer project to another namespace | | | | | ✓ | | Remove project | | | | | ✓ | | Delete issues | | | | | ✓ | +| Remove pages | | | | | ✓ | | Force push to protected branches [^4] | | | | | | | Remove protected branches [^4] | | | | | | -| Remove pages | | | | | ✓ | -| Manage clusters | | | | ✓ | ✓ | ## Project features permissions diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index eab04bcac65..1468069a991 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -776,24 +776,21 @@ module Gitlab end def revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) - OperationService.new(user, self).with_branch( - branch_name, - start_branch_name: start_branch_name, - start_repository: start_repository - ) do |start_commit| - - Gitlab::Git.check_namespace!(commit, start_repository) - - revert_tree_id = check_revert_content(commit, start_commit.sha) - raise CreateTreeError unless revert_tree_id - - committer = user_to_committer(user) + gitaly_migrate(:revert) do |is_enabled| + args = { + user: user, + commit: commit, + branch_name: branch_name, + message: message, + start_branch_name: start_branch_name, + start_repository: start_repository + } - create_commit(message: message, - author: committer, - committer: committer, - tree: revert_tree_id, - parents: [start_commit.sha]) + if is_enabled + gitaly_operations_client.user_revert(args) + else + rugged_revert(args) + end end end @@ -1769,6 +1766,28 @@ module Gitlab end end + def rugged_revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) + OperationService.new(user, self).with_branch( + branch_name, + start_branch_name: start_branch_name, + start_repository: start_repository + ) do |start_commit| + + Gitlab::Git.check_namespace!(commit, start_repository) + + revert_tree_id = check_revert_content(commit, start_commit.sha) + raise CreateTreeError unless revert_tree_id + + committer = user_to_committer(user) + + create_commit(message: message, + author: committer, + committer: committer, + tree: revert_tree_id, + parents: [start_commit.sha]) + end + end + def gitaly_add_branch(branch_name, user, target) gitaly_operation_client.user_create_branch(branch_name, user, target) rescue GRPC::FailedPrecondition => ex diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index 51de6a9a75d..400a4af363b 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -124,7 +124,31 @@ module Gitlab end def user_cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) - request = Gitaly::UserCherryPickRequest.new( + call_cherry_pick_or_revert(:cherry_pick, + user: user, + commit: commit, + branch_name: branch_name, + message: message, + start_branch_name: start_branch_name, + start_repository: start_repository) + end + + def user_revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) + call_cherry_pick_or_revert(:revert, + user: user, + commit: commit, + branch_name: branch_name, + message: message, + start_branch_name: start_branch_name, + start_repository: start_repository) + end + + private + + def call_cherry_pick_or_revert(rpc, user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) + request_class = "Gitaly::User#{rpc.to_s.camelcase}Request".constantize + + request = request_class.new( repository: @gitaly_repo, user: Gitlab::Git::User.from_gitlab(user).to_gitaly, commit: commit.to_gitaly_commit, @@ -137,11 +161,15 @@ module Gitlab response = GitalyClient.call( @repository.storage, :operation_service, - :user_cherry_pick, + :"user_#{rpc}", request, remote_storage: start_repository.storage ) + handle_cherry_pick_or_revert_response(response) + end + + def handle_cherry_pick_or_revert_response(response) if response.pre_receive_error.presence raise Gitlab::Git::HooksService::PreReceiveError, response.pre_receive_error elsif response.commit_error.presence diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index bfdad85c082..51d5d6a52b3 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -324,12 +324,12 @@ describe Projects::MergeRequestsController do end context 'when the pipeline succeeds is passed' do - def merge_when_pipeline_succeeds - post :merge, base_params.merge(sha: merge_request.diff_head_sha, merge_when_pipeline_succeeds: '1') + let!(:head_pipeline) do + create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request) end - before do - create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request) + def merge_when_pipeline_succeeds + post :merge, base_params.merge(sha: merge_request.diff_head_sha, merge_when_pipeline_succeeds: '1') end it 'returns :merge_when_pipeline_succeeds' do @@ -354,6 +354,18 @@ describe Projects::MergeRequestsController do project.update_column(:only_allow_merge_if_pipeline_succeeds, true) end + context 'and head pipeline is not the current one' do + before do + head_pipeline.update(sha: 'not_current_sha') + end + + it 'returns :failed' do + merge_when_pipeline_succeeds + + expect(json_response).to eq('status' => 'failed') + end + end + it 'returns :merge_when_pipeline_succeeds' do merge_when_pipeline_succeeds diff --git a/spec/features/merge_requests/pipelines_spec.rb b/spec/features/merge_requests/pipelines_spec.rb index a3fcc27cab0..307c860eac4 100644 --- a/spec/features/merge_requests/pipelines_spec.rb +++ b/spec/features/merge_requests/pipelines_spec.rb @@ -20,10 +20,14 @@ feature 'Pipelines for Merge Requests', :js do end before do - visit project_merge_request_path(project, merge_request) + merge_request.update_attribute(:head_pipeline_id, pipeline.id) end scenario 'user visits merge request pipelines tab' do + visit project_merge_request_path(project, merge_request) + + expect(page.find('.ci-widget')).to have_content('pending') + page.within('.merge-request-tabs') do click_link('Pipelines') end @@ -31,6 +35,15 @@ feature 'Pipelines for Merge Requests', :js do expect(page).to have_selector('.stage-cell') end + + scenario 'pipeline sha does not equal last commit sha' do + pipeline.update_attribute(:sha, '19e2e9b4ef76b422ce1154af39a91323ccc57434') + visit project_merge_request_path(project, merge_request) + wait_for_requests + + expect(page.find('.ci-widget')).to have_content( + 'Could not connect to the CI server. Please check your settings and try again') + end end context 'without pipelines' do diff --git a/spec/helpers/button_helper_spec.rb b/spec/helpers/button_helper_spec.rb index d2c7867febb..fee8df10129 100644 --- a/spec/helpers/button_helper_spec.rb +++ b/spec/helpers/button_helper_spec.rb @@ -92,6 +92,34 @@ describe ButtonHelper do end end + describe 'ssh and http clone buttons' do + let(:user) { create(:user) } + let(:project) { build_stubbed(:project) } + + def http_button_element + element = helper.http_clone_button(project, append_link: false) + + Nokogiri::HTML::DocumentFragment.parse(element).first_element_child + end + + def ssh_button_element + element = helper.ssh_clone_button(project, append_link: false) + + Nokogiri::HTML::DocumentFragment.parse(element).first_element_child + end + + before do + allow(helper).to receive(:current_user).and_return(user) + end + + it 'only shows the title of any of the clone buttons when append_link is false' do + expect(http_button_element.text).to eq('HTTP') + expect(http_button_element.search('.dropdown-menu-inner-content').first).to eq(nil) + expect(ssh_button_element.text).to eq('SSH') + expect(ssh_button_element.search('.dropdown-menu-inner-content').first).to eq(nil) + end + end + describe 'clipboard_button' do let(:user) { create(:user) } let(:project) { build_stubbed(:project) } diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 728028746d8..e606fb027b5 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -827,20 +827,47 @@ describe MergeRequest do end end - describe '#head_pipeline' do - describe 'when the source project exists' do - it 'returns the latest pipeline' do - pipeline = create(:ci_empty_pipeline, project: subject.source_project, ref: 'master', status: 'running', sha: "123abc", head_pipeline_of: subject) + context 'head pipeline' do + before do + allow(subject).to receive(:diff_head_sha).and_return('lastsha') + end + + describe '#head_pipeline' do + it 'returns nil for MR without head_pipeline_id' do + subject.update_attribute(:head_pipeline_id, nil) + + expect(subject.head_pipeline).to be_nil + end - expect(subject.head_pipeline).to eq(pipeline) + context 'when the source project does not exist' do + it 'returns nil' do + allow(subject).to receive(:source_project).and_return(nil) + + expect(subject.head_pipeline).to be_nil + end end end - describe 'when the source project does not exist' do - it 'returns nil' do + describe '#actual_head_pipeline' do + it 'returns nil for MR with old pipeline' do + pipeline = create(:ci_empty_pipeline, sha: 'notlatestsha') + subject.update_attribute(:head_pipeline_id, pipeline.id) + + expect(subject.actual_head_pipeline).to be_nil + end + + it 'returns the pipeline for MR with recent pipeline' do + pipeline = create(:ci_empty_pipeline, sha: 'lastsha') + subject.update_attribute(:head_pipeline_id, pipeline.id) + + expect(subject.actual_head_pipeline).to eq(subject.head_pipeline) + expect(subject.actual_head_pipeline).to eq(pipeline) + end + + it 'returns nil when source project does not exist' do allow(subject).to receive(:source_project).and_return(nil) - expect(subject.head_pipeline).to be_nil + expect(subject.actual_head_pipeline).to be_nil end end end @@ -1179,7 +1206,7 @@ describe MergeRequest do context 'when it is only allowed to merge when build is green' do context 'and a failed pipeline is associated' do before do - pipeline.update(status: 'failed') + pipeline.update(status: 'failed', sha: subject.diff_head_sha) allow(subject).to receive(:head_pipeline) { pipeline } end @@ -1188,7 +1215,7 @@ describe MergeRequest do context 'and a successful pipeline is associated' do before do - pipeline.update(status: 'success') + pipeline.update(status: 'success', sha: subject.diff_head_sha) allow(subject).to receive(:head_pipeline) { pipeline } end @@ -1197,7 +1224,7 @@ describe MergeRequest do context 'and a skipped pipeline is associated' do before do - pipeline.update(status: 'skipped') + pipeline.update(status: 'skipped', sha: subject.diff_head_sha) allow(subject).to receive(:head_pipeline) { pipeline } end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index af0c86abe86..d37e3d2c527 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1372,39 +1372,49 @@ describe Repository do end describe '#revert' do - let(:new_image_commit) { repository.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') } - let(:update_image_commit) { repository.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } - let(:message) { 'revert message' } + shared_examples 'reverting a commit' do + let(:new_image_commit) { repository.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') } + let(:update_image_commit) { repository.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:message) { 'revert message' } - context 'when there is a conflict' do - it 'raises an error' do - expect { repository.revert(user, new_image_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) + context 'when there is a conflict' do + it 'raises an error' do + expect { repository.revert(user, new_image_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) + end end - end - context 'when commit was already reverted' do - it 'raises an error' do - repository.revert(user, update_image_commit, 'master', message) + context 'when commit was already reverted' do + it 'raises an error' do + repository.revert(user, update_image_commit, 'master', message) - expect { repository.revert(user, update_image_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) + expect { repository.revert(user, update_image_commit, 'master', message) }.to raise_error(Gitlab::Git::Repository::CreateTreeError) + end end - end - context 'when commit can be reverted' do - it 'reverts the changes' do - expect(repository.revert(user, update_image_commit, 'master', message)).to be_truthy + context 'when commit can be reverted' do + it 'reverts the changes' do + expect(repository.revert(user, update_image_commit, 'master', message)).to be_truthy + end end - end - context 'reverting a merge commit' do - it 'reverts the changes' do - merge_commit - expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).to be_present + context 'reverting a merge commit' do + it 'reverts the changes' do + merge_commit + expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).to be_present - repository.revert(user, merge_commit, 'master', message) - expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present + repository.revert(user, merge_commit, 'master', message) + expect(repository.blob_at_branch('master', 'files/ruby/feature.rb')).not_to be_present + end end end + + context 'when Gitaly revert feature is enabled' do + it_behaves_like 'reverting a commit' + end + + context 'when Gitaly revert feature is disabled', :disable_gitaly do + it_behaves_like 'reverting a commit' + end end describe '#cherry_pick' do diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb index 5e114434a67..f325d1776e4 100644 --- a/spec/presenters/merge_request_presenter_spec.rb +++ b/spec/presenters/merge_request_presenter_spec.rb @@ -31,7 +31,7 @@ describe MergeRequestPresenter do let(:pipeline) { build_stubbed(:ci_pipeline) } before do - allow(resource).to receive(:head_pipeline).and_return(pipeline) + allow(resource).to receive(:actual_head_pipeline).and_return(pipeline) end context 'success with warnings' do diff --git a/spec/serializers/merge_request_entity_spec.rb b/spec/serializers/merge_request_entity_spec.rb index f9285049c0d..1ad672fd355 100644 --- a/spec/serializers/merge_request_entity_spec.rb +++ b/spec/serializers/merge_request_entity_spec.rb @@ -5,22 +5,34 @@ describe MergeRequestEntity do let(:resource) { create(:merge_request, source_project: project, target_project: project) } let(:user) { create(:user) } - let(:request) { double('request', current_user: user) } + let(:request) { double('request', current_user: user, project: project) } subject do described_class.new(resource, request: request).as_json end - it 'includes pipeline' do - req = double('request', current_user: user) - pipeline = build_stubbed(:ci_pipeline) - allow(resource).to receive(:head_pipeline).and_return(pipeline) + describe 'pipeline' do + let(:pipeline) { create(:ci_empty_pipeline, project: project, ref: resource.source_branch, sha: resource.source_branch_sha, head_pipeline_of: resource) } - pipeline_payload = PipelineDetailsEntity - .represent(pipeline, request: req) - .as_json + context 'when is up to date' do + let(:req) { double('request', current_user: user, project: project) } - expect(subject[:pipeline]).to eq(pipeline_payload) + it 'returns pipeline' do + pipeline_payload = PipelineDetailsEntity + .represent(pipeline, request: req) + .as_json + + expect(subject[:pipeline]).to eq(pipeline_payload) + end + end + + context 'when is not up to date' do + it 'returns nil' do + pipeline.update(sha: "not up to date") + + expect(subject[:pipeline]).to be_nil + end + end end it 'includes issues_links' do diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index befd0faf1b6..b0de8d447a2 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -57,19 +57,39 @@ describe Ci::CreatePipelineService do end context 'when merge requests already exist for this source branch' do - it 'updates head pipeline of each merge request' do - merge_request_1 = create(:merge_request, source_branch: 'master', - target_branch: "branch_1", - source_project: project) + let(:merge_request_1) do + create(:merge_request, source_branch: 'master', target_branch: "branch_1", source_project: project) + end + let(:merge_request_2) do + create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project) + end - merge_request_2 = create(:merge_request, source_branch: 'master', - target_branch: "branch_2", - source_project: project) + context 'when the head pipeline sha equals merge request sha' do + it 'updates head pipeline of each merge request' do + merge_request_1 + merge_request_2 - head_pipeline = execute_service + head_pipeline = execute_service - expect(merge_request_1.reload.head_pipeline).to eq(head_pipeline) - expect(merge_request_2.reload.head_pipeline).to eq(head_pipeline) + expect(merge_request_1.reload.head_pipeline).to eq(head_pipeline) + expect(merge_request_2.reload.head_pipeline).to eq(head_pipeline) + end + end + + context 'when the head pipeline sha does not equal merge request sha' do + it 'raises the ArgumentError error from worker and does not update the head piepeline of MRs' do + merge_request_1 + merge_request_2 + + allow_any_instance_of(Ci::Pipeline).to receive(:latest?).and_return(true) + + expect { execute_service(after: 'ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }.to raise_error(ArgumentError) + + last_pipeline = Ci::Pipeline.last + + expect(merge_request_1.reload.head_pipeline).not_to eq(last_pipeline) + expect(merge_request_2.reload.head_pipeline).not_to eq(last_pipeline) + end end context 'when there is no pipeline for source branch' do @@ -106,8 +126,7 @@ describe Ci::CreatePipelineService do target_branch: "branch_1", source_project: project) - allow_any_instance_of(Ci::Pipeline) - .to receive(:latest?).and_return(false) + allow_any_instance_of(Ci::Pipeline).to receive(:latest?).and_return(false) execute_service diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index a2c05761f6b..61ec4709c59 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -74,6 +74,20 @@ describe MergeRequests::RefreshService do end end + context 'when pipeline exists for the source branch' do + let!(:pipeline) { create(:ci_empty_pipeline, ref: @merge_request.source_branch, project: @project, sha: @commits.first.sha)} + + subject { service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') } + + it 'updates the head_pipeline_id for @merge_request' do + expect { subject }.to change { @merge_request.reload.head_pipeline_id }.from(nil).to(pipeline.id) + end + + it 'does not update the head_pipeline_id for @fork_merge_request' do + expect { subject }.not_to change { @fork_merge_request.reload.head_pipeline_id } + end + end + context 'push to origin repo source branch when an MR was reopened' do let(:refresh_service) { service.new(@project, @user) } diff --git a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb new file mode 100644 index 00000000000..522e1566271 --- /dev/null +++ b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe UpdateHeadPipelineForMergeRequestWorker do + describe '#perform' do + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, source_project: project) } + let(:latest_sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' } + + context 'when pipeline exists for the source project and branch' do + before do + create(:ci_empty_pipeline, project: project, ref: merge_request.source_branch, sha: latest_sha) + end + + it 'updates the head_pipeline_id of the merge_request' do + expect { subject.perform(merge_request.id) }.to change { merge_request.reload.head_pipeline_id } + end + + context 'when merge request sha does not equal pipeline sha' do + before do + merge_request.merge_request_diff.update(head_commit_sha: 'different_sha') + end + + it 'does not update head_pipeline_id' do + expect { subject.perform(merge_request.id) }.to raise_error(ArgumentError) + + expect(merge_request.reload.head_pipeline_id).to eq(nil) + end + end + end + + context 'when pipeline does not exist for the source project and branch' do + it 'does not update the head_pipeline_id of the merge_request' do + expect { subject.perform(merge_request.id) }.not_to change { merge_request.reload.head_pipeline_id } + end + end + end +end |