Merge branch 'explicit-pipeline-add-job-method' into 'master'

Extract Pipeline add-job service to remove duplicate logic

See merge request gitlab-org/gitlab!63620

app/services/ci/pipelines/add_job_service.rb

+# frozen_string_literal: true
+
+module Ci
+  module Pipelines
+    class AddJobService
+      attr_reader :pipeline
+
+      def initialize(pipeline)
+        @pipeline = pipeline
+
+        raise ArgumentError, "Pipeline must be persisted for this service to be used" unless @pipeline.persisted?
+      end
+
+      def execute!(job, &block)
+        assign_pipeline_attributes(job)
+
+        Ci::Pipeline.transaction do
+          yield(job)
+
+          job.update_older_statuses_retried! if Feature.enabled?(:ci_fix_commit_status_retried, @pipeline.project, default_enabled: :yaml)
+        end
+
+        ServiceResponse.success(payload: { job: job })
+      rescue StandardError => e
+        ServiceResponse.error(message: e.message, payload: { job: job })
+      end
+
+      private
+
+      def assign_pipeline_attributes(job)
+        job.pipeline = @pipeline
+        job.project = @pipeline.project
+        job.ref = @pipeline.ref
+      end
+    end
+  end
+end

app/services/ci/retry_build_service.rb

@@ -34,18 +34,15 @@ module Ci
     def reprocess!(build)
       check_access!(build)
 
-      attributes = self.class.clone_accessors.to_h do |attribute|
-        [attribute, build.public_send(attribute)] # rubocop:disable GitlabSecurity/PublicSend
-      end
-
-      attributes[:user] = current_user
-
-      Ci::Build.transaction do
-        create_build!(attributes).tap do |new_build|
-          new_build.update_older_statuses_retried!
-          build.reset # refresh the data to get new values of `retried` and `processed`.
+      new_build = clone_build(build)
+      ::Ci::Pipelines::AddJobService.new(build.pipeline).execute!(new_build) do |job|
+        BulkInsertableAssociations.with_bulk_insert do
+          job.save!
         end
       end
+      build.reset # refresh the data to get new values of `retried` and `processed`.
+
+      new_build
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -59,13 +56,19 @@ module Ci
 
     def check_assignable_runners!(build); end
 
-    def create_build!(attributes)
-      build = project.builds.new(attributes)
-      build.assign_attributes(::Gitlab::Ci::Pipeline::Seed::Build.environment_attributes_for(build))
-      BulkInsertableAssociations.with_bulk_insert do
-        build.save!
+    def clone_build(build)
+      project.builds.new(build_attributes(build)).tap do |new_build|
+        new_build.assign_attributes(::Gitlab::Ci::Pipeline::Seed::Build.environment_attributes_for(new_build))
       end
-      build
+    end
+
+    def build_attributes(build)
+      attributes = self.class.clone_accessors.to_h do |attribute|
+        [attribute, build.public_send(attribute)] # rubocop:disable GitlabSecurity/PublicSend
+      end
+
+      attributes[:user] = current_user
+      attributes
     end
   end
 end

app/services/projects/update_pages_service.rb

@@ -31,10 +31,11 @@ module Projects
       register_attempt
 
       # Create status notifying the deployment of pages
-      @status = create_status
-      @status.update_older_statuses_retried! if Feature.enabled?(:ci_fix_commit_status_retried, project, default_enabled: :yaml)
-      @status.enqueue!
-      @status.run!
+      @status = build_commit_status
+      ::Ci::Pipelines::AddJobService.new(@build.pipeline).execute!(@status) do |job|
+        job.enqueue!
+        job.run!
+      end
 
       raise InvalidStateError, 'missing pages artifacts' unless build.artifacts?
       raise InvalidStateError, 'build SHA is outdated for this ref' unless latest?
@@ -70,12 +71,9 @@ module Projects
       super
     end
 
-    def create_status
+    def build_commit_status
       GenericCommitStatus.new(
-        project: project,
-        pipeline: build.pipeline,
         user: build.user,
-        ref: build.ref,
         stage: 'deploy',
         name: 'pages:deploy'
       )

ee/spec/services/ci/retry_build_service_spec.rb

@@ -4,7 +4,8 @@ require 'spec_helper'
 RSpec.describe Ci::RetryBuildService do
   let_it_be(:user) { create(:user) }
 
-  let(:build) { create(:ci_build, project: project) }
+  let(:pipeline) { create(:ci_pipeline, project: project) }
+  let(:build) { create(:ci_build, pipeline: pipeline) }
 
   subject(:service) { described_class.new(project, user) }
 

lib/api/commit_statuses.rb

@@ -99,40 +99,26 @@ module API
         updatable_optional_attributes = %w[target_url description coverage]
         status.assign_attributes(attributes_for_keys(updatable_optional_attributes))
 
-        if status.valid?
-          status.update_older_statuses_retried! if Feature.enabled?(:ci_fix_commit_status_retried, user_project, default_enabled: :yaml)
-        else
-          render_validation_error!(status)
-        end
+        render_validation_error!(status) unless status.valid?
 
-        begin
-          case params[:state]
-          when 'pending'
-            status.enqueue!
-          when 'running'
-            status.enqueue
-            status.run!
-          when 'success'
-            status.success!
-          when 'failed'
-            status.drop!(:api_failure)
-          when 'canceled'
-            status.cancel!
-          else
-            render_api_error!('invalid state', 400)
-          end
+        response = ::Ci::Pipelines::AddJobService.new(pipeline).execute!(status) do |job|
+          apply_job_state!(job)
+        rescue ::StateMachines::InvalidTransition => e
+          render_api_error!(e.message, 400)
+        end
 
-          if pipeline.latest?
-            MergeRequest.where(source_project: user_project, source_branch: ref)
-              .update_all(head_pipeline_id: pipeline.id)
-          end
+        render_validation_error!(response.payload[:job]) unless response.success?
 
-          present status, with: Entities::CommitStatus
-        rescue StateMachines::InvalidTransition => e
-          render_api_error!(e.message, 400)
+        if pipeline.latest?
+          MergeRequest
+            .where(source_project: user_project, source_branch: ref)
+            .update_all(head_pipeline_id: pipeline.id)
         end
+
+        present response.payload[:job], with: Entities::CommitStatus
       end
       # rubocop: enable CodeReuse/ActiveRecord
+
       helpers do
         def commit
           strong_memoize(:commit) do
@@ -146,6 +132,24 @@ module API
           pipelines = pipelines.for_id(params[:pipeline_id]) if params[:pipeline_id]
           pipelines
         end
+
+        def apply_job_state!(job)
+          case params[:state]
+          when 'pending'
+            job.enqueue!
+          when 'running'
+            job.enqueue
+            job.run!
+          when 'success'
+            job.success!
+          when 'failed'
+            job.drop!(:api_failure)
+          when 'canceled'
+            job.cancel!
+          else
+            render_api_error!('invalid state', 400)
+          end
+        end
       end
     end
   end

spec/services/ci/pipelines/add_job_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::Pipelines::AddJobService do
+  let_it_be(:pipeline) { create(:ci_pipeline) }
+
+  let(:job) { build(:ci_build) }
+
+  subject(:service) { described_class.new(pipeline) }
+
+  context 'when the pipeline is not persisted' do
+    let(:pipeline) { build(:ci_pipeline) }
+
+    it 'raises error' do
+      expect { service }.to raise_error('Pipeline must be persisted for this service to be used')
+    end
+  end
+
+  describe '#execute!' do
+    subject(:execute) do
+      service.execute!(job) do |job|
+        job.save!
+      end
+    end
+
+    it 'assigns pipeline attributes to the job' do
+      expect do
+        execute
+      end.to change { job.slice(:pipeline, :project, :ref) }.to(
+        pipeline: pipeline, project: pipeline.project, ref: pipeline.ref
+      )
+    end
+
+    it 'returns a service response with the job as payload' do
+      expect(execute).to be_success
+      expect(execute.payload[:job]).to eq(job)
+    end
+
+    it 'calls update_older_statuses_retried!' do
+      expect(job).to receive(:update_older_statuses_retried!)
+
+      execute
+    end
+
+    context 'when the block raises an error' do
+      subject(:execute) do
+        service.execute!(job) do |job|
+          raise "this is an error"
+        end
+      end
+
+      it 'returns a service response with the error and the job as payload' do
+        expect(execute).to be_error
+        expect(execute.payload[:job]).to eq(job)
+        expect(execute.message).to eq('this is an error')
+      end
+    end
+
+    context 'when the FF ci_fix_commit_status_retried is disabled' do
+      before do
+        stub_feature_flags(ci_fix_commit_status_retried: false)
+      end
+
+      it 'does not call update_older_statuses_retried!' do
+        expect(job).not_to receive(:update_older_statuses_retried!)
+
+        execute
+      end
+    end
+  end
+end