From d35ace9f083015bf1f5dfb7830688560841869b7 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Wed, 14 Dec 2022 14:14:26 +0100 Subject: [PATCH] Replace housekeeping setting with a single one in API Instead of three housekeeping options: - housekeeping_full_repack_period - housekeeping_gc_period - housekeeping_incremental_repack_period that were responsible for calling deprecated housekeeping RPCs, now we have a single housekeeping_optimize_repository_period. It's an alias attribute for housekeeping_optimize_repository_period and all the existing application will use that value to decided how often OptimizeRepository RPC must be called Changelog: removed --- app/helpers/application_settings_helper.rb | 1 + app/models/application_setting.rb | 11 +--- .../_repository_check.html.haml | 4 +- doc/api/settings.md | 9 +-- lib/api/entities/application_setting.rb | 5 ++ lib/api/settings.rb | 12 +++- spec/models/application_setting_spec.rb | 30 +--------- spec/requests/api/settings_spec.rb | 57 +++++++++++++++++++ 8 files changed, 82 insertions(+), 47 deletions(-) diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 16ccf8ad78a278..339938245a0c24 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -273,6 +273,7 @@ def visible_attributes :housekeeping_full_repack_period, :housekeeping_gc_period, :housekeeping_incremental_repack_period, + :housekeeping_optimize_repository_period, :html_emails_enabled, :import_sources, :in_product_marketing_emails_enabled, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 4bc6d531beb356..59ad0650eb33e2 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -35,6 +35,7 @@ class ApplicationSetting < ApplicationRecord belongs_to :instance_group, class_name: "Group", foreign_key: 'instance_administrators_group_id' alias_attribute :instance_group_id, :instance_administrators_group_id alias_attribute :instance_administrators_group, :instance_group + alias_attribute :housekeeping_optimize_repository_period, :housekeeping_incremental_repack_period sanitizes! :default_branch_name @@ -256,18 +257,10 @@ def self.kroki_formats_attributes presence: { message: 'Domain denylist cannot be empty if denylist is enabled.' }, if: :domain_denylist_enabled? - validates :housekeeping_incremental_repack_period, + validates :housekeeping_optimize_repository_period, presence: true, numericality: { only_integer: true, greater_than: 0 } - validates :housekeeping_full_repack_period, - presence: true, - numericality: { only_integer: true, greater_than_or_equal_to: :housekeeping_incremental_repack_period } - - validates :housekeeping_gc_period, - presence: true, - numericality: { only_integer: true, greater_than_or_equal_to: :housekeeping_full_repack_period } - validates :terminal_max_session_time, presence: true, numericality: { only_integer: true, greater_than_or_equal_to: 0 } diff --git a/app/views/admin/application_settings/_repository_check.html.haml b/app/views/admin/application_settings/_repository_check.html.haml index 477ac5ca0d6896..b67cc29f296bf0 100644 --- a/app/views/admin/application_settings/_repository_check.html.haml +++ b/app/views/admin/application_settings/_repository_check.html.haml @@ -24,8 +24,8 @@ _("Enable automatic repository housekeeping"), help_text: '%{help_text} %{help_link}'.html_safe % { help_text: help_text, help_link: help_link } .form-group - = f.label :housekeeping_incremental_repack_period, _('Optimize repository period'), class: 'label-bold' - = f.number_field :housekeeping_incremental_repack_period, class: 'form-control gl-form-input' + = f.label :housekeeping_optimize_repository_period, _('Optimize repository period'), class: 'label-bold' + = f.number_field :housekeeping_optimize_repository_period, class: 'form-control gl-form-input' .form-text.text-muted = _('Number of Git pushes after which Gitaly is asked to optimize a repository.') .sub-section diff --git a/doc/api/settings.md b/doc/api/settings.md index 77f425fa13fe9d..671db520c23e04 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -378,10 +378,11 @@ listed in the descriptions of the relevant settings. | `hide_third_party_offers` | boolean | no | Do not display offers from third parties in GitLab. | | `home_page_url` | string | no | Redirect to this URL when not logged in. | | `housekeeping_bitmaps_enabled` | boolean | no | Git pack file bitmap creation is always enabled and cannot be changed via API and UI. This API field is deprecated and always returns `true`. | -| `housekeeping_enabled` | boolean | no | (**If enabled, requires:** `housekeeping_bitmaps_enabled`, `housekeeping_full_repack_period`, `housekeeping_gc_period`, and `housekeeping_incremental_repack_period`) Enable or disable Git housekeeping. | -| `housekeeping_full_repack_period` | integer | required by: `housekeeping_enabled` | Number of Git pushes after which an incremental `git repack` is run. | -| `housekeeping_gc_period` | integer | required by: `housekeeping_enabled` | Number of Git pushes after which `git gc` is run. | -| `housekeeping_incremental_repack_period` | integer | required by: `housekeeping_enabled` | Number of Git pushes after which an incremental `git repack` is run. | +| `housekeeping_enabled` | boolean | no | (**If enabled, requires either:** `housekeeping_bitmaps_enabled`, `housekeeping_full_repack_period`, `housekeeping_gc_period`, and `housekeeping_incremental_repack_period` **or** `housekeeping_optimize_repository_period`) Enable or disable Git housekeeping. | +| `housekeeping_full_repack_period` | integer | no | **Deprecated** (use `housekeeping_optimize_repository_period` instead): Number of Git pushes after which an incremental `git repack` is run. | +| `housekeeping_gc_period` | integer | no | **Deprecated** (use `housekeeping_optimize_repository_period` instead): Number of Git pushes after which `git gc` is run. | +| `housekeeping_incremental_repack_period` | integer | no | **Deprecated** (use `housekeeping_optimize_repository_period` instead): Number of Git pushes after which an incremental `git repack` is run. | +| `housekeeping_optimize_repository_period`| integer | no | Number of Git pushes after which an incremental `git repack` is run. | | `html_emails_enabled` | boolean | no | Enable HTML emails. | | `import_sources` | array of strings | no | Sources to allow project import from, possible values: `github`, `bitbucket`, `bitbucket_server`, `gitlab`, `fogbugz`, `git`, `gitlab_project`, `gitea`, `manifest`, and `phabricator`. | | `in_product_marketing_emails_enabled` | boolean | no | Enable [in-product marketing emails](../user/profile/notifications.md#global-notification-settings). Enabled by default. | diff --git a/lib/api/entities/application_setting.rb b/lib/api/entities/application_setting.rb index db51d4380d08e7..8aace9126d645a 100644 --- a/lib/api/entities/application_setting.rb +++ b/lib/api/entities/application_setting.rb @@ -43,6 +43,11 @@ def self.exposed_attributes # This field is deprecated and always returns true expose(:housekeeping_bitmaps_enabled) { |_settings, _options| true } + + # These fields are deprecated and always returns housekeeping_optimize_repository_period value + expose(:housekeeping_full_repack_period) { |settings, _options| settings.housekeeping_optimize_repository_period } + expose(:housekeeping_gc_period) { |settings, _options| settings.housekeeping_optimize_repository_period } + expose(:housekeeping_incremental_repack_period) { |settings, _options| settings.housekeeping_optimize_repository_period } end end end diff --git a/lib/api/settings.rb b/lib/api/settings.rb index 96b900c420a822..06b576a982bbe8 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -85,9 +85,15 @@ def filter_attributes_using_license(attrs) optional :home_page_url, type: String, desc: 'We will redirect non-logged in users to this page' optional :housekeeping_enabled, type: Boolean, desc: 'Enable automatic repository housekeeping (git repack, git gc)' given housekeeping_enabled: ->(val) { val } do - requires :housekeeping_full_repack_period, type: Integer, desc: "Number of Git pushes after which a full 'git repack' is run." - requires :housekeeping_gc_period, type: Integer, desc: "Number of Git pushes after which 'git gc' is run." - requires :housekeeping_incremental_repack_period, type: Integer, desc: "Number of Git pushes after which an incremental 'git repack' is run." + optional :housekeeping_full_repack_period, type: Integer, desc: "Number of Git pushes after which a full 'git repack' is run." + optional :housekeeping_gc_period, type: Integer, desc: "Number of Git pushes after which 'git gc' is run." + optional :housekeeping_incremental_repack_period, type: Integer, desc: "Number of Git pushes after which an incremental 'git repack' is run." + + optional :housekeeping_optimize_repository_period, type: Integer, desc: "Number of Git pushes after which Gitaly is asked to optimize a repository." + + # Requires either all three deprecated attributes (housekeeping_full_repack_period, housekeeping_gc_period, housekeeping_incremental_repack_period) or housekeeping_optimize_repository_period + all_or_none_of :housekeeping_full_repack_period, :housekeeping_gc_period, :housekeeping_incremental_repack_period + exactly_one_of :housekeeping_incremental_repack_period, :housekeeping_optimize_repository_period end optional :html_emails_enabled, type: Boolean, desc: 'By default GitLab sends emails in HTML and plain text formats so mail clients can choose what format to use. Disable this option if you only want to send emails in plain text format.' optional :import_sources, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index a35a903a13b20c..5b99c68ec80d0c 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -725,35 +725,7 @@ def expect_invalid end context 'housekeeping settings' do - it { is_expected.not_to allow_value(0).for(:housekeeping_incremental_repack_period) } - - it 'wants the full repack period to be at least the incremental repack period' do - subject.housekeeping_incremental_repack_period = 2 - subject.housekeeping_full_repack_period = 1 - - expect(subject).not_to be_valid - end - - it 'wants the gc period to be at least the full repack period' do - subject.housekeeping_full_repack_period = 100 - subject.housekeeping_gc_period = 90 - - expect(subject).not_to be_valid - end - - it 'allows the same period for incremental repack and full repack, effectively skipping incremental repack' do - subject.housekeeping_incremental_repack_period = 2 - subject.housekeeping_full_repack_period = 2 - - expect(subject).to be_valid - end - - it 'allows the same period for full repack and gc, effectively skipping full repack' do - subject.housekeeping_full_repack_period = 100 - subject.housekeeping_gc_period = 100 - - expect(subject).to be_valid - end + it { is_expected.not_to allow_value(0).for(:housekeeping_optimize_repository_period) } end context 'gitaly timeouts' do diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index e89988893c27e4..4d85849cff3237 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -806,5 +806,62 @@ .to include(a_string_matching('is not a number')) end end + + context 'with housekeeping enabled' do + it 'at least one of housekeeping_incremental_repack_period or housekeeping_optimize_repository_period is required' do + put api("/application/settings", admin), params: { + housekeeping_enabled: true + } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq( + "housekeeping_incremental_repack_period, housekeeping_optimize_repository_period are missing, exactly one parameter must be provided" + ) + end + + context 'when housekeeping_incremental_repack_period is specified' do + it 'requires all three housekeeping settings' do + put api("/application/settings", admin), params: { + housekeeping_enabled: true, + housekeeping_incremental_repack_period: 10 + } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq( + "housekeeping_full_repack_period, housekeeping_gc_period, housekeeping_incremental_repack_period provide all or none of parameters" + ) + end + + it 'returns housekeeping_optimize_repository_period value for all housekeeping settings attributes' do + put api("/application/settings", admin), params: { + housekeeping_enabled: true, + housekeeping_gc_period: 150, + housekeeping_full_repack_period: 125, + housekeeping_incremental_repack_period: 100 + } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['housekeeping_optimize_repository_period']).to eq(100) + expect(json_response['housekeeping_full_repack_period']).to eq(100) + expect(json_response['housekeeping_gc_period']).to eq(100) + expect(json_response['housekeeping_incremental_repack_period']).to eq(100) + end + end + + context 'when housekeeping_optimize_repository_period is specified' do + it 'returns housekeeping_optimize_repository_period value for all housekeeping settings attributes' do + put api("/application/settings", admin), params: { + housekeeping_enabled: true, + housekeeping_optimize_repository_period: 100 + } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['housekeeping_optimize_repository_period']).to eq(100) + expect(json_response['housekeeping_full_repack_period']).to eq(100) + expect(json_response['housekeeping_gc_period']).to eq(100) + expect(json_response['housekeeping_incremental_repack_period']).to eq(100) + end + end + end end end -- GitLab