diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index ca62f54813b054e95175997c8752a3a03d00ce23..fb00156d3208a90d76a31f6c79ada68ab5ac7e4a 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -50,6 +50,10 @@ def show end def toggle_shared_runners + if Feature.enabled?(:disable_shared_runners_on_group, default_enabled: true) && !project.shared_runners_enabled && project.group && project.group.shared_runners_setting == 'disabled_and_unoverridable' + return redirect_to project_runners_path(@project), alert: _("Cannot enable shared runners because parent group does not allow it") + end + project.toggle!(:shared_runners_enabled) redirect_to project_settings_ci_cd_path(@project, anchor: 'js-runners-settings') diff --git a/app/models/group.rb b/app/models/group.rb index abb3f6c96c4dccfaa51e6614012d6bf911282182..4ecd096b2b4529a8c4a3256f2b8d9225584967e5 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -19,8 +19,6 @@ class Group < Namespace ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10 - UpdateSharedRunnersError = Class.new(StandardError) - has_many :all_group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent has_many :group_members, -> { where(requested_at: nil).where.not(members: { access_level: Gitlab::Access::MINIMAL_ACCESS }) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent alias_method :members, :group_members @@ -538,53 +536,14 @@ def preload_shared_group_links preloader.preload(self, shared_with_group_links: [shared_with_group: :route]) end - def shared_runners_allowed? - shared_runners_enabled? || allow_descendants_override_disabled_shared_runners? - end - - def parent_allows_shared_runners? - return true unless has_parent? - - parent.shared_runners_allowed? - end - - def parent_enabled_shared_runners? - return true unless has_parent? - - parent.shared_runners_enabled? - end - - def enable_shared_runners! - raise UpdateSharedRunnersError, 'Shared Runners disabled for the parent group' unless parent_enabled_shared_runners? - - update_column(:shared_runners_enabled, true) - end - - def disable_shared_runners! - group_ids = self_and_descendants - return if group_ids.empty? - - Group.by_id(group_ids).update_all(shared_runners_enabled: false) - - all_projects.update_all(shared_runners_enabled: false) - end - - def allow_descendants_override_disabled_shared_runners! - raise UpdateSharedRunnersError, 'Shared Runners enabled' if shared_runners_enabled? - raise UpdateSharedRunnersError, 'Group level shared Runners not allowed' unless parent_allows_shared_runners? - - update_column(:allow_descendants_override_disabled_shared_runners, true) - end - - def disallow_descendants_override_disabled_shared_runners! - raise UpdateSharedRunnersError, 'Shared Runners enabled' if shared_runners_enabled? + def update_shared_runners_setting!(state) + raise ArgumentError unless SHARED_RUNNERS_SETTINGS.include?(state) - group_ids = self_and_descendants - return if group_ids.empty? - - Group.by_id(group_ids).update_all(allow_descendants_override_disabled_shared_runners: false) - - all_projects.update_all(shared_runners_enabled: false) + case state + when 'disabled_and_unoverridable' then disable_shared_runners! # also disallows override + when 'disabled_with_override' then disable_shared_runners_and_allow_override! + when 'enabled' then enable_shared_runners! # set both to true + end end def default_owner @@ -668,6 +627,45 @@ def self.groups_including_descendants_by(group_ids) .new(Group.where(id: group_ids)) .base_and_descendants end + + def disable_shared_runners! + update!( + shared_runners_enabled: false, + allow_descendants_override_disabled_shared_runners: false) + + group_ids = descendants + unless group_ids.empty? + Group.by_id(group_ids).update_all( + shared_runners_enabled: false, + allow_descendants_override_disabled_shared_runners: false) + end + + all_projects.update_all(shared_runners_enabled: false) + end + + def disable_shared_runners_and_allow_override! + # enabled -> disabled_with_override + if shared_runners_enabled? + update!( + shared_runners_enabled: false, + allow_descendants_override_disabled_shared_runners: true) + + group_ids = descendants + unless group_ids.empty? + Group.by_id(group_ids).update_all(shared_runners_enabled: false) + end + + all_projects.update_all(shared_runners_enabled: false) + + # disabled_and_unoverridable -> disabled_with_override + else + update!(allow_descendants_override_disabled_shared_runners: true) + end + end + + def enable_shared_runners! + update!(shared_runners_enabled: true) + end end Group.prepend_if_ee('EE::Group') diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 527fa9d52d09d1810d5da6bb3647ecd374cd4604..f0550713d0169266b00cef68ef60775575fda2e6 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -18,6 +18,8 @@ class Namespace < ApplicationRecord # Android repo (15) + some extra backup. NUMBER_OF_ANCESTORS_ALLOWED = 20 + SHARED_RUNNERS_SETTINGS = %w[disabled_and_unoverridable disabled_with_override enabled].freeze + cache_markdown_field :description, pipeline: :description has_many :projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -59,6 +61,8 @@ class Namespace < ApplicationRecord validates :max_artifacts_size, numericality: { only_integer: true, greater_than: 0, allow_nil: true } validate :nesting_level_allowed + validate :changing_shared_runners_enabled_is_allowed + validate :changing_allow_descendants_override_disabled_shared_runners_is_allowed validates_associated :runners @@ -378,6 +382,52 @@ def actual_plan_name actual_plan.name end + def changing_shared_runners_enabled_is_allowed + return unless Feature.enabled?(:disable_shared_runners_on_group, default_enabled: true) + return unless new_record? || changes.has_key?(:shared_runners_enabled) + + if shared_runners_enabled && has_parent? && parent.shared_runners_setting == 'disabled_and_unoverridable' + errors.add(:shared_runners_enabled, _('cannot be enabled because parent group has shared Runners disabled')) + end + end + + def changing_allow_descendants_override_disabled_shared_runners_is_allowed + return unless Feature.enabled?(:disable_shared_runners_on_group, default_enabled: true) + return unless new_record? || changes.has_key?(:allow_descendants_override_disabled_shared_runners) + + if shared_runners_enabled && !new_record? + errors.add(:allow_descendants_override_disabled_shared_runners, _('cannot be changed if shared runners are enabled')) + end + + if allow_descendants_override_disabled_shared_runners && has_parent? && parent.shared_runners_setting == 'disabled_and_unoverridable' + errors.add(:allow_descendants_override_disabled_shared_runners, _('cannot be enabled because parent group does not allow it')) + end + end + + def shared_runners_setting + if shared_runners_enabled + 'enabled' + else + if allow_descendants_override_disabled_shared_runners + 'disabled_with_override' + else + 'disabled_and_unoverridable' + end + end + end + + def shared_runners_setting_higher_than?(other_setting) + if other_setting == 'enabled' + false + elsif other_setting == 'disabled_with_override' + shared_runners_setting == 'enabled' + elsif other_setting == 'disabled_and_unoverridable' + shared_runners_setting == 'enabled' || shared_runners_setting == 'disabled_with_override' + else + raise ArgumentError + end + end + private def all_projects_with_pages diff --git a/app/models/project.rb b/app/models/project.rb index a6454acbb42fec06946cc1a741dce130bdadb785..2125c5a9cd371dd016ef0abeadc975638696d4a6 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -435,6 +435,7 @@ class Project < ApplicationRecord validate :visibility_level_allowed_by_group, if: :should_validate_visibility_level? validate :visibility_level_allowed_as_fork, if: :should_validate_visibility_level? validate :validate_pages_https_only, if: -> { changes.has_key?(:pages_https_only) } + validate :changing_shared_runners_enabled_is_allowed validates :repository_storage, presence: true, inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } } @@ -1189,6 +1190,15 @@ def validate_pages_https_only end end + def changing_shared_runners_enabled_is_allowed + return unless Feature.enabled?(:disable_shared_runners_on_group, default_enabled: true) + return unless new_record? || changes.has_key?(:shared_runners_enabled) + + if shared_runners_enabled && group && group.shared_runners_setting == 'disabled_and_unoverridable' + errors.add(:shared_runners_enabled, _('cannot be enabled because parent group does not allow it')) + end + end + def to_param if persisted? && errors.include?(:path) path_was diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index 51dca43fa4a66ff4bd262f170f618eb1fa836992..4747e1d5ac5ff8bd2d77f8f2c03d7ba3432e08cb 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -15,6 +15,8 @@ def execute after_build_hook(@group, params) + inherit_group_shared_runners_settings + unless can_use_visibility_level? && can_create_group? return @group end @@ -86,6 +88,13 @@ def set_visibility_level params[:visibility_level] = Gitlab::CurrentSettings.current_application_settings.default_group_visibility end + + def inherit_group_shared_runners_settings + return unless @group.parent + + @group.shared_runners_enabled = @group.parent.shared_runners_enabled + @group.allow_descendants_override_disabled_shared_runners = @group.parent.allow_descendants_override_disabled_shared_runners + end end end diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb index 2bd571f60afebafe33ed3548aac99c254f5c64dd..70f5c7e2ea7741748f0f8cae4513263a399a7e66 100644 --- a/app/services/groups/transfer_service.rb +++ b/app/services/groups/transfer_service.rb @@ -103,6 +103,9 @@ def update_group_attributes @group.parent = @new_parent_group @group.clear_memoization(:self_and_ancestors_ids) + + inherit_group_shared_runners_settings + @group.save! end @@ -161,6 +164,17 @@ def localized_error_messages group_contains_npm_packages: s_('TransferGroup|Group contains projects with NPM packages.') }.freeze end + + def inherit_group_shared_runners_settings + parent_setting = @group.parent&.shared_runners_setting + return unless parent_setting + + if @group.shared_runners_setting_higher_than?(parent_setting) + result = Groups::UpdateSharedRunnersService.new(@group, current_user, shared_runners_setting: parent_setting).execute + + raise TransferError, result[:message] unless result[:status] == :success + end + end end end diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 81393681dc04b83d6ad0a7782ea7e8c1f236224f..382a3dbf0f7325c387cd2364fbd52647c1fe0156 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -19,6 +19,8 @@ def execute return false unless valid_path_change_with_npm_packages? + return false unless update_shared_runners + before_assignment_hook(group, params) group.assign_attributes(params) @@ -98,6 +100,17 @@ def changing_share_with_group_lock? params[:share_with_group_lock] != group.share_with_group_lock end + + def update_shared_runners + return true if params[:shared_runners_setting].nil? + + result = Groups::UpdateSharedRunnersService.new(group, current_user, shared_runners_setting: params.delete(:shared_runners_setting)).execute + + return true if result[:status] == :success + + group.errors.add(:update_shared_runners, result[:message]) + false + end end end diff --git a/app/services/groups/update_shared_runners_service.rb b/app/services/groups/update_shared_runners_service.rb index 63f5710451034413eaa70559455d1d73bd1d806b..639c5bf6ae05861390a260518bafcaec7e872b73 100644 --- a/app/services/groups/update_shared_runners_service.rb +++ b/app/services/groups/update_shared_runners_service.rb @@ -7,44 +7,24 @@ def execute validate_params - enable_or_disable_shared_runners! - allow_or_disallow_descendants_override_disabled_shared_runners! + update_shared_runners success - rescue Group::UpdateSharedRunnersError => error + rescue ActiveRecord::RecordInvalid, ArgumentError => error error(error.message) end private def validate_params - if Gitlab::Utils.to_boolean(params[:shared_runners_enabled]) && !params[:allow_descendants_override_disabled_shared_runners].nil? - raise Group::UpdateSharedRunnersError, 'Cannot set shared_runners_enabled to true and allow_descendants_override_disabled_shared_runners' + unless Namespace::SHARED_RUNNERS_SETTINGS.include?(params[:shared_runners_setting]) + raise ArgumentError, "state must be one of: #{Namespace::SHARED_RUNNERS_SETTINGS.join(', ')}" end end - def enable_or_disable_shared_runners! - return if params[:shared_runners_enabled].nil? - - if Gitlab::Utils.to_boolean(params[:shared_runners_enabled]) - group.enable_shared_runners! - else - group.disable_shared_runners! - end - end - - def allow_or_disallow_descendants_override_disabled_shared_runners! - return if params[:allow_descendants_override_disabled_shared_runners].nil? - - # Needs to reset group because if both params are present could result in error - group.reset - - if Gitlab::Utils.to_boolean(params[:allow_descendants_override_disabled_shared_runners]) - group.allow_descendants_override_disabled_shared_runners! - else - group.disallow_descendants_override_disabled_shared_runners! - end + def update_shared_runners + group.update_shared_runners_setting!(params[:shared_runners_setting]) end end end diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 9943dc467317d5e2d7b05d7721e951f2693fbde0..6fc8e8f893590b2a49eff6862fbf61eccc97c578 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -19,6 +19,10 @@ def execute @project = Project.new(params) + # If a project is newly created it should have shared runners settings + # based on its group having it enabled. This is like the "default value" + @project.shared_runners_enabled = false if !params.key?(:shared_runners_enabled) && @project.group && @project.group.shared_runners_setting != 'enabled' + # Make sure that the user is allowed to use the specified visibility level if project_visibility.restricted? deny_visibility_level(@project, project_visibility.visibility_level) diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index dba5177718d7df68091be0c91079097acf73f374..013861631a10b4abba78f18e4ff7c6daec236e5d 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -88,6 +88,10 @@ def attempt_transfer_transaction # Move uploads move_project_uploads(project) + # If a project is being transferred to another group it means it can already + # have shared runners enabled but we need to check whether the new group allows that. + project.shared_runners_enabled = false if project.group && project.group.shared_runners_setting == 'disabled_and_unoverridable' + project.old_path_with_namespace = @old_path update_repository_configuration(@new_path) diff --git a/changelogs/unreleased/change-transfer-update-create-services.yml b/changelogs/unreleased/change-transfer-update-create-services.yml new file mode 100644 index 0000000000000000000000000000000000000000..a35016d229ceedfb2d82592c25466b74d01dd6d2 --- /dev/null +++ b/changelogs/unreleased/change-transfer-update-create-services.yml @@ -0,0 +1,5 @@ +--- +title: Change transfer, update and create services for groups and projects to take in consideration shared runners settings +merge_request: 36080 +author: Arthur de Lapertosa Lisboa +type: added diff --git a/config/feature_flags/development/disable_shared_runners_on_group.yml b/config/feature_flags/development/disable_shared_runners_on_group.yml new file mode 100644 index 0000000000000000000000000000000000000000..86ccf59c8a0f7334bf85edf1a31131a355bd70fe --- /dev/null +++ b/config/feature_flags/development/disable_shared_runners_on_group.yml @@ -0,0 +1,7 @@ +--- +name: disable_shared_runners_on_group +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/36080 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/258991 +type: development +group: group::runner +default_enabled: true diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 276a2a715c1b31b747b8d09266b3af3a66b9c5ee..48e30cbbde811dc45268516c35137300b800cbc5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4548,6 +4548,9 @@ msgstr "" msgid "Cannot create the abuse report. This user has been blocked." msgstr "" +msgid "Cannot enable shared runners because parent group does not allow it" +msgstr "" + msgid "Cannot have multiple Jira imports running at the same time" msgstr "" @@ -29951,6 +29954,15 @@ msgstr "" msgid "cannot be changed if a personal project has container registry tags." msgstr "" +msgid "cannot be changed if shared runners are enabled" +msgstr "" + +msgid "cannot be enabled because parent group does not allow it" +msgstr "" + +msgid "cannot be enabled because parent group has shared Runners disabled" +msgstr "" + msgid "cannot be enabled unless all domains have TLS certificates" msgstr "" diff --git a/spec/controllers/projects/runners_controller_spec.rb b/spec/controllers/projects/runners_controller_spec.rb index 66f20bd50c458849d2cf4632aa96ac559735046b..2443a823070d69ad0683a35c98d5168404d7a853 100644 --- a/spec/controllers/projects/runners_controller_spec.rb +++ b/spec/controllers/projects/runners_controller_spec.rb @@ -73,4 +73,45 @@ expect(runner.active).to eq(false) end end + + describe '#toggle_shared_runners' do + let(:group) { create(:group) } + let(:project) { create(:project, group: group) } + + it 'toggles shared_runners_enabled when the group allows shared runners' do + project.update!(shared_runners_enabled: true) + + post :toggle_shared_runners, params: params + + project.reload + + expect(response).to have_gitlab_http_status(:found) + expect(project.shared_runners_enabled).to eq(false) + end + + it 'toggles shared_runners_enabled when the group disallows shared runners but allows overrides' do + group.update!(shared_runners_enabled: false, allow_descendants_override_disabled_shared_runners: true) + project.update!(shared_runners_enabled: false) + + post :toggle_shared_runners, params: params + + project.reload + + expect(response).to have_gitlab_http_status(:found) + expect(project.shared_runners_enabled).to eq(true) + end + + it 'does not enable if the group disallows shared runners' do + group.update!(shared_runners_enabled: false, allow_descendants_override_disabled_shared_runners: false) + project.update!(shared_runners_enabled: false) + + post :toggle_shared_runners, params: params + + project.reload + + expect(response).to have_gitlab_http_status(:found) + expect(project.shared_runners_enabled).to eq(false) + expect(flash[:alert]).to eq("Cannot enable shared runners because parent group does not allow it") + end + end end diff --git a/spec/factories/namespaces.rb b/spec/factories/namespaces.rb index 0dcec086da93fe1a2a68295989e5a8ce753e5d13..0ec977b8234525a5b33aceecd1b6dce71c38eaba 100644 --- a/spec/factories/namespaces.rb +++ b/spec/factories/namespaces.rb @@ -63,5 +63,13 @@ def create_graph(parent: nil, children: 4, depth: 4) ) end end + + trait :shared_runners_disabled do + shared_runners_enabled { false } + end + + trait :allow_descendants_override_disabled_shared_runners do + allow_descendants_override_disabled_shared_runners { true } + end end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index f440efdfb072a4c6c2825d40f1b02ce628ec6338..ed027d02b5b5863b0a77a940602dd6ec7587057e 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1344,229 +1344,134 @@ def setup_group_members(group) end end - describe '#shared_runners_allowed?' do - using RSpec::Parameterized::TableSyntax - - where(:shared_runners_enabled, :allow_descendants_override, :expected_shared_runners_allowed) do - true | false | true - true | true | true - false | false | false - false | true | true - end - - with_them do - let!(:group) { create(:group, shared_runners_enabled: shared_runners_enabled, allow_descendants_override_disabled_shared_runners: allow_descendants_override) } - - it 'returns the expected result' do - expect(group.shared_runners_allowed?).to eq(expected_shared_runners_allowed) - end - end + def subject_and_reload(*models) + subject + models.map(&:reload) end - describe '#parent_allows_shared_runners?' do - context 'when parent group is present' do - using RSpec::Parameterized::TableSyntax - - where(:shared_runners_enabled, :allow_descendants_override, :expected_shared_runners_allowed) do - true | false | true - true | true | true - false | false | false - false | true | true - end - - with_them do - let!(:parent_group) { create(:group, shared_runners_enabled: shared_runners_enabled, allow_descendants_override_disabled_shared_runners: allow_descendants_override) } - let!(:group) { create(:group, parent: parent_group) } - - it 'returns the expected result' do - expect(group.parent_allows_shared_runners?).to eq(expected_shared_runners_allowed) + describe '#update_shared_runners_setting!' do + context 'enabled' do + subject { group.update_shared_runners_setting!('enabled') } + + context 'group that its ancestors have shared runners disabled' do + let_it_be(:parent) { create(:group, :shared_runners_disabled) } + let_it_be(:group) { create(:group, :shared_runners_disabled, parent: parent) } + let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } + + it 'raises error and does not enable shared Runners' do + expect { subject_and_reload(parent, group, project) } + .to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Shared runners enabled cannot be enabled because parent group has shared Runners disabled') + .and not_change { parent.shared_runners_enabled } + .and not_change { group.shared_runners_enabled } + .and not_change { project.shared_runners_enabled } end end - end - - context 'when parent group is missing' do - let!(:group) { create(:group) } - - it 'returns true' do - expect(group.parent_allows_shared_runners?).to be_truthy - end - end - end - - describe '#parent_enabled_shared_runners?' do - subject { group.parent_enabled_shared_runners? } - - context 'when parent group is present' do - context 'When shared Runners are disabled' do - let!(:parent_group) { create(:group, :shared_runners_disabled) } - let!(:group) { create(:group, parent: parent_group) } - - it { is_expected.to be_falsy } - end - - context 'When shared Runners are enabled' do - let!(:parent_group) { create(:group) } - let!(:group) { create(:group, parent: parent_group) } - - it { is_expected.to be_truthy } - end - end - context 'when parent group is missing' do - let!(:group) { create(:group) } + context 'root group with shared runners disabled' do + let_it_be(:group) { create(:group, :shared_runners_disabled) } + let_it_be(:sub_group) { create(:group, :shared_runners_disabled, parent: group) } + let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } - it { is_expected.to be_truthy } - end - end - - describe '#enable_shared_runners!' do - subject { group.enable_shared_runners! } - - context 'group that its ancestors have shared runners disabled' do - let_it_be(:parent) { create(:group, :shared_runners_disabled) } - let_it_be(:group) { create(:group, :shared_runners_disabled, parent: parent) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } - - it 'raises error and does not enable shared Runners' do - expect { subject } - .to raise_error(described_class::UpdateSharedRunnersError, 'Shared Runners disabled for the parent group') - .and not_change { parent.reload.shared_runners_enabled } - .and not_change { group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } + it 'enables shared Runners only for itself' do + expect { subject_and_reload(group, sub_group, project) } + .to change { group.shared_runners_enabled }.from(false).to(true) + .and not_change { sub_group.shared_runners_enabled } + .and not_change { project.shared_runners_enabled } + end end end - context 'root group with shared runners disabled' do - let_it_be(:group) { create(:group, :shared_runners_disabled) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } - - it 'enables shared Runners only for itself' do - expect { subject } - .to change { group.reload.shared_runners_enabled }.from(false).to(true) - .and not_change { sub_group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } + context 'disabled_and_unoverridable' do + let_it_be(:group) { create(:group) } + let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners, parent: group) } + let_it_be(:sub_group_2) { create(:group, parent: group) } + let_it_be(:project) { create(:project, group: group, shared_runners_enabled: true) } + let_it_be(:project_2) { create(:project, group: sub_group_2, shared_runners_enabled: true) } + + subject { group.update_shared_runners_setting!('disabled_and_unoverridable') } + + it 'disables shared Runners for all descendant groups and projects' do + expect { subject_and_reload(group, sub_group, sub_group_2, project, project_2) } + .to change { group.shared_runners_enabled }.from(true).to(false) + .and not_change { group.allow_descendants_override_disabled_shared_runners } + .and not_change { sub_group.shared_runners_enabled } + .and change { sub_group.allow_descendants_override_disabled_shared_runners }.from(true).to(false) + .and change { sub_group_2.shared_runners_enabled }.from(true).to(false) + .and not_change { sub_group_2.allow_descendants_override_disabled_shared_runners } + .and change { project.shared_runners_enabled }.from(true).to(false) + .and change { project_2.shared_runners_enabled }.from(true).to(false) + end + + context 'with override on self' do + let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners) } + + it 'disables it' do + expect { subject_and_reload(group) } + .to not_change { group.shared_runners_enabled } + .and change { group.allow_descendants_override_disabled_shared_runners }.from(true).to(false) + end end end - end - - describe '#disable_shared_runners!' do - let_it_be(:group) { create(:group) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners, parent: group) } - let_it_be(:sub_group_2) { create(:group, parent: group) } - let_it_be(:project) { create(:project, group: group, shared_runners_enabled: true) } - let_it_be(:project_2) { create(:project, group: sub_group_2, shared_runners_enabled: true) } - - subject { group.disable_shared_runners! } - - it 'disables shared Runners for all descendant groups and projects' do - expect { subject } - .to change { group.reload.shared_runners_enabled }.from(true).to(false) - .and not_change { group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { sub_group.reload.shared_runners_enabled } - .and not_change { sub_group.reload.allow_descendants_override_disabled_shared_runners } - .and change { sub_group_2.reload.shared_runners_enabled }.from(true).to(false) - .and not_change { sub_group_2.reload.allow_descendants_override_disabled_shared_runners } - .and change { project.reload.shared_runners_enabled }.from(true).to(false) - .and change { project_2.reload.shared_runners_enabled }.from(true).to(false) - end - end - - describe '#allow_descendants_override_disabled_shared_runners!' do - subject { group.allow_descendants_override_disabled_shared_runners! } - context 'top level group' do - let_it_be(:group) { create(:group, :shared_runners_disabled) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } + context 'disabled_with_override' do + subject { group.update_shared_runners_setting!('disabled_with_override') } - it 'enables allow descendants to override only for itself' do - expect { subject } - .to change { group.reload.allow_descendants_override_disabled_shared_runners }.from(false).to(true) - .and not_change { group.reload.shared_runners_enabled } - .and not_change { sub_group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { sub_group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } - end - end + context 'top level group' do + let_it_be(:group) { create(:group, :shared_runners_disabled) } + let_it_be(:sub_group) { create(:group, :shared_runners_disabled, parent: group) } + let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } - context 'group that its ancestors have shared Runners disabled but allows to override' do - let_it_be(:parent) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners) } - let_it_be(:group) { create(:group, :shared_runners_disabled, parent: parent) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } - - it 'enables allow descendants to override' do - expect { subject } - .to not_change { parent.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { parent.reload.shared_runners_enabled } - .and change { group.reload.allow_descendants_override_disabled_shared_runners }.from(false).to(true) - .and not_change { group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } + it 'enables allow descendants to override only for itself' do + expect { subject_and_reload(group, sub_group, project) } + .to change { group.allow_descendants_override_disabled_shared_runners }.from(false).to(true) + .and not_change { group.shared_runners_enabled } + .and not_change { sub_group.allow_descendants_override_disabled_shared_runners } + .and not_change { sub_group.shared_runners_enabled } + .and not_change { project.shared_runners_enabled } + end end - end - context 'when parent does not allow' do - let_it_be(:parent) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false ) } - let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false, parent: parent) } + context 'group that its ancestors have shared Runners disabled but allows to override' do + let_it_be(:parent) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners) } + let_it_be(:group) { create(:group, :shared_runners_disabled, parent: parent) } + let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } - it 'raises error and does not allow descendants to override' do - expect { subject } - .to raise_error(described_class::UpdateSharedRunnersError, 'Group level shared Runners not allowed') - .and not_change { parent.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { parent.reload.shared_runners_enabled } - .and not_change { group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { group.reload.shared_runners_enabled } + it 'enables allow descendants to override' do + expect { subject_and_reload(parent, group, project) } + .to not_change { parent.allow_descendants_override_disabled_shared_runners } + .and not_change { parent.shared_runners_enabled } + .and change { group.allow_descendants_override_disabled_shared_runners }.from(false).to(true) + .and not_change { group.shared_runners_enabled } + .and not_change { project.shared_runners_enabled } + end end - end - context 'top level group that has shared Runners enabled' do - let_it_be(:group) { create(:group, shared_runners_enabled: true) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } + context 'when parent does not allow' do + let_it_be(:parent) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false ) } + let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false, parent: parent) } - it 'raises error and does not change config' do - expect { subject } - .to raise_error(described_class::UpdateSharedRunnersError, 'Shared Runners enabled') - .and not_change { group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { group.reload.shared_runners_enabled } - .and not_change { sub_group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { sub_group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } + it 'raises error and does not allow descendants to override' do + expect { subject_and_reload(parent, group) } + .to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Allow descendants override disabled shared runners cannot be enabled because parent group does not allow it') + .and not_change { parent.allow_descendants_override_disabled_shared_runners } + .and not_change { parent.shared_runners_enabled } + .and not_change { group.allow_descendants_override_disabled_shared_runners } + .and not_change { group.shared_runners_enabled } + end end - end - end - describe '#disallow_descendants_override_disabled_shared_runners!' do - subject { group.disallow_descendants_override_disabled_shared_runners! } + context 'top level group that has shared Runners enabled' do + let_it_be(:group) { create(:group, shared_runners_enabled: true) } + let_it_be(:sub_group) { create(:group, shared_runners_enabled: true, parent: group) } + let_it_be(:project) { create(:project, shared_runners_enabled: true, group: sub_group) } - context 'top level group' do - let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners ) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners, parent: group) } - let_it_be(:project) { create(:project, shared_runners_enabled: true, group: sub_group) } - - it 'disables allow project to override for descendants and disables project shared Runners' do - expect { subject } - .to not_change { group.reload.shared_runners_enabled } - .and change { group.reload.allow_descendants_override_disabled_shared_runners }.from(true).to(false) - .and not_change { sub_group.reload.shared_runners_enabled } - .and change { sub_group.reload.allow_descendants_override_disabled_shared_runners }.from(true).to(false) - .and change { project.reload.shared_runners_enabled }.from(true).to(false) - end - end - - context 'top level group that has shared Runners enabled' do - let_it_be(:group) { create(:group, shared_runners_enabled: true) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } - - it 'results error and does not change config' do - expect { subject } - .to raise_error(described_class::UpdateSharedRunnersError, 'Shared Runners enabled') - .and not_change { group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { group.reload.shared_runners_enabled } - .and not_change { sub_group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { sub_group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } + it 'enables allow descendants to override & disables shared runners everywhere' do + expect { subject_and_reload(group, sub_group, project) } + .to change { group.shared_runners_enabled }.from(true).to(false) + .and change { group.allow_descendants_override_disabled_shared_runners }.from(false).to(true) + .and change { sub_group.shared_runners_enabled }.from(true).to(false) + .and change { project.shared_runners_enabled }.from(true).to(false) + end end end end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index ca1f06370d4cc519b25ed28e7956949cf1dbc0e2..2e607639e1be42349be656991e725023515fda07 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -1320,4 +1320,140 @@ def project_rugged(project) end end end + + describe '#shared_runners_setting' do + using RSpec::Parameterized::TableSyntax + + where(:shared_runners_enabled, :allow_descendants_override_disabled_shared_runners, :shared_runners_setting) do + true | true | 'enabled' + true | false | 'enabled' + false | true | 'disabled_with_override' + false | false | 'disabled_and_unoverridable' + end + + with_them do + let(:namespace) { build(:namespace, shared_runners_enabled: shared_runners_enabled, allow_descendants_override_disabled_shared_runners: allow_descendants_override_disabled_shared_runners)} + + it 'returns the result' do + expect(namespace.shared_runners_setting).to eq(shared_runners_setting) + end + end + end + + describe '#shared_runners_setting_higher_than?' do + using RSpec::Parameterized::TableSyntax + + where(:shared_runners_enabled, :allow_descendants_override_disabled_shared_runners, :other_setting, :result) do + true | true | 'enabled' | false + true | true | 'disabled_with_override' | true + true | true | 'disabled_and_unoverridable' | true + false | true | 'enabled' | false + false | true | 'disabled_with_override' | false + false | true | 'disabled_and_unoverridable' | true + false | false | 'enabled' | false + false | false | 'disabled_with_override' | false + false | false | 'disabled_and_unoverridable' | false + end + + with_them do + let(:namespace) { build(:namespace, shared_runners_enabled: shared_runners_enabled, allow_descendants_override_disabled_shared_runners: allow_descendants_override_disabled_shared_runners)} + + it 'returns the result' do + expect(namespace.shared_runners_setting_higher_than?(other_setting)).to eq(result) + end + end + end + + describe 'validation #changing_shared_runners_enabled_is_allowed' do + context 'without a parent' do + let(:namespace) { build(:namespace, shared_runners_enabled: true) } + + it 'is valid' do + expect(namespace).to be_valid + end + end + + context 'with a parent' do + context 'when parent has shared runners disabled' do + let(:parent) { create(:namespace, :shared_runners_disabled) } + let(:sub_namespace) { build(:namespace, shared_runners_enabled: true, parent_id: parent.id) } + + it 'is invalid' do + expect(sub_namespace).to be_invalid + expect(sub_namespace.errors[:shared_runners_enabled]).to include('cannot be enabled because parent group has shared Runners disabled') + end + end + + context 'when parent has shared runners disabled but allows override' do + let(:parent) { create(:namespace, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners) } + let(:sub_namespace) { build(:namespace, shared_runners_enabled: true, parent_id: parent.id) } + + it 'is valid' do + expect(sub_namespace).to be_valid + end + end + + context 'when parent has shared runners enabled' do + let(:parent) { create(:namespace, shared_runners_enabled: true) } + let(:sub_namespace) { build(:namespace, shared_runners_enabled: true, parent_id: parent.id) } + + it 'is valid' do + expect(sub_namespace).to be_valid + end + end + end + end + + describe 'validation #changing_allow_descendants_override_disabled_shared_runners_is_allowed' do + context 'without a parent' do + context 'with shared runners disabled' do + let(:namespace) { build(:namespace, :allow_descendants_override_disabled_shared_runners, :shared_runners_disabled) } + + it 'is valid' do + expect(namespace).to be_valid + end + end + + context 'with shared runners enabled' do + let(:namespace) { create(:namespace) } + + it 'is invalid' do + namespace.allow_descendants_override_disabled_shared_runners = true + + expect(namespace).to be_invalid + expect(namespace.errors[:allow_descendants_override_disabled_shared_runners]).to include('cannot be changed if shared runners are enabled') + end + end + end + + context 'with a parent' do + context 'when parent does not allow shared runners' do + let(:parent) { create(:namespace, :shared_runners_disabled) } + let(:sub_namespace) { build(:namespace, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners, parent_id: parent.id) } + + it 'is invalid' do + expect(sub_namespace).to be_invalid + expect(sub_namespace.errors[:allow_descendants_override_disabled_shared_runners]).to include('cannot be enabled because parent group does not allow it') + end + end + + context 'when parent allows shared runners and setting to true' do + let(:parent) { create(:namespace, shared_runners_enabled: true) } + let(:sub_namespace) { build(:namespace, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners, parent_id: parent.id) } + + it 'is valid' do + expect(sub_namespace).to be_valid + end + end + + context 'when parent allows shared runners and setting to false' do + let(:parent) { create(:namespace, shared_runners_enabled: true) } + let(:sub_namespace) { build(:namespace, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false, parent_id: parent.id) } + + it 'is valid' do + expect(sub_namespace).to be_valid + end + end + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 70bb590d885988f8344a47efef191b486cf5d06d..96645005d0aca65839d91b9235db4865264b0ee5 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -5813,6 +5813,38 @@ def enable_lfs end end + describe 'validation #changing_shared_runners_enabled_is_allowed' do + using RSpec::Parameterized::TableSyntax + + where(:shared_runners_setting, :project_shared_runners_enabled, :valid_record) do + 'enabled' | true | true + 'enabled' | false | true + 'disabled_with_override' | true | true + 'disabled_with_override' | false | true + 'disabled_and_unoverridable' | true | false + 'disabled_and_unoverridable' | false | true + end + + with_them do + let(:group) { create(:group) } + let(:project) { build(:project, namespace: group, shared_runners_enabled: project_shared_runners_enabled) } + + before do + allow_next_found_instance_of(Group) do |group| + allow(group).to receive(:shared_runners_setting).and_return(shared_runners_setting) + end + end + + it 'validates the configuration' do + expect(project.valid?).to eq(valid_record) + + unless valid_record + expect(project.errors[:shared_runners_enabled]).to contain_exactly('cannot be enabled because parent group does not allow it') + end + end + end + end + describe '#mark_pages_as_deployed' do let(:project) { create(:project) } let(:artifacts_archive) { create(:ci_job_artifact, project: project) } diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 6995caa9e6e0d065fa62bb71412ea71e9107537c..d5479fa2a06171c7ab7ae842637d397d186f8a34 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -185,4 +185,44 @@ end end end + + context 'shared runners configuration' do + context 'parent group present' do + using RSpec::Parameterized::TableSyntax + + where(:shared_runners_config, :descendants_override_disabled_shared_runners_config) do + true | false + false | false + # true | true # invalid at the group level, leaving as comment to make explicit + false | true + end + + with_them do + let!(:group) { create(:group, shared_runners_enabled: shared_runners_config, allow_descendants_override_disabled_shared_runners: descendants_override_disabled_shared_runners_config) } + let!(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) } + + before do + group.add_owner(user) + end + + it 'creates group following the parent config' do + new_group = service.execute + + expect(new_group.shared_runners_enabled).to eq(shared_runners_config) + expect(new_group.allow_descendants_override_disabled_shared_runners).to eq(descendants_override_disabled_shared_runners_config) + end + end + end + + context 'root group' do + let!(:service) { described_class.new(user) } + + it 'follows default config' do + new_group = service.execute + + expect(new_group.shared_runners_enabled).to eq(true) + expect(new_group.allow_descendants_override_disabled_shared_runners).to eq(false) + end + end + end end diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index 89e4d091ff7b9c4019c3a42fa564ce06bde066ff..6144b86a316a942d60e80bfe10e4ce490716f128 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -285,6 +285,44 @@ end end + context 'shared runners configuration' do + before do + create(:group_member, :owner, group: new_parent_group, user: user) + end + + context 'if parent group has disabled shared runners but allows overrides' do + let(:new_parent_group) { create(:group, shared_runners_enabled: false, allow_descendants_override_disabled_shared_runners: true) } + + it 'calls update service' do + expect(Groups::UpdateSharedRunnersService).to receive(:new).with(group, user, { shared_runners_setting: 'disabled_with_override' }).and_call_original + + transfer_service.execute(new_parent_group) + end + end + + context 'if parent group does not allow shared runners' do + let(:new_parent_group) { create(:group, shared_runners_enabled: false, allow_descendants_override_disabled_shared_runners: false) } + + it 'calls update service' do + expect(Groups::UpdateSharedRunnersService).to receive(:new).with(group, user, { shared_runners_setting: 'disabled_and_unoverridable' }).and_call_original + + transfer_service.execute(new_parent_group) + end + end + + context 'if parent group allows shared runners' do + let(:group) { create(:group, :public, :nested, shared_runners_enabled: false) } + let(:new_parent_group) { create(:group, shared_runners_enabled: true) } + + it 'does not call update service and keeps them disabled on the group' do + expect(Groups::UpdateSharedRunnersService).not_to receive(:new) + + transfer_service.execute(new_parent_group) + expect(group.reload.shared_runners_enabled).to be_falsy + end + end + end + context 'when a group is transferred to its subgroup' do let(:new_parent_group) { create(:group, parent: group) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 1e6a8d53354f93c056d8c1881a7e9077dca2e2de..a79cda86a86619e1a025a5ba9ff5f88754bb231f 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -283,6 +283,31 @@ end end + context 'change shared Runners config' do + let(:group) { create(:group) } + let(:project) { create(:project, shared_runners_enabled: true, group: group) } + + subject { described_class.new(group, user, shared_runners_setting: 'disabled_and_unoverridable').execute } + + before do + group.add_owner(user) + end + + it 'calls the shared runners update service' do + expect_any_instance_of(::Groups::UpdateSharedRunnersService).to receive(:execute).and_return({ status: :success }) + + expect(subject).to be_truthy + end + + it 'handles errors in the shared runners update service' do + expect_any_instance_of(::Groups::UpdateSharedRunnersService).to receive(:execute).and_return({ status: :error, message: 'something happened' }) + + expect(subject).to be_falsy + + expect(group.errors[:update_shared_runners].first).to eq('something happened') + end + end + def update_group(group, user, opts) Groups::UpdateService.new(group, user, opts).execute end diff --git a/spec/services/groups/update_shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb index 9fd8477a455d72f8c3d2179587c80037b5655323..e2838c4ce0b73b0cdbb46b1bcd0921f58b3cb0ff 100644 --- a/spec/services/groups/update_shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -13,17 +13,14 @@ context 'when current_user is not the group owner' do let_it_be(:group) { create(:group) } - let(:params) { { shared_runners_enabled: '0' } } + let(:params) { { shared_runners_setting: 'enabled' } } before do group.add_maintainer(user) end it 'results error and does not call any method' do - expect(group).not_to receive(:enable_shared_runners!) - expect(group).not_to receive(:disable_shared_runners!) - expect(group).not_to receive(:allow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:disallow_descendants_override_disabled_shared_runners!) + expect(group).not_to receive(:update_shared_runners_setting!) expect(subject[:status]).to eq(:error) expect(subject[:message]).to eq('Operation not allowed') @@ -37,191 +34,60 @@ end context 'enable shared Runners' do - where(:desired_params) do - ['1', true] - end - - with_them do - let(:params) { { shared_runners_enabled: desired_params } } - - context 'group that its ancestors have shared runners disabled' do - let_it_be(:parent) { create(:group, :shared_runners_disabled) } - let_it_be(:group) { create(:group, :shared_runners_disabled, parent: parent) } - - it 'results error' do - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Shared Runners disabled for the parent group') - end - end + let(:params) { { shared_runners_setting: 'enabled' } } - context 'root group with shared runners disabled' do - let_it_be(:group) { create(:group, :shared_runners_disabled) } + context 'group that its ancestors have shared runners disabled' do + let_it_be(:parent) { create(:group, :shared_runners_disabled) } + let_it_be(:group) { create(:group, :shared_runners_disabled, parent: parent) } - it 'receives correct method and succeeds' do - expect(group).to receive(:enable_shared_runners!) - expect(group).not_to receive(:disable_shared_runners!) - expect(group).not_to receive(:allow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:disallow_descendants_override_disabled_shared_runners!) - - expect(subject[:status]).to eq(:success) - end + it 'results error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Validation failed: Shared runners enabled cannot be enabled because parent group has shared Runners disabled') end end - end - - context 'disable shared Runners' do - let_it_be(:group) { create(:group) } - - where(:desired_params) do - ['0', false] - end - with_them do - let(:params) { { shared_runners_enabled: desired_params } } + context 'root group with shared runners disabled' do + let_it_be(:group) { create(:group, :shared_runners_disabled) } it 'receives correct method and succeeds' do - expect(group).to receive(:disable_shared_runners!) - expect(group).not_to receive(:enable_shared_runners!) - expect(group).not_to receive(:allow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:disallow_descendants_override_disabled_shared_runners!) + expect(group).to receive(:update_shared_runners_setting!).with('enabled') expect(subject[:status]).to eq(:success) end end end - context 'allow descendants to override' do - where(:desired_params) do - ['1', true] - end - - with_them do - let(:params) { { allow_descendants_override_disabled_shared_runners: desired_params } } - - context 'top level group' do - let_it_be(:group) { create(:group, :shared_runners_disabled) } - - it 'receives correct method and succeeds' do - expect(group).to receive(:allow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:disallow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:enable_shared_runners!) - expect(group).not_to receive(:disable_shared_runners!) - - expect(subject[:status]).to eq(:success) - end - end + context 'disable shared Runners' do + let_it_be(:group) { create(:group) } + let(:params) { { shared_runners_setting: 'disabled_and_unoverridable' } } - context 'when parent does not allow' do - let_it_be(:parent) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false ) } - let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false, parent: parent) } + it 'receives correct method and succeeds' do + expect(group).to receive(:update_shared_runners_setting!).with('disabled_and_unoverridable') - it 'results error' do - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Group level shared Runners not allowed') - end - end + expect(subject[:status]).to eq(:success) end end - context 'disallow descendants to override' do - where(:desired_params) do - ['0', false] - end - - with_them do - let(:params) { { allow_descendants_override_disabled_shared_runners: desired_params } } - - context 'top level group' do - let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners ) } - - it 'receives correct method and succeeds' do - expect(group).to receive(:disallow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:allow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:enable_shared_runners!) - expect(group).not_to receive(:disable_shared_runners!) - - expect(subject[:status]).to eq(:success) - end - end - - context 'top level group that has shared Runners enabled' do - let_it_be(:group) { create(:group, shared_runners_enabled: true) } - - it 'results error' do - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Shared Runners enabled') - end - end - end - end + context 'allow descendants to override' do + let(:params) { { shared_runners_setting: 'disabled_with_override' } } - context 'both params are present' do - context 'shared_runners_enabled: 1 and allow_descendants_override_disabled_shared_runners' do + context 'top level group' do let_it_be(:group) { create(:group, :shared_runners_disabled) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } - where(:allow_descendants_override) do - ['1', true, '0', false] - end + it 'receives correct method and succeeds' do + expect(group).to receive(:update_shared_runners_setting!).with('disabled_with_override') - with_them do - let(:params) { { shared_runners_enabled: '1', allow_descendants_override_disabled_shared_runners: allow_descendants_override } } - - it 'results in an error because shared Runners are enabled' do - expect { subject } - .to not_change { group.reload.shared_runners_enabled } - .and not_change { sub_group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } - .and not_change { group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { sub_group.reload.allow_descendants_override_disabled_shared_runners } - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Cannot set shared_runners_enabled to true and allow_descendants_override_disabled_shared_runners') - end + expect(subject[:status]).to eq(:success) end end - context 'shared_runners_enabled: 0 and allow_descendants_override_disabled_shared_runners: 0' do - let_it_be(:group) { create(:group, :allow_descendants_override_disabled_shared_runners) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners, parent: group) } - let_it_be(:sub_group_2) { create(:group, parent: group) } - let_it_be(:project) { create(:project, group: group, shared_runners_enabled: true) } - let_it_be(:project_2) { create(:project, group: sub_group_2, shared_runners_enabled: true) } - - let(:params) { { shared_runners_enabled: '0', allow_descendants_override_disabled_shared_runners: '0' } } - - it 'disables shared Runners and disable allow_descendants_override_disabled_shared_runners' do - expect { subject } - .to change { group.reload.shared_runners_enabled }.from(true).to(false) - .and change { group.reload.allow_descendants_override_disabled_shared_runners }.from(true).to(false) - .and not_change { sub_group.reload.shared_runners_enabled } - .and change { sub_group.reload.allow_descendants_override_disabled_shared_runners }.from(true).to(false) - .and change { sub_group_2.reload.shared_runners_enabled }.from(true).to(false) - .and not_change { sub_group_2.reload.allow_descendants_override_disabled_shared_runners } - .and change { project.reload.shared_runners_enabled }.from(true).to(false) - .and change { project_2.reload.shared_runners_enabled }.from(true).to(false) - end - end + context 'when parent does not allow' do + let_it_be(:parent) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false ) } + let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false, parent: parent) } - context 'shared_runners_enabled: 0 and allow_descendants_override_disabled_shared_runners: 1' do - let_it_be(:group) { create(:group) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:sub_group_2) { create(:group, parent: group) } - let_it_be(:project) { create(:project, group: group, shared_runners_enabled: true) } - let_it_be(:project_2) { create(:project, group: sub_group_2, shared_runners_enabled: true) } - - let(:params) { { shared_runners_enabled: '0', allow_descendants_override_disabled_shared_runners: '1' } } - - it 'disables shared Runners and enable allow_descendants_override_disabled_shared_runners only for itself' do - expect { subject } - .to change { group.reload.shared_runners_enabled }.from(true).to(false) - .and change { group.reload.allow_descendants_override_disabled_shared_runners }.from(false).to(true) - .and not_change { sub_group.reload.shared_runners_enabled } - .and not_change { sub_group.reload.allow_descendants_override_disabled_shared_runners } - .and change { sub_group_2.reload.shared_runners_enabled }.from(true).to(false) - .and not_change { sub_group_2.reload.allow_descendants_override_disabled_shared_runners } - .and change { project.reload.shared_runners_enabled }.from(true).to(false) - .and change { project_2.reload.shared_runners_enabled }.from(true).to(false) + it 'results error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Validation failed: Allow descendants override disabled shared runners cannot be enabled because parent group does not allow it') end end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index fde90dc8c802fa2fd8a5def5da798ee02fa80a13..b81b3e095cf8dd0943a19c9f4a6e4ed4182e51c8 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -782,4 +782,100 @@ def wiki_repo(project) def create_project(user, opts) Projects::CreateService.new(user, opts).execute end + + context 'shared Runners config' do + using RSpec::Parameterized::TableSyntax + + let_it_be(:user) { create :user } + + context 'when parent group is present' do + let_it_be(:group) do + create(:group) do |group| + group.add_owner(user) + end + end + + before do + allow_next_found_instance_of(Group) do |group| + allow(group).to receive(:shared_runners_setting).and_return(shared_runners_setting) + end + + user.refresh_authorized_projects # Ensure cache is warm + end + + context 'default value based on parent group setting' do + where(:shared_runners_setting, :desired_config_for_new_project, :expected_result_for_project) do + 'enabled' | nil | true + 'disabled_with_override' | nil | false + 'disabled_and_unoverridable' | nil | false + end + + with_them do + it 'creates project following the parent config' do + params = opts.merge(namespace_id: group.id) + params = params.merge(shared_runners_enabled: desired_config_for_new_project) unless desired_config_for_new_project.nil? + project = create_project(user, params) + + expect(project).to be_valid + expect(project.shared_runners_enabled).to eq(expected_result_for_project) + end + end + end + + context 'parent group is present and allows desired config' do + where(:shared_runners_setting, :desired_config_for_new_project, :expected_result_for_project) do + 'enabled' | true | true + 'enabled' | false | false + 'disabled_with_override' | false | false + 'disabled_with_override' | true | true + 'disabled_and_unoverridable' | false | false + end + + with_them do + it 'creates project following the parent config' do + params = opts.merge(namespace_id: group.id, shared_runners_enabled: desired_config_for_new_project) + project = create_project(user, params) + + expect(project).to be_valid + expect(project.shared_runners_enabled).to eq(expected_result_for_project) + end + end + end + + context 'parent group is present and disallows desired config' do + where(:shared_runners_setting, :desired_config_for_new_project) do + 'disabled_and_unoverridable' | true + end + + with_them do + it 'does not create project' do + params = opts.merge(namespace_id: group.id, shared_runners_enabled: desired_config_for_new_project) + project = create_project(user, params) + + expect(project.persisted?).to eq(false) + expect(project).to be_invalid + expect(project.errors[:shared_runners_enabled]).to include('cannot be enabled because parent group does not allow it') + end + end + end + end + + context 'parent group is not present' do + where(:desired_config, :expected_result) do + true | true + false | false + nil | true + end + + with_them do + it 'follows desired config' do + opts[:shared_runners_enabled] = desired_config unless desired_config.nil? + project = create_project(user, opts) + + expect(project).to be_valid + expect(project.shared_runners_enabled).to eq(expected_result) + end + end + end + end end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index a0e83fb4a219b06e1af1a76466bb92a92d68466f..3ae96d7a5ab0eda12a6856f0371a9aa3796acf33 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -314,6 +314,37 @@ def current_path end end + context 'shared Runners group level configurations' do + using RSpec::Parameterized::TableSyntax + + where(:project_shared_runners_enabled, :shared_runners_setting, :expected_shared_runners_enabled) do + true | 'disabled_and_unoverridable' | false + false | 'disabled_and_unoverridable' | false + true | 'disabled_with_override' | true + false | 'disabled_with_override' | false + true | 'enabled' | true + false | 'enabled' | false + end + + with_them do + let(:project) { create(:project, :public, :repository, namespace: user.namespace, shared_runners_enabled: project_shared_runners_enabled) } + let(:group) { create(:group) } + + before do + group.add_owner(user) + expect_next_found_instance_of(Group) do |group| + expect(group).to receive(:shared_runners_setting).and_return(shared_runners_setting) + end + + execute_transfer + end + + it 'updates shared runners based on the parent group' do + expect(project.shared_runners_enabled).to eq(expected_shared_runners_enabled) + end + end + end + context 'missing group labels applied to issues or merge requests' do it 'delegates transfer to Labels::TransferService' do group.add_owner(user) diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 7832d727220422094077a38e3d50f3598d179738..3375d9762c81c9a30acae94d95206cae6ea95857 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -151,6 +151,32 @@ def expect_to_call_unlink_fork_service expect(project.reload).to be_internal end end + + context 'when updating shared runners' do + context 'can enable shared runners' do + let(:group) { create(:group, shared_runners_enabled: true) } + let(:project) { create(:project, namespace: group, shared_runners_enabled: false) } + + it 'enables shared runners' do + result = update_project(project, user, shared_runners_enabled: true) + + expect(result).to eq({ status: :success }) + expect(project.reload.shared_runners_enabled).to be_truthy + end + end + + context 'cannot enable shared runners' do + let(:group) { create(:group, :shared_runners_disabled) } + let(:project) { create(:project, namespace: group, shared_runners_enabled: false) } + + it 'does not enable shared runners' do + result = update_project(project, user, shared_runners_enabled: true) + + expect(result).to eq({ status: :error, message: 'Shared runners enabled cannot be enabled because parent group does not allow it' }) + expect(project.reload.shared_runners_enabled).to be_falsey + end + end + end end describe 'when updating project that has forks' do