From f012157b1b7f1d5306f08d5f782938a8739d8429 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 Add changelog Fix doc Fix Add safe model attributes Use binary column Allow nil source sha and target sha Fix spec Fix doc Add scopes Add spec Fix coding offence Fix Fix spec ok ok Fix spec for ShaAttribute Catch any exceptions Fix --- app/models/ci/pipeline.rb | 38 +++++ app/models/concerns/sha_attribute.rb | 13 ++ 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, 296 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 d8252f6754bac7..e5a4adc74f6541 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -12,6 +12,7 @@ class Pipeline < ActiveRecord::Base include AtomicInternalId include EnumWithNil include HasRef + include ShaAttribute belongs_to :project, inverse_of: :all_pipelines belongs_to :user @@ -63,6 +64,9 @@ class Pipeline < ActiveRecord::Base validate :valid_commit_sha, unless: :importing? validates :source, exclusion: { in: %w(unknown), unless: :importing? }, on: :create + sha_attribute :source_sha + sha_attribute :target_sha + after_create :keep_around_commits, unless: :importing? # We use `Ci::PipelineEnums.sources` here so that EE can more easily extend @@ -191,6 +195,22 @@ class Pipeline < ActiveRecord::Base .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 @@ def predefined_variables 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 @@ def default_branch? 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 aad6cb0b2646ba..f05036d1ca7790 100644 --- a/app/models/concerns/sha_attribute.rb +++ b/app/models/concerns/sha_attribute.rb @@ -16,6 +16,11 @@ def sha_attribute(name) # 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) + unless database_exists? + warn "WARNING: sha_attribute #{name.inspect} is invalid since the database doesn't exist - you may need to create database at first" + return + end + 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,6 +40,14 @@ def validate_binary_column_exists!(name) Gitlab::AppLogger.error "ShaAttribute initialization: #{error.message}" raise end + + def database_exists? + ActiveRecord::Base.connection + rescue + false + else + true + end end end diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index d635a95b488f69..2d68b40bdae536 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -25,7 +25,9 @@ def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request 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 00000000000000..6957d156161bfe --- /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 00000000000000..45c7c0949c6d83 --- /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 833719f08dfb2d..23b280128f5f49 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" @@ -648,6 +648,8 @@ 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 928ae29a4a7b5f..62db4632849fc7 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 4163221137440a..164a4634d849f8 100644 --- a/lib/gitlab/ci/pipeline/chain/build.rb +++ b/lib/gitlab/ci/pipeline/chain/build.rb @@ -12,6 +12,8 @@ def perform! 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 222e4888ba0566..f7b30c958925dd 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -7,7 +7,7 @@ module Pipeline 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 fab071405dfd9c..c9d1d09a938c69 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 @@ 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 @@ 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 6aa802ce6fdd6f..dab0fb51bcc17d 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 @@ 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 cf1cff00833ea5..bf76effbcd27d2 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -238,6 +238,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 c51a45cc338d8f..c80859792ee3dd 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -134,6 +134,132 @@ 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 } @@ -404,10 +530,12 @@ def create_build(name, status) '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 8497e90bd8b0f8..93349ba7b5ba68 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -12,6 +12,7 @@ end describe '#execute' do + # rubocop:disable Metrics/ParameterLists def execute_service( source: :push, after: project.commit.id, @@ -20,17 +21,22 @@ def execute_service( 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 @@ def previous_commit_sha_from_ref(ref) 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 @@ def previous_commit_sha_from_ref(ref) 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 @@ def previous_commit_sha_from_ref(ref) 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