From eede4ab1a2509ef4aa14d21527386224c4116adc Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 16 Feb 2017 23:40:13 +0800
Subject: [PATCH] 0 for unlimited, disallow blank, feedback:
https://gitlab.com/gitlab-org/gitlab-ce/issues/27762#note_23520780
---
app/models/application_setting.rb | 16 +++---------
app/models/ci/build.rb | 2 +-
.../application_settings/_form.html.haml | 5 ++--
...acts_expiration_to_application_settings.rb | 4 +--
db/schema.rb | 4 +--
.../settings/continuous_integration.md | 8 +++---
spec/models/application_setting_spec.rb | 26 ++++++++++---------
spec/models/ci/build_spec.rb | 6 +++++
8 files changed, 36 insertions(+), 35 deletions(-)
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index d64a847d487..36832185b6f 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -269,14 +269,6 @@ class ApplicationSetting < ActiveRecord::Base
self.repository_storages = [value]
end
- def default_artifacts_expire_in=(value)
- if value.present?
- super(value.squish)
- else
- super(nil)
- end
- end
-
# Choose one of the available repository storage options. Currently all have
# equal weighting.
def pick_repository_storage
@@ -306,10 +298,10 @@ class ApplicationSetting < ActiveRecord::Base
end
def check_default_artifacts_expire_in
- if default_artifacts_expire_in &&
- ChronicDuration.parse(default_artifacts_expire_in).nil?
- errors.add(:default_artifacts_expiration,
- "can't be 0. Leave it blank for no expiration")
+ if default_artifacts_expire_in.blank?
+ errors.add(:default_artifacts_expiration, "is not presented")
+ else
+ ChronicDuration.parse(default_artifacts_expire_in)
end
rescue ChronicDuration::DurationParseError
errors.add(:default_artifacts_expiration, "is invalid")
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 8c1b076c2d7..60655bf2ea7 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -509,7 +509,7 @@ module Ci
def artifacts_expire_in=(value)
self.artifacts_expire_at =
if value
- Time.now + ChronicDuration.parse(value)
+ ChronicDuration.parse(value)&.seconds&.from_now
end
end
diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml
index 3004f32f1f2..12b12fd248b 100644
--- a/app/views/admin/application_settings/_form.html.haml
+++ b/app/views/admin/application_settings/_form.html.haml
@@ -219,10 +219,11 @@
.col-sm-10
= f.text_field :default_artifacts_expire_in, class: 'form-control'
.help-block
- Set the default expiration time for each job's artifacts
+ Set the default expiration time for each job's artifacts.
+ 0 for unlimited.
= surround '(', ')' do
= link_to 'syntax', help_page_path('ci/yaml/README', anchor: 'artifactsexpire_in')
- = link_to icon('question-circle'), help_page_path('user/admin_area/settings/continuous_integration', anchor: 'default-artifacts-expiration-time')
+ = 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 34905570739..e0e3ff8957a 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_expire_in,
- :string, null: true
+ :default_artifacts_expire_in, :string,
+ null: false, default: '0'
end
end
diff --git a/db/schema.rb b/db/schema.rb
index b11792d14f5..29ba7167bfa 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -111,7 +111,7 @@ ActiveRecord::Schema.define(version: 20170214111112) do
t.boolean "plantuml_enabled"
t.integer "max_pages_size", default: 100, null: false
t.integer "terminal_max_session_time", default: 0, null: false
- t.string "default_artifacts_expire_in", limit: 255
+ t.string "default_artifacts_expire_in", default: '0', null: false
end
create_table "audit_events", force: :cascade do |t|
@@ -1352,4 +1352,4 @@ ActiveRecord::Schema.define(version: 20170214111112) do
add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade
add_foreign_key "trending_projects", "projects", on_delete: :cascade
add_foreign_key "u2f_registrations", "users"
-end
\ No newline at end of file
+end
diff --git a/doc/user/admin_area/settings/continuous_integration.md b/doc/user/admin_area/settings/continuous_integration.md
index d7b598b7506..0fd165fc095 100644
--- a/doc/user/admin_area/settings/continuous_integration.md
+++ b/doc/user/admin_area/settings/continuous_integration.md
@@ -18,13 +18,13 @@ that this setting is set for each job.
[art-yml]: ../../../administration/build_artifacts
-## Default artifacts expiration time
+## Default artifacts expiration
-The default expiration time of the [build artifacts][art-yml] can be set in
+The default expiration time of the [job artifacts][art-yml] can be set in
the Admin area of your GitLab instance. The syntax of duration is described
in [artifacts:expire_in][duration-syntax]. The default is `30 days`. Note that
-this setting is set for each job. Leave it blank if you don't want to set
-default expiration time.
+this setting is set for each job. Set it to 0 if you don't want default
+expiration.
1. Go to **Admin area > Settings** (`/admin/application_settings`).
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 719eda9b5b3..f8c38ed8569 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -30,20 +30,16 @@ describe ApplicationSetting, models: true do
end
describe 'default_artifacts_expire_in' do
- it 'sets an error if it is invalid' do
+ it 'sets an error if it cannot parse' do
setting.update(default_artifacts_expire_in: 'a')
- expect(setting).to be_invalid
- expect(setting.errors.messages)
- .to have_key(:default_artifacts_expiration)
+ expect_invalid
end
- it 'does not allow 0' do
- setting.update(default_artifacts_expire_in: '0')
+ it 'sets an error if it is blank' do
+ setting.update(default_artifacts_expire_in: ' ')
- expect(setting).to be_invalid
- expect(setting.errors.messages)
- .to have_key(:default_artifacts_expiration)
+ expect_invalid
end
it 'sets the value if it is valid' do
@@ -53,11 +49,17 @@ describe ApplicationSetting, models: true do
expect(setting.default_artifacts_expire_in).to eq('30 days')
end
- it 'does not set it if it is blank' do
- setting.update(default_artifacts_expire_in: ' ')
+ it 'sets the value if it is 0' do
+ setting.update(default_artifacts_expire_in: '0')
expect(setting).to be_valid
- expect(setting.default_artifacts_expire_in).to be_nil
+ expect(setting.default_artifacts_expire_in).to eq('0')
+ end
+
+ def expect_invalid
+ expect(setting).to be_invalid
+ expect(setting.errors.messages)
+ .to have_key(:default_artifacts_expiration)
end
end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 1e88dc9d468..a4edabfbc5b 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -166,6 +166,12 @@ describe Ci::Build, :models do
is_expected.to be_nil
end
+
+ it 'when setting to 0' do
+ build.artifacts_expire_in = '0'
+
+ is_expected.to be_nil
+ end
end
describe '#commit' do
--
GitLab