From 2ccee7161a58ea04c66b216ccb57e522850f5d95 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Fri, 5 May 2017 16:00:18 -0300 Subject: Small code improvements and add migration spec --- app/services/ci/create_pipeline_service.rb | 5 ++- ...05316_add_head_pipeline_id_to_merge_requests.rb | 7 ---- ...05316_add_head_pipeline_id_to_merge_requests.rb | 7 ++++ ...547_add_head_pipeline_for_each_merge_request.rb | 25 -------------- ...547_add_head_pipeline_for_each_merge_request.rb | 25 ++++++++++++++ db/schema.rb | 3 +- spec/features/cycle_analytics_spec.rb | 6 ++-- spec/lib/gitlab/import_export/all_models.yml | 1 + ...dd_head_pipeline_for_each_merge_request_spec.rb | 33 ++++++++++++++++++ spec/models/merge_request_spec.rb | 3 +- spec/services/ci/create_pipeline_service_spec.rb | 40 +++++++++++++++++----- 11 files changed, 104 insertions(+), 51 deletions(-) delete mode 100644 db/migrate/20170427205316_add_head_pipeline_id_to_merge_requests.rb create mode 100644 db/migrate/20170507205316_add_head_pipeline_id_to_merge_requests.rb delete mode 100644 db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb create mode 100644 db/post_migrate/20170508170547_add_head_pipeline_for_each_merge_request.rb create mode 100644 spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 26b744ae3c4..1f6c1f4a7f6 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -119,9 +119,8 @@ module Ci end def update_merge_requests_head_pipeline - merge_requests = MergeRequest.where(source_branch: @pipeline.ref, source_project: @pipeline.project) - - merge_requests.update_all(head_pipeline_id: @pipeline.id) if merge_requests.any? + MergeRequest.where(source_branch: @pipeline.ref, source_project: @pipeline.project). + update_all(head_pipeline_id: @pipeline.id) end def error(message, save: false) diff --git a/db/migrate/20170427205316_add_head_pipeline_id_to_merge_requests.rb b/db/migrate/20170427205316_add_head_pipeline_id_to_merge_requests.rb deleted file mode 100644 index 8fc6e380a77..00000000000 --- a/db/migrate/20170427205316_add_head_pipeline_id_to_merge_requests.rb +++ /dev/null @@ -1,7 +0,0 @@ -class AddHeadPipelineIdToMergeRequests < ActiveRecord::Migration - DOWNTIME = false - - def change - add_column :merge_requests, :head_pipeline_id, :integer - end -end diff --git a/db/migrate/20170507205316_add_head_pipeline_id_to_merge_requests.rb b/db/migrate/20170507205316_add_head_pipeline_id_to_merge_requests.rb new file mode 100644 index 00000000000..8fc6e380a77 --- /dev/null +++ b/db/migrate/20170507205316_add_head_pipeline_id_to_merge_requests.rb @@ -0,0 +1,7 @@ +class AddHeadPipelineIdToMergeRequests < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column :merge_requests, :head_pipeline_id, :integer + end +end diff --git a/db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb b/db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb deleted file mode 100644 index bc3850c0c23..00000000000 --- a/db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb +++ /dev/null @@ -1,25 +0,0 @@ -class AddHeadPipelineForEachMergeRequest < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - def up - disable_statement_timeout - - pipelines = Arel::Table.new(:ci_pipelines) - merge_requests = Arel::Table.new(:merge_requests) - - head_id = pipelines. - project(Arel::Nodes::NamedFunction.new('max', [pipelines[:id]])). - from(pipelines). - where(pipelines[:ref].eq(merge_requests[:source_branch])). - where(pipelines[:project_id].eq(merge_requests[:source_project_id])) - - sub_query = Arel::Nodes::SqlLiteral.new(Arel::Nodes::Grouping.new(head_id).to_sql) - - update_column_in_batches(:merge_requests, :head_pipeline_id, sub_query) - end - - def down - end -end diff --git a/db/post_migrate/20170508170547_add_head_pipeline_for_each_merge_request.rb b/db/post_migrate/20170508170547_add_head_pipeline_for_each_merge_request.rb new file mode 100644 index 00000000000..bc3850c0c23 --- /dev/null +++ b/db/post_migrate/20170508170547_add_head_pipeline_for_each_merge_request.rb @@ -0,0 +1,25 @@ +class AddHeadPipelineForEachMergeRequest < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + disable_statement_timeout + + pipelines = Arel::Table.new(:ci_pipelines) + merge_requests = Arel::Table.new(:merge_requests) + + head_id = pipelines. + project(Arel::Nodes::NamedFunction.new('max', [pipelines[:id]])). + from(pipelines). + where(pipelines[:ref].eq(merge_requests[:source_branch])). + where(pipelines[:project_id].eq(merge_requests[:source_project_id])) + + sub_query = Arel::Nodes::SqlLiteral.new(Arel::Nodes::Grouping.new(head_id).to_sql) + + update_column_in_batches(:merge_requests, :head_pipeline_id, sub_query) + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index d0e10f00ffd..b91b3e6e977 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170506185517) do +ActiveRecord::Schema.define(version: 20170508170547) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -114,7 +114,6 @@ ActiveRecord::Schema.define(version: 20170506185517) do t.string "plantuml_url" t.boolean "plantuml_enabled" t.integer "terminal_max_session_time", default: 0, null: false - t.string "default_artifacts_expire_in", default: "0", null: false t.integer "unique_ips_limit_per_user" t.integer "unique_ips_limit_time_window" t.boolean "unique_ips_limit_enabled", default: false, null: false diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb index 5553687707d..cbeb73d9cae 100644 --- a/spec/features/cycle_analytics_spec.rb +++ b/spec/features/cycle_analytics_spec.rb @@ -9,10 +9,6 @@ feature 'Cycle Analytics', feature: true, js: true do let(:mr) { create_merge_request_closing_issue(issue, commit_message: "References #{issue.to_reference}") } let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha) } - before do - allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) - end - context 'as an allowed user' do context 'when project is new' do before do @@ -36,8 +32,10 @@ feature 'Cycle Analytics', feature: true, js: true do context "when there's cycle analytics data" do before do + allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) mr.update(head_pipeline: pipeline) project.add_master(user) + create_cycle deploy_master diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 63797c0fc0e..34f617e23a5 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -103,6 +103,7 @@ pipelines: - manual_actions - artifacts - pipeline_schedule +- merge_requests statuses: - project - pipeline diff --git a/spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb b/spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb new file mode 100644 index 00000000000..bd5f85b901d --- /dev/null +++ b/spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb @@ -0,0 +1,33 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20170508170547_add_head_pipeline_for_each_merge_request.rb') + +describe AddHeadPipelineForEachMergeRequest do + let(:migration) { described_class.new } + + let!(:project) { create(:empty_project) } + let!(:forked_project_link) { create(:forked_project_link, forked_from_project: project) } + let!(:other_project) { forked_project_link.forked_to_project } + + let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: "branch_1") } + let!(:pipeline_2) { create(:ci_pipeline, project: other_project, ref: "branch_1") } + let!(:pipeline_3) { create(:ci_pipeline, project: other_project, ref: "branch_1") } + let!(:pipeline_4) { create(:ci_pipeline, project: project, ref: "branch_2") } + + let!(:mr_1) { create(:merge_request, source_project: project, target_project: project, source_branch: "branch_1", target_branch: "target_1") } + let!(:mr_2) { create(:merge_request, source_project: other_project, target_project: project, source_branch: "branch_1", target_branch: "target_2") } + let!(:mr_3) { create(:merge_request, source_project: project, target_project: project, source_branch: "branch_2", target_branch: "master") } + let!(:mr_4) { create(:merge_request, source_project: project, target_project: project, source_branch: "branch_3", target_branch: "master") } + + context "#up" do + context "when source_project and source_branch of pipeline are the same of merge request" do + it "sets head_pipeline_id of given merge requests" do + migration.up + + expect(mr_1.reload.head_pipeline_id).to eq(pipeline_1.id) + expect(mr_2.reload.head_pipeline_id).to eq(pipeline_3.id) + expect(mr_3.reload.head_pipeline_id).to eq(pipeline_4.id) + expect(mr_4.reload.head_pipeline_id).to be_nil + end + end + end +end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 1db1640f5e5..ef349530761 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -760,8 +760,7 @@ describe MergeRequest, models: true do describe '#head_pipeline' do describe 'when the source project exists' do it 'returns the latest pipeline' do - sha = "123abc" - pipeline = create(:ci_empty_pipeline, project: subject.source_project, ref: 'master', status: 'running', sha: sha) + pipeline = create(:ci_empty_pipeline, project: subject.source_project, ref: 'master', status: 'running', sha: "123abc") subject.update(head_pipeline: pipeline) expect(subject.head_pipeline).to eq(pipeline) diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index f2ed66b5e8e..1ff1438ba06 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -34,16 +34,40 @@ describe Ci::CreatePipelineService, services: true do it { expect(pipeline).to have_attributes(status: 'pending') } it { expect(pipeline.builds.first).to be_kind_of(Ci::Build) } - 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) - merge_request_2 = create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project) - merge_request_3 = create(:merge_request, source_branch: 'other_branch', target_branch: "branch_2", source_project: project) + context '#update_merge_requests_head_pipeline' 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) + merge_request_2 = create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project) - head_pipeline = pipeline + head_pipeline = pipeline - 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_3.reload.head_pipeline).to be_nil + expect(merge_request_1.reload.head_pipeline).to eq(head_pipeline) + expect(merge_request_2.reload.head_pipeline).to eq(head_pipeline) + end + + context 'when there is no pipeline for source branch' do + it "does not update merge request head pipeline" do + merge_request = create(:merge_request, source_branch: 'other_branch', target_branch: "branch_1", source_project: project) + + head_pipeline = pipeline + + expect(merge_request.reload.head_pipeline).not_to eq(head_pipeline) + end + end + + context 'when merge request target project is different from source project' do + let!(:target_project) { create(:empty_project) } + let!(:forked_project_link) { create(:forked_project_link, forked_to_project: project, forked_from_project: target_project) } + + it 'updates head pipeline for merge request' do + merge_request = + create(:merge_request, source_branch: 'master', target_branch: "branch_1", source_project: project, target_project: target_project) + + head_pipeline = pipeline + + expect(merge_request.reload.head_pipeline).to eq(head_pipeline) + end + end end context 'auto-cancel enabled' do -- cgit v1.2.1