Skip to content

`CreatePipelineService#execute!` does not consider `save_on_errors: true`

The current code of Ci::CreatePipelineService#execute! method checks if a pipeline is persisted. If so, it considers it valid, otherwise it raises a CreateError.

def execute!(*args, &block)
  execute(*args, &block).tap do |pipeline|
    unless pipeline.persisted?
      raise CreateError, pipeline.error_messages
    end
  end
end

This logic is not always correct, quite the opposite. By default we use in save_on_errors: true which always saves a pipeline, even if it's "invalid" (with errors during the creation phase). Checking if a pipeline is persisted only makes sense if using save_on_errors: false.

I think we should remove this method entirely and change the code in the only place where it's used today: RunPipelineScheduleWorker and Git::BaseHookService

Diff to apply (excluding specs to fix):
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index d53e136effb..16f189a3e4c 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -4,8 +4,6 @@ module Ci
   class CreatePipelineService < BaseService
     attr_reader :pipeline, :logger

-    CreateError = Class.new(StandardError)
-
     LOG_MAX_DURATION_THRESHOLD = 3.seconds
     LOG_MAX_PIPELINE_SIZE = 2_000
     LOG_MAX_CREATION_THRESHOLD = 20.seconds
@@ -116,17 +114,6 @@ def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request
     end
     # rubocop: enable Metrics/ParameterLists

-    def execute!(*args, &block)
-      source = args[0]
-      params = Hash(args[1])
-
-      execute(source, **params, &block).tap do |response|
-        unless response.payload.persisted?
-          raise CreateError, pipeline.full_error_messages
-        end
-      end
-    end
-
     private

     def commit
diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb
index 63f3f73905a..a4687402637 100644
--- a/app/services/git/base_hooks_service.rb
+++ b/app/services/git/base_hooks_service.rb
@@ -53,11 +53,11 @@ def create_events
     def create_pipelines
       return unless params.fetch(:create_pipelines, true)

-      Ci::CreatePipelineService
+      response = Ci::CreatePipelineService
         .new(project, current_user, pipeline_params)
-        .execute!(:push, pipeline_options)
-    rescue Ci::CreatePipelineService::CreateError => ex
-      log_pipeline_errors(ex)
+        .execute(:push, pipeline_options)
+
+      log_pipeline_errors(response.message) if response.error?
     end

     def execute_project_hooks
@@ -148,14 +148,14 @@ def pipeline_options
       {}
     end

-    def log_pipeline_errors(exception)
+    def log_pipeline_errors(error_message)
       data = {
         class: self.class.name,
         correlation_id: Labkit::Correlation::CorrelationId.current_id.to_s,
         project_id: project.id,
         project_path: project.full_path,
         message: "Error creating pipeline",
-        errors: exception.to_s,
+        errors: error_message,
         pipeline_params: sanitized_pipeline_params
       }

diff --git a/app/workers/run_pipeline_schedule_worker.rb b/app/workers/run_pipeline_schedule_worker.rb
index 35e3e633c70..31cf13b8a8b 100644
--- a/app/workers/run_pipeline_schedule_worker.rb
+++ b/app/workers/run_pipeline_schedule_worker.rb
@@ -21,13 +21,14 @@ def perform(schedule_id, user_id)
   end

   def run_pipeline_schedule(schedule, user)
-    Ci::CreatePipelineService.new(schedule.project,
-                                  user,
-                                  ref: schedule.ref)
-      .execute!(:schedule, ignore_skip_ci: true, save_on_errors: false, schedule: schedule)
-  rescue Ci::CreatePipelineService::CreateError => e
-    # This is a user operation error such as corrupted .gitlab-ci.yml. Log the error for debugging purpose.
-    log_extra_metadata_on_done(:pipeline_creation_error, e)
+    response = Ci::CreatePipelineService
+      .new(schedule.project, user, ref: schedule.ref)
+      .execute(:schedule, ignore_skip_ci: true, save_on_errors: false, schedule: schedule)
+
+    if response.error?
+      # This is a user operation error such as corrupted .gitlab-ci.yml. Log the error for debugging purpose.
+      log_extra_metadata_on_done(:pipeline_creation_error, response.message)
+    end
   rescue StandardError => e
     error(schedule, e)
   end
diff --git a/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md b/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md
index 265c5278fd6..708877cd179 100644
--- a/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md
+++ b/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md
@@ -965,7 +965,8 @@ schedule = Ci::PipelineSchedule.find_by(id: <schedule_id>)
 user = User.find_by_username('<username>')

 # Run the schedule
-ps = Ci::CreatePipelineService.new(schedule.project, user, ref: schedule.ref).execute!(:schedule, ignore_skip_ci: true, save_on_errors: false, schedule: schedule)
+response = Ci::CreatePipelineService.new(schedule.project, user, ref: schedule.ref).execute(:schedule, ignore_skip_ci: true, save_on_errors: false, schedule: schedule)
+response.success?
Edited by Fabio Pitino