Merge branch 'backstage/gb/build-pipeline-in-a-separate-class' into 'master'

Extract class responsible for building a pipeline

Closes #38460

See merge request gitlab-org/gitlab-ce!14762

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: {

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")

lib/gitlab/ci/pipeline/chain/build.rb

+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?
+            )
+
+            @pipeline.set_config_source
+          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

spec/lib/gitlab/ci/pipeline/chain/build_spec.rb

+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_repository_ci_yaml_file(sha: anything)
+
+    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
+    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
+    expect(pipeline.repository_source?).to be true
+  end
+
+  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

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

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
 

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

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) }