Commit e5680f93 authored by Doug Stull's avatar Doug Stull

Change shared runner settings on import on conflict with group

- fixes a bug where it hits a validation error and shouldn't

Changelog: fixed
parent 39a234b1
......@@ -77,7 +77,7 @@ module Ci
def toggle_shared_runners_settings_data(project)
{
is_enabled: "#{project.shared_runners_enabled?}",
is_disabled_and_unoverridable: "#{project.group&.shared_runners_setting == 'disabled_and_unoverridable'}",
is_disabled_and_unoverridable: "#{project.group&.shared_runners_setting == Namespace::SR_DISABLED_AND_UNOVERRIDABLE}",
update_path: toggle_shared_runners_project_runners_path(project)
}
end
......
......@@ -707,9 +707,9 @@ class Group < Namespace
raise ArgumentError unless SHARED_RUNNERS_SETTINGS.include?(state)
case state
when 'disabled_and_unoverridable' then disable_shared_runners! # also disallows override
when 'disabled_with_override' then disable_shared_runners_and_allow_override!
when 'enabled' then enable_shared_runners! # set both to true
when SR_DISABLED_AND_UNOVERRIDABLE then disable_shared_runners! # also disallows override
when SR_DISABLED_WITH_OVERRIDE then disable_shared_runners_and_allow_override!
when SR_ENABLED then enable_shared_runners! # set both to true
end
end
......
......@@ -28,7 +28,10 @@ class Namespace < ApplicationRecord
# Android repo (15) + some extra backup.
NUMBER_OF_ANCESTORS_ALLOWED = 20
SHARED_RUNNERS_SETTINGS = %w[disabled_and_unoverridable disabled_with_override enabled].freeze
SR_DISABLED_AND_UNOVERRIDABLE = 'disabled_and_unoverridable'
SR_DISABLED_WITH_OVERRIDE = 'disabled_with_override'
SR_ENABLED = 'enabled'
SHARED_RUNNERS_SETTINGS = [SR_DISABLED_AND_UNOVERRIDABLE, SR_DISABLED_WITH_OVERRIDE, SR_ENABLED].freeze
URL_MAX_LENGTH = 255
cache_markdown_field :description, pipeline: :description
......@@ -431,7 +434,7 @@ class Namespace < ApplicationRecord
def changing_shared_runners_enabled_is_allowed
return unless new_record? || changes.has_key?(:shared_runners_enabled)
if shared_runners_enabled && has_parent? && parent.shared_runners_setting == 'disabled_and_unoverridable'
if shared_runners_enabled && has_parent? && parent.shared_runners_setting == SR_DISABLED_AND_UNOVERRIDABLE
errors.add(:shared_runners_enabled, _('cannot be enabled because parent group has shared Runners disabled'))
end
end
......@@ -443,30 +446,30 @@ class Namespace < ApplicationRecord
errors.add(:allow_descendants_override_disabled_shared_runners, _('cannot be changed if shared runners are enabled'))
end
if allow_descendants_override_disabled_shared_runners && has_parent? && parent.shared_runners_setting == 'disabled_and_unoverridable'
if allow_descendants_override_disabled_shared_runners && has_parent? && parent.shared_runners_setting == SR_DISABLED_AND_UNOVERRIDABLE
errors.add(:allow_descendants_override_disabled_shared_runners, _('cannot be enabled because parent group does not allow it'))
end
end
def shared_runners_setting
if shared_runners_enabled
'enabled'
SR_ENABLED
else
if allow_descendants_override_disabled_shared_runners
'disabled_with_override'
SR_DISABLED_WITH_OVERRIDE
else
'disabled_and_unoverridable'
SR_DISABLED_AND_UNOVERRIDABLE
end
end
end
def shared_runners_setting_higher_than?(other_setting)
if other_setting == 'enabled'
if other_setting == SR_ENABLED
false
elsif other_setting == 'disabled_with_override'
shared_runners_setting == 'enabled'
elsif other_setting == 'disabled_and_unoverridable'
shared_runners_setting == 'enabled' || shared_runners_setting == 'disabled_with_override'
elsif other_setting == SR_DISABLED_WITH_OVERRIDE
shared_runners_setting == SR_ENABLED
elsif other_setting == SR_DISABLED_AND_UNOVERRIDABLE
shared_runners_setting == SR_ENABLED || shared_runners_setting == SR_DISABLED_WITH_OVERRIDE
else
raise ArgumentError
end
......
......@@ -1309,11 +1309,21 @@ class Project < ApplicationRecord
def changing_shared_runners_enabled_is_allowed
return unless new_record? || changes.has_key?(:shared_runners_enabled)
if shared_runners_enabled && group && group.shared_runners_setting == 'disabled_and_unoverridable'
if shared_runners_setting_conflicting_with_group?
errors.add(:shared_runners_enabled, _('cannot be enabled because parent group does not allow it'))
end
end
def shared_runners_setting_conflicting_with_group?
shared_runners_enabled && group&.shared_runners_setting == Namespace::SR_DISABLED_AND_UNOVERRIDABLE
end
def reconcile_shared_runners_setting!
if shared_runners_setting_conflicting_with_group?
self.shared_runners_enabled = false
end
end
def to_param
if persisted? && errors.include?(:path)
path_was
......
......@@ -81,7 +81,7 @@ module Projects
# Apply changes to the project
update_namespace_and_visibility(@new_namespace)
update_shared_runners_settings
project.reconcile_shared_runners_setting!
project.save!
# Notifications
......@@ -239,14 +239,6 @@ module Projects
"#{new_path}#{::Gitlab::GlRepository::DESIGN.path_suffix}"
end
def update_shared_runners_settings
# If a project is being transferred to another group it means it can already
# have shared runners enabled but we need to check whether the new group allows that.
if project.group && project.group.shared_runners_setting == 'disabled_and_unoverridable'
project.shared_runners_enabled = false
end
end
def update_integrations
project.integrations.with_default_settings.delete_all
Integration.create_from_active_default_integrations(project, :project_id)
......
......@@ -37,7 +37,7 @@ module Gitlab
ActiveRecord::Base.no_touching do
update_params!
BulkInsertableAssociations.with_bulk_insert(enabled: @importable.instance_of?(::Project)) do
BulkInsertableAssociations.with_bulk_insert(enabled: project?) do
fix_ci_pipelines_not_sorted_on_legacy_project_json!
create_relations!
end
......@@ -55,6 +55,10 @@ module Gitlab
private
def project?
@importable.instance_of?(::Project)
end
# Loops through the tree of models defined in import_export.yml and
# finds them in the imported JSON so they can be instantiated and saved
# in the DB. The structure and relationships between models are guessed from
......@@ -75,7 +79,7 @@ module Gitlab
def process_relation_item!(relation_key, relation_definition, relation_index, data_hash)
relation_object = build_relation(relation_key, relation_definition, relation_index, data_hash)
return unless relation_object
return if importable_class == ::Project && group_model?(relation_object)
return if project? && group_model?(relation_object)
relation_object.assign_attributes(importable_class_sym => @importable)
......@@ -114,7 +118,8 @@ module Gitlab
excluded_keys: excluded_keys_for_relation(importable_class_sym))
@importable.assign_attributes(params)
@importable.drop_visibility_level! if importable_class == ::Project
modify_attributes
Gitlab::Timeless.timeless(@importable) do
@importable.save!
......@@ -141,6 +146,13 @@ module Gitlab
end
end
def modify_attributes
return unless project?
@importable.reconcile_shared_runners_setting!
@importable.drop_visibility_level!
end
def build_relations(relation_key, relation_definition, relation_index, data_hashes)
data_hashes
.map { |data_hash| build_relation(relation_key, relation_definition, relation_index, data_hash) }
......
......@@ -69,6 +69,20 @@ FactoryBot.define do
allow_descendants_override_disabled_shared_runners { true }
end
trait :disabled_and_unoverridable do
shared_runners_disabled
allow_descendants_override_disabled_shared_runners { false }
end
trait :disabled_with_override do
shared_runners_disabled
allow_descendants_override_disabled_shared_runners
end
trait :shared_runners_enabled do
shared_runners_enabled { true }
end
# Construct a hierarchy underneath the group.
# Each group will have `children` amount of children,
# and `depth` levels of descendants.
......
{"description":"Nisi et repellendus ut enim quo accusamus vel magnam.","import_type":"gitlab_project","creator_id":123,"visibility_level":10,"archived":false,"deploy_keys":[],"hooks":[]}
{
"description": "Nisi et repellendus ut enim quo accusamus vel magnam.",
"import_type": "gitlab_project",
"creator_id": 123,
"visibility_level": 10,
"archived": false,
"deploy_keys": [],
"hooks": [],
"shared_runners_enabled": true
}
......@@ -18,7 +18,7 @@ RSpec.describe Mutations::Groups::Update do
RSpec.shared_examples 'updating the group shared runners setting' do
it 'updates the group shared runners setting' do
expect { subject }
.to change { group.reload.shared_runners_setting }.from('enabled').to('disabled_and_unoverridable')
.to change { group.reload.shared_runners_setting }.from('enabled').to(Namespace::SR_DISABLED_AND_UNOVERRIDABLE)
end
it 'returns no errors' do
......@@ -51,7 +51,7 @@ RSpec.describe Mutations::Groups::Update do
context 'changing shared runners setting' do
let_it_be(:params) do
{ full_path: group.full_path,
shared_runners_setting: 'disabled_and_unoverridable' }
shared_runners_setting: Namespace::SR_DISABLED_AND_UNOVERRIDABLE }
end
where(:user_role, :shared_examples_name) do
......
......@@ -83,7 +83,7 @@ RSpec.describe Ci::RunnersHelper do
data = group_shared_runners_settings_data(group)
expect(data[:update_path]).to eq("/api/v4/groups/#{group.id}")
expect(data[:shared_runners_availability]).to eq('disabled_and_unoverridable')
expect(data[:shared_runners_availability]).to eq(Namespace::SR_DISABLED_AND_UNOVERRIDABLE)
expect(data[:parent_shared_runners_availability]).to eq('enabled')
end
end
......@@ -137,16 +137,15 @@ RSpec.describe Ci::RunnersHelper do
using RSpec::Parameterized::TableSyntax
where(:shared_runners_setting, :is_disabled_and_unoverridable) do
'enabled' | "false"
'disabled_with_override' | "false"
'disabled_and_unoverridable' | "true"
:shared_runners_enabled | "false"
:disabled_with_override | "false"
:disabled_and_unoverridable | "true"
end
with_them do
it 'returns the override runner status for project with group' do
group = create(:group)
project = create(:project, group: group)
allow(group).to receive(:shared_runners_setting).and_return(shared_runners_setting)
group = create(:group, shared_runners_setting)
project = create(:project, group: group, shared_runners_enabled: false)
data = helper.toggle_shared_runners_settings_data(project)
expect(data[:is_disabled_and_unoverridable]).to eq(is_disabled_and_unoverridable)
......
......@@ -30,18 +30,12 @@ RSpec.describe Gitlab::ImportExport::RelationTreeRestorer do
subject { relation_tree_restorer.restore }
shared_examples 'import project successfully' do
it 'restores project tree' do
expect(subject).to eq(true)
end
describe 'imported project' do
let(:project) { Project.find_by_path('project') }
it 'has the project attributes and relations', :aggregate_failures do
expect(subject).to eq(true)
before do
subject
end
project = Project.find_by_path('project')
it 'has the project attributes and relations' do
expect(project.description).to eq('Nisi et repellendus ut enim quo accusamus vel magnam.')
expect(project.labels.count).to eq(3)
expect(project.boards.count).to eq(1)
......@@ -86,7 +80,10 @@ RSpec.describe Gitlab::ImportExport::RelationTreeRestorer do
end
context 'when restoring a project' do
let(:importable) { create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project') }
let_it_be(:importable, reload: true) do
create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project')
end
let(:importable_name) { 'project' }
let(:importable_path) { 'project' }
let(:object_builder) { Gitlab::ImportExport::Project::ObjectBuilder }
......@@ -108,8 +105,10 @@ RSpec.describe Gitlab::ImportExport::RelationTreeRestorer do
it_behaves_like 'import project successfully'
context 'logging of relations creation' do
let(:group) { create(:group) }
let(:importable) { create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project', group: group) }
let_it_be(:group) { create(:group) }
let_it_be(:importable) do
create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project', group: group)
end
include_examples 'logging of relations creation'
end
......@@ -120,6 +119,18 @@ RSpec.describe Gitlab::ImportExport::RelationTreeRestorer do
let(:relation_reader) { Gitlab::ImportExport::Json::NdjsonReader.new(path) }
it_behaves_like 'import project successfully'
context 'when inside a group' do
let_it_be(:group) do
create(:group, :disabled_and_unoverridable)
end
before do
importable.update!(shared_runners_enabled: false, group: group)
end
it_behaves_like 'import project successfully'
end
end
context 'with invalid relations' do
......@@ -143,9 +154,10 @@ RSpec.describe Gitlab::ImportExport::RelationTreeRestorer do
end
context 'when restoring a group' do
let_it_be(:group) { create(:group) }
let_it_be(:importable) { create(:group, parent: group) }
let(:path) { 'spec/fixtures/lib/gitlab/import_export/group_exports/no_children/group.json' }
let(:group) { create(:group) }
let(:importable) { create(:group, parent: group) }
let(:importable_name) { nil }
let(:importable_path) { nil }
let(:object_builder) { Gitlab::ImportExport::Group::ObjectBuilder }
......
......@@ -2370,7 +2370,7 @@ RSpec.describe Group do
let_it_be(:project) { create(:project, group: group, shared_runners_enabled: true) }
let_it_be(:project_2) { create(:project, group: sub_group_2, shared_runners_enabled: true) }
subject { group.update_shared_runners_setting!('disabled_and_unoverridable') }
subject { group.update_shared_runners_setting!(Namespace::SR_DISABLED_AND_UNOVERRIDABLE) }
it 'disables shared Runners for all descendant groups and projects' do
expect { subject_and_reload(group, sub_group, sub_group_2, project, project_2) }
......@@ -2396,7 +2396,7 @@ RSpec.describe Group do
end
context 'disabled_with_override' do
subject { group.update_shared_runners_setting!('disabled_with_override') }
subject { group.update_shared_runners_setting!(Namespace::SR_DISABLED_WITH_OVERRIDE) }
context 'top level group' do
let_it_be(:group) { create(:group, :shared_runners_disabled) }
......
......@@ -1796,10 +1796,10 @@ RSpec.describe Namespace do
using RSpec::Parameterized::TableSyntax
where(:shared_runners_enabled, :allow_descendants_override_disabled_shared_runners, :shared_runners_setting) do
true | true | 'enabled'
true | false | 'enabled'
false | true | 'disabled_with_override'
false | false | 'disabled_and_unoverridable'
true | true | Namespace::SR_ENABLED
true | false | Namespace::SR_ENABLED
false | true | Namespace::SR_DISABLED_WITH_OVERRIDE
false | false | Namespace::SR_DISABLED_AND_UNOVERRIDABLE
end
with_them do
......@@ -1815,15 +1815,15 @@ RSpec.describe Namespace do
using RSpec::Parameterized::TableSyntax
where(:shared_runners_enabled, :allow_descendants_override_disabled_shared_runners, :other_setting, :result) do
true | true | 'enabled' | false
true | true | 'disabled_with_override' | true
true | true | 'disabled_and_unoverridable' | true
false | true | 'enabled' | false
false | true | 'disabled_with_override' | false
false | true | 'disabled_and_unoverridable' | true
false | false | 'enabled' | false
false | false | 'disabled_with_override' | false
false | false | 'disabled_and_unoverridable' | false
true | true | Namespace::SR_ENABLED | false
true | true | Namespace::SR_DISABLED_WITH_OVERRIDE | true
true | true | Namespace::SR_DISABLED_AND_UNOVERRIDABLE | true
false | true | Namespace::SR_ENABLED | false
false | true | Namespace::SR_DISABLED_WITH_OVERRIDE | false
false | true | Namespace::SR_DISABLED_AND_UNOVERRIDABLE | true
false | false | Namespace::SR_ENABLED | false
false | false | Namespace::SR_DISABLED_WITH_OVERRIDE | false
false | false | Namespace::SR_DISABLED_AND_UNOVERRIDABLE | false
end
with_them do
......
......@@ -6287,24 +6287,18 @@ RSpec.describe Project, factory_default: :keep do
describe 'validation #changing_shared_runners_enabled_is_allowed' do
where(:shared_runners_setting, :project_shared_runners_enabled, :valid_record) do
'enabled' | true | true
'enabled' | false | true
'disabled_with_override' | true | true
'disabled_with_override' | false | true
'disabled_and_unoverridable' | true | false
'disabled_and_unoverridable' | false | true
:shared_runners_enabled | true | true
:shared_runners_enabled | false | true
:disabled_with_override | true | true
:disabled_with_override | false | true
:disabled_and_unoverridable | true | false
:disabled_and_unoverridable | false | true
end
with_them do
let(:group) { create(:group) }
let(:group) { create(:group, shared_runners_setting) }
let(:project) { build(:project, namespace: group, shared_runners_enabled: project_shared_runners_enabled) }
before do
allow_next_found_instance_of(Group) do |group|
allow(group).to receive(:shared_runners_setting).and_return(shared_runners_setting)
end
end
it 'validates the configuration' do
expect(project.valid?).to eq(valid_record)
......
......@@ -362,7 +362,7 @@ RSpec.describe Groups::TransferService do
let(:new_parent_group) { create(:group, shared_runners_enabled: false, allow_descendants_override_disabled_shared_runners: true) }
it 'calls update service' do
expect(Groups::UpdateSharedRunnersService).to receive(:new).with(group, user, { shared_runners_setting: 'disabled_with_override' }).and_call_original
expect(Groups::UpdateSharedRunnersService).to receive(:new).with(group, user, { shared_runners_setting: Namespace::SR_DISABLED_WITH_OVERRIDE }).and_call_original
transfer_service.execute(new_parent_group)
end
......@@ -372,7 +372,7 @@ RSpec.describe Groups::TransferService do
let(:new_parent_group) { create(:group, shared_runners_enabled: false, allow_descendants_override_disabled_shared_runners: false) }
it 'calls update service' do
expect(Groups::UpdateSharedRunnersService).to receive(:new).with(group, user, { shared_runners_setting: 'disabled_and_unoverridable' }).and_call_original
expect(Groups::UpdateSharedRunnersService).to receive(:new).with(group, user, { shared_runners_setting: Namespace::SR_DISABLED_AND_UNOVERRIDABLE }).and_call_original
transfer_service.execute(new_parent_group)
end
......
......@@ -287,7 +287,7 @@ RSpec.describe Groups::UpdateService do
let(:group) { create(:group) }
let(:project) { create(:project, shared_runners_enabled: true, group: group) }
subject { described_class.new(group, user, shared_runners_setting: 'disabled_and_unoverridable').execute }
subject { described_class.new(group, user, shared_runners_setting: Namespace::SR_DISABLED_AND_UNOVERRIDABLE).execute }
before do
group.add_owner(user)
......
......@@ -85,10 +85,10 @@ RSpec.describe Groups::UpdateSharedRunnersService do
context 'disable shared Runners' do
let_it_be(:group) { create(:group) }
let(:params) { { shared_runners_setting: 'disabled_and_unoverridable' } }
let(:params) { { shared_runners_setting: Namespace::SR_DISABLED_AND_UNOVERRIDABLE } }
it 'receives correct method and succeeds' do
expect(group).to receive(:update_shared_runners_setting!).with('disabled_and_unoverridable')
expect(group).to receive(:update_shared_runners_setting!).with(Namespace::SR_DISABLED_AND_UNOVERRIDABLE)
expect(subject[:status]).to eq(:success)
end
......@@ -108,13 +108,13 @@ RSpec.describe Groups::UpdateSharedRunnersService do
end
context 'allow descendants to override' do
let(:params) { { shared_runners_setting: 'disabled_with_override' } }
let(:params) { { shared_runners_setting: Namespace::SR_DISABLED_WITH_OVERRIDE } }
context 'top level group' do
let_it_be(:group) { create(:group, :shared_runners_disabled) }
it 'receives correct method and succeeds' do
expect(group).to receive(:update_shared_runners_setting!).with('disabled_with_override')
expect(group).to receive(:update_shared_runners_setting!).with(Namespace::SR_DISABLED_WITH_OVERRIDE)
expect(subject[:status]).to eq(:success)
end
......
......@@ -839,25 +839,23 @@ RSpec.describe Projects::CreateService, '#execute' do
let_it_be(:user) { create :user }
context 'when parent group is present' do
let_it_be(:group) do
let_it_be(:group, reload: true) do
create(:group) do |group|
group.add_owner(user)
end
end
before do
allow_next_found_instance_of(Group) do |group|
allow(group).to receive(:shared_runners_setting).and_return(shared_runners_setting)
end
group.update_shared_runners_setting!(shared_runners_setting)
user.refresh_authorized_projects # Ensure cache is warm
end
context 'default value based on parent group setting' do
where(:shared_runners_setting, :desired_config_for_new_project, :expected_result_for_project) do
'enabled' | nil | true
'disabled_with_override' | nil | false
'disabled_and_unoverridable' | nil | false
Namespace::SR_ENABLED | nil | true
Namespace::SR_DISABLED_WITH_OVERRIDE | nil | false
Namespace::SR_DISABLED_AND_UNOVERRIDABLE | nil | false
end
with_them do
......@@ -874,11 +872,11 @@ RSpec.describe Projects::CreateService, '#execute' do
context 'parent group is present and allows desired config' do
where(:shared_runners_setting, :desired_config_for_new_project, :expected_result_for_project) do
'enabled' | true | true
'enabled' | false | false
'disabled_with_override' | false | false
'disabled_with_override' | true | true
'disabled_and_unoverridable' | false | false
Namespace::SR_ENABLED | true | true
Namespace::SR_ENABLED | false | false
Namespace::SR_DISABLED_WITH_OVERRIDE | false | false
Namespace::SR_DISABLED_WITH_OVERRIDE | true | true
Namespace::SR_DISABLED_AND_UNOVERRIDABLE | false | false
end
with_them do
......@@ -894,7 +892,7 @@ RSpec.describe Projects::CreateService, '#execute' do
context 'parent group is present and disallows desired config' do
where(:shared_runners_setting, :desired_config_for_new_project) do
'disabled_and_unoverridable' | true
Namespace::SR_DISABLED_AND_UNOVERRIDABLE | true
end
with_them do
......
......@@ -458,28 +458,23 @@ RSpec.describe Projects::TransferService do
using RSpec::Parameterized::TableSyntax
where(:project_shared_runners_enabled, :shared_runners_setting, :expected_shared_runners_enabled) do
true | 'disabled_and_unoverridable' | false
false | 'disabled_and_unoverridable' | false
true | 'disabled_with_override' | true
false | 'disabled_with_override' | false
true | 'enabled' | true
false | 'enabled' | false
true | :disabled_and_unoverridable | false
false | :disabled_and_unoverridable | false
true | :disabled_with_override | true
false | :disabled_with_override | false
true | :shared_runners_enabled | true
false | :shared_runners_enabled | false
end
with_them do
let(:project) { create(:project, :public, :repository, namespace: user.namespace, shared_runners_enabled: project_shared_runners_enabled) }
let(:group) { create(:group) }
let(:group) { create(:group, shared_runners_setting) }
before do
it 'updates shared runners based on the parent group' do
group.add_owner(user)
expect_next_found_instance_of(Group) do |group|
expect(group).to receive(:shared_runners_setting).and_return(shared_runners_setting)
end
execute_transfer
end
expect(execute_transfer).to eq(true)
it 'updates shared runners based on the parent group' do
expect(project.shared_runners_enabled).to eq(expected_shared_runners_enabled)
end
end
......
......@@ -3,7 +3,7 @@
RSpec.shared_context 'relation tree restorer shared context' do
include ImportExport::CommonUtil
let(:user) { create(:user) }
let_it_be(:user) { create(:user) }
let(:shared) { Gitlab::ImportExport::Shared.new(importable) }
let(:attributes) { relation_reader.consume_attributes(importable_name) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment