From f6aaabeff28e897d7f91aa5aed448e7b82f91160 Mon Sep 17 00:00:00 2001
From: Laura Montemayor <lmontemayor@gitlab.com>
Date: Tue, 9 Jul 2024 04:22:59 +0000
Subject: [PATCH] Remove EnsureStageService and prep for removal of `stage`
* Add the ignore_column for `stage`
* Remove the service from rubocop_todo
* Remove `stage` from clone_accessors in job models
Changelog: removed
---
.../naming/heredoc_delimiter_naming.yml | 1 -
.../style/inline_disable_annotation.yml | 1 -
app/models/commit_status.rb | 17 +----
app/services/ci/ensure_stage_service.rb | 56 ---------------
.../ci_remove_ensure_stage_service.yml | 8 ---
spec/factories/ci/processable.rb | 4 +-
spec/models/commit_status_spec.rb | 70 -------------------
spec/services/ci/ensure_stage_service_spec.rb | 57 ---------------
spec/support/rspec_order_todo.yml | 1 -
9 files changed, 3 insertions(+), 212 deletions(-)
delete mode 100644 app/services/ci/ensure_stage_service.rb
delete mode 100644 config/feature_flags/development/ci_remove_ensure_stage_service.yml
delete mode 100644 spec/services/ci/ensure_stage_service_spec.rb
diff --git a/.rubocop_todo/naming/heredoc_delimiter_naming.yml b/.rubocop_todo/naming/heredoc_delimiter_naming.yml
index 09e03a992dd0..cb457fd73794 100644
--- a/.rubocop_todo/naming/heredoc_delimiter_naming.yml
+++ b/.rubocop_todo/naming/heredoc_delimiter_naming.yml
@@ -4,7 +4,6 @@ Naming/HeredocDelimiterNaming:
- 'app/models/ci/build_trace_chunks/redis_base.rb'
- 'app/models/concerns/legacy_bulk_insert.rb'
- 'app/models/trending_project.rb'
- - 'app/services/ci/ensure_stage_service.rb'
- 'app/services/packages/debian/generate_distribution_key_service.rb'
- 'app/workers/concerns/limited_capacity/job_tracker.rb'
- 'config/initializers/01_secret_token.rb'
diff --git a/.rubocop_todo/style/inline_disable_annotation.yml b/.rubocop_todo/style/inline_disable_annotation.yml
index 99822bee577f..5044797c8363 100644
--- a/.rubocop_todo/style/inline_disable_annotation.yml
+++ b/.rubocop_todo/style/inline_disable_annotation.yml
@@ -578,7 +578,6 @@ Style/InlineDisableAnnotation:
- 'app/services/ci/create_pipeline_service.rb'
- 'app/services/ci/delete_objects_service.rb'
- 'app/services/ci/delete_unit_tests_service.rb'
- - 'app/services/ci/ensure_stage_service.rb'
- 'app/services/ci/expire_pipeline_cache_service.rb'
- 'app/services/ci/job_artifacts/bulk_delete_by_project_service.rb'
- 'app/services/ci/job_artifacts/destroy_batch_service.rb'
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 2b4feb80435c..6bfdc906bb8a 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -51,7 +51,7 @@ class CommitStatus < Ci::ApplicationRecord
validates :pipeline, presence: true, unless: :importing?
validates :name, presence: true, unless: :importing?
- validates :ci_stage, presence: true, on: :create, unless: :importing?, if: -> { Feature.enabled?(:ci_remove_ensure_stage_service, project) }
+ validates :ci_stage, presence: true, on: :create, unless: :importing?
validates :ref, :target_url, :description, length: { maximum: 255 }
alias_attribute :author, :user
@@ -120,21 +120,6 @@ class CommitStatus < Ci::ApplicationRecord
merge(or_conditions)
end
- ##
- # We still create some CommitStatuses outside of CreatePipelineService.
- #
- # These are pages deployments and external statuses.
- #
- before_create unless: :importing? do
- next if Feature.enabled?(:ci_remove_ensure_stage_service, project)
-
- # rubocop: disable CodeReuse/ServiceClass
- Ci::EnsureStageService.new(project, user).execute(self) do |stage|
- self.run_after_commit { StageUpdateWorker.perform_async(stage.id) }
- end
- # rubocop: enable CodeReuse/ServiceClass
- end
-
before_save if: :status_changed?, unless: :importing? do
# we mark `processed` as always changed:
# another process might change its value and our object
diff --git a/app/services/ci/ensure_stage_service.rb b/app/services/ci/ensure_stage_service.rb
deleted file mode 100644
index 9d5ccecbe338..000000000000
--- a/app/services/ci/ensure_stage_service.rb
+++ /dev/null
@@ -1,56 +0,0 @@
-# frozen_string_literal: true
-
-module Ci
- ##
- # We call this service everytime we persist a CI/CD job.
- #
- # In most cases a job should already have a stage assigned, but in cases it
- # doesn't have we need to either find existing one or create a brand new
- # stage.
- #
- class EnsureStageService < BaseService
- EnsureStageError = Class.new(StandardError)
-
- def execute(build)
- @build = build
-
- return if build.stage_id.present?
- return if build.invalid?
-
- ensure_stage.tap do |stage|
- build.stage_id = stage.id
-
- yield stage if block_given?
- end
- end
-
- private
-
- def ensure_stage(attempts: 2)
- find_stage || create_stage
- rescue ActiveRecord::RecordNotUnique
- retry if (attempts -= 1) > 0
-
- raise EnsureStageError, <<~EOS
- We failed to find or create a unique pipeline stage after 2 retries.
- This should never happen and is most likely the result of a bug in
- the database load balancing code.
- EOS
- end
-
- # rubocop: disable CodeReuse/ActiveRecord
- def find_stage
- @build.pipeline.stages.find_by(name: @build.stage)
- end
- # rubocop: enable CodeReuse/ActiveRecord
-
- def create_stage
- Ci::Stage.create!(
- name: @build.stage,
- position: @build.stage_idx,
- pipeline: @build.pipeline,
- project: @build.project
- )
- end
- end
-end
diff --git a/config/feature_flags/development/ci_remove_ensure_stage_service.yml b/config/feature_flags/development/ci_remove_ensure_stage_service.yml
deleted file mode 100644
index a742891d3475..000000000000
--- a/config/feature_flags/development/ci_remove_ensure_stage_service.yml
+++ /dev/null
@@ -1,8 +0,0 @@
----
-name: ci_remove_ensure_stage_service
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/107389
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/389076
-milestone: '15.9'
-type: development
-group: group::pipeline authoring
-default_enabled: true
diff --git a/spec/factories/ci/processable.rb b/spec/factories/ci/processable.rb
index cfe736bce500..221abcadc459 100644
--- a/spec/factories/ci/processable.rb
+++ b/spec/factories/ci/processable.rb
@@ -25,7 +25,7 @@
end
after(:build) do |processable, evaluator|
- next if processable.ci_stage || Feature.disabled?(:ci_remove_ensure_stage_service, processable.project)
+ next if processable.ci_stage
processable.stage = evaluator.stage
pipeline = processable.pipeline
@@ -57,7 +57,7 @@
end
before(:create) do |processable, evaluator|
- next if processable.ci_stage || Feature.disabled?(:ci_remove_ensure_stage_service, processable.project)
+ next if processable.ci_stage
processable.ci_stage =
if ci_stage = processable.pipeline.stages.find_by(name: evaluator.stage)
diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb
index 52f3aaae3e75..c3cba6a45d33 100644
--- a/spec/models/commit_status_spec.rb
+++ b/spec/models/commit_status_spec.rb
@@ -832,76 +832,6 @@ def create_status(**opts)
end
end
- describe 'ensure stage assignment' do
- before do
- stub_feature_flags(ci_remove_ensure_stage_service: false)
- end
-
- context 'when the feature flag ci_remove_ensure_stage_service is disabled' do
- context 'when commit status has a stage_id assigned' do
- let!(:stage) do
- create(:ci_stage, project: project, pipeline: pipeline)
- end
-
- let(:commit_status) do
- create(:commit_status, stage_id: stage.id, name: 'rspec', stage: 'test')
- end
-
- it 'does not create a new stage' do
- expect { commit_status }.not_to change { Ci::Stage.count }
- expect(commit_status.stage_id).to eq stage.id
- end
- end
-
- context 'when commit status does not have a stage_id assigned' do
- let(:commit_status) do
- FactoryBot.build(:commit_status, name: 'rspec', stage: 'test', status: :success)
- end
-
- let(:stage) { Ci::Stage.first }
-
- it 'creates a new stage', :sidekiq_might_not_need_inline do
- expect { commit_status.save! }.to change { Ci::Stage.count }.by(1)
-
- expect(stage.name).to eq 'test'
- expect(stage.project).to eq commit_status.project
- expect(stage.pipeline).to eq commit_status.pipeline
- expect(stage.status).to eq commit_status.status
- expect(commit_status.stage_id).to eq stage.id
- end
- end
-
- context 'when commit status does not have stage but it exists' do
- let!(:stage) do
- create(:ci_stage, project: project, pipeline: pipeline, name: 'test')
- end
-
- let(:commit_status) do
- FactoryBot.build(:commit_status, project: project, pipeline: pipeline, name: 'rspec', stage: 'test',
- status: :success)
- end
-
- it 'uses existing stage', :sidekiq_might_not_need_inline do
- expect { commit_status.save! }.not_to change { Ci::Stage.count }
-
- expect(commit_status.stage_id).to eq stage.id
- expect(stage.reload.status).to eq commit_status.status
- end
- end
-
- context 'when commit status is being imported' do
- let(:commit_status) do
- FactoryBot.build(:commit_status, name: 'rspec', stage: 'test', importing: true)
- end
-
- it 'does not create a new stage' do
- expect { commit_status.save! }.not_to change { Ci::Stage.count }
- expect(commit_status.stage_id).not_to be_present
- end
- end
- end
- end
-
describe '#all_met_to_become_pending?' do
subject { commit_status.all_met_to_become_pending? }
diff --git a/spec/services/ci/ensure_stage_service_spec.rb b/spec/services/ci/ensure_stage_service_spec.rb
deleted file mode 100644
index 8debc5eab73b..000000000000
--- a/spec/services/ci/ensure_stage_service_spec.rb
+++ /dev/null
@@ -1,57 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Ci::EnsureStageService, '#execute', feature_category: :continuous_integration do
- let_it_be(:project) { create(:project) }
- let_it_be(:user) { create(:user) }
-
- let(:stage) { create(:ci_stage) }
- let(:job) { build(:ci_build) }
-
- let(:service) { described_class.new(project, user) }
-
- before do
- stub_feature_flags(ci_remove_ensure_stage_service: false)
- end
-
- context 'when build has a stage assigned' do
- it 'does not create a new stage' do
- job.assign_attributes(stage_id: stage.id)
-
- expect { service.execute(job) }.not_to change { Ci::Stage.count }
- end
- end
-
- context 'when build does not have a stage assigned' do
- it 'creates a new stage' do
- job.assign_attributes(stage_id: nil, stage: 'test')
-
- expect { service.execute(job) }.to change { Ci::Stage.count }.by(1)
- end
- end
-
- context 'when build is invalid' do
- it 'does not create a new stage' do
- job.assign_attributes(stage_id: nil, ref: nil)
-
- expect { service.execute(job) }.not_to change { Ci::Stage.count }
- end
- end
-
- context 'when new stage can not be created because of an exception' do
- before do
- allow(Ci::Stage).to receive(:create!)
- .and_raise(ActiveRecord::RecordNotUnique.new('Duplicates!'))
- end
-
- it 'retries up to two times' do
- job.assign_attributes(stage_id: nil)
-
- expect(service).to receive(:find_stage).twice
-
- expect { service.execute(job) }
- .to raise_error(Ci::EnsureStageService::EnsureStageError)
- end
- end
-end
diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml
index c093558f1c52..2556b0b57721 100644
--- a/spec/support/rspec_order_todo.yml
+++ b/spec/support/rspec_order_todo.yml
@@ -8153,7 +8153,6 @@
- './spec/services/ci/destroy_pipeline_service_spec.rb'
- './spec/services/ci/destroy_secure_file_service_spec.rb'
- './spec/services/ci/drop_pipeline_service_spec.rb'
-- './spec/services/ci/ensure_stage_service_spec.rb'
- './spec/services/ci/expire_pipeline_cache_service_spec.rb'
- './spec/services/ci/external_pull_requests/create_pipeline_service_spec.rb'
- './spec/services/ci/find_exposed_artifacts_service_spec.rb'
--
GitLab