Merge branch 'jc-coerce-repository-storages-weighted-api' into 'master'

Coerce repository_storages_weighted, removes repository_storages

See merge request gitlab-org/gitlab!36376

app/helpers/application_settings_helper.rb

@@ -262,7 +262,7 @@ module ApplicationSettingsHelper
       :login_recaptcha_protection_enabled,
       :receive_max_input_size,
       :repository_checks_enabled,
-      :repository_storages,
+      :repository_storages_weighted,
       :require_two_factor_authentication,
       :restricted_visibility_levels,
       :rsa_key_restriction,

app/models/application_setting.rb

@@ -284,10 +284,6 @@ class ApplicationSetting < ApplicationRecord
 
   validates :allowed_key_types, presence: true
 
-  repository_storages_weighted_attributes.each do |attribute|
-    validates attribute, allow_nil: true, numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 100 }
-  end
-
   validates_each :restricted_visibility_levels do |record, attr, value|
     value&.each do |level|
       unless Gitlab::VisibilityLevel.options.value?(level)

app/models/application_setting_implementation.rb

@@ -452,6 +452,13 @@ module ApplicationSettingImplementation
     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
       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)
+    end
   end
 
   def terms_exist

changelogs/unreleased/jc-coerce-repository-storages-weighted-api.yml

+---
+title: Coerce repository_storages_weighted, removes repository_storages
+merge_request: 36376
+author:
+type: fixed

ee/lib/ee/api/helpers/settings_helpers.rb

@@ -36,7 +36,6 @@ module EE
             optional :help_text, type: String, desc: 'GitLab server administrator information'
             optional :repository_size_limit, type: Integer, desc: 'Size limit per repository (MB)'
             optional :file_template_project_id, type: Integer, desc: 'ID of project where instance-level file templates are stored.'
-            optional :repository_storages, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'A list of names of enabled storage paths, taken from `gitlab.yml`. New projects will be created in one of these stores, chosen at random.'
             optional :usage_ping_enabled, type: Grape::API::Boolean, desc: 'Every week GitLab will report license usage back to GitLab, Inc.'
             optional :updating_name_disabled_for_users, type: Grape::API::Boolean, desc: 'Flag indicating if users are permitted to update their profile name'
             optional :disable_overriding_approvers_per_merge_request, type: Grape::API::Boolean, desc: 'Disable Users ability to overwrite approvers in merge requests.'

lib/api/settings.rb

@@ -114,8 +114,7 @@ module API
         requires :recaptcha_private_key, type: String, desc: 'Generate private key at http://www.google.com/recaptcha'
       end
       optional :repository_checks_enabled, type: Boolean, desc: "GitLab will periodically run 'git fsck' in all project and wiki repositories to look for silent disk corruption issues."
-      optional :repository_storages, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Storage paths for new projects'
-      optional :repository_storages_weighted, type: Hash, desc: 'Storage paths for new projects with a weighted value between 0 and 100'
+      optional :repository_storages_weighted, type: Hash, coerce_with: Validations::Types::HashOfIntegerValues.coerce, desc: 'Storage paths for new projects with a weighted value ranging from 0 to 100'
       optional :require_two_factor_authentication, type: Boolean, desc: 'Require all users to set up Two-factor authentication'
       given require_two_factor_authentication: ->(val) { val } do
         requires :two_factor_grace_period, type: Integer, desc: 'Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication'

lib/api/validations/types/hash_of_integer_values.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Types
+      class HashOfIntegerValues
+        def self.coerce
+          lambda do |value|
+            case value
+            when Hash
+              value.transform_values(&:to_i)
+            else
+              value
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/models/application_setting_spec.rb

@@ -107,6 +107,7 @@ 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) }

spec/requests/api/settings_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe API::Settings, 'Settings' do
       expect(json_response).to be_an Hash
       expect(json_response['default_projects_limit']).to eq(42)
       expect(json_response['password_authentication_enabled_for_web']).to be_truthy
-      expect(json_response['repository_storages']).to eq(['default'])
+      expect(json_response['repository_storages_weighted']).to eq({ 'default' => 100 })
       expect(json_response['password_authentication_enabled']).to be_truthy
       expect(json_response['plantuml_enabled']).to be_falsey
       expect(json_response['plantuml_url']).to be_nil
@@ -55,6 +55,28 @@ RSpec.describe API::Settings, 'Settings' do
         stub_feature_flags(sourcegraph: true)
       end
 
+      it "coerces repository_storages_weighted to an int" do
+        put api("/application/settings", admin),
+          params: {
+            repository_storages_weighted: { 'custom' => '75' }
+          }
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['repository_storages_weighted']).to eq({ 'custom' => 75 })
+      end
+
+      context "repository_storages_weighted value is outside a 0-100 range" do
+        [-1, 101].each do |out_of_range_int|
+          it "returns a :bad_request for #{out_of_range_int}" do
+            put api("/application/settings", admin),
+              params: {
+                repository_storages_weighted: { 'custom' => out_of_range_int }
+              }
+            expect(response).to have_gitlab_http_status(:bad_request)
+          end
+        end
+      end
+
       it "updates application settings" do
         put api("/application/settings", admin),
           params: {
@@ -62,7 +84,7 @@ RSpec.describe API::Settings, 'Settings' do
             default_projects_limit: 3,
             default_project_creation: 2,
             password_authentication_enabled_for_web: false,
-            repository_storages: 'custom',
+            repository_storages_weighted: { 'custom' => 100 },
             plantuml_enabled: true,
             plantuml_url: 'http://plantuml.example.com',
             sourcegraph_enabled: true,
@@ -104,7 +126,7 @@ RSpec.describe API::Settings, 'Settings' do
         expect(json_response['default_projects_limit']).to eq(3)
         expect(json_response['default_project_creation']).to eq(::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS)
         expect(json_response['password_authentication_enabled_for_web']).to be_falsey
-        expect(json_response['repository_storages']).to eq(['custom'])
+        expect(json_response['repository_storages_weighted']).to eq({ 'custom' => 100 })
         expect(json_response['plantuml_enabled']).to be_truthy
         expect(json_response['plantuml_url']).to eq('http://plantuml.example.com')
         expect(json_response['sourcegraph_enabled']).to be_truthy