From ca22192aa4200cc1debed26061ea7119689345e1 Mon Sep 17 00:00:00 2001 From: James Fargher Date: Wed, 10 Feb 2021 15:28:18 +1300 Subject: [PATCH 1/4] Ensure new project storage selection never chooses an invalid storage --- app/models/application_setting_implementation.rb | 5 +++-- .../models/application_setting_shared_examples.rb | 15 ++++++++++----- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index 2911ae6b1c8f36..a1558bbcefaaf7 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -328,9 +328,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 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 92fd436313469a..60a02d85a1e9cd 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 -- GitLab From b8726fae2799f290c2c5e441b3301964322d87c2 Mon Sep 17 00:00:00 2001 From: James Fargher Date: Mon, 15 Feb 2021 16:01:00 +1300 Subject: [PATCH 2/4] Remove repository_storages_weighted as its implementation is the default --- app/models/application_setting_implementation.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index a1558bbcefaaf7..69f6e85bcf22ff 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 -- GitLab From 7662f92359620cf8b469b39001b0f7746b2f77ea Mon Sep 17 00:00:00 2001 From: James Fargher Date: Tue, 16 Feb 2021 09:56:51 +1300 Subject: [PATCH 3/4] Use hstore more directly in repository storages form Running the admin form off of the hstore directly should allow us to synchronise the database more easily with any storage config changes. --- .../admin/application_settings_controller.rb | 3 +- app/helpers/application_settings_helper.rb | 9 +--- app/models/application_setting.rb | 1 + .../application_setting_implementation.rb | 10 ++-- .../_repository_storage.html.haml | 9 ++-- .../fix_repo_storage_weights_admin.yml | 5 ++ locale/gitlab.pot | 9 ++++ spec/features/admin/admin_settings_spec.rb | 15 +++++- .../application_settings_helper_spec.rb | 15 ++---- spec/models/application_setting_spec.rb | 23 ++++----- .../_repository_storage.html.haml_spec.rb | 47 ++++++++++++------- 11 files changed, 89 insertions(+), 57 deletions(-) create mode 100644 changelogs/unreleased/fix_repo_storage_weights_admin.yml diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 7f7d38a09c5804..52cee08d8dced7 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -249,7 +249,8 @@ def visible_application_setting_attributes 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 244238e246d72b..551e08d1e31c0a 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 33c058dab96b47..43eed40c9aa211 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -502,6 +502,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 diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index 69f6e85bcf22ff..ffbefe91a6ceba 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -465,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 0862d1bf0b6e8b..437189cf47f013 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 00000000000000..c82ef578e018db --- /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 bc98ce39dd72ea..8787f71b9f36d4 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/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 52f39f65bd0e9d..249621f5835ed6 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 2cd01451e0d12c..c74ee3ce0ecee9 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 8f63f44e8af835..1a9cae1b5c5975 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) } @@ -1007,11 +1007,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/views/admin/application_settings/_repository_storage.html.haml_spec.rb b/spec/views/admin/application_settings/_repository_storage.html.haml_spec.rb index 2915fe1964f52a..dc8f259eb56f9c 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 -- GitLab From ed266170845726c0e6c12b7be5c7b180cd31d2be Mon Sep 17 00:00:00 2001 From: James Fargher Date: Tue, 16 Feb 2021 11:16:36 +1300 Subject: [PATCH 4/4] Remove store accessor for repository_storages_weighted Now that the form no longer uses these accessors they can be removed. --- .../admin/application_settings_controller.rb | 2 -- app/models/application_setting.rb | 11 ----------- .../admin/application_settings_controller_spec.rb | 4 ++-- spec/models/application_setting_spec.rb | 6 ------ 4 files changed, 2 insertions(+), 21 deletions(-) diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 52cee08d8dced7..7c6a444ce7a36a 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,7 +247,6 @@ def visible_application_setting_attributes :default_branch_name, disabled_oauth_sign_in_sources: [], import_sources: [], - repository_storages: [], restricted_visibility_levels: [], repository_storages_weighted: {} ] diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 43eed40c9aa211..49666f1a8bd3bc 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` @@ -583,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/spec/controllers/admin/application_settings_controller_spec.rb b/spec/controllers/admin/application_settings_controller_spec.rb index 71abf3191b89f3..2b562e2dd64efa 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/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 1a9cae1b5c5975..97ed0f5820c76e 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -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 -- GitLab