From 9ad48a7438048ca19ff847e5fa7f2ccd7c6d59af Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Oct 2017 13:07:55 +0200 Subject: Extract class responsible for building a pipeline --- app/services/ci/create_pipeline_service.rb | 55 +++++++---------------------- lib/gitlab/ci/pipeline/chain/build.rb | 56 ++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 43 deletions(-) create mode 100644 lib/gitlab/ci/pipeline/chain/build.rb diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 31a712ccc1b..7b9ea223d26 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -2,27 +2,24 @@ module Ci class CreatePipelineService < BaseService attr_reader :pipeline - SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Validate::Abilities, + SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Build, + Gitlab::Ci::Pipeline::Chain::Validate::Abilities, Gitlab::Ci::Pipeline::Chain::Validate::Repository, Gitlab::Ci::Pipeline::Chain::Validate::Config, Gitlab::Ci::Pipeline::Chain::Skip, Gitlab::Ci::Pipeline::Chain::Create].freeze def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil, &block) - @pipeline = Ci::Pipeline.new( - source: source, - project: project, - ref: ref, - sha: sha, - before_sha: before_sha, - tag: tag_exists?, - trigger_requests: Array(trigger_request), - user: current_user, - pipeline_schedule: schedule, - protected: project.protected_for?(ref) - ) - - command = OpenStruct.new(ignore_skip_ci: ignore_skip_ci, + @pipeline = Ci::Pipeline.new + + command = OpenStruct.new(source: source, + origin_ref: params[:ref], + checkout_sha: params[:checkout_sha], + after_sha: params[:after], + before_sha: params[:before], + trigger_request: trigger_request, + schedule: schedule, + ignore_skip_ci: ignore_skip_ci, save_incompleted: save_on_errors, seeds_block: block, project: project, @@ -45,14 +42,6 @@ module Ci private - def commit - @commit ||= project.commit(origin_sha || origin_ref) - end - - def sha - commit.try(:id) - end - def update_merge_requests_head_pipeline return unless pipeline.latest? @@ -76,26 +65,6 @@ module Ci .created_or_pending end - def before_sha - params[:checkout_sha] || params[:before] || Gitlab::Git::BLANK_SHA - end - - def origin_sha - params[:checkout_sha] || params[:after] - end - - def origin_ref - params[:ref] - end - - def tag_exists? - project.repository.tag_exists?(ref) - end - - def ref - @ref ||= Gitlab::Git.ref_name(origin_ref) - end - def pipeline_created_counter @pipeline_created_counter ||= Gitlab::Metrics .counter(:pipelines_created_total, "Counter of pipelines created") diff --git a/lib/gitlab/ci/pipeline/chain/build.rb b/lib/gitlab/ci/pipeline/chain/build.rb new file mode 100644 index 00000000000..efd1da733c5 --- /dev/null +++ b/lib/gitlab/ci/pipeline/chain/build.rb @@ -0,0 +1,56 @@ +module Gitlab + module Ci + module Pipeline + module Chain + class Build < Chain::Base + include Chain::Helpers + + def perform! + @pipeline.assign_attributes( + source: @command.source, + project: @project, + ref: ref, + sha: sha, + before_sha: before_sha, + tag: tag_exists?, + trigger_requests: Array(@command.trigger_request), + user: @current_user, + pipeline_schedule: @command.schedule, + protected: protected_ref? + ) + end + + def break? + false + end + + private + + def ref + @ref ||= Gitlab::Git.ref_name(origin_ref) + end + + def sha + @project.commit(origin_sha || origin_ref).try(:id) + end + + def origin_ref + @command.origin_ref + end + + def origin_sha + @command.checkout_sha || @command.after_sha + end + + def before_sha + @command.checkout_sha || @command.before_sha || Gitlab::Git::BLANK_SHA + end + + def protected_ref? + @project.protected_for?(ref) + end + end + end + end + end +end -- cgit v1.2.1 From 20fc42e99a5e8c917feaa0c7a2d34aceba4a8eb9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 10 Oct 2017 10:11:19 +0200 Subject: Improve post_receive spec by not stubbing private methods --- spec/workers/post_receive_spec.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 05eecf5f0bb..5d9b0679796 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -66,19 +66,21 @@ describe PostReceive do end context "gitlab-ci.yml" do + let(:changes) { "123456 789012 refs/heads/feature\n654321 210987 refs/tags/tag" } + subject { described_class.new.perform(gl_repository, key_id, base64_changes) } context "creates a Ci::Pipeline for every change" do before do stub_ci_pipeline_to_return_yaml_file - # TODO, don't stub private methods - # - allow_any_instance_of(Ci::CreatePipelineService) - .to receive(:commit).and_return(OpenStruct.new(id: '123456')) + allow_any_instance_of(Project) + .to receive(:commit) + .and_return(project.commit) allow_any_instance_of(Repository) - .to receive(:branch_exists?).and_return(true) + .to receive(:branch_exists?) + .and_return(true) end it { expect { subject }.to change { Ci::Pipeline.count }.by(2) } -- cgit v1.2.1 From 54f490a455dea705908d5e262fb5fe205147fa09 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 10 Oct 2017 14:00:56 +0200 Subject: Add initial specs for pipeline build chain class --- spec/lib/gitlab/ci/pipeline/chain/build_spec.rb | 43 +++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 spec/lib/gitlab/ci/pipeline/chain/build_spec.rb diff --git a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb new file mode 100644 index 00000000000..fbb72e6a51a --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Chain::Build do + set(:project) { create(:project, :repository) } + set(:user) { create(:user) } + let(:pipeline) { Ci::Pipeline.new } + + let(:command) do + double('command', source: :push, + origin_ref: 'master', + checkout_sha: project.commit.id, + after_sha: nil, + before_sha: nil, + trigger_request: nil, + schedule: nil, + project: project, + current_user: user) + end + + let(:step) { described_class.new(pipeline, command) } + + before do + stub_ci_pipeline_to_return_yaml_file + + step.perform! + end + + it 'never breaks the chain' do + expect(step.break?).to be false + end + + it 'fills pipeline object with data' do + expect(pipeline.sha).not_to be_empty + end + + it 'sets a valid config source' do + expect(pipeline.repository_source?).to be true + end + + it 'returns a valid pipeline' do + expect(pipeline).to be_valid + end +end -- cgit v1.2.1 From eedf43d69bb2d3e5ed73b751565beb5d1badd8c7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 11 Oct 2017 14:48:28 +0200 Subject: Set pipeline config source attribute in a build step --- lib/gitlab/ci/pipeline/chain/build.rb | 2 ++ spec/lib/gitlab/ci/pipeline/chain/build_spec.rb | 2 +- spec/services/ci/create_pipeline_service_spec.rb | 3 ++- spec/support/stub_gitlab_calls.rb | 6 ++++++ 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ci/pipeline/chain/build.rb b/lib/gitlab/ci/pipeline/chain/build.rb index efd1da733c5..a126dded1ae 100644 --- a/lib/gitlab/ci/pipeline/chain/build.rb +++ b/lib/gitlab/ci/pipeline/chain/build.rb @@ -18,6 +18,8 @@ module Gitlab pipeline_schedule: @command.schedule, protected: protected_ref? ) + + @pipeline.set_config_source end def break? diff --git a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb index fbb72e6a51a..82f70dd176d 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb @@ -20,7 +20,7 @@ describe Gitlab::Ci::Pipeline::Chain::Build do let(:step) { described_class.new(pipeline, command) } before do - stub_ci_pipeline_to_return_yaml_file + stub_repository_ci_yaml_file(sha: anything) step.perform! end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 08847183bf4..befd0faf1b6 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -8,7 +8,7 @@ describe Ci::CreatePipelineService do let(:ref_name) { 'refs/heads/master' } before do - stub_ci_pipeline_to_return_yaml_file + stub_repository_ci_yaml_file(sha: anything) end describe '#execute' do @@ -44,6 +44,7 @@ describe Ci::CreatePipelineService do expect(pipeline).to eq(project.pipelines.last) expect(pipeline).to have_attributes(user: user) expect(pipeline).to have_attributes(status: 'pending') + expect(pipeline.repository_source?).to be true expect(pipeline.builds.first).to be_kind_of(Ci::Build) end diff --git a/spec/support/stub_gitlab_calls.rb b/spec/support/stub_gitlab_calls.rb index 5f22d886910..c1618f5086c 100644 --- a/spec/support/stub_gitlab_calls.rb +++ b/spec/support/stub_gitlab_calls.rb @@ -21,6 +21,12 @@ module StubGitlabCalls allow_any_instance_of(Ci::Pipeline).to receive(:ci_yaml_file) { ci_yaml } end + def stub_repository_ci_yaml_file(sha:, path: '.gitlab-ci.yml') + allow_any_instance_of(Repository) + .to receive(:gitlab_ci_yml_for).with(sha, path) + .and_return(gitlab_ci_yaml) + end + def stub_ci_builds_disabled allow_any_instance_of(Project).to receive(:builds_enabled?).and_return(false) end -- cgit v1.2.1 From 1486950bc9396f9b8384763d68f36387326eb745 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 11 Oct 2017 14:51:39 +0200 Subject: Extend pipeline build chain specs --- spec/lib/gitlab/ci/pipeline/chain/build_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb index 82f70dd176d..0f1d72080c5 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb @@ -31,6 +31,10 @@ describe Gitlab::Ci::Pipeline::Chain::Build do it 'fills pipeline object with data' do expect(pipeline.sha).not_to be_empty + expect(pipeline.sha).to eq project.commit.id + expect(pipeline.ref).to eq 'master' + expect(pipeline.user).to eq user + expect(pipeline.project).to eq project end it 'sets a valid config source' do @@ -40,4 +44,8 @@ describe Gitlab::Ci::Pipeline::Chain::Build do it 'returns a valid pipeline' do expect(pipeline).to be_valid end + + it 'does not persist a pipeline' do + expect(pipeline).not_to be_persisted + end end -- cgit v1.2.1 From 9293842d096bf8de9d13ce2a714c646ffba27eb5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 30 Nov 2017 12:53:35 +0100 Subject: Do not set pipeline source after initialization --- app/models/ci/pipeline.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index fd64670f6b0..eebbf7c4218 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -40,7 +40,6 @@ module Ci validates :status, presence: { unless: :importing? } validate :valid_commit_sha, unless: :importing? - after_initialize :set_config_source, if: :new_record? after_create :keep_around_commits, unless: :importing? enum source: { -- cgit v1.2.1 From 16a8372455d4237149996305d477f4ec7625eee8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 30 Nov 2017 15:11:33 +0100 Subject: Fix pipeline config source specs and test it explicitly --- spec/models/ci/pipeline_spec.rb | 79 ++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 41 deletions(-) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 4cf0088ac9c..d4b1e7c8dd4 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -868,62 +868,59 @@ describe Ci::Pipeline, :mailer do end describe '#set_config_source' do - context 'on object initialisation' do - context 'when pipelines does not contain needed data' do - let(:pipeline) do - Ci::Pipeline.new - end + context 'when pipelines does not contain needed data' do + it 'defines source to be unknown' do + pipeline.set_config_source - it 'defines source to be unknown' do - expect(pipeline).to be_unknown_source - end + expect(pipeline).to be_unknown_source end + end - context 'when pipeline contains all needed data' do - let(:pipeline) do - Ci::Pipeline.new( - project: project, - sha: '1234', - ref: 'master', - source: :push) + context 'when pipeline contains all needed data' do + let(:pipeline) do + create(:ci_pipeline, project: project, + sha: '1234', + ref: 'master', + source: :push) + end + + context 'when the repository has a config file' do + before do + allow(project.repository).to receive(:gitlab_ci_yml_for) + .and_return('config') end - context 'when the repository has a config file' do - before do - allow(project.repository).to receive(:gitlab_ci_yml_for) - .and_return('config') - end + it 'defines source to be from repository' do + pipeline.set_config_source - it 'defines source to be from repository' do - expect(pipeline).to be_repository_source - end + expect(pipeline).to be_repository_source + end - context 'when loading an object' do - let(:new_pipeline) { Ci::Pipeline.find(pipeline.id) } + context 'when loading an object' do + let(:new_pipeline) { Ci::Pipeline.find(pipeline.id) } - it 'does not redefine the source' do - # force to overwrite the source - pipeline.unknown_source! + it 'does not redefine the source' do + # force to overwrite the source + pipeline.unknown_source! - expect(new_pipeline).to be_unknown_source - end + expect(new_pipeline).to be_unknown_source end end + end - context 'when the repository does not have a config file' do - let(:implied_yml) { Gitlab::Template::GitlabCiYmlTemplate.find('Auto-DevOps').content } + context 'when the repository does not have a config file' do + let(:implied_yml) { Gitlab::Template::GitlabCiYmlTemplate.find('Auto-DevOps').content } - context 'auto devops enabled' do - before do - stub_application_setting(auto_devops_enabled: true) - allow(project).to receive(:ci_config_path) { 'custom' } - end + context 'auto devops enabled' do + before do + stub_application_setting(auto_devops_enabled: true) + allow(project).to receive(:ci_config_path) { 'custom' } + end - it 'defines source to be auto devops' do - subject + it 'defines source to be auto devops' do + pipeline.set_config_source - expect(pipeline).to be_auto_devops_source - end + expect(pipeline).to be_auto_devops_source end end end -- cgit v1.2.1