Merge branch 'pipeline-creation-dry-run' into 'master'

Make CreatePipelineService to use a dry_run mode

See merge request gitlab-org/gitlab!37134

app/services/ci/create_pipeline_service.rb

@@ -19,9 +19,13 @@ module Ci
                 Gitlab::Ci::Pipeline::Chain::Limit::Size,
                 Gitlab::Ci::Pipeline::Chain::Validate::External,
                 Gitlab::Ci::Pipeline::Chain::Populate,
+                Gitlab::Ci::Pipeline::Chain::StopDryRun,
                 Gitlab::Ci::Pipeline::Chain::Create,
                 Gitlab::Ci::Pipeline::Chain::Limit::Activity,
-                Gitlab::Ci::Pipeline::Chain::Limit::JobActivity].freeze
+                Gitlab::Ci::Pipeline::Chain::Limit::JobActivity,
+                Gitlab::Ci::Pipeline::Chain::CancelPendingPipelines,
+                Gitlab::Ci::Pipeline::Chain::Metrics,
+                Gitlab::Ci::Pipeline::Chain::Pipeline::Process].freeze
 
     # Create a new pipeline in the specified project.
     #
@@ -68,21 +72,14 @@ module Ci
         bridge: bridge,
         **extra_options(options))
 
-      sequence = Gitlab::Ci::Pipeline::Chain::Sequence
-        .new(pipeline, command, SEQUENCE)
+      # Ensure we never persist the pipeline when dry_run: true
+      @pipeline.readonly! if command.dry_run?
 
-      sequence.build! do |pipeline, sequence|
-        schedule_head_pipeline_update
+      Gitlab::Ci::Pipeline::Chain::Sequence
+        .new(pipeline, command, SEQUENCE)
+        .build!
 
-        if sequence.complete?
-          cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
-          pipeline_created_counter.increment(source: source)
-
-          Ci::ProcessPipelineService
-            .new(pipeline)
-            .execute(nil, initial_process: true)
-        end
-      end
+      schedule_head_pipeline_update if pipeline.persisted?
 
       # If pipeline is not persisted, try to recover IID
       pipeline.reset_project_iid unless pipeline.persisted? ||
@@ -110,38 +107,14 @@ module Ci
       commit.try(:id)
     end
 
-    def cancel_pending_pipelines
-      Gitlab::OptimisticLocking.retry_lock(auto_cancelable_pipelines) do |cancelables|
-        cancelables.find_each do |cancelable|
-          cancelable.auto_cancel_running(pipeline)
-        end
-      end
-    end
-
-    # rubocop: disable CodeReuse/ActiveRecord
-    def auto_cancelable_pipelines
-      project.ci_pipelines
-        .where(ref: pipeline.ref)
-        .where.not(id: pipeline.same_family_pipeline_ids)
-        .where.not(sha: project.commit(pipeline.ref).try(:id))
-        .alive_or_scheduled
-        .with_only_interruptible_builds
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
-    def pipeline_created_counter
-      @pipeline_created_counter ||= Gitlab::Metrics
-        .counter(:pipelines_created_total, "Counter of pipelines created")
-    end
-
     def schedule_head_pipeline_update
       pipeline.all_merge_requests.opened.each do |merge_request|
         UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id)
       end
     end
 
-    def extra_options(content: nil)
-      { content: content }
+    def extra_options(content: nil, dry_run: false)
+      { content: content, dry_run: dry_run }
     end
   end
 end

ee/lib/ee/gitlab/ci/pipeline/chain/limit/size.rb

@@ -24,12 +24,8 @@ module EE
               def perform!
                 return unless limit.exceeded?
 
-                if command.save_incompleted
-                  pipeline.drop!(:size_limit_exceeded)
-                end
-
                 limit.log_error!(project_id: project.id, plan: project.actual_plan_name)
-                error(limit.message)
+                error(limit.message, drop_reason: :size_limit_exceeded)
               end
 
               override :break?

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class CancelPendingPipelines < Chain::Base
+          include Chain::Helpers
+
+          def perform!
+            return unless project.auto_cancel_pending_pipelines?
+
+            Gitlab::OptimisticLocking.retry_lock(auto_cancelable_pipelines) do |cancelables|
+              cancelables.find_each do |cancelable|
+                cancelable.auto_cancel_running(pipeline)
+              end
+            end
+          end
+
+          def break?
+            false
+          end
+
+          private
+
+          # rubocop: disable CodeReuse/ActiveRecord
+          def auto_cancelable_pipelines
+            project.ci_pipelines
+              .where(ref: pipeline.ref)
+              .where.not(id: pipeline.same_family_pipeline_ids)
+              .where.not(sha: project.commit(pipeline.ref).try(:id))
+              .alive_or_scheduled
+              .with_only_interruptible_builds
+          end
+          # rubocop: enable CodeReuse/ActiveRecord
+        end
+      end
+    end
+  end
+end

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

@@ -10,7 +10,7 @@ module Gitlab
           :trigger_request, :schedule, :merge_request, :external_pull_request,
           :ignore_skip_ci, :save_incompleted,
           :seeds_block, :variables_attributes, :push_options,
-          :chat_data, :allow_mirror_update, :bridge, :content,
+          :chat_data, :allow_mirror_update, :bridge, :content, :dry_run,
           # These attributes are set by Chains during processing:
           :config_content, :config_processor, :stage_seeds
         ) do
@@ -22,6 +22,8 @@ module Gitlab
             end
           end
 
+          alias_method :dry_run?, :dry_run
+
           def branch_exists?
             strong_memoize(:is_branch) do
               project.repository.branch_exists?(ref)

lib/gitlab/ci/pipeline/chain/config/content/parameter.rb

@@ -12,7 +12,6 @@ module Gitlab
               def content
                 strong_memoize(:content) do
                   next unless command.content.present?
-                  raise UnsupportedSourceError, "#{command.source} not a dangling build" unless command.dangling_build?
 
                   command.content
                 end

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

@@ -6,13 +6,13 @@ module Gitlab
       module Chain
         module Helpers
           def error(message, config_error: false, drop_reason: nil)
-            if config_error && command.save_incompleted
+            if config_error
               drop_reason = :config_error
               pipeline.yaml_errors = message
             end
 
             pipeline.add_error_message(message)
-            pipeline.drop!(drop_reason) if drop_reason
+            pipeline.drop!(drop_reason) if drop_reason && persist_pipeline?
 
             # TODO: consider not to rely on AR errors directly as they can be
             # polluted with other unrelated errors (e.g. state machine)
@@ -23,6 +23,10 @@ module Gitlab
           def warning(message)
             pipeline.add_warning_message(message)
           end
+
+          def persist_pipeline?
+            command.save_incompleted && !pipeline.readonly?
+          end
         end
       end
     end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class Metrics < Chain::Base
+          def perform!
+            counter.increment(source: @pipeline.source)
+          end
+
+          def break?
+            false
+          end
+
+          def counter
+            ::Gitlab::Ci::Pipeline::Metrics.new.pipelines_created_counter
+          end
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        module Pipeline
+          # After pipeline has been successfully created we can start processing it.
+          class Process < Chain::Base
+            def perform!
+              ::Ci::ProcessPipelineService
+                .new(@pipeline)
+                .execute(nil, initial_process: true)
+            end
+
+            def break?
+              false
+            end
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -9,30 +9,21 @@ module Gitlab
             @pipeline = pipeline
             @command = command
             @sequence = sequence
-            @completed = []
             @start = Time.now
           end
 
           def build!
-            @sequence.each do |chain|
-              step = chain.new(@pipeline, @command)
+            @sequence.each do |step_class|
+              step = step_class.new(@pipeline, @command)
 
               step.perform!
               break if step.break?
-
-              @completed.push(step)
             end
 
-            @pipeline.tap do
-              yield @pipeline, self if block_given?
-
-              @command.observe_creation_duration(Time.now - @start)
-              @command.observe_pipeline_size(@pipeline)
-            end
-          end
+            @command.observe_creation_duration(Time.now - @start)
+            @command.observe_pipeline_size(@pipeline)
 
-          def complete?
-            @completed.size == @sequence.size
+            @pipeline
           end
         end
       end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        # During the dry run we don't want to persist the pipeline and skip
+        # all the other steps that operate on a persisted context.
+        # This causes the chain to break at this point.
+        class StopDryRun < Chain::Base
+          def perform!
+            # no-op
+          end
+
+          def break?
+            @command.dry_run?
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/pipeline/metrics.rb

@@ -36,6 +36,15 @@ module Gitlab
             Gitlab::Metrics.counter(name, comment)
           end
         end
+
+        def pipelines_created_counter
+          strong_memoize(:pipelines_created_count) do
+            name = :pipelines_created_total
+            comment = 'Counter of pipelines created'
+
+            Gitlab::Metrics.counter(name, comment)
+          end
+        end
       end
     end
   end

spec/features/merge_request/user_sees_merge_request_pipelines_spec.rb

@@ -36,7 +36,7 @@ RSpec.describe 'Merge request > User sees pipelines triggered by merge request',
   end
 
   context 'when a user created a merge request in the parent project' do
-    let(:merge_request) do
+    let!(:merge_request) do
       create(:merge_request,
               source_project: project,
               target_project: project,

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

@@ -23,9 +23,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Sequence do
     end
 
     it 'does not process the second step' do
-      subject.build! do |pipeline, sequence|
-        expect(sequence).not_to be_complete
-      end
+      subject.build!
 
       expect(second_step).not_to have_received(:perform!)
     end
@@ -43,9 +41,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Sequence do
     end
 
     it 'iterates through entire sequence' do
-      subject.build! do |pipeline, sequence|
-        expect(sequence).to be_complete
-      end
+      subject.build!
 
       expect(first_step).to have_received(:perform!)
       expect(second_step).to have_received(:perform!)

spec/lib/gitlab/ci/pipeline/chain/validate/external_spec.rb

@@ -41,9 +41,10 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Validate::External do
     )
   end
 
+  let(:save_incompleted) { true }
   let(:command) do
     Gitlab::Ci::Pipeline::Chain::Command.new(
-      project: project, current_user: user, config_processor: yaml_processor
+      project: project, current_user: user, config_processor: yaml_processor, save_incompleted: save_incompleted
     )
   end
 
@@ -84,6 +85,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Validate::External do
         perform!
 
         expect(pipeline.status).to eq('failed')
+        expect(pipeline).to be_persisted
         expect(pipeline.errors.to_a).to include('External validation failed')
       end
 
@@ -98,6 +100,30 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Validate::External do
 
         perform!
       end
+
+      context 'when save_incompleted is false' do
+        let(:save_incompleted) { false}
+
+        it 'adds errors to the pipeline without dropping it' do
+          perform!
+
+          expect(pipeline.status).to eq('pending')
+          expect(pipeline).not_to be_persisted
+          expect(pipeline.errors.to_a).to include('External validation failed')
+        end
+
+        it 'breaks the chain' do
+          perform!
+
+          expect(step.break?).to be true
+        end
+
+        it 'logs the authorization' do
+          expect(Gitlab::AppLogger).to receive(:info).with(message: 'Pipeline not authorized', project_id: project.id, user_id: user.id)
+
+          perform!
+        end
+      end
     end
   end
 

spec/services/ci/create_pipeline_service/dry_run_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::CreatePipelineService do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:admin) }
+  let(:ref) { 'refs/heads/master' }
+  let(:service) { described_class.new(project, user, { ref: ref }) }
+
+  subject { service.execute(:push, dry_run: true) }
+
+  before do
+    stub_ci_pipeline_yaml_file(config)
+  end
+
+  describe 'dry run' do
+    shared_examples 'returns a non persisted pipeline' do
+      it 'does not persist the pipeline' do
+        expect(subject).not_to be_persisted
+        expect(subject.id).to be_nil
+      end
+
+      it 'does not process the pipeline' do
+        expect(Ci::ProcessPipelineService).not_to receive(:new)
+
+        subject
+      end
+
+      it 'does not schedule merge request head pipeline update' do
+        expect(service).not_to receive(:schedule_head_pipeline_update)
+
+        subject
+      end
+    end
+
+    context 'when pipeline is valid' do
+      let(:config) { gitlab_ci_yaml }
+
+      it_behaves_like 'returns a non persisted pipeline'
+
+      it 'returns a valid pipeline' do
+        expect(subject.error_messages).to be_empty
+        expect(subject.yaml_errors).to be_nil
+        expect(subject.errors).to be_empty
+      end
+    end
+
+    context 'when pipeline is not valid' do
+      context 'when there are syntax errors' do
+        let(:config) do
+          <<~YAML
+            rspec:
+              script: echo
+              something: wrong
+          YAML
+        end
+
+        it_behaves_like 'returns a non persisted pipeline'
+
+        it 'returns a pipeline with errors', :aggregate_failures do
+          error_message = 'jobs:rspec config contains unknown keys: something'
+
+          expect(subject.error_messages.map(&:content)).to eq([error_message])
+          expect(subject.errors).not_to be_empty
+          expect(subject.yaml_errors).to eq(error_message)
+        end
+      end
+
+      context 'when there are logical errors' do
+        let(:config) do
+          <<~YAML
+            build:
+              script: echo
+              stage: build
+              needs: [test]
+            test:
+              script: echo
+              stage: test
+          YAML
+        end
+
+        it_behaves_like 'returns a non persisted pipeline'
+
+        it 'returns a pipeline with errors', :aggregate_failures do
+          error_message = 'build job: need test is not defined in prior stages'
+
+          expect(subject.error_messages.map(&:content)).to eq([error_message])
+          expect(subject.errors).not_to be_empty
+        end
+      end
+
+      context 'when there are errors at the seeding stage' do
+        let(:config) do
+          <<~YAML
+            build:
+              stage: build
+              script: echo
+              rules:
+                - if: '$CI_MERGE_REQUEST_ID'
+            test:
+              stage: test
+              script: echo
+              needs: ['build']
+          YAML
+        end
+
+        it_behaves_like 'returns a non persisted pipeline'
+
+        it 'returns a pipeline with errors', :aggregate_failures do
+          error_message = "test: needs 'build'"
+
+          expect(subject.error_messages.map(&:content)).to eq([error_message])
+          expect(subject.errors).not_to be_empty
+        end
+      end
+    end
+  end
+end

spec/services/ci/create_pipeline_service/parameter_content_spec.rb

@@ -49,14 +49,5 @@ RSpec.describe Ci::CreatePipelineService do
         end
       end
     end
-
-    context 'when source is not a dangling build' do
-      subject { service.execute(:web, content: content) }
-
-      it 'raises an exception' do
-        klass = Gitlab::Ci::Pipeline::Chain::Config::Content::Parameter::UnsupportedSourceError
-        expect { subject }.to raise_error(klass)
-      end
-    end
   end
 end

spec/services/ci/create_pipeline_service_spec.rb

@@ -1692,16 +1692,23 @@ RSpec.describe Ci::CreatePipelineService do
       context 'when pipeline on feature is created' do
         let(:ref_name) { 'refs/heads/feature' }
 
+        shared_examples 'has errors' do
+          it 'contains the expected errors' do
+            expect(pipeline.builds).to be_empty
+            expect(pipeline.yaml_errors).to eq("test_a: needs 'build_a'")
+            expect(pipeline.error_messages.map(&:content)).to contain_exactly("test_a: needs 'build_a'")
+            expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'")
+          end
+        end
+
         context 'when save_on_errors is enabled' do
           let(:pipeline) { execute_service(save_on_errors: true) }
 
           it 'does create a pipeline as test_a depends on build_a' do
             expect(pipeline).to be_persisted
-            expect(pipeline.builds).to be_empty
-            expect(pipeline.yaml_errors).to eq("test_a: needs 'build_a'")
-            expect(pipeline.messages.pluck(:content)).to contain_exactly("test_a: needs 'build_a'")
-            expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'")
           end
+
+          it_behaves_like 'has errors'
         end
 
         context 'when save_on_errors is disabled' do
@@ -1709,11 +1716,9 @@ RSpec.describe Ci::CreatePipelineService do
 
           it 'does not create a pipeline as test_a depends on build_a' do
             expect(pipeline).not_to be_persisted
-            expect(pipeline.builds).to be_empty
-            expect(pipeline.yaml_errors).to be_nil
-            expect(pipeline.messages).not_to be_empty
-            expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'")
           end
+
+          it_behaves_like 'has errors'
         end
       end