From 314062fec5a1d1f56a63202fa16fc7dacc876083 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 20 Feb 2019 15:37:49 +0900 Subject: [PATCH] Persist source sha and target sha for merge pipelines source_sha and target_sha are used for merge request pipelines --- app/models/ci/pipeline.rb | 38 +++++ app/models/concerns/sha_attribute.rb | 10 ++ app/services/ci/create_pipeline_service.rb | 4 +- ...ource-sha-and-target-sha-for-pipelines.yml | 5 + ...20150130_add_extra_shas_to_ci_pipelines.rb | 12 ++ db/schema.rb | 4 +- doc/ci/variables/README.md | 2 + lib/gitlab/ci/pipeline/chain/build.rb | 2 + lib/gitlab/ci/pipeline/chain/command.rb | 2 +- .../gitlab/ci/pipeline/chain/build_spec.rb | 7 + .../gitlab/ci/pipeline/chain/command_spec.rb | 48 +++++++ .../import_export/safe_model_attributes.yml | 2 + spec/models/ci/pipeline_spec.rb | 130 +++++++++++++++++- .../ci/create_pipeline_service_spec.rb | 34 ++++- 14 files changed, 293 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/persist-source-sha-and-target-sha-for-pipelines.yml create mode 100644 db/migrate/20190220150130_add_extra_shas_to_ci_pipelines.rb diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index c0ebafd613d..d4586219333 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -12,6 +12,10 @@ module Ci include AtomicInternalId include EnumWithNil include HasRef + include ShaAttribute + + sha_attribute :source_sha + sha_attribute :target_sha belongs_to :project, inverse_of: :all_pipelines belongs_to :user @@ -191,6 +195,22 @@ module Ci .sort_by_merge_request_pipelines end + scope :triggered_by_merge_request, -> (merge_request) do + where(source: :merge_request, merge_request: merge_request) + end + + scope :detached_merge_request_pipelines, -> (merge_request) do + triggered_by_merge_request(merge_request).where(target_sha: nil) + end + + scope :merge_request_pipelines, -> (merge_request) do + triggered_by_merge_request(merge_request).where.not(target_sha: nil) + end + + scope :mergeable_merge_request_pipelines, -> (merge_request) do + triggered_by_merge_request(merge_request).where(target_sha: merge_request.target_branch_sha) + end + # Returns the pipelines in descending order (= newest first), optionally # limited to a number of references. # @@ -624,6 +644,8 @@ module Ci variables.append(key: 'CI_COMMIT_DESCRIPTION', value: git_commit_description.to_s) if merge_request? && merge_request + variables.append(key: 'CI_MERGE_REQUEST_SOURCE_BRANCH_SHA', value: source_sha.to_s) + variables.append(key: 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA', value: target_sha.to_s) variables.concat(merge_request.predefined_variables) end end @@ -708,6 +730,22 @@ module Ci ref == project.default_branch end + def triggered_by_merge_request? + merge_request? && merge_request_id.present? + end + + def detached_merge_request_pipeline? + triggered_by_merge_request? && target_sha.nil? + end + + def merge_request_pipeline? + triggered_by_merge_request? && target_sha.present? + end + + def mergeable_merge_request_pipeline? + triggered_by_merge_request? && target_sha == merge_request.target_branch_sha + end + private def ci_yaml_from_repo diff --git a/app/models/concerns/sha_attribute.rb b/app/models/concerns/sha_attribute.rb index e51b4e22c96..a479bef993c 100644 --- a/app/models/concerns/sha_attribute.rb +++ b/app/models/concerns/sha_attribute.rb @@ -16,6 +16,8 @@ module ShaAttribute # the column is the correct type. In production it should behave like any other attribute. # See https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/5502 for more discussion def validate_binary_column_exists!(name) + return unless database_exists? + unless table_exists? warn "WARNING: sha_attribute #{name.inspect} is invalid since the table doesn't exist - you may need to run database migrations" return @@ -35,5 +37,13 @@ module ShaAttribute Gitlab::AppLogger.error "ShaAttribute initialization: #{error.message}" raise end + + def database_exists? + ActiveRecord::Base.connection + + true + rescue + false + end end end diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 35a0efcd0a1..8973c5ffc9e 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -25,7 +25,9 @@ module Ci origin_ref: params[:ref], checkout_sha: params[:checkout_sha], after_sha: params[:after], - before_sha: params[:before], + before_sha: params[:before], # The base SHA of the source branch (i.e merge_request.diff_base_sha). + source_sha: params[:source_sha], # The HEAD SHA of the source branch (i.e merge_request.diff_head_sha). + target_sha: params[:target_sha], # The HEAD SHA of the target branch. trigger_request: trigger_request, schedule: schedule, merge_request: merge_request, diff --git a/changelogs/unreleased/persist-source-sha-and-target-sha-for-pipelines.yml b/changelogs/unreleased/persist-source-sha-and-target-sha-for-pipelines.yml new file mode 100644 index 00000000000..6957d156161 --- /dev/null +++ b/changelogs/unreleased/persist-source-sha-and-target-sha-for-pipelines.yml @@ -0,0 +1,5 @@ +--- +title: Persist source sha and target sha for merge pipelines +merge_request: 25417 +author: +type: added diff --git a/db/migrate/20190220150130_add_extra_shas_to_ci_pipelines.rb b/db/migrate/20190220150130_add_extra_shas_to_ci_pipelines.rb new file mode 100644 index 00000000000..45c7c0949c6 --- /dev/null +++ b/db/migrate/20190220150130_add_extra_shas_to_ci_pipelines.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class AddExtraShasToCiPipelines < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_pipelines, :source_sha, :binary + add_column :ci_pipelines, :target_sha, :binary + end +end diff --git a/db/schema.rb b/db/schema.rb index 1bdc9682cc9..d835e48938d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190204115450) do +ActiveRecord::Schema.define(version: 20190220150130) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -497,6 +497,8 @@ ActiveRecord::Schema.define(version: 20190204115450) do t.integer "failure_reason" t.integer "iid" t.integer "merge_request_id" + t.binary "source_sha" + t.binary "target_sha" t.index ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree t.index ["merge_request_id"], name: "index_ci_pipelines_on_merge_request_id", where: "(merge_request_id IS NOT NULL)", using: :btree t.index ["pipeline_schedule_id"], name: "index_ci_pipelines_on_pipeline_schedule_id", using: :btree diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index ca668ac526f..6fe352df48a 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -86,10 +86,12 @@ future GitLab releases.** | **CI_MERGE_REQUEST_PROJECT_URL** | 11.6 | all | The URL of the project of the merge request if it's [pipelines for merge requests](../merge_request_pipelines/index.md) (e.g. `http://192.168.10.15:3000/namespace/awesome-project`) | | **CI_MERGE_REQUEST_REF_PATH** | 11.6 | all | The ref path of the merge request if it's [pipelines for merge requests](../merge_request_pipelines/index.md). (e.g. `refs/merge-requests/1/head`) | | **CI_MERGE_REQUEST_SOURCE_BRANCH_NAME** | 11.6 | all | The source branch name of the merge request if it's [pipelines for merge requests](../merge_request_pipelines/index.md) | +| **CI_MERGE_REQUEST_SOURCE_BRANCH_SHA** | 11.9 | all | The HEAD sha of the source branch of the merge request if it's [pipelines for merge requests](../merge_request_pipelines/index.md) | | **CI_MERGE_REQUEST_SOURCE_PROJECT_ID** | 11.6 | all | The ID of the source project of the merge request if it's [pipelines for merge requests](../merge_request_pipelines/index.md) | | **CI_MERGE_REQUEST_SOURCE_PROJECT_PATH** | 11.6 | all | The path of the source project of the merge request if it's [pipelines for merge requests](../merge_request_pipelines/index.md) | | **CI_MERGE_REQUEST_SOURCE_PROJECT_URL** | 11.6 | all | The URL of the source project of the merge request if it's [pipelines for merge requests](../merge_request_pipelines/index.md) | | **CI_MERGE_REQUEST_TARGET_BRANCH_NAME** | 11.6 | all | The target branch name of the merge request if it's [pipelines for merge requests](../merge_request_pipelines/index.md) | +| **CI_MERGE_REQUEST_TARGET_BRANCH_SHA** | 11.9 | all | The HEAD sha of the target branch of the merge request if it's [pipelines for merge requests](../merge_request_pipelines/index.md) | | **CI_NODE_INDEX** | 11.5 | all | Index of the job in the job set. If the job is not parallelized, this variable is not set. | | **CI_NODE_TOTAL** | 11.5 | all | Total number of instances of this job running in parallel. If the job is not parallelized, this variable is set to `1`. | | **CI_API_V4_URL** | 11.7 | all | The GitLab API v4 root URL | diff --git a/lib/gitlab/ci/pipeline/chain/build.rb b/lib/gitlab/ci/pipeline/chain/build.rb index 41632211374..164a4634d84 100644 --- a/lib/gitlab/ci/pipeline/chain/build.rb +++ b/lib/gitlab/ci/pipeline/chain/build.rb @@ -12,6 +12,8 @@ module Gitlab ref: @command.ref, sha: @command.sha, before_sha: @command.before_sha, + source_sha: @command.source_sha, + target_sha: @command.target_sha, tag: @command.tag_exists?, trigger_requests: Array(@command.trigger_request), user: @command.current_user, diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index e4ed1424865..7b77e86feae 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -7,7 +7,7 @@ module Gitlab module Chain Command = Struct.new( :source, :project, :current_user, - :origin_ref, :checkout_sha, :after_sha, :before_sha, + :origin_ref, :checkout_sha, :after_sha, :before_sha, :source_sha, :target_sha, :trigger_request, :schedule, :merge_request, :ignore_skip_ci, :save_incompleted, :seeds_block, :variables_attributes, :push_options, diff --git a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb index fab071405df..c9d1d09a938 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb @@ -101,6 +101,8 @@ describe Gitlab::Ci::Pipeline::Chain::Build do checkout_sha: project.commit.id, after_sha: nil, before_sha: nil, + source_sha: merge_request.diff_head_sha, + target_sha: merge_request.target_branch_sha, trigger_request: nil, schedule: nil, merge_request: merge_request, @@ -118,5 +120,10 @@ describe Gitlab::Ci::Pipeline::Chain::Build do expect(pipeline).to be_merge_request expect(pipeline.merge_request).to eq(merge_request) end + + it 'correctly sets souce sha and target sha to pipeline' do + expect(pipeline.source_sha).to eq(merge_request.diff_head_sha) + expect(pipeline.target_sha).to eq(merge_request.target_branch_sha) + end end end diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index 6aa802ce6fd..dab0fb51bcc 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -161,6 +161,54 @@ describe Gitlab::Ci::Pipeline::Chain::Command do end end + describe '#source_sha' do + subject { command.source_sha } + + let(:command) do + described_class.new(project: project, + source_sha: source_sha, + merge_request: merge_request) + end + + let(:merge_request) do + create(:merge_request, target_project: project, source_project: project) + end + + let(:source_sha) { nil } + + context 'when source_sha is specified' do + let(:source_sha) { 'abc' } + + it 'returns the specified value' do + is_expected.to eq('abc') + end + end + end + + describe '#target_sha' do + subject { command.target_sha } + + let(:command) do + described_class.new(project: project, + target_sha: target_sha, + merge_request: merge_request) + end + + let(:merge_request) do + create(:merge_request, target_project: project, source_project: project) + end + + let(:target_sha) { nil } + + context 'when target_sha is specified' do + let(:target_sha) { 'abc' } + + it 'returns the specified value' do + is_expected.to eq('abc') + end + end + end + describe '#protected_ref?' do let(:command) { described_class.new(project: project, origin_ref: 'my-branch') } diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index baca8f6d542..ee96e5c4d42 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -235,6 +235,8 @@ Ci::Pipeline: - ref - sha - before_sha +- source_sha +- target_sha - push_data - created_at - updated_at diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 7ea701dd035..ee400bec04b 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -130,6 +130,132 @@ describe Ci::Pipeline, :mailer do end end + describe '.detached_merge_request_pipelines' do + subject { described_class.detached_merge_request_pipelines(merge_request) } + + let!(:pipeline) do + create(:ci_pipeline, source: :merge_request, merge_request: merge_request, target_sha: target_sha) + end + + let(:merge_request) { create(:merge_request) } + let(:target_sha) { nil } + + it 'returns detached merge request pipelines' do + is_expected.to eq([pipeline]) + end + + context 'when target sha exists' do + let(:target_sha) { merge_request.target_branch_sha } + + it 'returns empty array' do + is_expected.to be_empty + end + end + end + + describe '#detached_merge_request_pipeline?' do + subject { pipeline.detached_merge_request_pipeline? } + + let!(:pipeline) do + create(:ci_pipeline, source: :merge_request, merge_request: merge_request, target_sha: target_sha) + end + + let(:merge_request) { create(:merge_request) } + let(:target_sha) { nil } + + it { is_expected.to be_truthy } + + context 'when target sha exists' do + let(:target_sha) { merge_request.target_branch_sha } + + it { is_expected.to be_falsy } + end + end + + describe '.merge_request_pipelines' do + subject { described_class.merge_request_pipelines(merge_request) } + + let!(:pipeline) do + create(:ci_pipeline, source: :merge_request, merge_request: merge_request, target_sha: target_sha) + end + + let(:merge_request) { create(:merge_request) } + let(:target_sha) { merge_request.target_branch_sha } + + it 'returns merge pipelines' do + is_expected.to eq([pipeline]) + end + + context 'when target sha is empty' do + let(:target_sha) { nil } + + it 'returns empty array' do + is_expected.to be_empty + end + end + end + + describe '#merge_request_pipeline?' do + subject { pipeline.merge_request_pipeline? } + + let!(:pipeline) do + create(:ci_pipeline, source: :merge_request, merge_request: merge_request, target_sha: target_sha) + end + + let(:merge_request) { create(:merge_request) } + let(:target_sha) { merge_request.target_branch_sha } + + it { is_expected.to be_truthy } + + context 'when target sha is empty' do + let(:target_sha) { nil } + + it { is_expected.to be_falsy } + end + end + + describe '.mergeable_merge_request_pipelines' do + subject { described_class.mergeable_merge_request_pipelines(merge_request) } + + let!(:pipeline) do + create(:ci_pipeline, source: :merge_request, merge_request: merge_request, target_sha: target_sha) + end + + let(:merge_request) { create(:merge_request) } + let(:target_sha) { merge_request.target_branch_sha } + + it 'returns mergeable merge pipelines' do + is_expected.to eq([pipeline]) + end + + context 'when target sha does not point the head of the target branch' do + let(:target_sha) { merge_request.diff_head_sha } + + it 'returns empty array' do + is_expected.to be_empty + end + end + end + + describe '#mergeable_merge_request_pipeline?' do + subject { pipeline.mergeable_merge_request_pipeline? } + + let!(:pipeline) do + create(:ci_pipeline, source: :merge_request, merge_request: merge_request, target_sha: target_sha) + end + + let(:merge_request) { create(:merge_request) } + let(:target_sha) { merge_request.target_branch_sha } + + it { is_expected.to be_truthy } + + context 'when target sha does not point the head of the target branch' do + let(:target_sha) { merge_request.diff_head_sha } + + it { is_expected.to be_falsy } + end + end + describe '.merge_request' do subject { described_class.merge_request } @@ -400,10 +526,12 @@ describe Ci::Pipeline, :mailer do 'CI_MERGE_REQUEST_PROJECT_PATH' => merge_request.project.full_path, 'CI_MERGE_REQUEST_PROJECT_URL' => merge_request.project.web_url, 'CI_MERGE_REQUEST_TARGET_BRANCH_NAME' => merge_request.target_branch.to_s, + 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA' => pipeline.target_sha.to_s, 'CI_MERGE_REQUEST_SOURCE_PROJECT_ID' => merge_request.source_project.id.to_s, 'CI_MERGE_REQUEST_SOURCE_PROJECT_PATH' => merge_request.source_project.full_path, 'CI_MERGE_REQUEST_SOURCE_PROJECT_URL' => merge_request.source_project.web_url, - 'CI_MERGE_REQUEST_SOURCE_BRANCH_NAME' => merge_request.source_branch.to_s) + 'CI_MERGE_REQUEST_SOURCE_BRANCH_NAME' => merge_request.source_branch.to_s, + 'CI_MERGE_REQUEST_SOURCE_BRANCH_SHA' => pipeline.source_sha.to_s) end context 'when source project does not exist' do diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 8497e90bd8b..93349ba7b5b 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -12,6 +12,7 @@ describe Ci::CreatePipelineService do end describe '#execute' do + # rubocop:disable Metrics/ParameterLists def execute_service( source: :push, after: project.commit.id, @@ -20,17 +21,22 @@ describe Ci::CreatePipelineService do trigger_request: nil, variables_attributes: nil, merge_request: nil, - push_options: nil) + push_options: nil, + source_sha: nil, + target_sha: nil) params = { ref: ref, before: '00000000', after: after, commits: [{ message: message }], variables_attributes: variables_attributes, - push_options: push_options } + push_options: push_options, + source_sha: source_sha, + target_sha: target_sha } described_class.new(project, user, params).execute( source, trigger_request: trigger_request, merge_request: merge_request) end + # rubocop:enable Metrics/ParameterLists context 'valid params' do let(:pipeline) { execute_service } @@ -679,7 +685,11 @@ describe Ci::CreatePipelineService do describe 'Merge request pipelines' do let(:pipeline) do - execute_service(source: source, merge_request: merge_request, ref: ref_name) + execute_service(source: source, + merge_request: merge_request, + ref: ref_name, + source_sha: source_sha, + target_sha: target_sha) end before do @@ -687,6 +697,8 @@ describe Ci::CreatePipelineService do end let(:ref_name) { 'refs/heads/feature' } + let(:source_sha) { project.commit(ref_name).id } + let(:target_sha) { nil } context 'when source is merge request' do let(:source) { :merge_request } @@ -727,6 +739,22 @@ describe Ci::CreatePipelineService do expect(pipeline.builds.order(:stage_id).map(&:name)).to eq(%w[test]) end + it 'persists the specified source sha' do + expect(pipeline.source_sha).to eq(source_sha) + end + + it 'does not persist target sha for detached merge request pipeline' do + expect(pipeline.target_sha).to be_nil + end + + context 'when target sha is specified' do + let(:target_sha) { merge_request.target_branch_sha } + + it 'persists the target sha' do + expect(pipeline.target_sha).to eq(target_sha) + end + end + context 'when ref is tag' do let(:ref_name) { 'refs/tags/v1.1.0' } -- GitLab