Automatic merge of gitlab-org/gitlab master

app/controllers/admin/application_settings_controller.rb

@@ -237,6 +237,7 @@ 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,
@@ -247,8 +248,8 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       :default_branch_name,
       disabled_oauth_sign_in_sources: [],
       import_sources: [],
-      restricted_visibility_levels: [],
-      repository_storages_weighted: {}
+      repository_storages: [],
+      restricted_visibility_levels: []
     ]
   end
 

app/helpers/application_settings_helper.rb

@@ -37,8 +37,13 @@ module ApplicationSettingsHelper
   end
 
   def storage_weights
-    Gitlab.config.repositories.storages.keys.each_with_object(OpenStruct.new) do |storage, weights|
-      weights[storage.to_sym] = @application_setting.repository_storages_weighted[storage] || 0
+    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
+      }
     end
   end
 

app/models/application_setting.rb

@@ -25,6 +25,10 @@ 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: {
@@ -40,6 +44,7 @@ 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`
@@ -498,7 +503,6 @@ 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
@@ -579,6 +583,12 @@ 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))

app/models/application_setting_implementation.rb

@@ -298,6 +298,10 @@ 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
@@ -329,10 +333,9 @@ module ApplicationSettingImplementation
 
   def normalized_repository_storage_weights
     strong_memoize(:normalized_repository_storage_weights) do
-      repository_storages_weights = repository_storages_weighted.slice(*Gitlab.config.repositories.storages.keys)
-      weights_total = repository_storages_weights.values.reduce(:+)
+      weights_total = repository_storages_weighted.values.reduce(:+)
 
-      repository_storages_weights.transform_values do |w|
+      repository_storages_weighted.transform_values do |w|
         next w if weights_total == 0
 
         w.to_f / weights_total
@@ -470,20 +473,16 @@ 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, _("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)
+      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)
     end
   end
 

app/views/admin/application_settings/_repository_storage.html.haml

@@ -18,9 +18,8 @@
             = _('Enter weights for storages for new repositories.')
             = link_to sprite_icon('question-o'), help_page_path('administration/repository_storage_paths')
         .form-check
-          = 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
+          - 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.submit _('Save changes'), class: "gl-button btn btn-success qa-save-changes-button"

changelogs/unreleased/fix_repo_storage_weights_admin.yml

----
-title: Allow saving repository weights after a storage has been removed
-merge_request: 53803
-author:
-type: fixed

doc/user/project/clusters/add_eks_clusters.md

@@ -140,13 +140,11 @@ To create and add a new Kubernetes cluster to your project, group, or instance:
    1. Click **Review policy**.
    1. Enter a suitable name for this policy, and click **Create Policy**. You can now close this window.
 
-1. In the [IAM Management Console](https://console.aws.amazon.com/iam/home), create an EKS management IAM role.
-   To do so, follow the [Amazon EKS cluster IAM role](https://docs.aws.amazon.com/eks/latest/userguide/service_IAM_role.html) instructions
-   to create a IAM role suitable for managing the AWS EKS cluster's resources on your behalf.
+1. In the [IAM Management Console](https://console.aws.amazon.com/iam/home), create an **EKS IAM role** following the [Amazon EKS cluster IAM role instructions](https://docs.aws.amazon.com/eks/latest/userguide/service_IAM_role.html). This role should exist so that Kubernetes clusters managed by Amazon EKS can make calls to other AWS services on your behalf to manage the resources that you use with the service.
    In addition to the policies that guide suggests, you must also include the `AmazonEKSClusterPolicy`
    policy for this role in order for GitLab to manage the EKS cluster correctly.
-1. In the [IAM Management Console](https://console.aws.amazon.com/iam/home), create an IAM role:
-   1. From the left panel, select **Roles**.
+1. In the [IAM Management Console](https://console.aws.amazon.com/iam/home), create another IAM role which will be used by GitLab to authenticate with AWS. Follow these steps to create it:
+   1. On the AWS IAM console, select **Roles** from the left panel.
    1. Click **Create role**.
    1. Under `Select type of trusted entity`, select **Another AWS account**.
    1. Enter the Account ID from GitLab into the `Account ID` field.

locale/gitlab.pot

@@ -34923,9 +34923,6 @@ 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 ""
 
@@ -36332,12 +36329,6 @@ 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 ""
 

spec/controllers/admin/application_settings_controller_spec.rb

@@ -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).to eq('default' => 75)
+      expect(ApplicationSetting.current.repository_storages_weighted_default).to eq(75)
     end
 
     it 'updates kroki_formats setting' do

spec/features/admin/admin_settings_spec.rb

@@ -384,20 +384,7 @@ RSpec.describe 'Admin updates settings' do
           click_button 'Save changes'
         end
 
-        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)
+        expect(current_settings.repository_storages_weighted_default).to be 50
       end
     end
 

spec/helpers/application_settings_helper_spec.rb

@@ -130,15 +130,20 @@ 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(OpenStruct.new(
-                                             default: 100,
-                                             storage_1: 50,
-                                             storage_2: 0
-                                           ))
+      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 }
+        ])
     end
   end
 

spec/models/application_setting_spec.rb

@@ -105,14 +105,14 @@ RSpec.describe ApplicationSetting do
 
     it { is_expected.not_to allow_value(false).for(:hashed_storage_enabled) }
 
-    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.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(400).for(:notes_create_limit) }
     it { is_expected.not_to allow_value('two').for(:notes_create_limit) }
@@ -984,6 +984,12 @@ 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
@@ -1027,4 +1033,11 @@ 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

spec/support/shared_examples/models/application_setting_shared_examples.rb

@@ -289,7 +289,6 @@ 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
 
@@ -305,19 +304,15 @@ RSpec.shared_examples 'application settings examples' do
   describe '#normalized_repository_storage_weights' do
     using RSpec::Parameterized::TableSyntax
 
-    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 }
+    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 }
     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
 

spec/views/admin/application_settings/_repository_storage.html.haml_spec.rb

@@ -3,49 +3,34 @@
 require 'spec_helper'
 
 RSpec.describe 'admin/application_settings/_repository_storage.html.haml' do
-  let(:app_settings) { build(:application_setting, repository_storages_weighted: repository_storages_weighted) }
+  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
 
   before do
-    stub_storage_settings({ 'default': {}, 'mepmep': {}, 'foobar': {} })
+    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)
     assign(:application_setting, app_settings)
+    allow(ApplicationSetting).to receive(:repository_storages_weighted_attributes).and_return(repository_storages_weighted_attributes)
   end
 
-  context 'additional storage config' do
-    let(:repository_storages_weighted) do
-      {
-        'default' => 100,
-        'mepmep' => 50
-      }
-    end
-
+  context 'when multiple storages are available' do
     it 'lists them all' do
       render
 
-      Gitlab.config.repositories.storages.keys.each do |storage_name|
+      # lists storages that are saved with weights
+      repository_storages_weighted.each do |storage_name, storage_weight|
         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