From 7eb050ba32be302d1c95e8c29a88562481233db1 Mon Sep 17 00:00:00 2001 From: Dmitry Biryukov Date: Tue, 22 Apr 2025 23:02:16 +0200 Subject: [PATCH 01/11] Add a worker to update minimum override role Add a all_queueus reference Defer a worker Changelog: added --- GITLAB_ZOEKT_VERSION | 1 - app/models/project_ci_cd_setting.rb | 4 ++ ...safe_disable_pipeline_variables_service.rb | 61 +++++++++++++++++++ app/workers/all_queues.yml | 10 +++ .../safe_disable_pipeline_variables_worker.rb | 27 ++++++++ db/structure.sql | 1 + ...disable_pipeline_variables_service_spec.rb | 58 ++++++++++++++++++ ..._disable_pipeline_variables_worker_spec.rb | 21 +++++++ 8 files changed, 182 insertions(+), 1 deletion(-) delete mode 100644 GITLAB_ZOEKT_VERSION create mode 100644 app/services/ci/safe_disable_pipeline_variables_service.rb create mode 100644 app/workers/ci/safe_disable_pipeline_variables_worker.rb create mode 100644 spec/services/ci/safe_disable_pipeline_variables_service_spec.rb create mode 100644 spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb diff --git a/GITLAB_ZOEKT_VERSION b/GITLAB_ZOEKT_VERSION deleted file mode 100644 index 04a373efe6ba8b..00000000000000 --- a/GITLAB_ZOEKT_VERSION +++ /dev/null @@ -1 +0,0 @@ -0.16.0 diff --git a/app/models/project_ci_cd_setting.rb b/app/models/project_ci_cd_setting.rb index e369865b13f9c8..1d9b09b42945f5 100644 --- a/app/models/project_ci_cd_setting.rb +++ b/app/models/project_ci_cd_setting.rb @@ -56,11 +56,15 @@ class ProjectCiCdSetting < ApplicationRecord chronic_duration_attr :runner_token_expiration_interval_human_readable, :runner_token_expiration_interval chronic_duration_attr_writer :delete_pipelines_in_human_readable, :delete_pipelines_in_seconds + scope :for_namespace, ->(namespace) { joins(:project).where({ projects: { namespace: namespace } }) } scope :for_project, ->(ids) { where(project_id: ids) } scope :order_project_id_asc, -> { order(project_id: :asc) } scope :configured_to_delete_old_pipelines, -> do where.not(delete_pipelines_in_seconds: nil) end + scope :with_pipeline_variables_enabled, -> do + where.not(pipeline_variables_minimum_override_role: NO_ONE_ALLOWED_ROLE) + end def self.pluck_project_id(limit) limit(limit).pluck(:project_id) diff --git a/app/services/ci/safe_disable_pipeline_variables_service.rb b/app/services/ci/safe_disable_pipeline_variables_service.rb new file mode 100644 index 00000000000000..e7b7ac30b11bed --- /dev/null +++ b/app/services/ci/safe_disable_pipeline_variables_service.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module Ci + class SafeDisablePipelineVariablesService + def initialize(group_id:, logger:) + @group_id = group_id + @logger = logger + end + + def execute + @logger.info start_message + + migrate! + + @logger.info finish_message + end + + private + + def migrate! + parent_group = ::Group.find(@group_id) + + dependant_groups = parent_group.self_and_descendants + @logger.info("Processing parent group_id=#{@group_id} " \ + "with group(s) including descendants") + + dependant_groups.each_batch do |batch_group| + all_projects_ci_cd_settings = ProjectCiCdSetting.for_namespace(batch_group) + .with_pipeline_variables_enabled + + @logger.info("Updating projects without pipeline variables in #{batch_group.count} groups.") + + all_projects_ci_cd_settings + .each_batch do |batch_settings| + # rubocop: disable CodeReuse/ActiveRecord -- This is a highly custom query for this service, that's why it's not in the model. + # rubocop: disable Database/AvoidUsingPluckWithoutLimit -- This pluck is already limited by batch size. + + project_ids = batch_settings.pluck(:project_id) + + projects_with_vars = + Ci::PipelineVariable.where(project_id: project_ids).distinct.pluck(:project_id) + + Ci::JobVariable.where(project_id: project_ids).distinct.pluck(:project_id) + + ProjectCiCdSetting.where(project_id: project_ids - projects_with_vars) + .update_all(pipeline_variables_minimum_override_role: ProjectCiCdSetting::NO_ONE_ALLOWED_ROLE) + + # rubocop:enable Database/AvoidUsingPluckWithoutLimit + # rubocop: enable CodeReuse/ActiveRecord + end + end + end + + def start_message + "Starting migration of minimum override role for project(s) in provided groups..." + end + + def finish_message + "Successfully completed migration for all applicable project(s)." + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 808d9f72c0f0c1..03e8dc3d344a39 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -3397,6 +3397,16 @@ :idempotent: true :tags: [] :queue_namespace: +- :name: ci_safe_disable_pipeline_variables + :worker_name: Ci::SafeDisablePipelineVariablesWorker + :feature_category: :ci_variables + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] + :queue_namespace: - :name: ci_job_artifacts_expire_project_build_artifacts :worker_name: Ci::JobArtifacts::ExpireProjectBuildArtifactsWorker :feature_category: :job_artifacts diff --git a/app/workers/ci/safe_disable_pipeline_variables_worker.rb b/app/workers/ci/safe_disable_pipeline_variables_worker.rb new file mode 100644 index 00000000000000..dd2201aa970efe --- /dev/null +++ b/app/workers/ci/safe_disable_pipeline_variables_worker.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Ci + class SafeDisablePipelineVariablesWorker + include ApplicationWorker + + data_consistency :sticky + + feature_category :ci_variables + urgency :low + idempotent! + + defer_on_database_health_signal :gitlab_main, [], 1.minute + + attr_accessor :group_id + + def perform(group_id) + Ci::SafeDisablePipelineVariablesService.new(group_id: group_id, logger: logger).execute + end + + private + + def logger + Sidekiq.logger + end + end +end diff --git a/db/structure.sql b/db/structure.sql index bf60a538deedf6..2f9374e9a327f9 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9116,6 +9116,7 @@ CREATE TABLE application_settings ( vscode_extension_marketplace jsonb DEFAULT '{}'::jsonb NOT NULL, token_prefixes jsonb DEFAULT '{}'::jsonb NOT NULL, ci_cd_settings jsonb DEFAULT '{}'::jsonb NOT NULL, + duo_nano_features_enabled boolean, database_reindexing jsonb DEFAULT '{}'::jsonb NOT NULL, duo_chat jsonb DEFAULT '{}'::jsonb NOT NULL, group_settings jsonb DEFAULT '{}'::jsonb NOT NULL, diff --git a/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb b/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb new file mode 100644 index 00000000000000..f004e5a88ae5e2 --- /dev/null +++ b/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::SafeDisablePipelineVariablesService, feature_category: :ci_variables do + let(:log_output) { StringIO.new } + let(:logger) { Logger.new(log_output) } + let(:service) { described_class.new(group_id: group_id, logger: logger) } + + describe '#execute' do + subject(:update_role) { service.execute } + + context 'when group_id correctly supplied' do + let_it_be(:group1) { create(:group) } + let_it_be(:sub_group1) { create(:group, parent: group1) } + let_it_be(:project_in_group1) { create(:project, group: group1) } + let_it_be(:project_in_sub_group1) { create(:project, group: sub_group1) } + let_it_be(:expected_updated_projects) { [project_in_group1, project_in_sub_group1] } + + let_it_be(:project_with_variables_in_group1) { create(:project, group: group1) } + let_it_be(:project_with_job_variables_in_group1) { create(:project, group: group1) } + let_it_be(:expected_not_updated_projects) do + [project_with_variables_in_group1, project_with_job_variables_in_group1] + end + + let_it_be(:all_projects) { expected_updated_projects + expected_not_updated_projects } + + let(:group_id) { group1.id } + let_it_be_with_reload(:pipeline1) { create(:ci_pipeline, project: project_with_variables_in_group1) } + let_it_be_with_reload(:pipeline3) { create(:ci_pipeline, project: project_with_job_variables_in_group1) } + let(:build) { create(:ci_build, pipeline: pipeline3) } + + before do + all_projects.each do |project| + project.ci_pipeline_variables_minimum_override_role = 'developer' + project.save! + end + + create(:ci_pipeline_variable, pipeline: pipeline1, key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1') + + create(:ci_job_variable, job: build) + end + + it 'update ci cd settings with minimum override role to no one allowed' do + update_role + + expected_updated_settings = ProjectCiCdSetting.where(project: expected_updated_projects) + expected_not_updated_settings = ProjectCiCdSetting.where(project: expected_not_updated_projects) + + expect(expected_updated_settings.pluck(:pipeline_variables_minimum_override_role)) + .to eq(%w[no_one_allowed no_one_allowed]) + + expect(expected_not_updated_settings.pluck(:pipeline_variables_minimum_override_role)) + .to eq(%w[developer developer]) + end + end + end +end diff --git a/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb b/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb new file mode 100644 index 00000000000000..cbd3b9cbdbb9bf --- /dev/null +++ b/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::SafeDisablePipelineVariablesWorker, feature_category: :ci_variables do + let(:group_id) { 1 } + + describe '#perform' do + subject(:perform_service) { described_class.new.perform(args) } + + let(:args) { { 'group_id' => group_id } } + + it 'executes AutoMergeService' do + expect_next_instance_of(::Ci::SafeDisablePipelineVariablesService) do |service| + expect(service).to receive(:execute) + end + + perform_service + end + end +end -- GitLab From 14d4f9e23e8da3cada41f29db1b9668185ec4ba7 Mon Sep 17 00:00:00 2001 From: Dmitry Biryukov Date: Fri, 23 May 2025 16:46:46 +0200 Subject: [PATCH 02/11] Update all_queues --- app/workers/all_queues.yml | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 03e8dc3d344a39..f097150a75d83e 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -3397,16 +3397,6 @@ :idempotent: true :tags: [] :queue_namespace: -- :name: ci_safe_disable_pipeline_variables - :worker_name: Ci::SafeDisablePipelineVariablesWorker - :feature_category: :ci_variables - :has_external_dependencies: false - :urgency: :low - :resource_boundary: :unknown - :weight: 1 - :idempotent: true - :tags: [] - :queue_namespace: - :name: ci_job_artifacts_expire_project_build_artifacts :worker_name: Ci::JobArtifacts::ExpireProjectBuildArtifactsWorker :feature_category: :job_artifacts @@ -3467,6 +3457,16 @@ :idempotent: true :tags: [] :queue_namespace: +- :name: ci_safe_disable_pipeline_variables + :worker_name: Ci::SafeDisablePipelineVariablesWorker + :feature_category: :ci_variables + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] + :queue_namespace: - :name: ci_unlock_pipelines_in_queue :worker_name: Ci::UnlockPipelinesInQueueWorker :feature_category: :job_artifacts -- GitLab From 3da9a5e95ba576f40d1c9dac5340b35ff4be9358 Mon Sep 17 00:00:00 2001 From: Dmitry Biryukov Date: Fri, 23 May 2025 16:49:22 +0200 Subject: [PATCH 03/11] Update sidekiq_queues --- config/sidekiq_queues.yml | 2 ++ db/structure.sql | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 8ae57aa3685d34..bab06daa790f3d 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -215,6 +215,8 @@ - 1 - - ci_runners_update_project_runners_owner - 1 +- - ci_safe_disable_pipeline_variables + - 1 - - ci_unlock_pipelines_in_queue - 1 - - ci_update_approval_rules_for_related_mrs diff --git a/db/structure.sql b/db/structure.sql index 2f9374e9a327f9..bf60a538deedf6 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9116,7 +9116,6 @@ CREATE TABLE application_settings ( vscode_extension_marketplace jsonb DEFAULT '{}'::jsonb NOT NULL, token_prefixes jsonb DEFAULT '{}'::jsonb NOT NULL, ci_cd_settings jsonb DEFAULT '{}'::jsonb NOT NULL, - duo_nano_features_enabled boolean, database_reindexing jsonb DEFAULT '{}'::jsonb NOT NULL, duo_chat jsonb DEFAULT '{}'::jsonb NOT NULL, group_settings jsonb DEFAULT '{}'::jsonb NOT NULL, -- GitLab From d3bbe4cb677e67b310606a90fe6c04dcaab8c2a1 Mon Sep 17 00:00:00 2001 From: Dmitry Biryukov Date: Mon, 26 May 2025 10:19:13 +0200 Subject: [PATCH 04/11] Restore zoekt version --- GITLAB_ZOEKT_VERSION | 1 + 1 file changed, 1 insertion(+) create mode 100644 GITLAB_ZOEKT_VERSION diff --git a/GITLAB_ZOEKT_VERSION b/GITLAB_ZOEKT_VERSION new file mode 100644 index 00000000000000..04a373efe6ba8b --- /dev/null +++ b/GITLAB_ZOEKT_VERSION @@ -0,0 +1 @@ +0.16.0 -- GitLab From 2b8ab2d6bce35da3f73c678522aecfa63e5d869a Mon Sep 17 00:00:00 2001 From: Dmitry Biryukov Date: Tue, 27 May 2025 18:16:09 +0200 Subject: [PATCH 05/11] Complete all the suggestions --- ...safe_disable_pipeline_variables_service.rb | 38 ++++------ .../safe_disable_pipeline_variables_worker.rb | 14 +++- ...disable_pipeline_variables_service_spec.rb | 74 ++++++++++++++----- ..._disable_pipeline_variables_worker_spec.rb | 32 ++++++-- 4 files changed, 106 insertions(+), 52 deletions(-) diff --git a/app/services/ci/safe_disable_pipeline_variables_service.rb b/app/services/ci/safe_disable_pipeline_variables_service.rb index e7b7ac30b11bed..1df8dbdd1ea5ec 100644 --- a/app/services/ci/safe_disable_pipeline_variables_service.rb +++ b/app/services/ci/safe_disable_pipeline_variables_service.rb @@ -1,28 +1,18 @@ # frozen_string_literal: true module Ci - class SafeDisablePipelineVariablesService - def initialize(group_id:, logger:) - @group_id = group_id + class SafeDisablePipelineVariablesService < BaseService + def initialize(current_user:, group:, logger:) + @current_user = current_user + @parent_group = group @logger = logger end def execute - @logger.info start_message + return ServiceResponse.error(message: 'You are not authorized to perform this action') unless authorized? - migrate! - - @logger.info finish_message - end - - private - - def migrate! - parent_group = ::Group.find(@group_id) - - dependant_groups = parent_group.self_and_descendants - @logger.info("Processing parent group_id=#{@group_id} " \ - "with group(s) including descendants") + updated_count = 0 + dependant_groups = @parent_group.self_and_descendants dependant_groups.each_batch do |batch_group| all_projects_ci_cd_settings = ProjectCiCdSetting.for_namespace(batch_group) @@ -41,21 +31,23 @@ def migrate! Ci::PipelineVariable.where(project_id: project_ids).distinct.pluck(:project_id) + Ci::JobVariable.where(project_id: project_ids).distinct.pluck(:project_id) - ProjectCiCdSetting.where(project_id: project_ids - projects_with_vars) + batch_updated_count = ProjectCiCdSetting.where(project_id: project_ids - projects_with_vars) .update_all(pipeline_variables_minimum_override_role: ProjectCiCdSetting::NO_ONE_ALLOWED_ROLE) + updated_count += batch_updated_count + # rubocop:enable Database/AvoidUsingPluckWithoutLimit # rubocop: enable CodeReuse/ActiveRecord end end - end - def start_message - "Starting migration of minimum override role for project(s) in provided groups..." + ServiceResponse.success(payload: { updated_count: updated_count }) end - def finish_message - "Successfully completed migration for all applicable project(s)." + private + + def authorized? + @current_user.can?(:admin_group, @parent_group) end end end diff --git a/app/workers/ci/safe_disable_pipeline_variables_worker.rb b/app/workers/ci/safe_disable_pipeline_variables_worker.rb index dd2201aa970efe..b83f905e404db9 100644 --- a/app/workers/ci/safe_disable_pipeline_variables_worker.rb +++ b/app/workers/ci/safe_disable_pipeline_variables_worker.rb @@ -12,10 +12,18 @@ class SafeDisablePipelineVariablesWorker defer_on_database_health_signal :gitlab_main, [], 1.minute - attr_accessor :group_id + attr_accessor :group_id, :current_user_id - def perform(group_id) - Ci::SafeDisablePipelineVariablesService.new(group_id: group_id, logger: logger).execute + def perform(current_user_id, group_id) + current_user = UserFinder.new(current_user_id).find_by_id + group = ::Group.find_by_id(group_id) + result = Ci::SafeDisablePipelineVariablesService.new( + current_user: current_user, group: group, logger: logger + ).execute + + return unless result.status == :success + + log_extra_metadata_on_done(:disabled_pipeline_variables_count, result.payload[:updated_count]) end private diff --git a/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb b/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb index f004e5a88ae5e2..b0c05c3db47839 100644 --- a/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb +++ b/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb @@ -5,54 +5,88 @@ RSpec.describe Ci::SafeDisablePipelineVariablesService, feature_category: :ci_variables do let(:log_output) { StringIO.new } let(:logger) { Logger.new(log_output) } - let(:service) { described_class.new(group_id: group_id, logger: logger) } + let(:service) { described_class.new(current_user: current_user, group: parent_group, logger: logger) } describe '#execute' do subject(:update_role) { service.execute } context 'when group_id correctly supplied' do - let_it_be(:group1) { create(:group) } - let_it_be(:sub_group1) { create(:group, parent: group1) } - let_it_be(:project_in_group1) { create(:project, group: group1) } + let_it_be(:parent_group) { create(:group) } + let_it_be(:sub_group1) { create(:group, parent: parent_group) } + let_it_be(:project_in_group1) { create(:project, group: parent_group) } let_it_be(:project_in_sub_group1) { create(:project, group: sub_group1) } let_it_be(:expected_updated_projects) { [project_in_group1, project_in_sub_group1] } - let_it_be(:project_with_variables_in_group1) { create(:project, group: group1) } - let_it_be(:project_with_job_variables_in_group1) { create(:project, group: group1) } + let_it_be(:project_with_variables_in_group1) { create(:project, group: parent_group) } + let_it_be(:project_with_job_variables_in_group1) { create(:project, group: parent_group) } let_it_be(:expected_not_updated_projects) do [project_with_variables_in_group1, project_with_job_variables_in_group1] end let_it_be(:all_projects) { expected_updated_projects + expected_not_updated_projects } - let(:group_id) { group1.id } + let_it_be(:current_user) { create(:user) } let_it_be_with_reload(:pipeline1) { create(:ci_pipeline, project: project_with_variables_in_group1) } let_it_be_with_reload(:pipeline3) { create(:ci_pipeline, project: project_with_job_variables_in_group1) } let(:build) { create(:ci_build, pipeline: pipeline3) } before do - all_projects.each do |project| - project.ci_pipeline_variables_minimum_override_role = 'developer' - project.save! - end - create(:ci_pipeline_variable, pipeline: pipeline1, key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1') create(:ci_job_variable, job: build) end - it 'update ci cd settings with minimum override role to no one allowed' do - update_role + before_all do + reset_all_projects_override_role(:developer) + end + + context 'and current user has permissions to update a group' do + before_all do + parent_group.add_owner(current_user) + end + + it 'updates ci cd settings with minimum override role to no one allowed' do + response = update_role + + expected_updated_settings = ProjectCiCdSetting.where(project: expected_updated_projects) + expected_not_updated_settings = ProjectCiCdSetting.where(project: expected_not_updated_projects) + + expect(expected_updated_settings.pluck(:pipeline_variables_minimum_override_role)) + .to eq(%w[no_one_allowed no_one_allowed]) + + expect(expected_not_updated_settings.pluck(:pipeline_variables_minimum_override_role)) + .to eq(%w[developer developer]) - expected_updated_settings = ProjectCiCdSetting.where(project: expected_updated_projects) - expected_not_updated_settings = ProjectCiCdSetting.where(project: expected_not_updated_projects) + expect(response.status).to eq(:success) + expect(response.payload[:updated_count]).to eq(2) + end - expect(expected_updated_settings.pluck(:pipeline_variables_minimum_override_role)) - .to eq(%w[no_one_allowed no_one_allowed]) + it 'is idempotent' do + response = service.execute + expect(response.status).to eq(:success) - expect(expected_not_updated_settings.pluck(:pipeline_variables_minimum_override_role)) - .to eq(%w[developer developer]) + second_response = service.execute + expect(second_response.status).to eq(:success) + expect(second_response.payload[:updated_count]).to eq(0) + end end + + context 'and current user has no permissions to update a group' do + it 'does not updates ci cd settings with minimum override role to no one allowed' do + response = update_role + + expect(response.status).to eq(:error) + expect(response.message).to eq('You are not authorized to perform this action') + expected_updated_settings = ProjectCiCdSetting.where(project: expected_updated_projects) + + expect(expected_updated_settings.pluck(:pipeline_variables_minimum_override_role)) + .to eq(%w[developer developer]) + end + end + end + + def reset_all_projects_override_role(role) + ProjectCiCdSetting.update_all(pipeline_variables_minimum_override_role: role) end end end diff --git a/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb b/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb index cbd3b9cbdbb9bf..a9d5505f426143 100644 --- a/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb +++ b/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb @@ -3,19 +3,39 @@ require 'spec_helper' RSpec.describe Ci::SafeDisablePipelineVariablesWorker, feature_category: :ci_variables do - let(:group_id) { 1 } + let(:group_id) { create(:group).id } + let(:current_user_id) { create(:user).id } + let(:worker) { described_class.new } describe '#perform' do - subject(:perform_service) { described_class.new.perform(args) } + subject(:perform_worker) { worker.perform(*job_args) } - let(:args) { { 'group_id' => group_id } } + let(:job_args) { [current_user_id, group_id] } + + it_behaves_like 'an idempotent worker' + + it 'executes SafeDisablePipelineVariablesService' do + expected_response = ServiceResponse.success(payload: { updated_count: 5 }) + + expect_next_instance_of(::Ci::SafeDisablePipelineVariablesService) do |service| + expect(service).to receive(:execute).and_return(expected_response) + end + + expect(worker).to receive(:log_extra_metadata_on_done).with(:disabled_pipeline_variables_count, 5) + + perform_worker + end + + it 'fails to execute' do + expected_response = ServiceResponse.error(message: 'error') - it 'executes AutoMergeService' do expect_next_instance_of(::Ci::SafeDisablePipelineVariablesService) do |service| - expect(service).to receive(:execute) + expect(service).to receive(:execute).and_return(expected_response) end - perform_service + expect(worker).not_to receive(:log_extra_metadata_on_done) + + perform_worker end end end -- GitLab From 292c0c0202243b8380c109d4071e67a2e14ae6b1 Mon Sep 17 00:00:00 2001 From: Dmitry Biryukov Date: Tue, 27 May 2025 22:34:58 +0200 Subject: [PATCH 06/11] Add tables into defer on health signal --- app/workers/ci/safe_disable_pipeline_variables_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/ci/safe_disable_pipeline_variables_worker.rb b/app/workers/ci/safe_disable_pipeline_variables_worker.rb index b83f905e404db9..0c5c8dc2b6aa98 100644 --- a/app/workers/ci/safe_disable_pipeline_variables_worker.rb +++ b/app/workers/ci/safe_disable_pipeline_variables_worker.rb @@ -10,7 +10,7 @@ class SafeDisablePipelineVariablesWorker urgency :low idempotent! - defer_on_database_health_signal :gitlab_main, [], 1.minute + defer_on_database_health_signal :gitlab_main, [:namespaces, :project_ci_cd_settings], 1.minute attr_accessor :group_id, :current_user_id -- GitLab From 703b59c70b3ea2eee5a237ddd2828fc44f258cc3 Mon Sep 17 00:00:00 2001 From: Dmitry Biryukov Date: Mon, 2 Jun 2025 15:46:17 +0200 Subject: [PATCH 07/11] Address suggestions and questions --- app/models/project_ci_cd_setting.rb | 4 ++++ ...safe_disable_pipeline_variables_service.rb | 22 +++++++++---------- .../safe_disable_pipeline_variables_worker.rb | 8 +------ ...disable_pipeline_variables_service_spec.rb | 9 ++------ ..._disable_pipeline_variables_worker_spec.rb | 4 ++-- 5 files changed, 19 insertions(+), 28 deletions(-) diff --git a/app/models/project_ci_cd_setting.rb b/app/models/project_ci_cd_setting.rb index 1d9b09b42945f5..da49ee327f8587 100644 --- a/app/models/project_ci_cd_setting.rb +++ b/app/models/project_ci_cd_setting.rb @@ -70,6 +70,10 @@ def self.pluck_project_id(limit) limit(limit).pluck(:project_id) end + def self.bulk_restrict_pipeline_variables!(project_ids:) + for_project(project_ids).update_all(pipeline_variables_minimum_override_role: NO_ONE_ALLOWED_ROLE) + end + def keep_latest_artifacts_available? # The project level feature can only be enabled when the feature is enabled instance wide Gitlab::CurrentSettings.current_application_settings.keep_latest_artifact? && keep_latest_artifact? diff --git a/app/services/ci/safe_disable_pipeline_variables_service.rb b/app/services/ci/safe_disable_pipeline_variables_service.rb index 1df8dbdd1ea5ec..b05f1c08f2545b 100644 --- a/app/services/ci/safe_disable_pipeline_variables_service.rb +++ b/app/services/ci/safe_disable_pipeline_variables_service.rb @@ -2,37 +2,33 @@ module Ci class SafeDisablePipelineVariablesService < BaseService - def initialize(current_user:, group:, logger:) + def initialize(current_user:, group:) @current_user = current_user @parent_group = group - @logger = logger end def execute return ServiceResponse.error(message: 'You are not authorized to perform this action') unless authorized? updated_count = 0 - dependant_groups = @parent_group.self_and_descendants - dependant_groups.each_batch do |batch_group| - all_projects_ci_cd_settings = ProjectCiCdSetting.for_namespace(batch_group) + parent_group.self_and_descendants.each_batch do |group_batch| + all_projects_ci_cd_settings = ProjectCiCdSetting.for_namespace(group_batch) .with_pipeline_variables_enabled - @logger.info("Updating projects without pipeline variables in #{batch_group.count} groups.") - all_projects_ci_cd_settings - .each_batch do |batch_settings| + .each_batch do |ci_cd_settings| # rubocop: disable CodeReuse/ActiveRecord -- This is a highly custom query for this service, that's why it's not in the model. # rubocop: disable Database/AvoidUsingPluckWithoutLimit -- This pluck is already limited by batch size. - project_ids = batch_settings.pluck(:project_id) + project_ids = ci_cd_settings.pluck(:project_id) projects_with_vars = Ci::PipelineVariable.where(project_id: project_ids).distinct.pluck(:project_id) + Ci::JobVariable.where(project_id: project_ids).distinct.pluck(:project_id) - batch_updated_count = ProjectCiCdSetting.where(project_id: project_ids - projects_with_vars) - .update_all(pipeline_variables_minimum_override_role: ProjectCiCdSetting::NO_ONE_ALLOWED_ROLE) + batch_updated_count = + ProjectCiCdSetting.bulk_restrict_pipeline_variables!(project_ids: project_ids - projects_with_vars) updated_count += batch_updated_count @@ -46,8 +42,10 @@ def execute private + attr_reader :current_user, :parent_group + def authorized? - @current_user.can?(:admin_group, @parent_group) + current_user.can?(:admin_group, parent_group) end end end diff --git a/app/workers/ci/safe_disable_pipeline_variables_worker.rb b/app/workers/ci/safe_disable_pipeline_variables_worker.rb index 0c5c8dc2b6aa98..3716c45600b087 100644 --- a/app/workers/ci/safe_disable_pipeline_variables_worker.rb +++ b/app/workers/ci/safe_disable_pipeline_variables_worker.rb @@ -18,18 +18,12 @@ def perform(current_user_id, group_id) current_user = UserFinder.new(current_user_id).find_by_id group = ::Group.find_by_id(group_id) result = Ci::SafeDisablePipelineVariablesService.new( - current_user: current_user, group: group, logger: logger + current_user: current_user, group: group ).execute return unless result.status == :success log_extra_metadata_on_done(:disabled_pipeline_variables_count, result.payload[:updated_count]) end - - private - - def logger - Sidekiq.logger - end end end diff --git a/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb b/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb index b0c05c3db47839..b7ffa214b68356 100644 --- a/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb +++ b/spec/services/ci/safe_disable_pipeline_variables_service_spec.rb @@ -4,8 +4,7 @@ RSpec.describe Ci::SafeDisablePipelineVariablesService, feature_category: :ci_variables do let(:log_output) { StringIO.new } - let(:logger) { Logger.new(log_output) } - let(:service) { described_class.new(current_user: current_user, group: parent_group, logger: logger) } + let(:service) { described_class.new(current_user: current_user, group: parent_group) } describe '#execute' do subject(:update_role) { service.execute } @@ -37,7 +36,7 @@ end before_all do - reset_all_projects_override_role(:developer) + ProjectCiCdSetting.update_all(pipeline_variables_minimum_override_role: :developer) end context 'and current user has permissions to update a group' do @@ -84,9 +83,5 @@ end end end - - def reset_all_projects_override_role(role) - ProjectCiCdSetting.update_all(pipeline_variables_minimum_override_role: role) - end end end diff --git a/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb b/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb index a9d5505f426143..0b1bcee220a341 100644 --- a/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb +++ b/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb @@ -3,8 +3,8 @@ require 'spec_helper' RSpec.describe Ci::SafeDisablePipelineVariablesWorker, feature_category: :ci_variables do - let(:group_id) { create(:group).id } - let(:current_user_id) { create(:user).id } + let_it_be(:group_id) { create(:group).id } + let_it_be(:current_user_id) { create(:user).id } let(:worker) { described_class.new } describe '#perform' do -- GitLab From 4e8515962b0070bd62a05b4310004640b7e5a6c2 Mon Sep 17 00:00:00 2001 From: Dmitry Biryukov Date: Mon, 2 Jun 2025 17:20:24 +0200 Subject: [PATCH 08/11] Refactor to avoid rubocop complains and add guards to the worker group and user entities --- app/models/project_ci_cd_setting.rb | 10 +++++ ...safe_disable_pipeline_variables_service.rb | 20 +++------ .../safe_disable_pipeline_variables_worker.rb | 4 ++ ..._disable_pipeline_variables_worker_spec.rb | 43 ++++++++++++++++--- 4 files changed, 57 insertions(+), 20 deletions(-) diff --git a/app/models/project_ci_cd_setting.rb b/app/models/project_ci_cd_setting.rb index da49ee327f8587..d91d93ed576166 100644 --- a/app/models/project_ci_cd_setting.rb +++ b/app/models/project_ci_cd_setting.rb @@ -74,6 +74,16 @@ def self.bulk_restrict_pipeline_variables!(project_ids:) for_project(project_ids).update_all(pipeline_variables_minimum_override_role: NO_ONE_ALLOWED_ROLE) end + def self.project_ids_not_using_variables(settings_relation, limit) + project_ids = settings_relation.limit(limit).pluck(:project_id) + + projects_with_vars = + Ci::PipelineVariable.where(project_id: project_ids).distinct.limit(limit).pluck(:project_id) + + Ci::JobVariable.where(project_id: project_ids).distinct.limit(limit).pluck(:project_id) + + project_ids - projects_with_vars + end + def keep_latest_artifacts_available? # The project level feature can only be enabled when the feature is enabled instance wide Gitlab::CurrentSettings.current_application_settings.keep_latest_artifact? && keep_latest_artifact? diff --git a/app/services/ci/safe_disable_pipeline_variables_service.rb b/app/services/ci/safe_disable_pipeline_variables_service.rb index b05f1c08f2545b..239846339b1590 100644 --- a/app/services/ci/safe_disable_pipeline_variables_service.rb +++ b/app/services/ci/safe_disable_pipeline_variables_service.rb @@ -2,6 +2,8 @@ module Ci class SafeDisablePipelineVariablesService < BaseService + PROJECTS_BATCH_SIZE = 1000 + def initialize(current_user:, group:) @current_user = current_user @parent_group = group @@ -17,23 +19,13 @@ def execute .with_pipeline_variables_enabled all_projects_ci_cd_settings - .each_batch do |ci_cd_settings| - # rubocop: disable CodeReuse/ActiveRecord -- This is a highly custom query for this service, that's why it's not in the model. - # rubocop: disable Database/AvoidUsingPluckWithoutLimit -- This pluck is already limited by batch size. - - project_ids = ci_cd_settings.pluck(:project_id) - - projects_with_vars = - Ci::PipelineVariable.where(project_id: project_ids).distinct.pluck(:project_id) + - Ci::JobVariable.where(project_id: project_ids).distinct.pluck(:project_id) - + .each_batch(of: PROJECTS_BATCH_SIZE) do |ci_cd_settings| batch_updated_count = - ProjectCiCdSetting.bulk_restrict_pipeline_variables!(project_ids: project_ids - projects_with_vars) + ProjectCiCdSetting.bulk_restrict_pipeline_variables!( + project_ids: ProjectCiCdSetting.project_ids_not_using_variables(ci_cd_settings, PROJECTS_BATCH_SIZE) + ) updated_count += batch_updated_count - - # rubocop:enable Database/AvoidUsingPluckWithoutLimit - # rubocop: enable CodeReuse/ActiveRecord end end diff --git a/app/workers/ci/safe_disable_pipeline_variables_worker.rb b/app/workers/ci/safe_disable_pipeline_variables_worker.rb index 3716c45600b087..5d13701a836606 100644 --- a/app/workers/ci/safe_disable_pipeline_variables_worker.rb +++ b/app/workers/ci/safe_disable_pipeline_variables_worker.rb @@ -16,7 +16,11 @@ class SafeDisablePipelineVariablesWorker def perform(current_user_id, group_id) current_user = UserFinder.new(current_user_id).find_by_id + return unless current_user + group = ::Group.find_by_id(group_id) + return unless group + result = Ci::SafeDisablePipelineVariablesService.new( current_user: current_user, group: group ).execute diff --git a/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb b/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb index 0b1bcee220a341..c1a03415ef42a8 100644 --- a/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb +++ b/spec/workers/ci/safe_disable_pipeline_variables_worker_spec.rb @@ -14,6 +14,15 @@ it_behaves_like 'an idempotent worker' + shared_examples 'fails to execute the service' do + it 'fails to execute' do + expect(worker).not_to receive(:log_extra_metadata_on_done) + expect(::Ci::SafeDisablePipelineVariablesService).not_to receive(:new) + + perform_worker + end + end + it 'executes SafeDisablePipelineVariablesService' do expected_response = ServiceResponse.success(payload: { updated_count: 5 }) @@ -26,16 +35,38 @@ perform_worker end - it 'fails to execute' do - expected_response = ServiceResponse.error(message: 'error') + context 'when service response is an error' do + before do + expected_response = ServiceResponse.error(message: 'error') - expect_next_instance_of(::Ci::SafeDisablePipelineVariablesService) do |service| - expect(service).to receive(:execute).and_return(expected_response) + expect_next_instance_of(::Ci::SafeDisablePipelineVariablesService) do |service| + allow(service).to receive(:execute).and_return(expected_response) + end end - expect(worker).not_to receive(:log_extra_metadata_on_done) + it_behaves_like 'fails to execute the service' + end - perform_worker + context 'when group or user is not found' do + before do + expected_response = ServiceResponse.success(payload: { updated_count: 5 }) + + allow_next_instance_of(::Ci::SafeDisablePipelineVariablesService) do |service| + allow(service).to receive(:execute).and_return(expected_response) + end + end + + context 'and group does not exist' do + let(:job_args) { [current_user_id, group_id + 1] } + + it_behaves_like 'fails to execute the service' + end + + context 'and user does not exist' do + let(:job_args) { [current_user_id + 1, group_id] } + + it_behaves_like 'fails to execute the service' + end end end end -- GitLab From 281cb9eb6a72a54126ae54c333a0792ba0c7356b Mon Sep 17 00:00:00 2001 From: Dmytro Biryukov Date: Wed, 11 Jun 2025 17:35:39 +0200 Subject: [PATCH 09/11] Optimize query with raw sql and unnest array --- app/models/ci/job_variable.rb | 18 ++++++++++++++++++ app/models/ci/pipeline_variable.rb | 18 ++++++++++++++++++ app/models/project_ci_cd_setting.rb | 4 ++-- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/app/models/ci/job_variable.rb b/app/models/ci/job_variable.rb index 6033e4a0886543..61febc1ae9fc84 100644 --- a/app/models/ci/job_variable.rb +++ b/app/models/ci/job_variable.rb @@ -21,6 +21,24 @@ class JobVariable < Ci::ApplicationRecord enum :source, { internal: 0, dotenv: 1 }, suffix: true + def self.projects_with_variables(project_ids, limit) + sql_pipeline_variables = <<~SQL + WITH input_projects AS ( + SELECT unnest(ARRAY[?]) AS project_id + ) + SELECT ip.project_id + FROM input_projects ip + WHERE EXISTS ( + SELECT 1 + FROM ci_job_variables pcjv + WHERE pcjv.project_id = ip.project_id + ) + LIMIT ? + SQL + + connection.select_values(sanitize_sql_array([sql_pipeline_variables, project_ids, limit])) + end + def set_project_id self.project_id ||= job&.project_id end diff --git a/app/models/ci/pipeline_variable.rb b/app/models/ci/pipeline_variable.rb index 7b86c2b804fbfa..5696b30c2810df 100644 --- a/app/models/ci/pipeline_variable.rb +++ b/app/models/ci/pipeline_variable.rb @@ -25,6 +25,24 @@ class PipelineVariable < Ci::ApplicationRecord validates :key, :pipeline, presence: true validates :project_id, presence: true + def self.projects_with_variables(project_ids, limit) + sql_pipeline_variables = <<~SQL + WITH input_projects AS ( + SELECT unnest(ARRAY[?]) AS project_id + ) + SELECT ip.project_id + FROM input_projects ip + WHERE EXISTS ( + SELECT 1 + FROM p_ci_pipeline_variables pcpv + WHERE pcpv.project_id = ip.project_id + ) + LIMIT ? + SQL + + connection.select_values(sanitize_sql_array([sql_pipeline_variables, project_ids, limit])) + end + def hook_attrs { key: key, value: value } end diff --git a/app/models/project_ci_cd_setting.rb b/app/models/project_ci_cd_setting.rb index d91d93ed576166..21c40dbcf52ce1 100644 --- a/app/models/project_ci_cd_setting.rb +++ b/app/models/project_ci_cd_setting.rb @@ -78,8 +78,8 @@ def self.project_ids_not_using_variables(settings_relation, limit) project_ids = settings_relation.limit(limit).pluck(:project_id) projects_with_vars = - Ci::PipelineVariable.where(project_id: project_ids).distinct.limit(limit).pluck(:project_id) + - Ci::JobVariable.where(project_id: project_ids).distinct.limit(limit).pluck(:project_id) + Ci::PipelineVariable.projects_with_variables(project_ids, limit) + + Ci::JobVariable.projects_with_variables(project_ids, limit) project_ids - projects_with_vars end -- GitLab From 4ffb107aeca1e79eda426381f5dd5802d416f16a Mon Sep 17 00:00:00 2001 From: Dmytro Biryukov Date: Wed, 11 Jun 2025 17:49:52 +0200 Subject: [PATCH 10/11] Apply the suggestion with the result success improvements. --- app/workers/ci/safe_disable_pipeline_variables_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/ci/safe_disable_pipeline_variables_worker.rb b/app/workers/ci/safe_disable_pipeline_variables_worker.rb index 5d13701a836606..8b80c88fb311b1 100644 --- a/app/workers/ci/safe_disable_pipeline_variables_worker.rb +++ b/app/workers/ci/safe_disable_pipeline_variables_worker.rb @@ -25,7 +25,7 @@ def perform(current_user_id, group_id) current_user: current_user, group: group ).execute - return unless result.status == :success + return unless result.success? log_extra_metadata_on_done(:disabled_pipeline_variables_count, result.payload[:updated_count]) end -- GitLab From 26e51e8daebf92682b191dcce4db3da102b9c9e4 Mon Sep 17 00:00:00 2001 From: Dmytro Biryukov Date: Thu, 12 Jun 2025 12:52:42 +0200 Subject: [PATCH 11/11] Move project_with_pipeline_variables to a concern --- app/models/ci/job_variable.rb | 19 +----- app/models/ci/pipeline_variable.rb | 19 +----- .../ci/projects_with_variables_query.rb | 27 ++++++++ ...safe_disable_pipeline_variables_service.rb | 2 +- spec/models/ci/job_variable_spec.rb | 8 +++ spec/models/ci/pipeline_variable_spec.rb | 8 +++ ...cts_with_variables_query_shared_example.rb | 64 +++++++++++++++++++ 7 files changed, 110 insertions(+), 37 deletions(-) create mode 100644 app/models/concerns/ci/projects_with_variables_query.rb create mode 100644 spec/support/shared_examples/projects_with_variables_query_shared_example.rb diff --git a/app/models/ci/job_variable.rb b/app/models/ci/job_variable.rb index 034413dbd3453c..407e40e2e6c690 100644 --- a/app/models/ci/job_variable.rb +++ b/app/models/ci/job_variable.rb @@ -5,6 +5,7 @@ class JobVariable < Ci::ApplicationRecord include Ci::Partitionable include Ci::NewHasVariable include Ci::RawVariable + include Ci::ProjectsWithVariablesQuery before_validation :set_project_id, on: :create @@ -21,24 +22,6 @@ class JobVariable < Ci::ApplicationRecord scope :for_jobs, ->(jobs) { where(job: jobs) } - def self.projects_with_variables(project_ids, limit) - sql_pipeline_variables = <<~SQL - WITH input_projects AS ( - SELECT unnest(ARRAY[?]) AS project_id - ) - SELECT ip.project_id - FROM input_projects ip - WHERE EXISTS ( - SELECT 1 - FROM ci_job_variables pcjv - WHERE pcjv.project_id = ip.project_id - ) - LIMIT ? - SQL - - connection.select_values(sanitize_sql_array([sql_pipeline_variables, project_ids, limit])) - end - def set_project_id self.project_id ||= job&.project_id end diff --git a/app/models/ci/pipeline_variable.rb b/app/models/ci/pipeline_variable.rb index d85f9b5dd83345..ee9b7afcb0f000 100644 --- a/app/models/ci/pipeline_variable.rb +++ b/app/models/ci/pipeline_variable.rb @@ -5,6 +5,7 @@ class PipelineVariable < Ci::ApplicationRecord include Ci::Partitionable include Ci::HasVariable include Ci::RawVariable + include Ci::ProjectsWithVariablesQuery before_validation :ensure_project_id @@ -23,24 +24,6 @@ class PipelineVariable < Ci::ApplicationRecord validates :key, :pipeline, presence: true validates :project_id, presence: true - def self.projects_with_variables(project_ids, limit) - sql_pipeline_variables = <<~SQL - WITH input_projects AS ( - SELECT unnest(ARRAY[?]) AS project_id - ) - SELECT ip.project_id - FROM input_projects ip - WHERE EXISTS ( - SELECT 1 - FROM p_ci_pipeline_variables pcpv - WHERE pcpv.project_id = ip.project_id - ) - LIMIT ? - SQL - - connection.select_values(sanitize_sql_array([sql_pipeline_variables, project_ids, limit])) - end - def hook_attrs { key: key, value: value } end diff --git a/app/models/concerns/ci/projects_with_variables_query.rb b/app/models/concerns/ci/projects_with_variables_query.rb new file mode 100644 index 00000000000000..ec33d6f0bc8bde --- /dev/null +++ b/app/models/concerns/ci/projects_with_variables_query.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Ci + module ProjectsWithVariablesQuery + extend ActiveSupport::Concern + + class_methods do + def projects_with_variables(project_ids, limit) + project_ids_sql = <<~SQL.squish + WITH input_projects AS ( + SELECT unnest(ARRAY[?]) AS project_id + ) + SELECT input_projects.project_id + FROM input_projects + WHERE EXISTS ( + SELECT 1 + FROM #{quoted_table_name} + WHERE #{quoted_table_name}.project_id = input_projects.project_id + ) + LIMIT ? + SQL + + connection.select_values(sanitize_sql_array([project_ids_sql, project_ids, limit])) + end + end + end +end diff --git a/app/services/ci/safe_disable_pipeline_variables_service.rb b/app/services/ci/safe_disable_pipeline_variables_service.rb index 239846339b1590..a43317d3119208 100644 --- a/app/services/ci/safe_disable_pipeline_variables_service.rb +++ b/app/services/ci/safe_disable_pipeline_variables_service.rb @@ -2,7 +2,7 @@ module Ci class SafeDisablePipelineVariablesService < BaseService - PROJECTS_BATCH_SIZE = 1000 + PROJECTS_BATCH_SIZE = 500 def initialize(current_user:, group:) @current_user = current_user diff --git a/spec/models/ci/job_variable_spec.rb b/spec/models/ci/job_variable_spec.rb index c26dfefa4c188b..6224cff394020f 100644 --- a/spec/models/ci/job_variable_spec.rb +++ b/spec/models/ci/job_variable_spec.rb @@ -93,4 +93,12 @@ let!(:model) { create(:ci_job_variable, project_id: parent.id) } end end + + describe 'projects_with_pipeline_variables_query concern' do + def create_variable(project) + create(:ci_job_variable, job: create(:ci_build, project: project)) + end + + it_behaves_like 'projects_with_variables_query' + end end diff --git a/spec/models/ci/pipeline_variable_spec.rb b/spec/models/ci/pipeline_variable_spec.rb index 5e6f50cda3a5f2..1a90885c875c9a 100644 --- a/spec/models/ci/pipeline_variable_spec.rb +++ b/spec/models/ci/pipeline_variable_spec.rb @@ -63,4 +63,12 @@ end.not_to change { variable.project_id }.from(another_project.id) end end + + describe 'projects_with_pipeline_variables_query concern' do + def create_variable(project) + create(:ci_pipeline_variable, pipeline: create(:ci_pipeline, project: project)) + end + + it_behaves_like 'projects_with_variables_query' + end end diff --git a/spec/support/shared_examples/projects_with_variables_query_shared_example.rb b/spec/support/shared_examples/projects_with_variables_query_shared_example.rb new file mode 100644 index 00000000000000..f06dacbbc08a7c --- /dev/null +++ b/spec/support/shared_examples/projects_with_variables_query_shared_example.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.shared_examples 'projects_with_variables_query' do + describe '.projects_with_variables' do + let_it_be(:project1) { create(:project) } + let_it_be(:project2) { create(:project) } + let_it_be(:project3) { create(:project) } + let_it_be(:project4) { create(:project) } + + let(:project_ids) { [project1.id, project2.id, project3.id, project4.id] } + + before do + # Create variables for some projects + create_variable(project1) + create_variable(project1) # Multiple variables for same project + create_variable(project3) + # project2 and project4 have no variables + end + + context 'when projects have variables' do + it 'returns project IDs that have variables' do + result = described_class.projects_with_variables(project_ids, 10) + + expect(result).to match_array([project1.id, project3.id]) + end + end + + context 'when no projects have variables' do + let(:empty_project_ids) { [project2.id, project4.id] } + + it 'returns empty array' do + result = described_class.projects_with_variables(empty_project_ids, 10) + + expect(result).to be_empty + end + end + + context 'when project_ids contains non-existent project IDs' do + let(:mixed_project_ids) { [project1.id, project4.id + 1, project3.id, project4.id + 2] } + + it 'only returns existing project IDs that have variables' do + result = described_class.projects_with_variables(mixed_project_ids, 10) + + expect(result).to match_array([project1.id, project3.id]) + end + end + + context 'with limit parameter' do + it 'respects the limit when more projects than limit have variables' do + result = described_class.projects_with_variables(project_ids, 1) + + expect(result.size).to eq(1) + end + + it 'returns all matching projects when limit is higher than matches' do + result = described_class.projects_with_variables(project_ids, 10) + + expect(result).to match_array([project1.id, project3.id]) + end + end + end +end -- GitLab