From 53c94f9ea25c9d6a25b58a01db5f855f0719dbf4 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Feb 2017 22:54:46 +0800 Subject: [PATCH] Use the same syntax for default expiration Feedback: * https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9219#note_23343951 * https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9219#note_23344036 * https://gitlab.com/gitlab-org/gitlab-ce/issues/27762#note_23344797 --- .../admin/application_settings_controller.rb | 2 +- app/models/application_setting.rb | 20 +++++++++---------- app/models/ci/build.rb | 12 ----------- .../application_settings/_form.html.haml | 10 +++++----- ...acts_expiration_to_application_settings.rb | 4 ++-- lib/api/settings.rb | 4 ++-- lib/ci/api/builds.rb | 5 ++++- spec/requests/ci/api/builds_spec.rb | 6 +++--- 8 files changed, 27 insertions(+), 36 deletions(-) diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 0559b5fa1bd..d807e6263ee 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -83,7 +83,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController :akismet_api_key, :akismet_enabled, :container_registry_token_expire_delay, - :default_artifacts_expiration, + :default_artifacts_expire_in, :default_branch_protection, :default_group_visibility, :default_project_visibility, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index a871dc9e5b9..2b97027c018 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -80,9 +80,7 @@ class ApplicationSetting < ActiveRecord::Base presence: true, numericality: { only_integer: true, greater_than: 0 } - validates :default_artifacts_expiration, - presence: true, - numericality: { only_integer: true, greater_than_or_equal_to: 0 } + validate :check_default_artifacts_expire_in validates :container_registry_token_expire_delay, presence: true, @@ -176,7 +174,7 @@ class ApplicationSetting < ActiveRecord::Base after_sign_up_text: nil, akismet_enabled: false, container_registry_token_expire_delay: 5, - default_artifacts_expiration: 30, + default_artifacts_expire_in: '30 days', default_branch_protection: Settings.gitlab['default_branch_protection'], default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'], default_projects_limit: Settings.gitlab['default_projects_limit'], @@ -291,12 +289,6 @@ class ApplicationSetting < ActiveRecord::Base sidekiq_throttling_enabled end - def default_artifacts_expire_in - if default_artifacts_expiration.nonzero? - "#{default_artifacts_expiration} days" - end - end - private def check_repository_storages @@ -304,4 +296,12 @@ class ApplicationSetting < ActiveRecord::Base errors.add(:repository_storages, "can't include: #{invalid.join(", ")}") unless invalid.empty? end + + def check_default_artifacts_expire_in + ChronicDuration.parse(default_artifacts_expire_in) + true + rescue ChronicDuration::DurationParseError => e + errors.add(:default_artifacts_expire_in, ": #{e.message}") + false + end end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index d14f025aeaa..8c1b076c2d7 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -513,18 +513,6 @@ module Ci end end - def set_artifacts_expire_in(expire_in) - value = - if expire_in - expire_in - else - Gitlab::CurrentSettings.current_application_settings - .default_artifacts_expire_in - end - - self.artifacts_expire_in = value - end - def has_expiring_artifacts? artifacts_expire_at.present? end diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 86ce534ea18..f96ddc38af1 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -213,14 +213,14 @@ = f.number_field :max_artifacts_size, class: 'form-control' .help-block Set the maximum file size for each job's artifacts - = link_to "(?)", help_page_path("user/admin_area/settings/continuous_integration", anchor: "maximum-artifacts-size") + = link_to icon('question-circle'), help_page_path('user/admin_area/settings/continuous_integration', anchor: 'maximum-artifacts-size') .form-group - = f.label :default_artifacts_expiration, 'Default artifacts expiration (days)', class: 'control-label col-sm-2' + = f.label :default_artifacts_expire_in, 'Default artifacts expiration', class: 'control-label col-sm-2' .col-sm-10 - = f.number_field :default_artifacts_expiration, class: 'form-control' + = f.text_field :default_artifacts_expire_in, class: 'form-control' .help-block - Set the default expiration time for each job's artifacts (0 as never expired) - = link_to "(?)", help_page_path("user/admin_area/settings/continuous_integration", anchor: "default-artifacts-expiration") + Set the default expiration time for each job's artifacts + = link_to icon('question-circle'), help_page_path('user/admin_area/settings/continuous_integration', anchor: 'default-artifacts-expiration') - if Gitlab.config.registry.enabled %fieldset diff --git a/db/migrate/20170214084746_add_default_artifacts_expiration_to_application_settings.rb b/db/migrate/20170214084746_add_default_artifacts_expiration_to_application_settings.rb index 728d581251c..34905570739 100644 --- a/db/migrate/20170214084746_add_default_artifacts_expiration_to_application_settings.rb +++ b/db/migrate/20170214084746_add_default_artifacts_expiration_to_application_settings.rb @@ -5,7 +5,7 @@ class AddDefaultArtifactsExpirationToApplicationSettings < ActiveRecord::Migrati def change add_column :application_settings, - :default_artifacts_expiration, - :integer, default: 0, null: false + :default_artifacts_expire_in, + :string, null: true end end diff --git a/lib/api/settings.rb b/lib/api/settings.rb index dabe6281755..f46e7e0bcf1 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -57,7 +57,7 @@ module API requires :shared_runners_text, type: String, desc: 'Shared runners text ' end optional :max_artifacts_size, type: Integer, desc: "Set the maximum file size for each job's artifacts" - optional :default_artifacts_expiration, type: Integer, desc: "Set the default expiration time for each job's artifacts" + optional :default_artifacts_expire_in, type: Integer, desc: "Set the default expiration time for each job's artifacts" optional :max_pages_size, type: Integer, desc: 'Maximum size of pages in MB' optional :container_registry_token_expire_delay, type: Integer, desc: 'Authorization token duration (minutes)' optional :metrics_enabled, type: Boolean, desc: 'Enable the InfluxDB metrics' @@ -119,7 +119,7 @@ module API :after_sign_up_text, :signin_enabled, :require_two_factor_authentication, :home_page_url, :after_sign_out_path, :sign_in_text, :help_page_text, :shared_runners_enabled, :max_artifacts_size, - :default_artifacts_expiration, :max_pages_size, + :default_artifacts_expire_in, :max_pages_size, :container_registry_token_expire_delay, :metrics_enabled, :sidekiq_throttling_enabled, :recaptcha_enabled, :akismet_enabled, :admin_notification_email, :sentry_enabled, diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index 7aad6c50b7b..2018191c4bd 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -167,7 +167,10 @@ module Ci build.artifacts_file = artifacts build.artifacts_metadata = metadata - build.set_artifacts_expire_in(params['expire_in']) + build.artifacts_expire_in = + params['expire_in'] || + Gitlab::CurrentSettings.current_application_settings + .default_artifacts_expire_in if build.save present(build, with: Entities::BuildDetails) diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 0f2c6f2dc69..f2385da8c13 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -630,7 +630,7 @@ describe Ci::API::Builds do context 'with an expire date' do let!(:artifacts) { file_upload } - let(:default_artifacts_expiration) { 0 } + let(:default_artifacts_expire_in) {} let(:post_data) do { 'file.path' => artifacts.path, @@ -640,7 +640,7 @@ describe Ci::API::Builds do before do stub_application_setting( - default_artifacts_expiration: default_artifacts_expiration) + default_artifacts_expire_in: default_artifacts_expire_in) post(post_url, post_data, headers_with_token) end @@ -668,7 +668,7 @@ describe Ci::API::Builds do end context 'with application default' do - let(:default_artifacts_expiration) { 5 } + let(:default_artifacts_expire_in) { '5 days' } it 'sets to application default' do build.reload