0 for unlimited, disallow blank, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/issues/27762#note_23520780

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