diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 7f7d38a09c580489c6105b591a88d43f60d56683..7c6a444ce7a36ad07be17a5d9b0764e2c7ed78b2 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -237,7 +237,6 @@ def visible_application_setting_attributes [ *::ApplicationSettingsHelper.visible_attributes, *::ApplicationSettingsHelper.external_authorization_service_attributes, - *ApplicationSetting.repository_storages_weighted_attributes, *ApplicationSetting.kroki_formats_attributes.keys.map { |key| "kroki_formats_#{key}".to_sym }, :lets_encrypt_notification_email, :lets_encrypt_terms_of_service_accepted, @@ -248,8 +247,8 @@ def visible_application_setting_attributes :default_branch_name, disabled_oauth_sign_in_sources: [], import_sources: [], - repository_storages: [], - restricted_visibility_levels: [] + restricted_visibility_levels: [], + repository_storages_weighted: {} ] end diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 244238e246d72bb7c565a4ae98c6115bc9b0ce25..551e08d1e31c0a7a24d9d11bda26e0833314502b 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -37,13 +37,8 @@ def kroki_available_formats end def storage_weights - ApplicationSetting.repository_storages_weighted_attributes.map do |attribute| - storage = attribute.to_s.delete_prefix('repository_storages_weighted_') - { - name: attribute, - label: storage, - value: @application_setting.repository_storages_weighted[storage] || 0 - } + Gitlab.config.repositories.storages.keys.each_with_object(OpenStruct.new) do |storage, weights| + weights[storage.to_sym] = @application_setting.repository_storages_weighted[storage] || 0 end end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 33c058dab96b47da57dacde2665620daa13f6b23..49666f1a8bd3bce62a9b732240495c4882f0eb96 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -25,10 +25,6 @@ class ApplicationSetting < ApplicationRecord alias_attribute :instance_group_id, :instance_administrators_group_id alias_attribute :instance_administrators_group, :instance_group - def self.repository_storages_weighted_attributes - @repository_storages_weighted_atributes ||= Gitlab.config.repositories.storages.keys.map { |k| "repository_storages_weighted_#{k}".to_sym }.freeze - end - def self.kroki_formats_attributes { blockdiag: { @@ -44,7 +40,6 @@ def self.kroki_formats_attributes end store_accessor :kroki_formats, *ApplicationSetting.kroki_formats_attributes.keys, prefix: true - store_accessor :repository_storages_weighted, *Gitlab.config.repositories.storages.keys, prefix: true # Include here so it can override methods from # `add_authentication_token_field` @@ -502,6 +497,7 @@ def self.kroki_formats_attributes inclusion: { in: [true, false], message: _('must be a boolean value') } before_validation :ensure_uuid! + before_validation :coerce_repository_storages_weighted, if: :repository_storages_weighted_changed? before_save :ensure_runners_registration_token before_save :ensure_health_check_access_token @@ -582,12 +578,6 @@ def recaptcha_or_login_protection_enabled recaptcha_enabled || login_recaptcha_protection_enabled end - repository_storages_weighted_attributes.each do |attribute| - define_method :"#{attribute}=" do |value| - super(value.to_i) - end - end - kroki_formats_attributes.keys.each do |key| define_method :"kroki_formats_#{key}=" do |value| super(::Gitlab::Utils.to_boolean(value)) diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index 2911ae6b1c8f362247547d84c92905eee7c17edb..ffbefe91a6ceba1144eadc6b53a9de7103cb1182 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -293,10 +293,6 @@ def repository_storages Array(read_attribute(:repository_storages)) end - def repository_storages_weighted - read_attribute(:repository_storages_weighted) - end - def commit_email_hostname super.presence || self.class.default_commit_email_hostname end @@ -328,9 +324,10 @@ def performance_bar_enabled def normalized_repository_storage_weights strong_memoize(:normalized_repository_storage_weights) do - weights_total = repository_storages_weighted.values.reduce(:+) + repository_storages_weights = repository_storages_weighted.slice(*Gitlab.config.repositories.storages.keys) + weights_total = repository_storages_weights.values.reduce(:+) - repository_storages_weighted.transform_values do |w| + repository_storages_weights.transform_values do |w| next w if weights_total == 0 w.to_f / weights_total @@ -468,16 +465,20 @@ def check_repository_storages invalid.empty? end + def coerce_repository_storages_weighted + repository_storages_weighted.transform_values!(&:to_i) + end + def check_repository_storages_weighted invalid = repository_storages_weighted.keys - Gitlab.config.repositories.storages.keys - errors.add(:repository_storages_weighted, "can't include: %{invalid_storages}" % { invalid_storages: invalid.join(", ") }) unless + errors.add(:repository_storages_weighted, _("can't include: %{invalid_storages}") % { invalid_storages: invalid.join(", ") }) unless invalid.empty? repository_storages_weighted.each do |key, val| next unless val.present? - errors.add(:"repository_storages_weighted_#{key}", "value must be an integer") unless val.is_a?(Integer) - errors.add(:"repository_storages_weighted_#{key}", "value must be between 0 and 100") unless val.between?(0, 100) + errors.add(:repository_storages_weighted, _("value for '%{storage}' must be an integer") % { storage: key }) unless val.is_a?(Integer) + errors.add(:repository_storages_weighted, _("value for '%{storage}' must be between 0 and 100") % { storage: key }) unless val.between?(0, 100) end end diff --git a/app/views/admin/application_settings/_repository_storage.html.haml b/app/views/admin/application_settings/_repository_storage.html.haml index 0862d1bf0b6e8b438f194024f7b64bd420c76a01..437189cf47f01370d52d1aa4eebe47b000c3bc15 100644 --- a/app/views/admin/application_settings/_repository_storage.html.haml +++ b/app/views/admin/application_settings/_repository_storage.html.haml @@ -18,8 +18,9 @@ = _('Enter weights for storages for new repositories.') = link_to sprite_icon('question-o'), help_page_path('administration/repository_storage_paths') .form-check - - storage_weights.each do |attribute| - = f.text_field attribute[:name], class: 'form-text-input', value: attribute[:value] - = f.label attribute[:label], attribute[:label], class: 'label-bold form-check-label' - %br + = f.fields_for :repository_storages_weighted, storage_weights do |storage_form| + - Gitlab.config.repositories.storages.keys.each do |storage| + = storage_form.text_field storage, class: 'form-text-input' + = storage_form.label storage, storage, class: 'label-bold form-check-label' + %br = f.submit _('Save changes'), class: "gl-button btn btn-success qa-save-changes-button" diff --git a/changelogs/unreleased/fix_repo_storage_weights_admin.yml b/changelogs/unreleased/fix_repo_storage_weights_admin.yml new file mode 100644 index 0000000000000000000000000000000000000000..c82ef578e018dbb44d3e752209fab3cf4827f8d0 --- /dev/null +++ b/changelogs/unreleased/fix_repo_storage_weights_admin.yml @@ -0,0 +1,5 @@ +--- +title: Allow saving repository weights after a storage has been removed +merge_request: 53803 +author: +type: fixed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bc98ce39dd72eac28d0026857b0bd2b9ae5610bb..8787f71b9f36d4c82e5fb876c9a9842bc684582a 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -34882,6 +34882,9 @@ msgstr "" msgid "can't be enabled because signed commits are required for this project" msgstr "" +msgid "can't include: %{invalid_storages}" +msgstr "" + msgid "cannot be a date in the past" msgstr "" @@ -36288,6 +36291,12 @@ msgstr "" msgid "v%{version} published %{timeAgo}" msgstr "" +msgid "value for '%{storage}' must be an integer" +msgstr "" + +msgid "value for '%{storage}' must be between 0 and 100" +msgstr "" + msgid "verify ownership" msgstr "" diff --git a/spec/controllers/admin/application_settings_controller_spec.rb b/spec/controllers/admin/application_settings_controller_spec.rb index 71abf3191b89f3992508bbfc1868c42da44a3e67..2b562e2dd64efafa5e574aa28f649786ccc439e6 100644 --- a/spec/controllers/admin/application_settings_controller_spec.rb +++ b/spec/controllers/admin/application_settings_controller_spec.rb @@ -144,10 +144,10 @@ end it 'updates repository_storages_weighted setting' do - put :update, params: { application_setting: { repository_storages_weighted_default: 75 } } + put :update, params: { application_setting: { repository_storages_weighted: { default: 75 } } } expect(response).to redirect_to(general_admin_application_settings_path) - expect(ApplicationSetting.current.repository_storages_weighted_default).to eq(75) + expect(ApplicationSetting.current.repository_storages_weighted).to eq('default' => 75) end it 'updates kroki_formats setting' do diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 52f39f65bd0e9dc9a7061cf087095a5107a0e187..249621f5835ed6e3cb16de91b0111ff2802fbc5a 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -384,7 +384,20 @@ click_button 'Save changes' end - expect(current_settings.repository_storages_weighted_default).to be 50 + expect(current_settings.repository_storages_weighted).to eq('default' => 50) + end + + it 'still saves when settings are outdated' do + current_settings.update_attribute :repository_storages_weighted, { 'default' => 100, 'outdated' => 100 } + + visit repository_admin_application_settings_path + + page.within('.as-repository-storage') do + fill_in 'application_setting_repository_storages_weighted_default', with: 50 + click_button 'Save changes' + end + + expect(current_settings.repository_storages_weighted).to eq('default' => 50) end end diff --git a/spec/helpers/application_settings_helper_spec.rb b/spec/helpers/application_settings_helper_spec.rb index 2cd01451e0d12c3f310b4e3589cee13929e19ac7..c74ee3ce0ecee95a990667d8fc9142e370397be9 100644 --- a/spec/helpers/application_settings_helper_spec.rb +++ b/spec/helpers/application_settings_helper_spec.rb @@ -130,20 +130,15 @@ before do helper.instance_variable_set(:@application_setting, application_setting) stub_storage_settings({ 'default': {}, 'storage_1': {}, 'storage_2': {} }) - allow(ApplicationSetting).to receive(:repository_storages_weighted_attributes).and_return( - [:repository_storages_weighted_default, - :repository_storages_weighted_storage_1, - :repository_storages_weighted_storage_2]) - stub_application_setting(repository_storages_weighted: { 'default' => 100, 'storage_1' => 50, 'storage_2' => nil }) end it 'returns storages correctly' do - expect(helper.storage_weights).to eq([ - { name: :repository_storages_weighted_default, label: 'default', value: 100 }, - { name: :repository_storages_weighted_storage_1, label: 'storage_1', value: 50 }, - { name: :repository_storages_weighted_storage_2, label: 'storage_2', value: 0 } - ]) + expect(helper.storage_weights).to eq(OpenStruct.new( + default: 100, + storage_1: 50, + storage_2: 0 + )) end end diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 8f63f44e8af8355193db71c565589a05ab2dab52..97ed0f5820c76e792b8c75476e70a84548359568 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -105,14 +105,14 @@ it { is_expected.not_to allow_value(false).for(:hashed_storage_enabled) } - it { is_expected.not_to allow_value(101).for(:repository_storages_weighted_default) } - it { is_expected.to allow_value('90').for(:repository_storages_weighted_default) } - it { is_expected.not_to allow_value(-1).for(:repository_storages_weighted_default) } - it { is_expected.to allow_value(100).for(:repository_storages_weighted_default) } - it { is_expected.to allow_value(0).for(:repository_storages_weighted_default) } - it { is_expected.to allow_value(50).for(:repository_storages_weighted_default) } - it { is_expected.to allow_value(nil).for(:repository_storages_weighted_default) } - it { is_expected.not_to allow_value({ default: 100, shouldntexist: 50 }).for(:repository_storages_weighted) } + it { is_expected.to allow_value('default' => 0).for(:repository_storages_weighted) } + it { is_expected.to allow_value('default' => 50).for(:repository_storages_weighted) } + it { is_expected.to allow_value('default' => 100).for(:repository_storages_weighted) } + it { is_expected.to allow_value('default' => '90').for(:repository_storages_weighted) } + it { is_expected.to allow_value('default' => nil).for(:repository_storages_weighted) } + it { is_expected.not_to allow_value('default' => -1).for(:repository_storages_weighted).with_message("value for 'default' must be between 0 and 100") } + it { is_expected.not_to allow_value('default' => 101).for(:repository_storages_weighted).with_message("value for 'default' must be between 0 and 100") } + it { is_expected.not_to allow_value('default' => 100, shouldntexist: 50).for(:repository_storages_weighted).with_message("can't include: shouldntexist") } it { is_expected.to allow_value(400).for(:notes_create_limit) } it { is_expected.not_to allow_value('two').for(:notes_create_limit) } @@ -958,12 +958,6 @@ def expect_invalid it_behaves_like 'application settings examples' - describe 'repository_storages_weighted_attributes' do - it 'returns the keys for repository_storages_weighted' do - expect(subject.class.repository_storages_weighted_attributes).to eq([:repository_storages_weighted_default]) - end - end - describe 'kroki_format_supported?' do it 'returns true when Excalidraw is enabled' do subject.kroki_formats_excalidraw = true @@ -1007,11 +1001,4 @@ def expect_invalid expect(subject.kroki_formats_excalidraw).to eq(true) end end - - it 'does not allow to set weight for non existing storage' do - setting.repository_storages_weighted = { invalid_storage: 100 } - - expect(setting).not_to be_valid - expect(setting.errors.messages[:repository_storages_weighted]).to match_array(["can't include: invalid_storage"]) - end end diff --git a/spec/support/shared_examples/models/application_setting_shared_examples.rb b/spec/support/shared_examples/models/application_setting_shared_examples.rb index 92fd436313469a15bcbea46662c8a8c09d88a439..60a02d85a1e9cd8e108e264d5181869b9a422f23 100644 --- a/spec/support/shared_examples/models/application_setting_shared_examples.rb +++ b/spec/support/shared_examples/models/application_setting_shared_examples.rb @@ -289,6 +289,7 @@ describe '#pick_repository_storage' do before do + allow(Gitlab.config.repositories.storages).to receive(:keys).and_return(%w(default backup)) allow(setting).to receive(:repository_storages_weighted).and_return({ 'default' => 20, 'backup' => 80 }) end @@ -304,15 +305,19 @@ describe '#normalized_repository_storage_weights' do using RSpec::Parameterized::TableSyntax - where(:storages, :normalized) do - { 'default' => 0, 'backup' => 100 } | { 'default' => 0.0, 'backup' => 1.0 } - { 'default' => 100, 'backup' => 100 } | { 'default' => 0.5, 'backup' => 0.5 } - { 'default' => 20, 'backup' => 80 } | { 'default' => 0.2, 'backup' => 0.8 } - { 'default' => 0, 'backup' => 0 } | { 'default' => 0.0, 'backup' => 0.0 } + where(:config_storages, :storages, :normalized) do + %w(default backup) | { 'default' => 0, 'backup' => 100 } | { 'default' => 0.0, 'backup' => 1.0 } + %w(default backup) | { 'default' => 100, 'backup' => 100 } | { 'default' => 0.5, 'backup' => 0.5 } + %w(default backup) | { 'default' => 20, 'backup' => 80 } | { 'default' => 0.2, 'backup' => 0.8 } + %w(default backup) | { 'default' => 0, 'backup' => 0 } | { 'default' => 0.0, 'backup' => 0.0 } + %w(default) | { 'default' => 0, 'backup' => 100 } | { 'default' => 0.0 } + %w(default) | { 'default' => 100, 'backup' => 100 } | { 'default' => 1.0 } + %w(default) | { 'default' => 20, 'backup' => 80 } | { 'default' => 1.0 } end with_them do before do + allow(Gitlab.config.repositories.storages).to receive(:keys).and_return(config_storages) allow(setting).to receive(:repository_storages_weighted).and_return(storages) end diff --git a/spec/views/admin/application_settings/_repository_storage.html.haml_spec.rb b/spec/views/admin/application_settings/_repository_storage.html.haml_spec.rb index 2915fe1964f52ae87a6dce409c3fc95efe73cd08..dc8f259eb56f9c15ed4ef51a5e5a286234767e9b 100644 --- a/spec/views/admin/application_settings/_repository_storage.html.haml_spec.rb +++ b/spec/views/admin/application_settings/_repository_storage.html.haml_spec.rb @@ -3,34 +3,49 @@ require 'spec_helper' RSpec.describe 'admin/application_settings/_repository_storage.html.haml' do - let(:app_settings) { create(:application_setting) } - let(:repository_storages_weighted_attributes) { [:repository_storages_weighted_default, :repository_storages_weighted_mepmep, :repository_storages_weighted_foobar]} - let(:repository_storages_weighted) do - { - "default" => 100, - "mepmep" => 50 - } - end + let(:app_settings) { build(:application_setting, repository_storages_weighted: repository_storages_weighted) } before do - allow(app_settings).to receive(:repository_storages_weighted).and_return(repository_storages_weighted) - allow(app_settings).to receive(:repository_storages_weighted_mepmep).and_return(100) - allow(app_settings).to receive(:repository_storages_weighted_foobar).and_return(50) + stub_storage_settings({ 'default': {}, 'mepmep': {}, 'foobar': {} }) assign(:application_setting, app_settings) - allow(ApplicationSetting).to receive(:repository_storages_weighted_attributes).and_return(repository_storages_weighted_attributes) end - context 'when multiple storages are available' do + context 'additional storage config' do + let(:repository_storages_weighted) do + { + 'default' => 100, + 'mepmep' => 50 + } + end + it 'lists them all' do render - # lists storages that are saved with weights - repository_storages_weighted.each do |storage_name, storage_weight| + Gitlab.config.repositories.storages.keys.each do |storage_name| expect(rendered).to have_content(storage_name) end - # lists storage not saved with weight expect(rendered).to have_content('foobar') end end + + context 'fewer storage configs' do + let(:repository_storages_weighted) do + { + 'default' => 100, + 'mepmep' => 50, + 'something_old' => 100 + } + end + + it 'lists only configured storages' do + render + + Gitlab.config.repositories.storages.keys.each do |storage_name| + expect(rendered).to have_content(storage_name) + end + + expect(rendered).not_to have_content('something_old') + end + end end