From 5a96d645f8258567bed7fcf38a47663074628b52 Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Fri, 29 May 2020 11:59:14 -0300 Subject: [PATCH 01/16] Add Groups::SharedRunnersService, related tests and migrations --- app/models/namespace.rb | 14 + app/services/groups/shared_runners_service.rb | 50 ++++ ...-SharedRunnersService-tests-migrations.yml | 5 + ...ers_disabled_and_override_to_namespaces.rb | 21 ++ db/structure.sql | 5 +- .../groups/shared_runners_service_spec.rb | 257 ++++++++++++++++++ 6 files changed, 351 insertions(+), 1 deletion(-) create mode 100644 app/services/groups/shared_runners_service.rb create mode 100644 changelogs/unreleased/add-Groups-SharedRunnersService-tests-migrations.yml create mode 100644 db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb create mode 100644 spec/services/groups/shared_runners_service_spec.rb diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 6176e8db9207b5..14f8743ff975c8 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -72,6 +72,8 @@ class Namespace < ApplicationRecord scope :for_user, -> { where('type IS NULL') } scope :sort_by_type, -> { order(Gitlab::Database.nulls_first_order(:type)) } + scope :with_id, ->(namespaces) { where(id: namespaces) } + scope :with_statistics, -> do joins('LEFT JOIN project_statistics ps ON ps.namespace_id = namespaces.id') .group('namespaces.id') @@ -215,6 +217,18 @@ def shared_runners_enabled? projects.with_shared_runners.any? end + def parent_group_disallows_shared_runners? + return false unless parent + + parent.shared_runners_disabled && !parent.allow_descendants_override_shared_runners + end + + def parent_group_disables_shared_runners? + return false unless parent + + parent.shared_runners_disabled + end + # Returns all ancestors, self, and descendants of the current namespace. def self_and_hierarchy Gitlab::ObjectHierarchy diff --git a/app/services/groups/shared_runners_service.rb b/app/services/groups/shared_runners_service.rb new file mode 100644 index 00000000000000..88932ee6a1337a --- /dev/null +++ b/app/services/groups/shared_runners_service.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module Groups + class SharedRunnersService < Groups::BaseService + def execute + raise Gitlab::Access::AccessDeniedError unless can?(@current_user, :admin_group, @group) + + if @params.fetch(:shared_runners_disabled, nil) == '1' + disable + elsif @params.fetch(:shared_runners_disabled, nil) == '0' + enable + end + + if @params.fetch(:allow_descendants_override_shared_runners, nil) == '1' + allow_descendants_override + elsif @params.fetch(:allow_descendants_override_shared_runners, nil) == '0' + disallow_descendants_override + end + end + + private + + def enable + raise 'Group level shared runners disabled' if @group.parent_group_disables_shared_runners? + + @group.update(shared_runners_disabled: false) + end + + def disable + group_ids = @group.self_and_descendants + Group.with_id(group_ids).update(shared_runners_disabled: true) + Group.with_id(group_ids).update(allow_descendants_override_shared_runners: false) + + Project.for_group_and_its_subgroups(@group).update(shared_runners_enabled: false) + end + + def allow_descendants_override + raise 'Group level shared runners not allowed' if @group.parent_group_disallows_shared_runners? + + @group.update(allow_descendants_override_shared_runners: true) + end + + def disallow_descendants_override + group_ids = @group.self_and_descendants + Group.with_id(group_ids).update(allow_descendants_override_shared_runners: false) + + Project.for_group_and_its_subgroups(@group).update(shared_runners_enabled: false) + end + end +end diff --git a/changelogs/unreleased/add-Groups-SharedRunnersService-tests-migrations.yml b/changelogs/unreleased/add-Groups-SharedRunnersService-tests-migrations.yml new file mode 100644 index 00000000000000..6178d696df1eb0 --- /dev/null +++ b/changelogs/unreleased/add-Groups-SharedRunnersService-tests-migrations.yml @@ -0,0 +1,5 @@ +--- +title: Add Groups::SharedRunnersService, related tests and migrations +merge_request: 33411 +author: Arthur de Lapertosa Lisboa +type: added diff --git a/db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb b/db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb new file mode 100644 index 00000000000000..45ff0e443c956c --- /dev/null +++ b/db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class AddSharedRunnersDisabledAndOverrideToNamespaces < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + with_lock_retries do + add_column :namespaces, :shared_runners_disabled, :boolean + add_column :namespaces, :allow_descendants_override_shared_runners, :boolean + end + end + + def down + with_lock_retries do + remove_column :namespaces, :shared_runners_disabled, :boolean + remove_column :namespaces, :allow_descendants_override_shared_runners, :boolean + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 93fcdaa4a1cef5..edb6017418983c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13086,7 +13086,9 @@ CREATE TABLE public.namespaces ( default_branch_protection smallint, unlock_membership_to_ldap boolean, max_personal_access_token_lifetime integer, - push_rule_id bigint + push_rule_id bigint, + shared_runners_disabled boolean, + allow_descendants_override_shared_runners boolean ); CREATE SEQUENCE public.namespaces_id_seq @@ -23366,6 +23368,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200424043515 20200424050250 20200424101920 +20200424102023 20200424135319 20200427064130 20200428134356 diff --git a/spec/services/groups/shared_runners_service_spec.rb b/spec/services/groups/shared_runners_service_spec.rb new file mode 100644 index 00000000000000..e5dace2641f008 --- /dev/null +++ b/spec/services/groups/shared_runners_service_spec.rb @@ -0,0 +1,257 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Groups::SharedRunnersService do + let(:user) { create(:user) } + + describe '#execute' do + context 'disable shared runners on top group level' do + let_it_be(:group) { create(:group) } + let_it_be(:sub_group_1) { create(:group, parent: group) } + let_it_be(:sub_group_2) { create(:group, parent: group) } + let_it_be(:sub_group_3) { create(:group, parent: sub_group_1) } + let_it_be(:sub_group_4) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true, parent: sub_group_3) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:project_2) { create(:project, group: group) } + let_it_be(:project_3) { create(:project, group: sub_group_1) } + let_it_be(:project_4) { create(:project, group: sub_group_2) } + let_it_be(:project_5) { create(:project, group: sub_group_3) } + let_it_be(:project_6) { create(:project, group: sub_group_4) } + + let(:params) { { shared_runners_disabled: '1' } } + + before do + group.add_owner(user) + end + + it 'disables shared runners for all descendant groups and projects' do + described_class.new(group, user, params).execute + + expect(group.reload.shared_runners_disabled).to be_truthy + expect(sub_group_1.reload.shared_runners_disabled).to be_truthy + expect(sub_group_2.reload.shared_runners_disabled).to be_truthy + expect(sub_group_3.reload.shared_runners_disabled).to be_truthy + expect(sub_group_4.reload.shared_runners_disabled).to be_truthy + expect(sub_group_4.reload.allow_descendants_override_shared_runners).to be_falsy + expect(project.reload.shared_runners_enabled).to be_falsy + expect(project_2.reload.shared_runners_enabled).to be_falsy + expect(project_3.reload.shared_runners_enabled).to be_falsy + expect(project_4.reload.shared_runners_enabled).to be_falsy + expect(project_5.reload.shared_runners_enabled).to be_falsy + expect(project_6.reload.shared_runners_enabled).to be_falsy + end + end + + context 'enable shared runners on group that its ascentors have shared runners disabled' do + let_it_be(:group) { create(:group, shared_runners_disabled: true) } + let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, parent: group) } + let_it_be(:sub_group_2) { create(:group, shared_runners_disabled: true, parent: sub_group_1) } + let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group_2) } + + let(:params) { { shared_runners_disabled: '0' } } + + subject { described_class.new(sub_group_2, user, params).execute } + + before do + group.add_owner(user) + end + + it 'raises error and does not enable shared runners' do + expect { subject }.to raise_error(RuntimeError) + + expect(group.reload.shared_runners_disabled).to be_truthy + expect(sub_group_1.reload.shared_runners_disabled).to be_truthy + expect(sub_group_2.reload.shared_runners_disabled).to be_truthy + expect(project.reload.shared_runners_enabled).to be_falsy + end + end + + context 'enable shared runners on top level group' do + let_it_be(:group) { create(:group, shared_runners_disabled: true) } + let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, parent: group) } + let_it_be(:sub_group_2) { create(:group, shared_runners_disabled: true, parent: sub_group_1) } + let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } + let_it_be(:project_2) { create(:project, shared_runners_enabled: false, group: sub_group_2) } + + let(:params) { { shared_runners_disabled: '0' } } + + before do + group.add_owner(user) + end + + it 'enables shared runners only for itself' do + described_class.new(group, user, params).execute + + expect(group.reload.shared_runners_disabled).to be_falsy + expect(sub_group_1.reload.shared_runners_disabled).to be_truthy + expect(sub_group_2.reload.shared_runners_disabled).to be_truthy + expect(project.reload.shared_runners_enabled).to be_falsy + expect(project_2.reload.shared_runners_enabled).to be_falsy + end + end + + context 'allow descendants to override' do + context 'top level group' do + let_it_be(:group) { create(:group, shared_runners_disabled: true) } + let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, parent: group) } + let_it_be(:sub_group_2) { create(:group, shared_runners_disabled: true, parent: sub_group_1) } + let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } + let_it_be(:project_2) { create(:project, shared_runners_enabled: false, group: sub_group_2) } + + let(:params) { { allow_descendants_override_shared_runners: '1' } } + + before do + group.add_owner(user) + end + + it 'enables allow descendants to override only for itself' do + described_class.new(group, user, params).execute + + expect(group.reload.allow_descendants_override_shared_runners).to be_truthy + expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_nil + expect(sub_group_2.reload.allow_descendants_override_shared_runners).to be_nil + expect(group.reload.shared_runners_disabled).to be_truthy + expect(sub_group_1.reload.shared_runners_disabled).to be_truthy + expect(sub_group_2.reload.shared_runners_disabled).to be_truthy + expect(project.reload.shared_runners_enabled).to be_falsy + expect(project_2.reload.shared_runners_enabled).to be_falsy + end + end + + context 'enable allow_descendants_override_shared_runners on group that its ascentors have shared runners disabled but allows to override' do + let_it_be(:group) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true) } + let_it_be(:sub_group) { create(:group, shared_runners_disabled: true, parent: group) } + let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } + + let(:params) { { allow_descendants_override_shared_runners: '1' } } + + before do + group.add_owner(user) + end + + it 'enables allow descendants to override' do + described_class.new(sub_group, user, params).execute + + expect(group.reload.allow_descendants_override_shared_runners).to be_truthy + expect(sub_group.reload.allow_descendants_override_shared_runners).to be_truthy + expect(group.reload.shared_runners_disabled).to be_truthy + expect(sub_group.reload.shared_runners_disabled).to be_truthy + expect(project.reload.shared_runners_enabled).to be_falsy + end + end + end + + context 'disallow descendants to override' do + let_it_be(:group) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true ) } + let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true, parent: group) } + let_it_be(:sub_group_2) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true, parent: sub_group_1) } + let_it_be(:project) { create(:project, shared_runners_enabled: true, group: group) } + let_it_be(:project_2) { create(:project, shared_runners_enabled: true, group: sub_group_2) } + + let(:params) { { allow_descendants_override_shared_runners: '0' } } + + before do + group.add_owner(user) + end + + it 'disables allow project to override for all descendants and disables projects' do + described_class.new(group, user, params).execute + + expect(group.reload.allow_descendants_override_shared_runners).to be_falsy + expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy + expect(sub_group_2.reload.allow_descendants_override_shared_runners).to be_falsy + expect(group.reload.shared_runners_disabled).to be_truthy + expect(sub_group_1.reload.shared_runners_disabled).to be_truthy + expect(sub_group_2.reload.shared_runners_disabled).to be_truthy + expect(project.reload.shared_runners_enabled).to be_falsy + expect(project_2.reload.shared_runners_enabled).to be_falsy + end + end + + context 'allow descendants to override when parent does not allow' do + context 'when allow_descendants_override_shared_runners is false' do + let_it_be(:group) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: false ) } + let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: false, parent: group) } + + let(:params) { { allow_descendants_override_shared_runners: '1' } } + + subject { described_class.new(sub_group_1, user, params).execute } + + before do + group.add_owner(user) + end + + it 'raises error and does not allow descendants to override' do + expect { subject }.to raise_error(RuntimeError) + + expect(group.reload.allow_descendants_override_shared_runners).to be_falsy + expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy + expect(group.reload.shared_runners_disabled).to be_truthy + expect(sub_group_1.reload.shared_runners_disabled).to be_truthy + end + end + + context 'when allow_descendants_override_shared_runners is nil' do + let_it_be(:group) { create(:group, shared_runners_disabled: true ) } + let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, parent: group) } + + let(:params) { { allow_descendants_override_shared_runners: '1' } } + + subject { described_class.new(sub_group_1, user, params).execute } + + before do + group.add_owner(user) + end + + it 'raises error and does not allow descendants to override' do + expect { subject }.to raise_error(RuntimeError) + + expect(group.reload.allow_descendants_override_shared_runners).to be_nil + expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_nil + expect(group.reload.shared_runners_disabled).to be_truthy + expect(sub_group_1.reload.shared_runners_disabled).to be_truthy + end + end + end + + context 'non admin user' do + let_it_be(:group) { create(:group) } + let_it_be(:sub_group_1) { create(:group, parent: group) } + let_it_be(:sub_group_2) { create(:group, parent: group) } + let_it_be(:sub_group_3) { create(:group, parent: sub_group_1) } + let_it_be(:sub_group_4) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true, parent: sub_group_3) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:project_2) { create(:project, group: group) } + let_it_be(:project_3) { create(:project, group: sub_group_1) } + let_it_be(:project_4) { create(:project, group: sub_group_2) } + let_it_be(:project_5) { create(:project, group: sub_group_3) } + let_it_be(:project_6) { create(:project, group: sub_group_4) } + + subject { described_class.new(sub_group_1, user, params).execute } + + let(:params) { { shared_runners_disabled: '1' } } + + before do + group.add_maintainer(user) + end + + it 'raises error and does not change any config' do + expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError) + + expect(group.reload.shared_runners_disabled).to be_nil + expect(sub_group_1.reload.shared_runners_disabled).to be_nil + expect(sub_group_2.reload.shared_runners_disabled).to be_nil + expect(sub_group_3.reload.shared_runners_disabled).to be_nil + expect(sub_group_4.reload.shared_runners_disabled).to be_truthy + expect(sub_group_4.reload.allow_descendants_override_shared_runners).to be_truthy + expect(project.reload.shared_runners_enabled).to be_truthy + expect(project_2.reload.shared_runners_enabled).to be_truthy + expect(project_3.reload.shared_runners_enabled).to be_truthy + expect(project_4.reload.shared_runners_enabled).to be_truthy + expect(project_5.reload.shared_runners_enabled).to be_truthy + expect(project_6.reload.shared_runners_enabled).to be_truthy + end + end + end +end -- GitLab From 5c97f05913747934706edeeb41838a44fe472f3a Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Tue, 2 Jun 2020 16:10:03 -0300 Subject: [PATCH 02/16] Add changes according to code review --- app/models/group.rb | 2 + app/models/namespace.rb | 4 +- app/services/groups/shared_runners_service.rb | 16 ++--- ...ers_disabled_and_override_to_namespaces.rb | 4 +- db/structure.sql | 4 +- spec/models/namespace_spec.rb | 60 +++++++++++++++++++ .../groups/shared_runners_service_spec.rb | 22 +++---- 7 files changed, 87 insertions(+), 25 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 71f58a5fd1ab67..510c2104241074 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -89,6 +89,8 @@ class Group < Namespace scope :with_users, -> { includes(:users) } + scope :with_id, ->(groups) { where(id: groups) } + class << self def sort_by_attribute(method) if method == 'storage_size_desc' diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 14f8743ff975c8..8c06a8f0516f37 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -72,8 +72,6 @@ class Namespace < ApplicationRecord scope :for_user, -> { where('type IS NULL') } scope :sort_by_type, -> { order(Gitlab::Database.nulls_first_order(:type)) } - scope :with_id, ->(namespaces) { where(id: namespaces) } - scope :with_statistics, -> do joins('LEFT JOIN project_statistics ps ON ps.namespace_id = namespaces.id') .group('namespaces.id') @@ -223,7 +221,7 @@ def parent_group_disallows_shared_runners? parent.shared_runners_disabled && !parent.allow_descendants_override_shared_runners end - def parent_group_disables_shared_runners? + def parent_group_disabled_shared_runners? return false unless parent parent.shared_runners_disabled diff --git a/app/services/groups/shared_runners_service.rb b/app/services/groups/shared_runners_service.rb index 88932ee6a1337a..758640e01b25f5 100644 --- a/app/services/groups/shared_runners_service.rb +++ b/app/services/groups/shared_runners_service.rb @@ -2,6 +2,8 @@ module Groups class SharedRunnersService < Groups::BaseService + SharedRunnersServiceError = Class.new(StandardError) + def execute raise Gitlab::Access::AccessDeniedError unless can?(@current_user, :admin_group, @group) @@ -21,30 +23,30 @@ def execute private def enable - raise 'Group level shared runners disabled' if @group.parent_group_disables_shared_runners? + raise SharedRunnersServiceError, 'Group level shared runners disabled' if @group.parent_group_disabled_shared_runners? @group.update(shared_runners_disabled: false) end def disable group_ids = @group.self_and_descendants - Group.with_id(group_ids).update(shared_runners_disabled: true) - Group.with_id(group_ids).update(allow_descendants_override_shared_runners: false) + Group.with_id(group_ids).update_all(shared_runners_disabled: true) + Group.with_id(group_ids).update_all(allow_descendants_override_shared_runners: false) - Project.for_group_and_its_subgroups(@group).update(shared_runners_enabled: false) + Project.for_group_and_its_subgroups(@group).update_all(shared_runners_enabled: false) end def allow_descendants_override - raise 'Group level shared runners not allowed' if @group.parent_group_disallows_shared_runners? + raise SharedRunnersServiceError, 'Group level shared runners not allowed' if @group.parent_group_disallows_shared_runners? @group.update(allow_descendants_override_shared_runners: true) end def disallow_descendants_override group_ids = @group.self_and_descendants - Group.with_id(group_ids).update(allow_descendants_override_shared_runners: false) + Group.with_id(group_ids).update_all(allow_descendants_override_shared_runners: false) - Project.for_group_and_its_subgroups(@group).update(shared_runners_enabled: false) + Project.for_group_and_its_subgroups(@group).update_all(shared_runners_enabled: false) end end end diff --git a/db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb b/db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb index 45ff0e443c956c..cd25a7a5778882 100644 --- a/db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb +++ b/db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb @@ -7,8 +7,8 @@ class AddSharedRunnersDisabledAndOverrideToNamespaces < ActiveRecord::Migration[ def up with_lock_retries do - add_column :namespaces, :shared_runners_disabled, :boolean - add_column :namespaces, :allow_descendants_override_shared_runners, :boolean + add_column :namespaces, :shared_runners_disabled, :boolean, default: false, null: false + add_column :namespaces, :allow_descendants_override_shared_runners, :boolean, default: false, null: false end end diff --git a/db/structure.sql b/db/structure.sql index edb6017418983c..605d96d436bab5 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13087,8 +13087,8 @@ CREATE TABLE public.namespaces ( unlock_membership_to_ldap boolean, max_personal_access_token_lifetime integer, push_rule_id bigint, - shared_runners_disabled boolean, - allow_descendants_override_shared_runners boolean + shared_runners_disabled boolean DEFAULT false NOT NULL, + allow_descendants_override_shared_runners boolean DEFAULT false NOT NULL ); CREATE SEQUENCE public.namespaces_id_seq diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 245d027d485f15..be8de90eb3fed0 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -591,6 +591,66 @@ def project_rugged(project) end end + describe '#parent_group_disallows_shared_runners?' do + context 'when parent group is present' do + using RSpec::Parameterized::TableSyntax + + where(:shared_runners_disabled, :allow_descendants_override, :shared_runners_disallowed?) do + false | false | false + false | true | false + true | false | true + true | true | false + end + + with_them do + let!(:parent_group) { create(:group, shared_runners_disabled: shared_runners_disabled, allow_descendants_override_shared_runners: allow_descendants_override) } + let!(:group) { create(:group, parent: parent_group) } + + it 'returns the expected result' do + expect(group.parent_group_disallows_shared_runners?).to eq(shared_runners_disallowed?) + end + end + end + + context 'when parent group is missing' do + let!(:group) { create(:group) } + + it 'returns false' do + expect(group.parent_group_disallows_shared_runners?).to be_falsy + end + end + end + + describe '#parent_group_disabled_shared_runners?' do + context 'when parent group is present' do + context 'When shared_runners_disabled is true' do + let!(:parent_group) { create(:group, shared_runners_disabled: true) } + let!(:group) { create(:group, parent: parent_group) } + + it 'returns true' do + expect(group.parent_group_disabled_shared_runners?).to be_truthy + end + end + + context 'When shared_runners_disabled is false' do + let!(:parent_group) { create(:group, shared_runners_disabled: false) } + let!(:group) { create(:group, parent: parent_group) } + + it 'returns false' do + expect(group.parent_group_disabled_shared_runners?).to be_falsy + end + end + end + + context 'when parent group is missing' do + let!(:group) { create(:group) } + + it 'returns false' do + expect(group.parent_group_disabled_shared_runners?).to be_falsy + end + end + end + describe '#self_and_hierarchy' do let!(:group) { create(:group, path: 'git_lab') } let!(:nested_group) { create(:group, parent: group) } diff --git a/spec/services/groups/shared_runners_service_spec.rb b/spec/services/groups/shared_runners_service_spec.rb index e5dace2641f008..641c7ca233344f 100644 --- a/spec/services/groups/shared_runners_service_spec.rb +++ b/spec/services/groups/shared_runners_service_spec.rb @@ -58,7 +58,7 @@ end it 'raises error and does not enable shared runners' do - expect { subject }.to raise_error(RuntimeError) + expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError) expect(group.reload.shared_runners_disabled).to be_truthy expect(sub_group_1.reload.shared_runners_disabled).to be_truthy @@ -109,8 +109,8 @@ described_class.new(group, user, params).execute expect(group.reload.allow_descendants_override_shared_runners).to be_truthy - expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_nil - expect(sub_group_2.reload.allow_descendants_override_shared_runners).to be_nil + expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy + expect(sub_group_2.reload.allow_descendants_override_shared_runners).to be_falsy expect(group.reload.shared_runners_disabled).to be_truthy expect(sub_group_1.reload.shared_runners_disabled).to be_truthy expect(sub_group_2.reload.shared_runners_disabled).to be_truthy @@ -183,7 +183,7 @@ end it 'raises error and does not allow descendants to override' do - expect { subject }.to raise_error(RuntimeError) + expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError) expect(group.reload.allow_descendants_override_shared_runners).to be_falsy expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy @@ -205,10 +205,10 @@ end it 'raises error and does not allow descendants to override' do - expect { subject }.to raise_error(RuntimeError) + expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError) - expect(group.reload.allow_descendants_override_shared_runners).to be_nil - expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_nil + expect(group.reload.allow_descendants_override_shared_runners).to be_falsy + expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy expect(group.reload.shared_runners_disabled).to be_truthy expect(sub_group_1.reload.shared_runners_disabled).to be_truthy end @@ -239,10 +239,10 @@ it 'raises error and does not change any config' do expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError) - expect(group.reload.shared_runners_disabled).to be_nil - expect(sub_group_1.reload.shared_runners_disabled).to be_nil - expect(sub_group_2.reload.shared_runners_disabled).to be_nil - expect(sub_group_3.reload.shared_runners_disabled).to be_nil + expect(group.reload.shared_runners_disabled).to be_falsy + expect(sub_group_1.reload.shared_runners_disabled).to be_falsy + expect(sub_group_2.reload.shared_runners_disabled).to be_falsy + expect(sub_group_3.reload.shared_runners_disabled).to be_falsy expect(sub_group_4.reload.shared_runners_disabled).to be_truthy expect(sub_group_4.reload.allow_descendants_override_shared_runners).to be_truthy expect(project.reload.shared_runners_enabled).to be_truthy -- GitLab From fdceaa92b4ff0d6c51b12b6f9effb7ca2cd789d0 Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Tue, 2 Jun 2020 16:11:34 -0300 Subject: [PATCH 03/16] Add traits according to code review --- spec/factories/groups.rb | 8 ++++ .../groups/shared_runners_service_spec.rb | 40 +++++++++---------- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb index d51c437f83ae8b..a7e1038d20269b 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -51,5 +51,13 @@ trait :owner_subgroup_creation_only do subgroup_creation_level { ::Gitlab::Access::OWNER_SUBGROUP_ACCESS } end + + trait :shared_runners_disabled do + shared_runners_disabled { true } + end + + trait :allow_descendants_override_shared_runners do + allow_descendants_override_shared_runners { true } + end end end diff --git a/spec/services/groups/shared_runners_service_spec.rb b/spec/services/groups/shared_runners_service_spec.rb index 641c7ca233344f..9395f00f906d1f 100644 --- a/spec/services/groups/shared_runners_service_spec.rb +++ b/spec/services/groups/shared_runners_service_spec.rb @@ -11,7 +11,7 @@ let_it_be(:sub_group_1) { create(:group, parent: group) } let_it_be(:sub_group_2) { create(:group, parent: group) } let_it_be(:sub_group_3) { create(:group, parent: sub_group_1) } - let_it_be(:sub_group_4) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true, parent: sub_group_3) } + let_it_be(:sub_group_4) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: sub_group_3) } let_it_be(:project) { create(:project, group: group) } let_it_be(:project_2) { create(:project, group: group) } let_it_be(:project_3) { create(:project, group: sub_group_1) } @@ -44,9 +44,9 @@ end context 'enable shared runners on group that its ascentors have shared runners disabled' do - let_it_be(:group) { create(:group, shared_runners_disabled: true) } - let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, parent: group) } - let_it_be(:sub_group_2) { create(:group, shared_runners_disabled: true, parent: sub_group_1) } + let_it_be(:group) { create(:group, :shared_runners_disabled) } + let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, parent: group) } + let_it_be(:sub_group_2) { create(:group, :shared_runners_disabled, parent: sub_group_1) } let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group_2) } let(:params) { { shared_runners_disabled: '0' } } @@ -68,9 +68,9 @@ end context 'enable shared runners on top level group' do - let_it_be(:group) { create(:group, shared_runners_disabled: true) } - let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, parent: group) } - let_it_be(:sub_group_2) { create(:group, shared_runners_disabled: true, parent: sub_group_1) } + let_it_be(:group) { create(:group, :shared_runners_disabled) } + let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, parent: group) } + let_it_be(:sub_group_2) { create(:group, :shared_runners_disabled, parent: sub_group_1) } let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } let_it_be(:project_2) { create(:project, shared_runners_enabled: false, group: sub_group_2) } @@ -93,9 +93,9 @@ context 'allow descendants to override' do context 'top level group' do - let_it_be(:group) { create(:group, shared_runners_disabled: true) } - let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, parent: group) } - let_it_be(:sub_group_2) { create(:group, shared_runners_disabled: true, parent: sub_group_1) } + let_it_be(:group) { create(:group, :shared_runners_disabled) } + let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, parent: group) } + let_it_be(:sub_group_2) { create(:group, :shared_runners_disabled, parent: sub_group_1) } let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } let_it_be(:project_2) { create(:project, shared_runners_enabled: false, group: sub_group_2) } @@ -120,8 +120,8 @@ end context 'enable allow_descendants_override_shared_runners on group that its ascentors have shared runners disabled but allows to override' do - let_it_be(:group) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true) } - let_it_be(:sub_group) { create(:group, shared_runners_disabled: true, parent: group) } + let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners) } + 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) } let(:params) { { allow_descendants_override_shared_runners: '1' } } @@ -143,9 +143,9 @@ end context 'disallow descendants to override' do - let_it_be(:group) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true ) } - let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true, parent: group) } - let_it_be(:sub_group_2) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true, parent: sub_group_1) } + let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners ) } + let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: group) } + let_it_be(:sub_group_2) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: sub_group_1) } let_it_be(:project) { create(:project, shared_runners_enabled: true, group: group) } let_it_be(:project_2) { create(:project, shared_runners_enabled: true, group: sub_group_2) } @@ -171,8 +171,8 @@ context 'allow descendants to override when parent does not allow' do context 'when allow_descendants_override_shared_runners is false' do - let_it_be(:group) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: false ) } - let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: false, parent: group) } + let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false ) } + let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false, parent: group) } let(:params) { { allow_descendants_override_shared_runners: '1' } } @@ -193,8 +193,8 @@ end context 'when allow_descendants_override_shared_runners is nil' do - let_it_be(:group) { create(:group, shared_runners_disabled: true ) } - let_it_be(:sub_group_1) { create(:group, shared_runners_disabled: true, parent: group) } + let_it_be(:group) { create(:group, :shared_runners_disabled ) } + let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, parent: group) } let(:params) { { allow_descendants_override_shared_runners: '1' } } @@ -220,7 +220,7 @@ let_it_be(:sub_group_1) { create(:group, parent: group) } let_it_be(:sub_group_2) { create(:group, parent: group) } let_it_be(:sub_group_3) { create(:group, parent: sub_group_1) } - let_it_be(:sub_group_4) { create(:group, shared_runners_disabled: true, allow_descendants_override_shared_runners: true, parent: sub_group_3) } + let_it_be(:sub_group_4) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: sub_group_3) } let_it_be(:project) { create(:project, group: group) } let_it_be(:project_2) { create(:project, group: group) } let_it_be(:project_3) { create(:project, group: sub_group_1) } -- GitLab From fd8e75d69c790b6d0f7bdca95989f2d68af4fd2b Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Thu, 4 Jun 2020 15:08:46 -0300 Subject: [PATCH 04/16] Modify method names according to code review --- app/services/groups/shared_runners_service.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/services/groups/shared_runners_service.rb b/app/services/groups/shared_runners_service.rb index 758640e01b25f5..e99deda5e04973 100644 --- a/app/services/groups/shared_runners_service.rb +++ b/app/services/groups/shared_runners_service.rb @@ -8,27 +8,27 @@ def execute raise Gitlab::Access::AccessDeniedError unless can?(@current_user, :admin_group, @group) if @params.fetch(:shared_runners_disabled, nil) == '1' - disable + disable_shared_runners! elsif @params.fetch(:shared_runners_disabled, nil) == '0' - enable + enable_shared_runners! end if @params.fetch(:allow_descendants_override_shared_runners, nil) == '1' - allow_descendants_override + allow_descendants_override_shared_runners! elsif @params.fetch(:allow_descendants_override_shared_runners, nil) == '0' - disallow_descendants_override + disallow_descendants_override_shared_runners! end end private - def enable + def enable_shared_runners! raise SharedRunnersServiceError, 'Group level shared runners disabled' if @group.parent_group_disabled_shared_runners? @group.update(shared_runners_disabled: false) end - def disable + def disable_shared_runners! group_ids = @group.self_and_descendants Group.with_id(group_ids).update_all(shared_runners_disabled: true) Group.with_id(group_ids).update_all(allow_descendants_override_shared_runners: false) @@ -36,13 +36,13 @@ def disable Project.for_group_and_its_subgroups(@group).update_all(shared_runners_enabled: false) end - def allow_descendants_override + def allow_descendants_override_shared_runners! raise SharedRunnersServiceError, 'Group level shared runners not allowed' if @group.parent_group_disallows_shared_runners? @group.update(allow_descendants_override_shared_runners: true) end - def disallow_descendants_override + def disallow_descendants_override_shared_runners! group_ids = @group.self_and_descendants Group.with_id(group_ids).update_all(allow_descendants_override_shared_runners: false) -- GitLab From e29763a03354edffa0ccf66488f78677b44c3b9c Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Mon, 8 Jun 2020 13:47:01 -0300 Subject: [PATCH 05/16] Add changes according to code review --- app/models/group.rb | 14 +- app/models/namespace.rb | 12 - app/services/groups/shared_runners_service.rb | 56 ++-- spec/models/group_spec.rb | 60 ++++ spec/models/namespace_spec.rb | 60 ---- .../groups/shared_runners_service_spec.rb | 297 +++++++----------- 6 files changed, 212 insertions(+), 287 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 510c2104241074..6edb0a00a08011 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -89,7 +89,7 @@ class Group < Namespace scope :with_users, -> { includes(:users) } - scope :with_id, ->(groups) { where(id: groups) } + scope :by_id, ->(groups) { where(id: groups) } class << self def sort_by_attribute(method) @@ -506,6 +506,18 @@ def preload_shared_group_links preloader.preload(self, shared_with_group_links: [shared_with_group: :route]) end + def parent_group_disallows_shared_runners? + return false unless has_parent? + + parent.shared_runners_disabled? && !parent.allow_descendants_override_shared_runners? + end + + def parent_group_disabled_shared_runners? + return false unless has_parent? + + parent.shared_runners_disabled? + end + private def update_two_factor_requirement diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 8c06a8f0516f37..6176e8db9207b5 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -215,18 +215,6 @@ def shared_runners_enabled? projects.with_shared_runners.any? end - def parent_group_disallows_shared_runners? - return false unless parent - - parent.shared_runners_disabled && !parent.allow_descendants_override_shared_runners - end - - def parent_group_disabled_shared_runners? - return false unless parent - - parent.shared_runners_disabled - end - # Returns all ancestors, self, and descendants of the current namespace. def self_and_hierarchy Gitlab::ObjectHierarchy diff --git a/app/services/groups/shared_runners_service.rb b/app/services/groups/shared_runners_service.rb index e99deda5e04973..ba75582a3b29a5 100644 --- a/app/services/groups/shared_runners_service.rb +++ b/app/services/groups/shared_runners_service.rb @@ -5,48 +5,54 @@ class SharedRunnersService < Groups::BaseService SharedRunnersServiceError = Class.new(StandardError) def execute - raise Gitlab::Access::AccessDeniedError unless can?(@current_user, :admin_group, @group) + raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_group, group) - if @params.fetch(:shared_runners_disabled, nil) == '1' - disable_shared_runners! - elsif @params.fetch(:shared_runners_disabled, nil) == '0' - enable_shared_runners! - end - - if @params.fetch(:allow_descendants_override_shared_runners, nil) == '1' - allow_descendants_override_shared_runners! - elsif @params.fetch(:allow_descendants_override_shared_runners, nil) == '0' - disallow_descendants_override_shared_runners! - end + enable_or_disable_shared_runners! + allow_or_disallow_descendants_override_shared_runners! end private - def enable_shared_runners! - raise SharedRunnersServiceError, 'Group level shared runners disabled' if @group.parent_group_disabled_shared_runners? + def enable_or_disable_shared_runners! + return unless params[:shared_runners_disabled].present? + + if params[:shared_runners_disabled] == '1' + disable_shared_runners! + else + raise SharedRunnersServiceError, 'Group level shared Runners disabled' if group.parent_group_disabled_shared_runners? - @group.update(shared_runners_disabled: false) + group.update_column(:shared_runners_disabled, false) + end end def disable_shared_runners! - group_ids = @group.self_and_descendants - Group.with_id(group_ids).update_all(shared_runners_disabled: true) - Group.with_id(group_ids).update_all(allow_descendants_override_shared_runners: false) + group_ids = group.self_and_descendants + return if group_ids.empty? + + Group.by_id(group_ids).update_all(shared_runners_disabled: true, allow_descendants_override_shared_runners: false) - Project.for_group_and_its_subgroups(@group).update_all(shared_runners_enabled: false) + group.all_projects.update_all(shared_runners_enabled: false) end - def allow_descendants_override_shared_runners! - raise SharedRunnersServiceError, 'Group level shared runners not allowed' if @group.parent_group_disallows_shared_runners? + def allow_or_disallow_descendants_override_shared_runners! + return unless params[:allow_descendants_override_shared_runners].present? + + if params[:allow_descendants_override_shared_runners] == '1' + raise SharedRunnersServiceError, 'Group level shared Runners not allowed' if group.parent_group_disallows_shared_runners? - @group.update(allow_descendants_override_shared_runners: true) + group.update_column(:allow_descendants_override_shared_runners, true) + else + disallow_descendants_override_shared_runners! + end end def disallow_descendants_override_shared_runners! - group_ids = @group.self_and_descendants - Group.with_id(group_ids).update_all(allow_descendants_override_shared_runners: false) + group_ids = group.self_and_descendants + return if group_ids.empty? + + Group.by_id(group_ids).update_all(allow_descendants_override_shared_runners: false) - Project.for_group_and_its_subgroups(@group).update_all(shared_runners_enabled: false) + Project.for_group_and_its_subgroups(group).update_all(shared_runners_enabled: false) end end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index da1bf026f6078c..0fce6004706694 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1313,4 +1313,64 @@ def setup_group_members(group) expect(groups).to contain_exactly(parent_group1, parent_group2, child_group1, child_group2, child_group3) end end + + describe '#parent_group_disallows_shared_runners?' do + context 'when parent group is present' do + using RSpec::Parameterized::TableSyntax + + where(:shared_runners_disabled, :allow_descendants_override, :shared_runners_disallowed?) do + false | false | false + false | true | false + true | false | true + true | true | false + end + + with_them do + let!(:parent_group) { create(:group, shared_runners_disabled: shared_runners_disabled, allow_descendants_override_shared_runners: allow_descendants_override) } + let!(:group) { create(:group, parent: parent_group) } + + it 'returns the expected result' do + expect(group.parent_group_disallows_shared_runners?).to eq(shared_runners_disallowed?) + end + end + end + + context 'when parent group is missing' do + let!(:group) { create(:group) } + + it 'returns false' do + expect(group.parent_group_disallows_shared_runners?).to be_falsy + end + end + end + + describe '#parent_group_disabled_shared_runners?' do + context 'when parent group is present' do + context 'When shared_runners_disabled is true' do + let!(:parent_group) { create(:group, shared_runners_disabled: true) } + let!(:group) { create(:group, parent: parent_group) } + + it 'returns true' do + expect(group.parent_group_disabled_shared_runners?).to be_truthy + end + end + + context 'When shared_runners_disabled is false' do + let!(:parent_group) { create(:group, shared_runners_disabled: false) } + let!(:group) { create(:group, parent: parent_group) } + + it 'returns false' do + expect(group.parent_group_disabled_shared_runners?).to be_falsy + end + end + end + + context 'when parent group is missing' do + let!(:group) { create(:group) } + + it 'returns false' do + expect(group.parent_group_disabled_shared_runners?).to be_falsy + end + end + end end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index be8de90eb3fed0..245d027d485f15 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -591,66 +591,6 @@ def project_rugged(project) end end - describe '#parent_group_disallows_shared_runners?' do - context 'when parent group is present' do - using RSpec::Parameterized::TableSyntax - - where(:shared_runners_disabled, :allow_descendants_override, :shared_runners_disallowed?) do - false | false | false - false | true | false - true | false | true - true | true | false - end - - with_them do - let!(:parent_group) { create(:group, shared_runners_disabled: shared_runners_disabled, allow_descendants_override_shared_runners: allow_descendants_override) } - let!(:group) { create(:group, parent: parent_group) } - - it 'returns the expected result' do - expect(group.parent_group_disallows_shared_runners?).to eq(shared_runners_disallowed?) - end - end - end - - context 'when parent group is missing' do - let!(:group) { create(:group) } - - it 'returns false' do - expect(group.parent_group_disallows_shared_runners?).to be_falsy - end - end - end - - describe '#parent_group_disabled_shared_runners?' do - context 'when parent group is present' do - context 'When shared_runners_disabled is true' do - let!(:parent_group) { create(:group, shared_runners_disabled: true) } - let!(:group) { create(:group, parent: parent_group) } - - it 'returns true' do - expect(group.parent_group_disabled_shared_runners?).to be_truthy - end - end - - context 'When shared_runners_disabled is false' do - let!(:parent_group) { create(:group, shared_runners_disabled: false) } - let!(:group) { create(:group, parent: parent_group) } - - it 'returns false' do - expect(group.parent_group_disabled_shared_runners?).to be_falsy - end - end - end - - context 'when parent group is missing' do - let!(:group) { create(:group) } - - it 'returns false' do - expect(group.parent_group_disabled_shared_runners?).to be_falsy - end - end - end - describe '#self_and_hierarchy' do let!(:group) { create(:group, path: 'git_lab') } let!(:nested_group) { create(:group, parent: group) } diff --git a/spec/services/groups/shared_runners_service_spec.rb b/spec/services/groups/shared_runners_service_spec.rb index 9395f00f906d1f..05f9105ac8f87f 100644 --- a/spec/services/groups/shared_runners_service_spec.rb +++ b/spec/services/groups/shared_runners_service_spec.rb @@ -6,251 +6,170 @@ let(:user) { create(:user) } describe '#execute' do - context 'disable shared runners on top group level' do + context 'when current_user is not the group owner' do let_it_be(:group) { create(:group) } - let_it_be(:sub_group_1) { create(:group, parent: group) } - let_it_be(:sub_group_2) { create(:group, parent: group) } - let_it_be(:sub_group_3) { create(:group, parent: sub_group_1) } - let_it_be(:sub_group_4) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: sub_group_3) } let_it_be(:project) { create(:project, group: group) } - let_it_be(:project_2) { create(:project, group: group) } - let_it_be(:project_3) { create(:project, group: sub_group_1) } - let_it_be(:project_4) { create(:project, group: sub_group_2) } - let_it_be(:project_5) { create(:project, group: sub_group_3) } - let_it_be(:project_6) { create(:project, group: sub_group_4) } - let(:params) { { shared_runners_disabled: '1' } } - - before do - group.add_owner(user) - end - - it 'disables shared runners for all descendant groups and projects' do - described_class.new(group, user, params).execute - - expect(group.reload.shared_runners_disabled).to be_truthy - expect(sub_group_1.reload.shared_runners_disabled).to be_truthy - expect(sub_group_2.reload.shared_runners_disabled).to be_truthy - expect(sub_group_3.reload.shared_runners_disabled).to be_truthy - expect(sub_group_4.reload.shared_runners_disabled).to be_truthy - expect(sub_group_4.reload.allow_descendants_override_shared_runners).to be_falsy - expect(project.reload.shared_runners_enabled).to be_falsy - expect(project_2.reload.shared_runners_enabled).to be_falsy - expect(project_3.reload.shared_runners_enabled).to be_falsy - expect(project_4.reload.shared_runners_enabled).to be_falsy - expect(project_5.reload.shared_runners_enabled).to be_falsy - expect(project_6.reload.shared_runners_enabled).to be_falsy - end - end - - context 'enable shared runners on group that its ascentors have shared runners disabled' do - let_it_be(:group) { create(:group, :shared_runners_disabled) } - let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:sub_group_2) { create(:group, :shared_runners_disabled, parent: sub_group_1) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group_2) } + subject { described_class.new(group, user, params).execute } - let(:params) { { shared_runners_disabled: '0' } } - - subject { described_class.new(sub_group_2, user, params).execute } + let(:params) { { shared_runners_disabled: '1' } } before do - group.add_owner(user) + group.add_maintainer(user) end - it 'raises error and does not enable shared runners' do - expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError) + it 'raises error and does not change any config' do + expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError) - expect(group.reload.shared_runners_disabled).to be_truthy - expect(sub_group_1.reload.shared_runners_disabled).to be_truthy - expect(sub_group_2.reload.shared_runners_disabled).to be_truthy - expect(project.reload.shared_runners_enabled).to be_falsy + expect(group.reload.shared_runners_disabled).to be_falsy + expect(project.reload.shared_runners_enabled).to be_truthy end end - context 'enable shared runners on top level group' do - let_it_be(:group) { create(:group, :shared_runners_disabled) } - let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:sub_group_2) { create(:group, :shared_runners_disabled, parent: sub_group_1) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } - let_it_be(:project_2) { create(:project, shared_runners_enabled: false, group: sub_group_2) } - - let(:params) { { shared_runners_disabled: '0' } } - + context 'when current_user is the group owner' do before do group.add_owner(user) end - it 'enables shared runners only for itself' do - described_class.new(group, user, params).execute + context 'enable shared Runners' do + let(:params) { { shared_runners_disabled: '0' } } - expect(group.reload.shared_runners_disabled).to be_falsy - expect(sub_group_1.reload.shared_runners_disabled).to be_truthy - expect(sub_group_2.reload.shared_runners_disabled).to be_truthy - expect(project.reload.shared_runners_enabled).to be_falsy - expect(project_2.reload.shared_runners_enabled).to be_falsy - end - end + context 'group that its ancestors have 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) } - context 'allow descendants to override' do - context 'top level group' do - let_it_be(:group) { create(:group, :shared_runners_disabled) } - let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:sub_group_2) { create(:group, :shared_runners_disabled, parent: sub_group_1) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } - let_it_be(:project_2) { create(:project, shared_runners_enabled: false, group: sub_group_2) } + subject { described_class.new(sub_group, user, params).execute } - let(:params) { { allow_descendants_override_shared_runners: '1' } } + it 'raises error and does not enable shared Runners' do + expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners disabled') - before do - group.add_owner(user) + expect(group.reload.shared_runners_disabled).to be_truthy + expect(sub_group.reload.shared_runners_disabled).to be_truthy + expect(project.reload.shared_runners_enabled).to be_falsy + end end - it 'enables allow descendants to override only for itself' do - described_class.new(group, user, params).execute + 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) } - expect(group.reload.allow_descendants_override_shared_runners).to be_truthy - expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy - expect(sub_group_2.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.reload.shared_runners_disabled).to be_truthy - expect(sub_group_1.reload.shared_runners_disabled).to be_truthy - expect(sub_group_2.reload.shared_runners_disabled).to be_truthy - expect(project.reload.shared_runners_enabled).to be_falsy - expect(project_2.reload.shared_runners_enabled).to be_falsy + it 'enables shared Runners only for itself' do + described_class.new(group, user, params).execute + + expect(group.reload.shared_runners_disabled).to be_falsy + expect(sub_group.reload.shared_runners_disabled).to be_truthy + expect(project.reload.shared_runners_enabled).to be_falsy + end end end - context 'enable allow_descendants_override_shared_runners on group that its ascentors have shared runners disabled but allows to override' do - let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners) } - 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) } - - let(:params) { { allow_descendants_override_shared_runners: '1' } } + context 'disable shared Runners' do + let_it_be(:group) { create(:group) } + let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:project_2) { create(:project, group: sub_group) } - before do - group.add_owner(user) - end + let(:params) { { shared_runners_disabled: '1' } } - it 'enables allow descendants to override' do - described_class.new(sub_group, user, params).execute + it 'disables shared Runners for all descendant groups and projects' do + described_class.new(group, user, params).execute - expect(group.reload.allow_descendants_override_shared_runners).to be_truthy - expect(sub_group.reload.allow_descendants_override_shared_runners).to be_truthy expect(group.reload.shared_runners_disabled).to be_truthy expect(sub_group.reload.shared_runners_disabled).to be_truthy + expect(sub_group.allow_descendants_override_shared_runners).to be_falsy expect(project.reload.shared_runners_enabled).to be_falsy + expect(project_2.reload.shared_runners_enabled).to be_falsy end end - end - context 'disallow descendants to override' do - let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners ) } - let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: group) } - let_it_be(:sub_group_2) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: sub_group_1) } - let_it_be(:project) { create(:project, shared_runners_enabled: true, group: group) } - let_it_be(:project_2) { create(:project, shared_runners_enabled: true, group: sub_group_2) } + context 'allow descendants to override' do + let(:params) { { allow_descendants_override_shared_runners: '1' } } - let(:params) { { allow_descendants_override_shared_runners: '0' } } + 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) } - before do - group.add_owner(user) - end + it 'enables allow descendants to override only for itself' do + described_class.new(group, user, params).execute - it 'disables allow project to override for all descendants and disables projects' do - described_class.new(group, user, params).execute - - expect(group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy - expect(sub_group_2.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.reload.shared_runners_disabled).to be_truthy - expect(sub_group_1.reload.shared_runners_disabled).to be_truthy - expect(sub_group_2.reload.shared_runners_disabled).to be_truthy - expect(project.reload.shared_runners_enabled).to be_falsy - expect(project_2.reload.shared_runners_enabled).to be_falsy - end - end - - context 'allow descendants to override when parent does not allow' do - context 'when allow_descendants_override_shared_runners is false' do - let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false ) } - let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false, parent: group) } + expect(group.reload.allow_descendants_override_shared_runners).to be_truthy + expect(sub_group.reload.allow_descendants_override_shared_runners).to be_falsy + expect(group.shared_runners_disabled).to be_truthy + expect(sub_group.shared_runners_disabled).to be_truthy + expect(project.reload.shared_runners_enabled).to be_falsy + end + end - let(:params) { { allow_descendants_override_shared_runners: '1' } } + context 'group that its ancestors have shared Runners disabled but allows to override' do + let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners) } + 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) } - subject { described_class.new(sub_group_1, user, params).execute } + it 'enables allow descendants to override' do + described_class.new(sub_group, user, params).execute - before do - group.add_owner(user) + expect(group.reload.allow_descendants_override_shared_runners).to be_truthy + expect(sub_group.reload.allow_descendants_override_shared_runners).to be_truthy + expect(group.reload.shared_runners_disabled).to be_truthy + expect(sub_group.reload.shared_runners_disabled).to be_truthy + expect(project.reload.shared_runners_enabled).to be_falsy + end end - it 'raises error and does not allow descendants to override' do - expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError) + context 'when parent does not allow' do + context 'when allow_descendants_override_shared_runners is false' do + let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false ) } + let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false, parent: group) } - expect(group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.reload.shared_runners_disabled).to be_truthy - expect(sub_group_1.reload.shared_runners_disabled).to be_truthy - end - end + subject { described_class.new(sub_group_1, user, params).execute } - context 'when allow_descendants_override_shared_runners is nil' do - let_it_be(:group) { create(:group, :shared_runners_disabled ) } - let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, parent: group) } + it 'raises error and does not allow descendants to override' do + expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners not allowed') - let(:params) { { allow_descendants_override_shared_runners: '1' } } + expect(group.reload.allow_descendants_override_shared_runners).to be_falsy + expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy + expect(group.shared_runners_disabled).to be_truthy + expect(sub_group_1.shared_runners_disabled).to be_truthy + end + end - subject { described_class.new(sub_group_1, user, params).execute } + context 'when allow_descendants_override_shared_runners is nil' do + let_it_be(:group) { create(:group, :shared_runners_disabled ) } + let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, parent: group) } - before do - group.add_owner(user) - end + subject { described_class.new(sub_group_1, user, params).execute } - it 'raises error and does not allow descendants to override' do - expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError) + it 'raises error and does not allow descendants to override' do + expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners not allowed') - expect(group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.reload.shared_runners_disabled).to be_truthy - expect(sub_group_1.reload.shared_runners_disabled).to be_truthy + expect(group.reload.allow_descendants_override_shared_runners).to be_falsy + expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy + expect(group.reload.shared_runners_disabled).to be_truthy + expect(sub_group_1.reload.shared_runners_disabled).to be_truthy + end + end end end - end - context 'non admin user' do - let_it_be(:group) { create(:group) } - let_it_be(:sub_group_1) { create(:group, parent: group) } - let_it_be(:sub_group_2) { create(:group, parent: group) } - let_it_be(:sub_group_3) { create(:group, parent: sub_group_1) } - let_it_be(:sub_group_4) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: sub_group_3) } - let_it_be(:project) { create(:project, group: group) } - let_it_be(:project_2) { create(:project, group: group) } - let_it_be(:project_3) { create(:project, group: sub_group_1) } - let_it_be(:project_4) { create(:project, group: sub_group_2) } - let_it_be(:project_5) { create(:project, group: sub_group_3) } - let_it_be(:project_6) { create(:project, group: sub_group_4) } + context 'disallow descendants to override' do + let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners ) } + let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: group) } + let_it_be(:project) { create(:project, shared_runners_enabled: true, group: sub_group) } - subject { described_class.new(sub_group_1, user, params).execute } - - let(:params) { { shared_runners_disabled: '1' } } - - before do - group.add_maintainer(user) - end + let(:params) { { allow_descendants_override_shared_runners: '0' } } - it 'raises error and does not change any config' do - expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError) + it 'disables allow project to override for descendants and disables project shared Runners' do + described_class.new(group, user, params).execute - expect(group.reload.shared_runners_disabled).to be_falsy - expect(sub_group_1.reload.shared_runners_disabled).to be_falsy - expect(sub_group_2.reload.shared_runners_disabled).to be_falsy - expect(sub_group_3.reload.shared_runners_disabled).to be_falsy - expect(sub_group_4.reload.shared_runners_disabled).to be_truthy - expect(sub_group_4.reload.allow_descendants_override_shared_runners).to be_truthy - expect(project.reload.shared_runners_enabled).to be_truthy - expect(project_2.reload.shared_runners_enabled).to be_truthy - expect(project_3.reload.shared_runners_enabled).to be_truthy - expect(project_4.reload.shared_runners_enabled).to be_truthy - expect(project_5.reload.shared_runners_enabled).to be_truthy - expect(project_6.reload.shared_runners_enabled).to be_truthy + expect(group.reload.allow_descendants_override_shared_runners).to be_falsy + expect(sub_group.reload.allow_descendants_override_shared_runners).to be_falsy + expect(group.shared_runners_disabled).to be_truthy + expect(sub_group.shared_runners_disabled).to be_truthy + expect(project.reload.shared_runners_enabled).to be_falsy + end end end end -- GitLab From 0d1cbaaec1f32e5ce0ee2198a3cc0752a7cd9e74 Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Tue, 9 Jun 2020 17:34:13 -0300 Subject: [PATCH 06/16] Change column from disabled to enabled and necessary changes --- app/models/group.rb | 8 ++- app/models/namespace.rb | 2 +- app/services/groups/shared_runners_service.rb | 16 ++--- ...ers_enabled_and_override_to_namespaces.rb} | 6 +- db/structure.sql | 2 +- ee/app/models/ee/namespace.rb | 4 +- .../_shared_runner_status.html.haml | 2 +- .../namespaces/pipelines_quota/_list.haml | 4 +- ee/spec/models/ee/namespace_spec.rb | 8 +-- spec/factories/groups.rb | 2 +- spec/models/group_spec.rb | 40 +++++------ .../groups/shared_runners_service_spec.rb | 68 +++++++------------ 12 files changed, 72 insertions(+), 90 deletions(-) rename db/migrate/{20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb => 20200424102023_add_shared_runners_enabled_and_override_to_namespaces.rb} (61%) diff --git a/app/models/group.rb b/app/models/group.rb index 6edb0a00a08011..6696690f929816 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -506,13 +506,17 @@ def preload_shared_group_links preloader.preload(self, shared_with_group_links: [shared_with_group: :route]) end - def parent_group_disallows_shared_runners? + def shared_runners_disabled? + !shared_runners_enabled? + end + + def parent_disallows_shared_runners? return false unless has_parent? parent.shared_runners_disabled? && !parent.allow_descendants_override_shared_runners? end - def parent_group_disabled_shared_runners? + def parent_disabled_shared_runners? return false unless has_parent? parent.shared_runners_disabled? diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 6176e8db9207b5..bc23b7d6af0354 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -211,7 +211,7 @@ def lfs_enabled? Gitlab.config.lfs.enabled end - def shared_runners_enabled? + def any_project_with_shared_runners_enabled? projects.with_shared_runners.any? end diff --git a/app/services/groups/shared_runners_service.rb b/app/services/groups/shared_runners_service.rb index ba75582a3b29a5..eb9ecba9bc3041 100644 --- a/app/services/groups/shared_runners_service.rb +++ b/app/services/groups/shared_runners_service.rb @@ -14,14 +14,14 @@ def execute private def enable_or_disable_shared_runners! - return unless params[:shared_runners_disabled].present? + return unless params[:shared_runners_enabled].present? - if params[:shared_runners_disabled] == '1' - disable_shared_runners! - else - raise SharedRunnersServiceError, 'Group level shared Runners disabled' if group.parent_group_disabled_shared_runners? + if params[:shared_runners_enabled] == '1' + raise SharedRunnersServiceError, 'Group level shared Runners disabled' if group.parent_disabled_shared_runners? - group.update_column(:shared_runners_disabled, false) + group.update_column(:shared_runners_enabled, true) + else + disable_shared_runners! end end @@ -29,7 +29,7 @@ def disable_shared_runners! group_ids = group.self_and_descendants return if group_ids.empty? - Group.by_id(group_ids).update_all(shared_runners_disabled: true, allow_descendants_override_shared_runners: false) + Group.by_id(group_ids).update_all(shared_runners_enabled: false, allow_descendants_override_shared_runners: false) group.all_projects.update_all(shared_runners_enabled: false) end @@ -38,7 +38,7 @@ def allow_or_disallow_descendants_override_shared_runners! return unless params[:allow_descendants_override_shared_runners].present? if params[:allow_descendants_override_shared_runners] == '1' - raise SharedRunnersServiceError, 'Group level shared Runners not allowed' if group.parent_group_disallows_shared_runners? + raise SharedRunnersServiceError, 'Group level shared Runners not allowed' if group.parent_disallows_shared_runners? group.update_column(:allow_descendants_override_shared_runners, true) else diff --git a/db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb b/db/migrate/20200424102023_add_shared_runners_enabled_and_override_to_namespaces.rb similarity index 61% rename from db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb rename to db/migrate/20200424102023_add_shared_runners_enabled_and_override_to_namespaces.rb index cd25a7a5778882..c024b087fb2d23 100644 --- a/db/migrate/20200424102023_add_shared_runners_disabled_and_override_to_namespaces.rb +++ b/db/migrate/20200424102023_add_shared_runners_enabled_and_override_to_namespaces.rb @@ -1,20 +1,20 @@ # frozen_string_literal: true -class AddSharedRunnersDisabledAndOverrideToNamespaces < ActiveRecord::Migration[6.0] +class AddSharedRunnersEnabledAndOverrideToNamespaces < ActiveRecord::Migration[6.0] include Gitlab::Database::MigrationHelpers DOWNTIME = false def up with_lock_retries do - add_column :namespaces, :shared_runners_disabled, :boolean, default: false, null: false + add_column :namespaces, :shared_runners_enabled, :boolean, default: true, null: false add_column :namespaces, :allow_descendants_override_shared_runners, :boolean, default: false, null: false end end def down with_lock_retries do - remove_column :namespaces, :shared_runners_disabled, :boolean + remove_column :namespaces, :shared_runners_enabled, :boolean remove_column :namespaces, :allow_descendants_override_shared_runners, :boolean end end diff --git a/db/structure.sql b/db/structure.sql index 605d96d436bab5..8ae1b22cd1c064 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13087,7 +13087,7 @@ CREATE TABLE public.namespaces ( unlock_membership_to_ldap boolean, max_personal_access_token_lifetime integer, push_rule_id bigint, - shared_runners_disabled boolean DEFAULT false NOT NULL, + shared_runners_enabled boolean DEFAULT true NOT NULL, allow_descendants_override_shared_runners boolean DEFAULT false NOT NULL ); diff --git a/ee/app/models/ee/namespace.rb b/ee/app/models/ee/namespace.rb index e86eaa8a6a9fba..edc28858efe618 100644 --- a/ee/app/models/ee/namespace.rb +++ b/ee/app/models/ee/namespace.rb @@ -212,7 +212,7 @@ def actual_shared_runners_minutes_limit(include_extra: true) def shared_runners_minutes_limit_enabled? shared_runner_minutes_supported? && - shared_runners_enabled? && + any_project_with_shared_runners_enabled? && actual_shared_runners_minutes_limit.nonzero? end @@ -238,7 +238,7 @@ def extra_shared_runners_minutes_used? extra_shared_runners_minutes.to_i >= extra_shared_runners_minutes_limit end - def shared_runners_enabled? + def any_project_with_shared_runners_enabled? all_projects.with_shared_runners.any? end diff --git a/ee/app/views/namespaces/_shared_runner_status.html.haml b/ee/app/views/namespaces/_shared_runner_status.html.haml index ab039197bdb691..125356e465c43e 100644 --- a/ee/app/views/namespaces/_shared_runner_status.html.haml +++ b/ee/app/views/namespaces/_shared_runner_status.html.haml @@ -1,7 +1,7 @@ - namespace = local_assigns.fetch(:namespace) - minutes_quota = namespace.ci_minutes_quota -- if namespace.shared_runners_enabled? +- if namespace.any_project_with_shared_runners_enabled? %li %span.light Pipeline minutes quota: %strong diff --git a/ee/app/views/namespaces/pipelines_quota/_list.haml b/ee/app/views/namespaces/pipelines_quota/_list.haml index 64592bd514c449..20f1643c462476 100644 --- a/ee/app/views/namespaces/pipelines_quota/_list.haml +++ b/ee/app/views/namespaces/pipelines_quota/_list.haml @@ -29,7 +29,7 @@ .col-sm-6.right - if namespace.shared_runners_minutes_limit_enabled? #{minutes_quota.monthly_percent_used}% used - - elsif !namespace.shared_runners_enabled? + - elsif !namespace.any_project_with_shared_runners_enabled? 0% used - else = s_('UsageQuota|Unlimited') @@ -47,7 +47,7 @@ = _('Minutes') %tbody - - if !namespace.shared_runners_enabled? + - if !namespace.any_project_with_shared_runners_enabled? %tr %td{ colspan: 2 } .nothing-here-block diff --git a/ee/spec/models/ee/namespace_spec.rb b/ee/spec/models/ee/namespace_spec.rb index 6819a6d4632e9b..7ffa5d29f4efcd 100644 --- a/ee/spec/models/ee/namespace_spec.rb +++ b/ee/spec/models/ee/namespace_spec.rb @@ -429,8 +429,8 @@ end end - describe '#shared_runners_enabled?' do - subject { namespace.shared_runners_enabled? } + describe '#any_project_with_shared_runners_enabled?' do + subject { namespace.any_project_with_shared_runners_enabled? } context 'without projects' do it { is_expected.to be_falsey } @@ -557,8 +557,8 @@ end end - describe '#shared_runners_enabled?' do - subject { namespace.shared_runners_enabled? } + describe '#any_project_with_shared_runners_enabled?' do + subject { namespace.any_project_with_shared_runners_enabled? } context 'subgroup with shared runners enabled project' do let(:subgroup) { create(:group, parent: namespace) } diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb index a7e1038d20269b..eafee39911788d 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -53,7 +53,7 @@ end trait :shared_runners_disabled do - shared_runners_disabled { true } + shared_runners_enabled { false } end trait :allow_descendants_override_shared_runners do diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 0fce6004706694..3005d8ce01c337 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1314,23 +1314,23 @@ def setup_group_members(group) end end - describe '#parent_group_disallows_shared_runners?' do + describe '#parent_disallows_shared_runners?' do context 'when parent group is present' do using RSpec::Parameterized::TableSyntax - where(:shared_runners_disabled, :allow_descendants_override, :shared_runners_disallowed?) do - false | false | false - false | true | false - true | false | true + where(:shared_runners_enabled, :allow_descendants_override, :shared_runners_disallowed?) do + true | false | false true | true | false + false | false | true + false | true | false end with_them do - let!(:parent_group) { create(:group, shared_runners_disabled: shared_runners_disabled, allow_descendants_override_shared_runners: allow_descendants_override) } + let!(:parent_group) { create(:group, shared_runners_enabled: shared_runners_enabled, allow_descendants_override_shared_runners: allow_descendants_override) } let!(:group) { create(:group, parent: parent_group) } it 'returns the expected result' do - expect(group.parent_group_disallows_shared_runners?).to eq(shared_runners_disallowed?) + expect(group.parent_disallows_shared_runners?).to eq(shared_runners_disallowed?) end end end @@ -1339,38 +1339,34 @@ def setup_group_members(group) let!(:group) { create(:group) } it 'returns false' do - expect(group.parent_group_disallows_shared_runners?).to be_falsy + expect(group.parent_disallows_shared_runners?).to be_falsy end end end - describe '#parent_group_disabled_shared_runners?' do + describe '#parent_disabled_shared_runners?' do + subject { group.parent_disabled_shared_runners? } + context 'when parent group is present' do - context 'When shared_runners_disabled is true' do - let!(:parent_group) { create(:group, shared_runners_disabled: true) } + context 'When shared Runners are disabled' do + let!(:parent_group) { create(:group, :shared_runners_disabled) } let!(:group) { create(:group, parent: parent_group) } - it 'returns true' do - expect(group.parent_group_disabled_shared_runners?).to be_truthy - end + it { is_expected.to be_truthy } end - context 'When shared_runners_disabled is false' do - let!(:parent_group) { create(:group, shared_runners_disabled: false) } + context 'When shared Runners are enabled' do + let!(:parent_group) { create(:group) } let!(:group) { create(:group, parent: parent_group) } - it 'returns false' do - expect(group.parent_group_disabled_shared_runners?).to be_falsy - end + it { is_expected.to be_falsy } end end context 'when parent group is missing' do let!(:group) { create(:group) } - it 'returns false' do - expect(group.parent_group_disabled_shared_runners?).to be_falsy - end + it { is_expected.to be_falsy } end end end diff --git a/spec/services/groups/shared_runners_service_spec.rb b/spec/services/groups/shared_runners_service_spec.rb index 05f9105ac8f87f..94aa0119ca4462 100644 --- a/spec/services/groups/shared_runners_service_spec.rb +++ b/spec/services/groups/shared_runners_service_spec.rb @@ -12,7 +12,7 @@ subject { described_class.new(group, user, params).execute } - let(:params) { { shared_runners_disabled: '1' } } + let(:params) { { shared_runners_enabled: '0' } } before do group.add_maintainer(user) @@ -21,7 +21,7 @@ it 'raises error and does not change any config' do expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError) - expect(group.reload.shared_runners_disabled).to be_falsy + expect(group.reload.shared_runners_enabled).to be_truthy expect(project.reload.shared_runners_enabled).to be_truthy end end @@ -32,7 +32,7 @@ end context 'enable shared Runners' do - let(:params) { { shared_runners_disabled: '0' } } + let(:params) { { shared_runners_enabled: '1' } } context 'group that its ancestors have shared runners disabled' do let_it_be(:group) { create(:group, :shared_runners_disabled) } @@ -44,8 +44,8 @@ it 'raises error and does not enable shared Runners' do expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners disabled') - expect(group.reload.shared_runners_disabled).to be_truthy - expect(sub_group.reload.shared_runners_disabled).to be_truthy + expect(group.reload.shared_runners_enabled).to be_falsy + expect(sub_group.reload.shared_runners_enabled).to be_falsy expect(project.reload.shared_runners_enabled).to be_falsy end end @@ -58,8 +58,8 @@ it 'enables shared Runners only for itself' do described_class.new(group, user, params).execute - expect(group.reload.shared_runners_disabled).to be_falsy - expect(sub_group.reload.shared_runners_disabled).to be_truthy + expect(group.reload.shared_runners_enabled).to be_truthy + expect(sub_group.reload.shared_runners_enabled).to be_falsy expect(project.reload.shared_runners_enabled).to be_falsy end end @@ -71,13 +71,13 @@ let_it_be(:project) { create(:project, group: group) } let_it_be(:project_2) { create(:project, group: sub_group) } - let(:params) { { shared_runners_disabled: '1' } } + let(:params) { { shared_runners_enabled: '0' } } it 'disables shared Runners for all descendant groups and projects' do described_class.new(group, user, params).execute - expect(group.reload.shared_runners_disabled).to be_truthy - expect(sub_group.reload.shared_runners_disabled).to be_truthy + expect(group.reload.shared_runners_enabled).to be_falsy + expect(sub_group.reload.shared_runners_enabled).to be_falsy expect(sub_group.allow_descendants_override_shared_runners).to be_falsy expect(project.reload.shared_runners_enabled).to be_falsy expect(project_2.reload.shared_runners_enabled).to be_falsy @@ -97,8 +97,8 @@ expect(group.reload.allow_descendants_override_shared_runners).to be_truthy expect(sub_group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.shared_runners_disabled).to be_truthy - expect(sub_group.shared_runners_disabled).to be_truthy + expect(group.shared_runners_enabled).to be_falsy + expect(sub_group.shared_runners_enabled).to be_falsy expect(project.reload.shared_runners_enabled).to be_falsy end end @@ -113,43 +113,25 @@ expect(group.reload.allow_descendants_override_shared_runners).to be_truthy expect(sub_group.reload.allow_descendants_override_shared_runners).to be_truthy - expect(group.reload.shared_runners_disabled).to be_truthy - expect(sub_group.reload.shared_runners_disabled).to be_truthy + expect(group.reload.shared_runners_enabled).to be_falsy + expect(sub_group.reload.shared_runners_enabled).to be_falsy expect(project.reload.shared_runners_enabled).to be_falsy end end context 'when parent does not allow' do - context 'when allow_descendants_override_shared_runners is false' do - let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false ) } - let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false, parent: group) } + let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false ) } + let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false, parent: group) } - subject { described_class.new(sub_group_1, user, params).execute } + subject { described_class.new(sub_group_1, user, params).execute } - it 'raises error and does not allow descendants to override' do - expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners not allowed') + it 'raises error and does not allow descendants to override' do + expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners not allowed') - expect(group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.shared_runners_disabled).to be_truthy - expect(sub_group_1.shared_runners_disabled).to be_truthy - end - end - - context 'when allow_descendants_override_shared_runners is nil' do - let_it_be(:group) { create(:group, :shared_runners_disabled ) } - let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, parent: group) } - - subject { described_class.new(sub_group_1, user, params).execute } - - it 'raises error and does not allow descendants to override' do - expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners not allowed') - - expect(group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.reload.shared_runners_disabled).to be_truthy - expect(sub_group_1.reload.shared_runners_disabled).to be_truthy - end + expect(group.reload.allow_descendants_override_shared_runners).to be_falsy + expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy + expect(group.shared_runners_enabled).to be_falsy + expect(sub_group_1.shared_runners_enabled).to be_falsy end end end @@ -166,8 +148,8 @@ expect(group.reload.allow_descendants_override_shared_runners).to be_falsy expect(sub_group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.shared_runners_disabled).to be_truthy - expect(sub_group.shared_runners_disabled).to be_truthy + expect(group.shared_runners_enabled).to be_falsy + expect(sub_group.shared_runners_enabled).to be_falsy expect(project.reload.shared_runners_enabled).to be_falsy end end -- GitLab From 161d57475bdb79a6784935f510045222ae00e0af Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Fri, 12 Jun 2020 14:27:42 -0300 Subject: [PATCH 07/16] Minor changes on shared_runners_service and its test --- app/services/groups/shared_runners_service.rb | 2 +- .../groups/shared_runners_service_spec.rb | 52 +++++++++---------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/app/services/groups/shared_runners_service.rb b/app/services/groups/shared_runners_service.rb index eb9ecba9bc3041..c121536b3fdd74 100644 --- a/app/services/groups/shared_runners_service.rb +++ b/app/services/groups/shared_runners_service.rb @@ -52,7 +52,7 @@ def disallow_descendants_override_shared_runners! Group.by_id(group_ids).update_all(allow_descendants_override_shared_runners: false) - Project.for_group_and_its_subgroups(group).update_all(shared_runners_enabled: false) + group.all_projects.update_all(shared_runners_enabled: false) end end end diff --git a/spec/services/groups/shared_runners_service_spec.rb b/spec/services/groups/shared_runners_service_spec.rb index 94aa0119ca4462..667d3e5e3999a6 100644 --- a/spec/services/groups/shared_runners_service_spec.rb +++ b/spec/services/groups/shared_runners_service_spec.rb @@ -4,14 +4,16 @@ describe Groups::SharedRunnersService do let(:user) { create(:user) } + let(:group) { create(:group) } + let(:params) { {} } describe '#execute' do + subject { described_class.new(group, user, params).execute } + context 'when current_user is not the group owner' do let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, group: group) } - subject { described_class.new(group, user, params).execute } - let(:params) { { shared_runners_enabled: '0' } } before do @@ -35,28 +37,26 @@ let(:params) { { shared_runners_enabled: '1' } } context 'group that its ancestors have 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) } - - subject { described_class.new(sub_group, user, params).execute } + 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(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners disabled') + expect(parent.reload.shared_runners_enabled).to be_falsy expect(group.reload.shared_runners_enabled).to be_falsy - expect(sub_group.reload.shared_runners_enabled).to be_falsy expect(project.reload.shared_runners_enabled).to be_falsy end end - context 'top level group' do + 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 - described_class.new(group, user, params).execute + subject expect(group.reload.shared_runners_enabled).to be_truthy expect(sub_group.reload.shared_runners_enabled).to be_falsy @@ -68,13 +68,13 @@ context 'disable shared Runners' do let_it_be(:group) { create(:group) } let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: group) } - let_it_be(:project) { create(:project, group: group) } - let_it_be(:project_2) { create(:project, group: sub_group) } + let_it_be(:project) { create(:project, group: group, shared_runners_enabled: true) } + let_it_be(:project_2) { create(:project, group: sub_group, shared_runners_enabled: true) } let(:params) { { shared_runners_enabled: '0' } } it 'disables shared Runners for all descendant groups and projects' do - described_class.new(group, user, params).execute + subject expect(group.reload.shared_runners_enabled).to be_falsy expect(sub_group.reload.shared_runners_enabled).to be_falsy @@ -93,7 +93,7 @@ let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } it 'enables allow descendants to override only for itself' do - described_class.new(group, user, params).execute + subject expect(group.reload.allow_descendants_override_shared_runners).to be_truthy expect(sub_group.reload.allow_descendants_override_shared_runners).to be_falsy @@ -104,34 +104,32 @@ end context 'group that its ancestors have shared Runners disabled but allows to override' do - let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners) } - 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) } + let_it_be(:parent) { create(:group, :shared_runners_disabled, :allow_descendants_override_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 - described_class.new(sub_group, user, params).execute + subject + expect(parent.reload.allow_descendants_override_shared_runners).to be_truthy expect(group.reload.allow_descendants_override_shared_runners).to be_truthy - expect(sub_group.reload.allow_descendants_override_shared_runners).to be_truthy + expect(parent.reload.shared_runners_enabled).to be_falsy expect(group.reload.shared_runners_enabled).to be_falsy - expect(sub_group.reload.shared_runners_enabled).to be_falsy expect(project.reload.shared_runners_enabled).to be_falsy end end context 'when parent does not allow' do - let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false ) } - let_it_be(:sub_group_1) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false, parent: group) } - - subject { described_class.new(sub_group_1, user, params).execute } + let_it_be(:parent) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false ) } + let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false, parent: parent) } it 'raises error and does not allow descendants to override' do expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners not allowed') + expect(parent.reload.allow_descendants_override_shared_runners).to be_falsy expect(group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(sub_group_1.reload.allow_descendants_override_shared_runners).to be_falsy + expect(parent.shared_runners_enabled).to be_falsy expect(group.shared_runners_enabled).to be_falsy - expect(sub_group_1.shared_runners_enabled).to be_falsy end end end @@ -144,7 +142,7 @@ let(:params) { { allow_descendants_override_shared_runners: '0' } } it 'disables allow project to override for descendants and disables project shared Runners' do - described_class.new(group, user, params).execute + subject expect(group.reload.allow_descendants_override_shared_runners).to be_falsy expect(sub_group.reload.allow_descendants_override_shared_runners).to be_falsy -- GitLab From 5f27b6ab2da7621176d7eff8eda1869c98105044 Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Fri, 12 Jun 2020 14:31:44 -0300 Subject: [PATCH 08/16] Simplify group methods --- app/models/group.rb | 16 +++---- app/services/groups/shared_runners_service.rb | 4 +- spec/models/group_spec.rb | 47 +++++++++++++------ 3 files changed, 43 insertions(+), 24 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 6696690f929816..5209fc638f9e97 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -506,20 +506,20 @@ def preload_shared_group_links preloader.preload(self, shared_with_group_links: [shared_with_group: :route]) end - def shared_runners_disabled? - !shared_runners_enabled? + def shared_runners_allowed? + shared_runners_enabled? || allow_descendants_override_shared_runners? end - def parent_disallows_shared_runners? - return false unless has_parent? + def parent_allows_shared_runners? + return true unless has_parent? - parent.shared_runners_disabled? && !parent.allow_descendants_override_shared_runners? + parent.shared_runners_allowed? end - def parent_disabled_shared_runners? - return false unless has_parent? + def parent_enabled_shared_runners? + return true unless has_parent? - parent.shared_runners_disabled? + parent.shared_runners_enabled? end private diff --git a/app/services/groups/shared_runners_service.rb b/app/services/groups/shared_runners_service.rb index c121536b3fdd74..891d09f5f41c1b 100644 --- a/app/services/groups/shared_runners_service.rb +++ b/app/services/groups/shared_runners_service.rb @@ -17,7 +17,7 @@ def enable_or_disable_shared_runners! return unless params[:shared_runners_enabled].present? if params[:shared_runners_enabled] == '1' - raise SharedRunnersServiceError, 'Group level shared Runners disabled' if group.parent_disabled_shared_runners? + raise SharedRunnersServiceError, 'Group level shared Runners disabled' unless group.parent_enabled_shared_runners? group.update_column(:shared_runners_enabled, true) else @@ -38,7 +38,7 @@ def allow_or_disallow_descendants_override_shared_runners! return unless params[:allow_descendants_override_shared_runners].present? if params[:allow_descendants_override_shared_runners] == '1' - raise SharedRunnersServiceError, 'Group level shared Runners not allowed' if group.parent_disallows_shared_runners? + raise SharedRunnersServiceError, 'Group level shared Runners not allowed' unless group.parent_allows_shared_runners? group.update_column(:allow_descendants_override_shared_runners, true) else diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 3005d8ce01c337..e5eb7c461512db 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1314,15 +1314,34 @@ def setup_group_members(group) end end - describe '#parent_disallows_shared_runners?' do + 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_shared_runners: allow_descendants_override) } + + it 'returns the expected result' do + expect(group.shared_runners_allowed?).to eq(expected_shared_runners_allowed) + end + end + 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, :shared_runners_disallowed?) do - true | false | false - true | true | false - false | false | true - false | true | false + 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 @@ -1330,7 +1349,7 @@ def setup_group_members(group) let!(:group) { create(:group, parent: parent_group) } it 'returns the expected result' do - expect(group.parent_disallows_shared_runners?).to eq(shared_runners_disallowed?) + expect(group.parent_allows_shared_runners?).to eq(expected_shared_runners_allowed) end end end @@ -1338,35 +1357,35 @@ def setup_group_members(group) context 'when parent group is missing' do let!(:group) { create(:group) } - it 'returns false' do - expect(group.parent_disallows_shared_runners?).to be_falsy + it 'returns true' do + expect(group.parent_allows_shared_runners?).to be_truthy end end end - describe '#parent_disabled_shared_runners?' do - subject { group.parent_disabled_shared_runners? } + 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_truthy } + 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_falsy } + it { is_expected.to be_truthy } end end context 'when parent group is missing' do let!(:group) { create(:group) } - it { is_expected.to be_falsy } + it { is_expected.to be_truthy } end end end -- GitLab From 6eb9473d233fecfc6487ace73ed8d997b920ac83 Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Wed, 17 Jun 2020 14:08:12 -0300 Subject: [PATCH 09/16] Change test pattern to "change" and "not_change" --- .../groups/shared_runners_service_spec.rb | 90 +++++++++---------- 1 file changed, 42 insertions(+), 48 deletions(-) diff --git a/spec/services/groups/shared_runners_service_spec.rb b/spec/services/groups/shared_runners_service_spec.rb index 667d3e5e3999a6..b486b5e039f0a0 100644 --- a/spec/services/groups/shared_runners_service_spec.rb +++ b/spec/services/groups/shared_runners_service_spec.rb @@ -21,10 +21,10 @@ end it 'raises error and does not change any config' do - expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError) - - expect(group.reload.shared_runners_enabled).to be_truthy - expect(project.reload.shared_runners_enabled).to be_truthy + expect { subject } + .to raise_error(Gitlab::Access::AccessDeniedError) + .and not_change { group.reload.shared_runners_enabled } + .and not_change { project.reload.shared_runners_enabled } end end @@ -42,11 +42,11 @@ 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(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners disabled') - - expect(parent.reload.shared_runners_enabled).to be_falsy - expect(group.reload.shared_runners_enabled).to be_falsy - expect(project.reload.shared_runners_enabled).to be_falsy + expect { subject } + .to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError) + .and not_change { parent.reload.shared_runners_enabled } + .and not_change { group.reload.shared_runners_enabled } + .and not_change { project.reload.shared_runners_enabled } end end @@ -56,11 +56,10 @@ let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } it 'enables shared Runners only for itself' do - subject - - expect(group.reload.shared_runners_enabled).to be_truthy - expect(sub_group.reload.shared_runners_enabled).to be_falsy - expect(project.reload.shared_runners_enabled).to be_falsy + 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 } end end end @@ -74,13 +73,11 @@ let(:params) { { shared_runners_enabled: '0' } } it 'disables shared Runners for all descendant groups and projects' do - subject - - expect(group.reload.shared_runners_enabled).to be_falsy - expect(sub_group.reload.shared_runners_enabled).to be_falsy - expect(sub_group.allow_descendants_override_shared_runners).to be_falsy - expect(project.reload.shared_runners_enabled).to be_falsy - expect(project_2.reload.shared_runners_enabled).to be_falsy + expect { subject } + .to change { group.reload.shared_runners_enabled }.from(true).to(false) + .and change { sub_group.reload.allow_descendants_override_shared_runners }.from(true).to(false) + .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 @@ -93,13 +90,12 @@ let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } it 'enables allow descendants to override only for itself' do - subject - - expect(group.reload.allow_descendants_override_shared_runners).to be_truthy - expect(sub_group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.shared_runners_enabled).to be_falsy - expect(sub_group.shared_runners_enabled).to be_falsy - expect(project.reload.shared_runners_enabled).to be_falsy + expect { subject } + .to change { group.reload.allow_descendants_override_shared_runners }.from(false).to(true) + .and not_change { group.shared_runners_enabled } + .and not_change { sub_group.reload.allow_descendants_override_shared_runners } + .and not_change { sub_group.reload.shared_runners_enabled } + .and not_change { project.reload.shared_runners_enabled } end end @@ -109,13 +105,12 @@ let_it_be(:project) { create(:project, shared_runners_enabled: false, group: group) } it 'enables allow descendants to override' do - subject - - expect(parent.reload.allow_descendants_override_shared_runners).to be_truthy - expect(group.reload.allow_descendants_override_shared_runners).to be_truthy - expect(parent.reload.shared_runners_enabled).to be_falsy - expect(group.reload.shared_runners_enabled).to be_falsy - expect(project.reload.shared_runners_enabled).to be_falsy + expect { subject } + .to not_change { parent.reload.allow_descendants_override_shared_runners } + .and not_change { parent.reload.shared_runners_enabled } + .and change { group.reload.allow_descendants_override_shared_runners }.from(false).to(true) + .and not_change { group.reload.shared_runners_enabled } + .and not_change { project.reload.shared_runners_enabled } end end @@ -124,12 +119,12 @@ let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false, parent: parent) } it 'raises error and does not allow descendants to override' do - expect { subject }.to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners not allowed') - - expect(parent.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(parent.shared_runners_enabled).to be_falsy - expect(group.shared_runners_enabled).to be_falsy + expect { subject } + .to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners not allowed') + .and not_change { parent.reload.allow_descendants_override_shared_runners } + .and not_change { parent.reload.shared_runners_enabled } + .and not_change { group.reload.allow_descendants_override_shared_runners } + .and not_change { group.reload.shared_runners_enabled } end end end @@ -142,13 +137,12 @@ let(:params) { { allow_descendants_override_shared_runners: '0' } } it 'disables allow project to override for descendants and disables project shared Runners' do - subject - - expect(group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(sub_group.reload.allow_descendants_override_shared_runners).to be_falsy - expect(group.shared_runners_enabled).to be_falsy - expect(sub_group.shared_runners_enabled).to be_falsy - expect(project.reload.shared_runners_enabled).to be_falsy + expect { subject } + .to not_change { group.reload.shared_runners_enabled } + .and change { group.reload.allow_descendants_override_shared_runners }.from(true).to(false) + .and not_change { sub_group.reload.shared_runners_enabled } + .and change { sub_group.reload.allow_descendants_override_shared_runners }.from(true).to(false) + .and change { project.reload.shared_runners_enabled }.from(true).to(false) end end end -- GitLab From 1016044f744fc6643b2140519518f5d08f847554 Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Wed, 17 Jun 2020 14:12:28 -0300 Subject: [PATCH 10/16] Rename service to UpdateSharedRunnersService --- ...unners_service.rb => update_shared_runners_service.rb} | 8 ++++---- ...vice_spec.rb => update_shared_runners_service_spec.rb} | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) rename app/services/groups/{shared_runners_service.rb => update_shared_runners_service.rb} (80%) rename spec/services/groups/{shared_runners_service_spec.rb => update_shared_runners_service_spec.rb} (96%) diff --git a/app/services/groups/shared_runners_service.rb b/app/services/groups/update_shared_runners_service.rb similarity index 80% rename from app/services/groups/shared_runners_service.rb rename to app/services/groups/update_shared_runners_service.rb index 891d09f5f41c1b..14fc37c1d0ac88 100644 --- a/app/services/groups/shared_runners_service.rb +++ b/app/services/groups/update_shared_runners_service.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true module Groups - class SharedRunnersService < Groups::BaseService - SharedRunnersServiceError = Class.new(StandardError) + class UpdateSharedRunnersService < Groups::BaseService + UpdateSharedRunnersServiceError = Class.new(StandardError) def execute raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_group, group) @@ -17,7 +17,7 @@ def enable_or_disable_shared_runners! return unless params[:shared_runners_enabled].present? if params[:shared_runners_enabled] == '1' - raise SharedRunnersServiceError, 'Group level shared Runners disabled' unless group.parent_enabled_shared_runners? + raise UpdateSharedRunnersServiceError, 'Group level shared Runners disabled' unless group.parent_enabled_shared_runners? group.update_column(:shared_runners_enabled, true) else @@ -38,7 +38,7 @@ def allow_or_disallow_descendants_override_shared_runners! return unless params[:allow_descendants_override_shared_runners].present? if params[:allow_descendants_override_shared_runners] == '1' - raise SharedRunnersServiceError, 'Group level shared Runners not allowed' unless group.parent_allows_shared_runners? + raise UpdateSharedRunnersServiceError, 'Group level shared Runners not allowed' unless group.parent_allows_shared_runners? group.update_column(:allow_descendants_override_shared_runners, true) else diff --git a/spec/services/groups/shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb similarity index 96% rename from spec/services/groups/shared_runners_service_spec.rb rename to spec/services/groups/update_shared_runners_service_spec.rb index b486b5e039f0a0..31836739d36ace 100644 --- a/spec/services/groups/shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe Groups::SharedRunnersService do +describe Groups::UpdateSharedRunnersService do let(:user) { create(:user) } let(:group) { create(:group) } let(:params) { {} } @@ -43,7 +43,7 @@ it 'raises error and does not enable shared Runners' do expect { subject } - .to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError) + .to raise_error(Groups::UpdateSharedRunnersService::UpdateSharedRunnersServiceError) .and not_change { parent.reload.shared_runners_enabled } .and not_change { group.reload.shared_runners_enabled } .and not_change { project.reload.shared_runners_enabled } @@ -120,7 +120,7 @@ it 'raises error and does not allow descendants to override' do expect { subject } - .to raise_error(Groups::SharedRunnersService::SharedRunnersServiceError, 'Group level shared Runners not allowed') + .to raise_error(Groups::UpdateSharedRunnersService::UpdateSharedRunnersServiceError, 'Group level shared Runners not allowed') .and not_change { parent.reload.allow_descendants_override_shared_runners } .and not_change { parent.reload.shared_runners_enabled } .and not_change { group.reload.allow_descendants_override_shared_runners } -- GitLab From 59bb2324a14824ed1075171f2dd9f09b78fb5b03 Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Thu, 25 Jun 2020 16:27:14 -0300 Subject: [PATCH 11/16] Change methods according to code review --- .../groups/update_shared_runners_service.rb | 5 +- .../update_shared_runners_service_spec.rb | 71 ++++++++++++++----- 2 files changed, 58 insertions(+), 18 deletions(-) diff --git a/app/services/groups/update_shared_runners_service.rb b/app/services/groups/update_shared_runners_service.rb index 14fc37c1d0ac88..a9c0859cbf34b1 100644 --- a/app/services/groups/update_shared_runners_service.rb +++ b/app/services/groups/update_shared_runners_service.rb @@ -17,7 +17,7 @@ def enable_or_disable_shared_runners! return unless params[:shared_runners_enabled].present? if params[:shared_runners_enabled] == '1' - raise UpdateSharedRunnersServiceError, 'Group level shared Runners disabled' unless group.parent_enabled_shared_runners? + raise UpdateSharedRunnersServiceError, 'Shared Runners disabled for the parent group' unless group.parent_enabled_shared_runners? group.update_column(:shared_runners_enabled, true) else @@ -29,13 +29,14 @@ def disable_shared_runners! group_ids = group.self_and_descendants return if group_ids.empty? - Group.by_id(group_ids).update_all(shared_runners_enabled: false, allow_descendants_override_shared_runners: false) + Group.by_id(group_ids).update_all(shared_runners_enabled: false) group.all_projects.update_all(shared_runners_enabled: false) end def allow_or_disallow_descendants_override_shared_runners! return unless params[:allow_descendants_override_shared_runners].present? + raise UpdateSharedRunnersServiceError, 'Shared Runners enabled' if group.shared_runners_enabled? if params[:allow_descendants_override_shared_runners] == '1' raise UpdateSharedRunnersServiceError, 'Group level shared Runners not allowed' unless group.parent_allows_shared_runners? diff --git a/spec/services/groups/update_shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb index 31836739d36ace..e17b133268d414 100644 --- a/spec/services/groups/update_shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -43,7 +43,7 @@ it 'raises error and does not enable shared Runners' do expect { subject } - .to raise_error(Groups::UpdateSharedRunnersService::UpdateSharedRunnersServiceError) + .to raise_error(described_class::UpdateSharedRunnersServiceError, '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 } @@ -67,15 +67,20 @@ context 'disable shared Runners' do let_it_be(:group) { create(:group) } let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_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, 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' } } 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 change { sub_group.reload.allow_descendants_override_shared_runners }.from(true).to(false) + .and not_change { group.reload.allow_descendants_override_shared_runners } + .and not_change { sub_group.reload.shared_runners_enabled } + .and not_change { sub_group.reload.allow_descendants_override_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_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 @@ -92,7 +97,7 @@ it 'enables allow descendants to override only for itself' do expect { subject } .to change { group.reload.allow_descendants_override_shared_runners }.from(false).to(true) - .and not_change { group.shared_runners_enabled } + .and not_change { group.reload.shared_runners_enabled } .and not_change { sub_group.reload.allow_descendants_override_shared_runners } .and not_change { sub_group.reload.shared_runners_enabled } .and not_change { project.reload.shared_runners_enabled } @@ -120,29 +125,63 @@ it 'raises error and does not allow descendants to override' do expect { subject } - .to raise_error(Groups::UpdateSharedRunnersService::UpdateSharedRunnersServiceError, 'Group level shared Runners not allowed') + .to raise_error(described_class::UpdateSharedRunnersServiceError, 'Group level shared Runners not allowed') .and not_change { parent.reload.allow_descendants_override_shared_runners } .and not_change { parent.reload.shared_runners_enabled } .and not_change { group.reload.allow_descendants_override_shared_runners } .and not_change { group.reload.shared_runners_enabled } 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 'raises error and does not change config' do + expect { subject } + .to raise_error(described_class::UpdateSharedRunnersServiceError, 'Shared Runners enabled') + .and not_change { group.reload.allow_descendants_override_shared_runners } + .and not_change { group.reload.shared_runners_enabled } + .and not_change { sub_group.reload.allow_descendants_override_shared_runners } + .and not_change { sub_group.reload.shared_runners_enabled } + .and not_change { project.reload.shared_runners_enabled } + end + end end context 'disallow descendants to override' do - let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners ) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: group) } - let_it_be(:project) { create(:project, shared_runners_enabled: true, group: sub_group) } - let(:params) { { allow_descendants_override_shared_runners: '0' } } - 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_shared_runners }.from(true).to(false) - .and not_change { sub_group.reload.shared_runners_enabled } - .and change { sub_group.reload.allow_descendants_override_shared_runners }.from(true).to(false) - .and change { project.reload.shared_runners_enabled }.from(true).to(false) + context 'top level group' do + let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners ) } + let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_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_shared_runners }.from(true).to(false) + .and not_change { sub_group.reload.shared_runners_enabled } + .and change { sub_group.reload.allow_descendants_override_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 'raises error and does not change config' do + expect { subject } + .to raise_error(described_class::UpdateSharedRunnersServiceError, 'Shared Runners enabled') + .and not_change { group.reload.allow_descendants_override_shared_runners } + .and not_change { group.reload.shared_runners_enabled } + .and not_change { sub_group.reload.allow_descendants_override_shared_runners } + .and not_change { sub_group.reload.shared_runners_enabled } + .and not_change { project.reload.shared_runners_enabled } + end end end end -- GitLab From a66576a64d14b02d330ee72b3160eaf0ec3f2aa8 Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Mon, 29 Jun 2020 15:02:47 -0300 Subject: [PATCH 12/16] Change field name to allow_descendants_override_disabled_shared_runners --- app/models/group.rb | 2 +- .../groups/update_shared_runners_service.rb | 16 +++---- ...ners_enabled_and_override_to_namespaces.rb | 4 +- db/structure.sql | 2 +- spec/factories/groups.rb | 4 +- spec/models/group_spec.rb | 4 +- .../update_shared_runners_service_spec.rb | 46 +++++++++---------- 7 files changed, 39 insertions(+), 39 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 5209fc638f9e97..8ec5220908f250 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -507,7 +507,7 @@ def preload_shared_group_links end def shared_runners_allowed? - shared_runners_enabled? || allow_descendants_override_shared_runners? + shared_runners_enabled? || allow_descendants_override_disabled_shared_runners? end def parent_allows_shared_runners? diff --git a/app/services/groups/update_shared_runners_service.rb b/app/services/groups/update_shared_runners_service.rb index a9c0859cbf34b1..eb30312e40719b 100644 --- a/app/services/groups/update_shared_runners_service.rb +++ b/app/services/groups/update_shared_runners_service.rb @@ -8,7 +8,7 @@ def execute raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_group, group) enable_or_disable_shared_runners! - allow_or_disallow_descendants_override_shared_runners! + allow_or_disallow_descendants_override_disabled_shared_runners! end private @@ -34,24 +34,24 @@ def disable_shared_runners! group.all_projects.update_all(shared_runners_enabled: false) end - def allow_or_disallow_descendants_override_shared_runners! - return unless params[:allow_descendants_override_shared_runners].present? + def allow_or_disallow_descendants_override_disabled_shared_runners! + return unless params[:allow_descendants_override_disabled_shared_runners].present? raise UpdateSharedRunnersServiceError, 'Shared Runners enabled' if group.shared_runners_enabled? - if params[:allow_descendants_override_shared_runners] == '1' + if params[:allow_descendants_override_disabled_shared_runners] == '1' raise UpdateSharedRunnersServiceError, 'Group level shared Runners not allowed' unless group.parent_allows_shared_runners? - group.update_column(:allow_descendants_override_shared_runners, true) + group.update_column(:allow_descendants_override_disabled_shared_runners, true) else - disallow_descendants_override_shared_runners! + disallow_descendants_override_disabled_shared_runners! end end - def disallow_descendants_override_shared_runners! + def disallow_descendants_override_disabled_shared_runners! group_ids = group.self_and_descendants return if group_ids.empty? - Group.by_id(group_ids).update_all(allow_descendants_override_shared_runners: false) + Group.by_id(group_ids).update_all(allow_descendants_override_disabled_shared_runners: false) group.all_projects.update_all(shared_runners_enabled: false) end diff --git a/db/migrate/20200424102023_add_shared_runners_enabled_and_override_to_namespaces.rb b/db/migrate/20200424102023_add_shared_runners_enabled_and_override_to_namespaces.rb index c024b087fb2d23..2555a50be44fe1 100644 --- a/db/migrate/20200424102023_add_shared_runners_enabled_and_override_to_namespaces.rb +++ b/db/migrate/20200424102023_add_shared_runners_enabled_and_override_to_namespaces.rb @@ -8,14 +8,14 @@ class AddSharedRunnersEnabledAndOverrideToNamespaces < ActiveRecord::Migration[6 def up with_lock_retries do add_column :namespaces, :shared_runners_enabled, :boolean, default: true, null: false - add_column :namespaces, :allow_descendants_override_shared_runners, :boolean, default: false, null: false + add_column :namespaces, :allow_descendants_override_disabled_shared_runners, :boolean, default: false, null: false end end def down with_lock_retries do remove_column :namespaces, :shared_runners_enabled, :boolean - remove_column :namespaces, :allow_descendants_override_shared_runners, :boolean + remove_column :namespaces, :allow_descendants_override_disabled_shared_runners, :boolean end end end diff --git a/db/structure.sql b/db/structure.sql index 8ae1b22cd1c064..eb628da59f1f1d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13088,7 +13088,7 @@ CREATE TABLE public.namespaces ( max_personal_access_token_lifetime integer, push_rule_id bigint, shared_runners_enabled boolean DEFAULT true NOT NULL, - allow_descendants_override_shared_runners boolean DEFAULT false NOT NULL + allow_descendants_override_disabled_shared_runners boolean DEFAULT false NOT NULL ); CREATE SEQUENCE public.namespaces_id_seq diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb index eafee39911788d..60d427dde00879 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -56,8 +56,8 @@ shared_runners_enabled { false } end - trait :allow_descendants_override_shared_runners do - allow_descendants_override_shared_runners { true } + 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 e5eb7c461512db..6c217a490e374d 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1325,7 +1325,7 @@ def setup_group_members(group) end with_them do - let!(:group) { create(:group, shared_runners_enabled: shared_runners_enabled, allow_descendants_override_shared_runners: allow_descendants_override) } + 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) @@ -1345,7 +1345,7 @@ def setup_group_members(group) end with_them do - let!(:parent_group) { create(:group, shared_runners_enabled: shared_runners_enabled, allow_descendants_override_shared_runners: allow_descendants_override) } + 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 diff --git a/spec/services/groups/update_shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb index e17b133268d414..8a642d9ac43463 100644 --- a/spec/services/groups/update_shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -66,7 +66,7 @@ context 'disable shared Runners' do let_it_be(:group) { create(:group) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: 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) } @@ -76,18 +76,18 @@ 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_shared_runners } + .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_shared_runners } + .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_shared_runners } + .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 'allow descendants to override' do - let(:params) { { allow_descendants_override_shared_runners: '1' } } + let(:params) { { allow_descendants_override_disabled_shared_runners: '1' } } context 'top level group' do let_it_be(:group) { create(:group, :shared_runners_disabled) } @@ -96,39 +96,39 @@ it 'enables allow descendants to override only for itself' do expect { subject } - .to change { group.reload.allow_descendants_override_shared_runners }.from(false).to(true) + .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_shared_runners } + .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 '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_shared_runners) } + 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_shared_runners } + .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_shared_runners }.from(false).to(true) + .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 } end end context 'when parent does not allow' do - let_it_be(:parent) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false ) } - let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_shared_runners: false, parent: parent) } + 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 allow descendants to override' do expect { subject } .to raise_error(described_class::UpdateSharedRunnersServiceError, 'Group level shared Runners not allowed') - .and not_change { parent.reload.allow_descendants_override_shared_runners } + .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_shared_runners } + .and not_change { group.reload.allow_descendants_override_disabled_shared_runners } .and not_change { group.reload.shared_runners_enabled } end end @@ -141,9 +141,9 @@ it 'raises error and does not change config' do expect { subject } .to raise_error(described_class::UpdateSharedRunnersServiceError, 'Shared Runners enabled') - .and not_change { group.reload.allow_descendants_override_shared_runners } + .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_shared_runners } + .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 @@ -151,19 +151,19 @@ end context 'disallow descendants to override' do - let(:params) { { allow_descendants_override_shared_runners: '0' } } + let(:params) { { allow_descendants_override_disabled_shared_runners: '0' } } context 'top level group' do - let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners ) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_shared_runners, parent: group) } + 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_shared_runners }.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_shared_runners }.from(true).to(false) + .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 @@ -176,9 +176,9 @@ it 'raises error and does not change config' do expect { subject } .to raise_error(described_class::UpdateSharedRunnersServiceError, 'Shared Runners enabled') - .and not_change { group.reload.allow_descendants_override_shared_runners } + .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_shared_runners } + .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 -- GitLab From 2d3f3d8d982ce18002962edf31177d977e817d3e Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Mon, 29 Jun 2020 17:17:36 -0300 Subject: [PATCH 13/16] Move group update methods to group model --- app/models/group.rb | 35 ++++ .../groups/update_shared_runners_service.rb | 53 +++--- spec/models/group_spec.rb | 152 ++++++++++++++++++ .../update_shared_runners_service_spec.rb | 109 +++++++++++-- 4 files changed, 302 insertions(+), 47 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 8ec5220908f250..c38ddbdf6fbcb3 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -18,6 +18,8 @@ class Group < Namespace ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10 + UpdateSharedRunnersError = Class.new(StandardError) + has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent alias_method :members, :group_members has_many :users, through: :group_members @@ -522,6 +524,39 @@ def parent_enabled_shared_runners? 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? + + 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) + end + private def update_two_factor_requirement diff --git a/app/services/groups/update_shared_runners_service.rb b/app/services/groups/update_shared_runners_service.rb index eb30312e40719b..50cbdf90c8a19f 100644 --- a/app/services/groups/update_shared_runners_service.rb +++ b/app/services/groups/update_shared_runners_service.rb @@ -2,58 +2,49 @@ module Groups class UpdateSharedRunnersService < Groups::BaseService - UpdateSharedRunnersServiceError = Class.new(StandardError) - def execute - raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_group, group) + return error('Operation not allowed', 403) unless can?(current_user, :admin_group, group) + + validate_params enable_or_disable_shared_runners! allow_or_disallow_descendants_override_disabled_shared_runners! + + success + + rescue Group::UpdateSharedRunnersError => 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' + end + end + def enable_or_disable_shared_runners! return unless params[:shared_runners_enabled].present? - if params[:shared_runners_enabled] == '1' - raise UpdateSharedRunnersServiceError, 'Shared Runners disabled for the parent group' unless group.parent_enabled_shared_runners? - - group.update_column(:shared_runners_enabled, true) + if Gitlab::Utils.to_boolean(params[:shared_runners_enabled]) + group.enable_shared_runners! else - disable_shared_runners! + group.disable_shared_runners! end end - def disable_shared_runners! - group_ids = group.self_and_descendants - return if group_ids.empty? - - Group.by_id(group_ids).update_all(shared_runners_enabled: false) - - group.all_projects.update_all(shared_runners_enabled: false) - end - def allow_or_disallow_descendants_override_disabled_shared_runners! return unless params[:allow_descendants_override_disabled_shared_runners].present? - raise UpdateSharedRunnersServiceError, 'Shared Runners enabled' if group.shared_runners_enabled? - if params[:allow_descendants_override_disabled_shared_runners] == '1' - raise UpdateSharedRunnersServiceError, 'Group level shared Runners not allowed' unless group.parent_allows_shared_runners? + # Needs to reset group because if both params are present could result in error + group.reset - group.update_column(:allow_descendants_override_disabled_shared_runners, true) + if Gitlab::Utils.to_boolean(params[:allow_descendants_override_disabled_shared_runners]) + group.allow_descendants_override_disabled_shared_runners! else - disallow_descendants_override_disabled_shared_runners! + group.disallow_descendants_override_disabled_shared_runners! end end - - def disallow_descendants_override_disabled_shared_runners! - group_ids = group.self_and_descendants - return if group_ids.empty? - - Group.by_id(group_ids).update_all(allow_descendants_override_disabled_shared_runners: false) - - group.all_projects.update_all(shared_runners_enabled: false) - end end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 6c217a490e374d..4184f2d07cc6ff 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1388,4 +1388,156 @@ def setup_group_members(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 } + 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 } + 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) } + + 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 '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 } + 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) } + + 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 } + 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 '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 } + end + end + end + + describe '#disallow_descendants_override_disabled_shared_runners!' do + subject { group.disallow_descendants_override_disabled_shared_runners! } + + 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 } + end + end + end end diff --git a/spec/services/groups/update_shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb index 8a642d9ac43463..f4717c4df2f7dc 100644 --- a/spec/services/groups/update_shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe Groups::UpdateSharedRunnersService do +RSpec.describe Groups::UpdateSharedRunnersService do let(:user) { create(:user) } let(:group) { create(:group) } let(:params) { {} } @@ -20,11 +20,13 @@ group.add_maintainer(user) end - it 'raises error and does not change any config' do + it 'results error and does not change any config' do expect { subject } - .to raise_error(Gitlab::Access::AccessDeniedError) - .and not_change { group.reload.shared_runners_enabled } + .to not_change { group.reload.shared_runners_enabled } .and not_change { project.reload.shared_runners_enabled } + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Operation not allowed') + expect(subject[:http_status]).to eq(403) end end @@ -41,12 +43,13 @@ 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 + it 'results error and does not enable shared Runners' do expect { subject } - .to raise_error(described_class::UpdateSharedRunnersServiceError, 'Shared Runners disabled for the parent group') - .and not_change { parent.reload.shared_runners_enabled } + .to not_change { parent.reload.shared_runners_enabled } .and not_change { group.reload.shared_runners_enabled } .and not_change { project.reload.shared_runners_enabled } + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Shared Runners disabled for the parent group') end end @@ -123,13 +126,14 @@ 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 allow descendants to override' do + it 'results error and does not allow descendants to override' do expect { subject } - .to raise_error(described_class::UpdateSharedRunnersServiceError, 'Group level shared Runners not allowed') - .and not_change { parent.reload.allow_descendants_override_disabled_shared_runners } + .to 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 } + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Group level shared Runners not allowed') end end @@ -138,14 +142,15 @@ 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 'raises error and does not change config' do + it 'results error and does not change config' do expect { subject } - .to raise_error(described_class::UpdateSharedRunnersServiceError, 'Shared Runners enabled') - .and not_change { group.reload.allow_descendants_override_disabled_shared_runners } + .to 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 } + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Shared Runners enabled') end end end @@ -173,14 +178,86 @@ 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 'raises error and does not change config' do + it 'results error and does not change config' do expect { subject } - .to raise_error(described_class::UpdateSharedRunnersServiceError, 'Shared Runners enabled') - .and not_change { group.reload.allow_descendants_override_disabled_shared_runners } + .to 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 } + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Shared Runners enabled') + end + end + end + + context 'both params are present' do + context 'shared_runners_enabled: 1 and allow_descendants_override_disabled_shared_runners' 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 + %w(1 0) + end + + with_them do + let(:params) { { shared_runners_enabled: '1', allow_descendants_override_disabled_shared_runners: allow_descendants_override } } + + it 'results errors 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 + 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 '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) end end end -- GitLab From 0711e096721240afc72d0bb08f0987496aa1c45f Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Tue, 30 Jun 2020 12:28:03 -0300 Subject: [PATCH 14/16] Fix support for boolean params to the service --- .../groups/update_shared_runners_service.rb | 4 +- .../update_shared_runners_service_spec.rb | 246 ++++++++++-------- 2 files changed, 137 insertions(+), 113 deletions(-) diff --git a/app/services/groups/update_shared_runners_service.rb b/app/services/groups/update_shared_runners_service.rb index 50cbdf90c8a19f..63f57104510344 100644 --- a/app/services/groups/update_shared_runners_service.rb +++ b/app/services/groups/update_shared_runners_service.rb @@ -25,7 +25,7 @@ def validate_params end def enable_or_disable_shared_runners! - return unless params[:shared_runners_enabled].present? + return if params[:shared_runners_enabled].nil? if Gitlab::Utils.to_boolean(params[:shared_runners_enabled]) group.enable_shared_runners! @@ -35,7 +35,7 @@ def enable_or_disable_shared_runners! end def allow_or_disallow_descendants_override_disabled_shared_runners! - return unless params[:allow_descendants_override_disabled_shared_runners].present? + 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 diff --git a/spec/services/groups/update_shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb index f4717c4df2f7dc..0311561a40eecf 100644 --- a/spec/services/groups/update_shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -36,33 +36,39 @@ end context 'enable shared Runners' do - let(:params) { { shared_runners_enabled: '1' } } + where(:desired_params) do + ['1', true] + end - 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) } + with_them do + let(:params) { { shared_runners_enabled: desired_params } } - it 'results error and does not enable shared Runners' do - expect { subject } - .to not_change { parent.reload.shared_runners_enabled } - .and not_change { group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Shared Runners disabled for the parent group') + 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 'results error and does not enable shared Runners' do + expect { subject } + .to not_change { parent.reload.shared_runners_enabled } + .and not_change { group.reload.shared_runners_enabled } + .and not_change { project.reload.shared_runners_enabled } + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Shared Runners disabled for the parent group') + 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) } + 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 } + 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 } + end end end end @@ -74,119 +80,137 @@ 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' } } - - 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) + where(:desired_params) do + ['0', false] end - end - context 'allow descendants to override' do - let(:params) { { allow_descendants_override_disabled_shared_runners: '1' } } - - 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) } + with_them do + let(:params) { { shared_runners_enabled: desired_params } } - it 'enables allow descendants to override only for itself' do + it 'disables shared Runners for all descendant groups and projects' 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 } + .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 { project.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 + end - 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) } + context 'allow descendants to override' do + where(:desired_params) do + ['1', true] + end - 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 } + 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) } + 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 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 - 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 'results error and does not 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 not_change { group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { group.reload.shared_runners_enabled } - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Group level shared Runners not allowed') + 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 } + 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 'results error and does not change config' do - expect { subject } - .to 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 } - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Shared Runners enabled') + it 'results error and does not 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 not_change { group.reload.allow_descendants_override_disabled_shared_runners } + .and not_change { group.reload.shared_runners_enabled } + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Group level shared Runners not allowed') + 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 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 } + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Shared Runners enabled') + end end end end context 'disallow descendants to override' do - let(:params) { { allow_descendants_override_disabled_shared_runners: '0' } } + where(:desired_params) do + ['0', false] + end - 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) } + with_them do + let(:params) { { allow_descendants_override_disabled_shared_runners: desired_params } } - 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) + 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 - 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 '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 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 } - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Shared Runners enabled') + it 'results error and does not change config' do + expect { subject } + .to 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 } + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Shared Runners enabled') + end end end end @@ -198,7 +222,7 @@ let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } where(:allow_descendants_override) do - %w(1 0) + ['1', true, '0', false] end with_them do -- GitLab From 6de355f05000f9ef1ad746b64adc1645e7ff6ed9 Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Tue, 30 Jun 2020 13:11:53 -0300 Subject: [PATCH 15/16] Simplify and mock tests for the service --- .../update_shared_runners_service_spec.rb | 134 +++++------------- 1 file changed, 37 insertions(+), 97 deletions(-) diff --git a/spec/services/groups/update_shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb index 0311561a40eecf..9fd8477a455d72 100644 --- a/spec/services/groups/update_shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -12,7 +12,6 @@ context 'when current_user is not the group owner' do let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, group: group) } let(:params) { { shared_runners_enabled: '0' } } @@ -20,10 +19,12 @@ group.add_maintainer(user) end - it 'results error and does not change any config' do - expect { subject } - .to not_change { group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } + 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(subject[:status]).to eq(:error) expect(subject[:message]).to eq('Operation not allowed') expect(subject[:http_status]).to eq(403) @@ -46,13 +47,8 @@ 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 'results error and does not enable shared Runners' do - expect { subject } - .to not_change { parent.reload.shared_runners_enabled } - .and not_change { group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } + it 'results error' do expect(subject[:status]).to eq(:error) expect(subject[:message]).to eq('Shared Runners disabled for the parent group') end @@ -60,14 +56,14 @@ 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 } + 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 end end @@ -75,10 +71,6 @@ context '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) } where(:desired_params) do ['0', false] @@ -87,16 +79,13 @@ with_them do let(:params) { { shared_runners_enabled: desired_params } } - 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) + 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(subject[:status]).to eq(:success) end end end @@ -111,31 +100,14 @@ 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) } - - 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 '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 '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!) - 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 } + expect(subject[:status]).to eq(:success) end end @@ -143,33 +115,11 @@ 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 'results error and does not 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 not_change { group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { group.reload.shared_runners_enabled } + it 'results error' do expect(subject[:status]).to eq(:error) expect(subject[:message]).to eq('Group level shared Runners not allowed') 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 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 } - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Shared Runners enabled') - end - end end end @@ -183,31 +133,21 @@ 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) + 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) } - 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 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 'results error' do expect(subject[:status]).to eq(:error) expect(subject[:message]).to eq('Shared Runners enabled') end @@ -228,7 +168,7 @@ with_them do let(:params) { { shared_runners_enabled: '1', allow_descendants_override_disabled_shared_runners: allow_descendants_override } } - it 'results errors because shared Runners are enabled' do + 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 } -- GitLab From eb871bc02ac3ceb59b2fcf235d1f9756832b2c49 Mon Sep 17 00:00:00 2001 From: Arthur de Lapertosa Lisboa Date: Tue, 30 Jun 2020 13:12:53 -0300 Subject: [PATCH 16/16] Change changelog title --- .../add-Groups-SharedRunnersService-tests-migrations.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/add-Groups-SharedRunnersService-tests-migrations.yml b/changelogs/unreleased/add-Groups-SharedRunnersService-tests-migrations.yml index 6178d696df1eb0..308225344b5cc6 100644 --- a/changelogs/unreleased/add-Groups-SharedRunnersService-tests-migrations.yml +++ b/changelogs/unreleased/add-Groups-SharedRunnersService-tests-migrations.yml @@ -1,5 +1,5 @@ --- -title: Add Groups::SharedRunnersService, related tests and migrations +title: Add setting to enable and disable shared Runners for a group and its descendants merge_request: 33411 author: Arthur de Lapertosa Lisboa type: added -- GitLab