Remove the save flag

app/services/ci/pipelines/add_job_service.rb

@@ -11,23 +11,23 @@ module Ci
         raise ArgumentError, "Pipeline must be persisted for this service to be used" unless @pipeline.persisted?
       end
 
-      def execute!(job, save: true)
+      def execute!(job, &block)
         assign_pipeline_attributes(job)
 
         Ci::Pipeline.transaction do
-          BulkInsertableAssociations.with_bulk_insert { job.save! } if save
+          yield(job)
 
           job.update_older_statuses_retried! if Feature.enabled?(:ci_fix_commit_status_retried, @pipeline.project, default_enabled: :yaml)
         end
 
-        job
+        ServiceResponse.success(payload: { job: job })
+      rescue StandardError => e
+        ServiceResponse.error(message: e.message, payload: { job: job })
       end
 
       private
 
       def assign_pipeline_attributes(job)
-        # these also ensures other invariants such as
-        # a job takes project and ref from the pipeline it belongs to
         job.pipeline = @pipeline
         job.project = @pipeline.project
         job.ref = @pipeline.ref

app/services/ci/retry_build_service.rb

@@ -35,7 +35,11 @@ module Ci
       check_access!(build)
 
       new_build = clone_build(build)
-      ::Ci::Pipelines::AddJobService.new(build.pipeline).execute!(new_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

app/services/projects/update_pages_service.rb

@@ -32,10 +32,10 @@ module Projects
 
       # Create status notifying the deployment of pages
       @status = build_commit_status
-      ::Ci::Pipelines::AddJobService.new(@build.pipeline).execute!(@status)
-
-      @status.enqueue!
-      @status.run!
+      ::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?

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?
-          ::Ci::Pipelines::AddJobService.new(pipeline).execute!(status, save: false)
-        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)
+        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
 
+        render_validation_error!(response.payload[:job]) unless response.success?
+
         if pipeline.latest?
-            MergeRequest.where(source_project: user_project, source_branch: ref)
+          MergeRequest
+            .where(source_project: user_project, source_branch: ref)
             .update_all(head_pipeline_id: pipeline.id)
         end
 
-          present status, with: Entities::CommitStatus
-        rescue StateMachines::InvalidTransition => e
-          render_api_error!(e.message, 400)
-        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

@@ -17,22 +17,44 @@ RSpec.describe Ci::Pipelines::AddJobService do
     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
-      service.execute!(job)
+        execute
       end.to change { job.slice(:pipeline, :project, :ref) }.to(
         pipeline: pipeline, project: pipeline.project, ref: pipeline.ref
       )
     end
 
-  it 'returns the job itself' do
-    expect(service.execute!(job)).to eq(job)
+    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!)
 
-    service.execute!(job)
+      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
@@ -43,7 +65,8 @@ RSpec.describe Ci::Pipelines::AddJobService do
       it 'does not call update_older_statuses_retried!' do
         expect(job).not_to receive(:update_older_statuses_retried!)
 
-      service.execute!(job)
+        execute
+      end
     end
   end
 end