`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