Commit d32ba0c0 authored by David Fernandez's avatar David Fernandez

Merge branch 'fix_repo_storage_weights_admin' into 'master'

Allow saving repository weights after a storage has been removed

See merge request gitlab-org/gitlab!53803
parents c54dd7b0 ed266170
......@@ -237,7 +237,6 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
[
*::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 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
:default_branch_name,
disabled_oauth_sign_in_sources: [],
import_sources: [],
repository_storages: [],
restricted_visibility_levels: []
restricted_visibility_levels: [],
repository_storages_weighted: {}
]
end
......
......@@ -37,13 +37,8 @@ module ApplicationSettingsHelper
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
......
......@@ -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 @@ class ApplicationSetting < ApplicationRecord
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 @@ class ApplicationSetting < ApplicationRecord
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 @@ class ApplicationSetting < ApplicationRecord
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))
......
......@@ -293,10 +293,6 @@ module ApplicationSettingImplementation
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 @@ module ApplicationSettingImplementation
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 @@ module ApplicationSettingImplementation
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
......
......@@ -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'
= 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"
---
title: Allow saving repository weights after a storage has been removed
merge_request: 53803
author:
type: fixed
......@@ -34920,6 +34920,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 ""
......@@ -36326,6 +36329,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 ""
......
......@@ -144,10 +144,10 @@ RSpec.describe Admin::ApplicationSettingsController do
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
......
......@@ -384,7 +384,20 @@ RSpec.describe 'Admin updates settings' do
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
......
......@@ -130,20 +130,15 @@ RSpec.describe ApplicationSettingsHelper do
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
......
......@@ -105,14 +105,14 @@ RSpec.describe ApplicationSetting do
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 @@ RSpec.describe ApplicationSetting do
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 @@ RSpec.describe ApplicationSetting do
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
......@@ -289,6 +289,7 @@ RSpec.shared_examples 'application settings examples' do
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 @@ RSpec.shared_examples 'application settings examples' do
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
......
......@@ -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
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