Merge branch 'use-chronic-duration-attribute-for-project-build-timeout' into 'master'

Use chronic duration attribute for project build timeout

See merge request gitlab-org/gitlab-ce!17386

16 files changed, 208 insertions, 152 deletions

diff --git a/app/controllers/projects/pipelines_settings_controller.rb b/app/controllers/projects/pipelines_settings_controller.rb
index 557671ab186..73c613b26f3 100644
--- a/app/controllers/projects/pipelines_settings_controller.rb
+++ b/app/controllers/projects/pipelines_settings_controller.rb
@@ -4,41 +4,4 @@ class Projects::PipelinesSettingsController < Projects::ApplicationController
   def show
     redirect_to project_settings_ci_cd_path(@project, params: params)
   end
-
-  def update
-    Projects::UpdateService.new(project, current_user, update_params).tap do |service|
-      if service.execute
-        flash[:notice] = "Pipelines settings for '#{@project.name}' were successfully updated."
-
-        run_autodevops_pipeline(service)
-
-        redirect_to project_settings_ci_cd_path(@project)
-      else
-        render 'show'
-      end
-    end
-  end
-
-  private
-
-  def run_autodevops_pipeline(service)
-    return unless service.run_auto_devops_pipeline?
-
-    if @project.empty_repo?
-      flash[:warning] = "This repository is currently empty. A new Auto DevOps pipeline will be created after a new file has been pushed to a branch."
-      return
-    end
-
-    CreatePipelineWorker.perform_async(project.id, current_user.id, project.default_branch, :web, ignore_skip_ci: true, save_on_errors: false)
-    flash[:success] = "A new Auto DevOps pipeline has been created, go to <a href=\"#{project_pipelines_path(@project)}\">Pipelines page</a> for details".html_safe
-  end
-
-  def update_params
-    params.require(:project).permit(
-      :runners_token, :builds_enabled, :build_allow_git_fetch,
-      :build_timeout_in_minutes, :build_coverage_regex, :public_builds,
-      :auto_cancel_pending_pipelines, :ci_config_path,
-      auto_devops_attributes: [:id, :domain, :enabled]
-    )
-  end
 end
diff --git a/app/controllers/projects/refs_controller.rb b/app/controllers/projects/refs_controller.rb
index 2376f469213..48a09e1ddb8 100644
--- a/app/controllers/projects/refs_controller.rb
+++ b/app/controllers/projects/refs_controller.rb
@@ -25,7 +25,7 @@ class Projects::RefsController < Projects::ApplicationController
           when "graphs_commits"
             commits_project_graph_path(@project, @id)
           when "badges"
-            project_pipelines_settings_path(@project, ref: @id)
+            project_settings_ci_cd_path(@project, ref: @id)
           else
             project_commits_path(@project, @id)
           end
diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb
index 96125b549b7..d80ef8113aa 100644
--- a/app/controllers/projects/settings/ci_cd_controller.rb
+++ b/app/controllers/projects/settings/ci_cd_controller.rb
@@ -2,13 +2,24 @@ module Projects
   module Settings
     class CiCdController < Projects::ApplicationController
       before_action :authorize_admin_pipeline!
+      before_action :define_variables
 
       def show
-        define_runners_variables
-        define_secret_variables
-        define_triggers_variables
-        define_badges_variables
-        define_auto_devops_variables
+      end
+
+      def update
+        Projects::UpdateService.new(project, current_user, update_params).tap do |service|
+          result = service.execute
+          if result[:status] == :success
+            flash[:notice] = "Pipelines settings for '#{@project.name}' were successfully updated."
+
+            run_autodevops_pipeline(service)
+
+            redirect_to project_settings_ci_cd_path(@project)
+          else
+            render 'show'
+          end
+        end
       end
 
       def reset_cache
@@ -25,6 +36,35 @@ module Projects
 
       private
 
+      def update_params
+        params.require(:project).permit(
+          :runners_token, :builds_enabled, :build_allow_git_fetch,
+          :build_timeout_human_readable, :build_coverage_regex, :public_builds,
+          :auto_cancel_pending_pipelines, :ci_config_path,
+          auto_devops_attributes: [:id, :domain, :enabled]
+        )
+      end
+
+      def run_autodevops_pipeline(service)
+        return unless service.run_auto_devops_pipeline?
+
+        if @project.empty_repo?
+          flash[:warning] = "This repository is currently empty. A new Auto DevOps pipeline will be created after a new file has been pushed to a branch."
+          return
+        end
+
+        CreatePipelineWorker.perform_async(project.id, current_user.id, project.default_branch, :web, ignore_skip_ci: true, save_on_errors: false)
+        flash[:success] = "A new Auto DevOps pipeline has been created, go to <a href=\"#{project_pipelines_path(@project)}\">Pipelines page</a> for details".html_safe
+      end
+
+      def define_variables
+        define_runners_variables
+        define_secret_variables
+        define_triggers_variables
+        define_badges_variables
+        define_auto_devops_variables
+      end
+
       def define_runners_variables
         @project_runners = @project.runners.ordered
         @assignable_runners = current_user.ci_authorized_runners
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index ee197c75764..37f14230196 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -324,7 +324,7 @@ class ProjectsController < Projects::ApplicationController
       :avatar,
       :build_allow_git_fetch,
       :build_coverage_regex,
-      :build_timeout_in_minutes,
+      :build_timeout_human_readable,
       :resolve_outdated_diff_discussions,
       :container_registry_enabled,
       :default_branch,
diff --git a/app/models/concerns/chronic_duration_attribute.rb b/app/models/concerns/chronic_duration_attribute.rb
index fa1eafb1d7a..593a9b3d71d 100644
--- a/app/models/concerns/chronic_duration_attribute.rb
+++ b/app/models/concerns/chronic_duration_attribute.rb
@@ -8,14 +8,14 @@ module ChronicDurationAttribute
       end
     end
 
-    def chronic_duration_attr_writer(virtual_attribute, source_attribute)
+    def chronic_duration_attr_writer(virtual_attribute, source_attribute, parameters = {})
       chronic_duration_attr_reader(virtual_attribute, source_attribute)
 
       define_method("#{virtual_attribute}=") do |value|
-        chronic_duration_attributes[virtual_attribute] = value.presence || ''
+        chronic_duration_attributes[virtual_attribute] = value.presence || parameters[:default].presence.to_s
 
         begin
-          new_value = ChronicDuration.parse(value).to_i if value.present?
+          new_value = value.present? ? ChronicDuration.parse(value).to_i : parameters[:default].presence
           assign_attributes(source_attribute => new_value)
         rescue ChronicDuration::DurationParseError
           # ignore error as it will be caught by validation
diff --git a/app/models/project.rb b/app/models/project.rb
index 32289106f28..ea9df9b10ef 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -21,6 +21,7 @@ class Project < ActiveRecord::Base
   include Gitlab::SQL::Pattern
   include DeploymentPlatform
   include ::Gitlab::Utils::StrongMemoize
+  include ChronicDurationAttribute
 
   extend Gitlab::ConfigHelper
 
@@ -325,6 +326,12 @@ class Project < ActiveRecord::Base
 
   enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 }
 
+  chronic_duration_attr :build_timeout_human_readable, :build_timeout, default: 3600
+
+  validates :build_timeout, allow_nil: true,
+                            numericality: { greater_than_or_equal_to: 600,
+                                            message: 'needs to be at least 10 minutes' }
+
   # Returns a collection of projects that is either public or visible to the
   # logged in user.
   def self.public_or_visible_to_user(user = nil)
@@ -1309,14 +1316,6 @@ class Project < ActiveRecord::Base
     self.runners_token && ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.runners_token)
   end
 
-  def build_timeout_in_minutes
-    build_timeout / 60
-  end
-
-  def build_timeout_in_minutes=(value)
-    self.build_timeout = value.to_i * 60
-  end
-
   def open_issues_count
     Projects::OpenIssuesCountService.new(self).count
   end
diff --git a/app/views/projects/pipelines_settings/_badge.html.haml b/app/views/projects/settings/ci_cd/_badge.html.haml
index e8028059487..e8028059487 100644
--- a/app/views/projects/pipelines_settings/_badge.html.haml
+++ b/app/views/projects/settings/ci_cd/_badge.html.haml
diff --git a/app/views/projects/pipelines_settings/_show.html.haml b/app/views/projects/settings/ci_cd/_form.html.haml
index 63a1f25bfec..20868f9ba5d 100644
--- a/app/views/projects/pipelines_settings/_show.html.haml
+++ b/app/views/projects/settings/ci_cd/_form.html.haml
@@ -1,6 +1,7 @@
 .row.prepend-top-default
   .col-lg-12
-    = form_for @project, url: project_pipelines_settings_path(@project) do |f|
+    = form_for @project, url: project_settings_ci_cd_path(@project) do |f|
+      = form_errors(@project)
       %fieldset.builds-feature
         .form-group
           %h5 Auto DevOps (Beta)
@@ -73,10 +74,10 @@
 
         %hr
         .form-group
-          = f.label :build_timeout_in_minutes, 'Timeout', class: 'label-light'
-          = f.number_field :build_timeout_in_minutes, class: 'form-control', min: '0'
+          = f.label :build_timeout_human_readable, 'Timeout', class: 'label-light'
+          = f.text_field :build_timeout_human_readable, class: 'form-control'
           %p.help-block
-            Per job in minutes. If a job passes this threshold, it will be marked as failed
+            Per job. If a job passes this threshold, it will be marked as failed
             = link_to icon('question-circle'), help_page_path('user/project/pipelines/settings', anchor: 'timeout'), target: '_blank'
 
         %hr
@@ -160,4 +161,4 @@
 %hr
 
 .row.prepend-top-default
-  = render partial: 'projects/pipelines_settings/badge', collection: @badges
+  = render partial: 'badge', collection: @badges
diff --git a/app/views/projects/settings/ci_cd/show.html.haml b/app/views/projects/settings/ci_cd/show.html.haml
index d65341dbd40..09268c9943b 100644
--- a/app/views/projects/settings/ci_cd/show.html.haml
+++ b/app/views/projects/settings/ci_cd/show.html.haml
@@ -3,8 +3,9 @@
 - page_title "CI / CD"
 
 - expanded = Rails.env.test?
+- general_expanded = @project.errors.empty? ? expanded : true
 
-%section.settings#js-general-pipeline-settings.no-animate{ class: ('expanded' if expanded) }
+%section.settings#js-general-pipeline-settings.no-animate{ class: ('expanded' if general_expanded) }
   .settings-header
     %h4
       General pipelines settings
@@ -13,7 +14,7 @@
     %p
       Update your CI/CD configuration, like job timeout or Auto DevOps.
   .settings-content
-    = render 'projects/pipelines_settings/show'
+    = render 'form'
 
 %section.settings.no-animate{ class: ('expanded' if expanded) }
   .settings-header
diff --git a/changelogs/unreleased/use-chronic-duration-attribute-for-project-build-timeout.yml b/changelogs/unreleased/use-chronic-duration-attribute-for-project-build-timeout.yml
new file mode 100644
index 00000000000..675d347b64c
--- /dev/null
+++ b/changelogs/unreleased/use-chronic-duration-attribute-for-project-build-timeout.yml
@@ -0,0 +1,5 @@
+---
+title: Use human readable value build_timeout in Project
+merge_request: 17386
+author:
+type: changed
diff --git a/config/routes/project.rb b/config/routes/project.rb
index 48ba8ef06f9..0f2ea1c01d1 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -420,7 +420,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       end
       namespace :settings do
         get :members, to: redirect("%{namespace_id}/%{project_id}/project_members")
-        resource :ci_cd, only: [:show], controller: 'ci_cd' do
+        resource :ci_cd, only: [:show, :update], controller: 'ci_cd' do
           post :reset_cache
         end
         resource :integrations, only: [:show]
diff --git a/spec/controllers/projects/pipelines_settings_controller_spec.rb b/spec/controllers/projects/pipelines_settings_controller_spec.rb
index 913b9bd804a..694896b6bcf 100644
--- a/spec/controllers/projects/pipelines_settings_controller_spec.rb
+++ b/spec/controllers/projects/pipelines_settings_controller_spec.rb
@@ -11,82 +11,11 @@ describe Projects::PipelinesSettingsController do
     sign_in(user)
   end
 
-  describe 'PATCH update' do
-    subject do
-      patch :update,
-        namespace_id: project.namespace.to_param,
-        project_id: project,
-        project: {
-          auto_devops_attributes: params
-        }
-    end
-
-    context 'when updating the auto_devops settings' do
-      let(:params) { { enabled: '', domain: 'mepmep.md' } }
-
-      it 'redirects to the settings page' do
-        subject
-
-        expect(response).to have_gitlab_http_status(302)
-        expect(flash[:notice]).to eq("Pipelines settings for '#{project.name}' were successfully updated.")
-      end
-
-      context 'following the instance default' do
-        let(:params) { { enabled: '' } }
-
-        it 'allows enabled to be set to nil' do
-          subject
-          project_auto_devops.reload
-
-          expect(project_auto_devops.enabled).to be_nil
-        end
-      end
-
-      context 'when run_auto_devops_pipeline is true' do
-        before do
-          expect_any_instance_of(Projects::UpdateService).to receive(:run_auto_devops_pipeline?).and_return(true)
-        end
-
-        context 'when the project repository is empty' do
-          it 'sets a warning flash' do
-            expect(subject).to set_flash[:warning]
-          end
-
-          it 'does not queue a CreatePipelineWorker' do
-            expect(CreatePipelineWorker).not_to receive(:perform_async).with(project.id, user.id, project.default_branch, :web, any_args)
-
-            subject
-          end
-        end
-
-        context 'when the project repository is not empty' do
-          let(:project) { create(:project, :repository) }
-
-          it 'sets a success flash' do
-            allow(CreatePipelineWorker).to receive(:perform_async).with(project.id, user.id, project.default_branch, :web, any_args)
-
-            expect(subject).to set_flash[:success]
-          end
-
-          it 'queues a CreatePipelineWorker' do
-            expect(CreatePipelineWorker).to receive(:perform_async).with(project.id, user.id, project.default_branch, :web, any_args)
-
-            subject
-          end
-        end
-      end
-
-      context 'when run_auto_devops_pipeline is not true' do
-        before do
-          expect_any_instance_of(Projects::UpdateService).to receive(:run_auto_devops_pipeline?).and_return(false)
-        end
-
-        it 'does not queue a CreatePipelineWorker' do
-          expect(CreatePipelineWorker).not_to receive(:perform_async).with(project.id, user.id, :web, any_args)
+  describe 'GET show' do
+    it 'redirects with 302 status code' do
+      get :show, namespace_id: project.namespace, project_id: project
 
-          subject
-        end
-      end
+      expect(response).to have_gitlab_http_status(302)
     end
   end
 end
diff --git a/spec/controllers/projects/settings/ci_cd_controller_spec.rb b/spec/controllers/projects/settings/ci_cd_controller_spec.rb
index 293e76798ae..7dae9b85d78 100644
--- a/spec/controllers/projects/settings/ci_cd_controller_spec.rb
+++ b/spec/controllers/projects/settings/ci_cd_controller_spec.rb
@@ -1,8 +1,9 @@
 require('spec_helper')
 
 describe Projects::Settings::CiCdController do
-  let(:project) { create(:project, :public, :access_requestable) }
-  let(:user) { create(:user) }
+  set(:user) { create(:user) }
+  set(:project_auto_devops) { create(:project_auto_devops) }
+  let(:project) { project_auto_devops.project }
 
   before do
     project.add_master(user)
@@ -55,4 +56,107 @@ describe Projects::Settings::CiCdController do
       end
     end
   end
+
+  describe 'PATCH update' do
+    let(:params) { { ci_config_path: '' } }
+
+    subject do
+      patch :update,
+            namespace_id: project.namespace.to_param,
+            project_id: project,
+            project: params
+    end
+
+    it 'redirects to the settings page' do
+      subject
+
+      expect(response).to have_gitlab_http_status(302)
+      expect(flash[:notice]).to eq("Pipelines settings for '#{project.name}' were successfully updated.")
+    end
+
+    context 'when updating the auto_devops settings' do
+      let(:params) { { auto_devops_attributes: { enabled: '', domain: 'mepmep.md' } } }
+
+      context 'following the instance default' do
+        let(:params) { { auto_devops_attributes: { enabled: '' } } }
+
+        it 'allows enabled to be set to nil' do
+          subject
+          project_auto_devops.reload
+
+          expect(project_auto_devops.enabled).to be_nil
+        end
+      end
+
+      context 'when run_auto_devops_pipeline is true' do
+        before do
+          expect_any_instance_of(Projects::UpdateService).to receive(:run_auto_devops_pipeline?).and_return(true)
+        end
+
+        context 'when the project repository is empty' do
+          it 'sets a warning flash' do
+            expect(subject).to set_flash[:warning]
+          end
+
+          it 'does not queue a CreatePipelineWorker' do
+            expect(CreatePipelineWorker).not_to receive(:perform_async).with(project.id, user.id, project.default_branch, :web, any_args)
+
+            subject
+          end
+        end
+
+        context 'when the project repository is not empty' do
+          let(:project) { create(:project, :repository) }
+
+          it 'sets a success flash' do
+            allow(CreatePipelineWorker).to receive(:perform_async).with(project.id, user.id, project.default_branch, :web, any_args)
+
+            expect(subject).to set_flash[:success]
+          end
+
+          it 'queues a CreatePipelineWorker' do
+            expect(CreatePipelineWorker).to receive(:perform_async).with(project.id, user.id, project.default_branch, :web, any_args)
+
+            subject
+          end
+        end
+      end
+
+      context 'when run_auto_devops_pipeline is not true' do
+        before do
+          expect_any_instance_of(Projects::UpdateService).to receive(:run_auto_devops_pipeline?).and_return(false)
+        end
+
+        it 'does not queue a CreatePipelineWorker' do
+          expect(CreatePipelineWorker).not_to receive(:perform_async).with(project.id, user.id, :web, any_args)
+
+          subject
+        end
+      end
+    end
+
+    context 'when updating general settings' do
+      context 'when build_timeout_human_readable is not specified' do
+        let(:params) { { build_timeout_human_readable: '' } }
+
+        it 'set default timeout' do
+          subject
+
+          project.reload
+          expect(project.build_timeout).to eq(3600)
+        end
+      end
+
+      context 'when build_timeout_human_readable is specified' do
+        let(:params) { { build_timeout_human_readable: '1h 30m' } }
+
+        it 'set specified timeout' do
+          subject
+
+          project.reload
+          expect(project.build_timeout).to eq(5400)
+        end
+      end
+    end
+  end
 end
diff --git a/spec/features/projects/badges/list_spec.rb b/spec/features/projects/badges/list_spec.rb
index c705e479690..0abef4bc447 100644
--- a/spec/features/projects/badges/list_spec.rb
+++ b/spec/features/projects/badges/list_spec.rb
@@ -6,7 +6,7 @@ feature 'list of badges' do
     project = create(:project, :repository)
     project.add_master(user)
     sign_in(user)
-    visit project_pipelines_settings_path(project)
+    visit project_settings_ci_cd_path(project)
   end
 
   scenario 'user wants to see build status badge' do
diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb
index 27c86e60e60..8847623f705 100644
--- a/spec/models/concerns/chronic_duration_attribute_spec.rb
+++ b/spec/models/concerns/chronic_duration_attribute_spec.rb
@@ -63,8 +63,8 @@ shared_examples 'ChronicDurationAttribute writer' do
       subject.send("#{virtual_field}=", '')
     end
 
-    it 'writes nil' do
-      expect(subject.send(source_field)).to be_nil
+    it 'writes default value' do
+      expect(subject.send(source_field)).to eq(default_value)
     end
 
     it 'passes validation' do
@@ -77,8 +77,8 @@ shared_examples 'ChronicDurationAttribute writer' do
       subject.send("#{virtual_field}=", nil)
     end
 
-    it 'writes nil' do
-      expect(subject.send(source_field)).to be_nil
+    it 'writes default value' do
+      expect(subject.send(source_field)).to eq(default_value)
     end
 
     it 'passes validation' do
@@ -92,20 +92,34 @@ shared_examples 'ChronicDurationAttribute writer' do
 end
 
 describe 'ChronicDurationAttribute' do
-  let(:source_field) {:maximum_timeout}
-  let(:virtual_field) {:maximum_timeout_human_readable}
+  context 'when default value is not set' do
+    let(:source_field) {:maximum_timeout}
+    let(:virtual_field) {:maximum_timeout_human_readable}
+    let(:default_value) { nil }
 
-  subject { Ci::Runner.new }
+    subject { create(:ci_runner) }
 
-  it_behaves_like 'ChronicDurationAttribute reader'
-  it_behaves_like 'ChronicDurationAttribute writer'
+    it_behaves_like 'ChronicDurationAttribute reader'
+    it_behaves_like 'ChronicDurationAttribute writer'
+  end
+
+  context 'when default value is set' do
+    let(:source_field) {:build_timeout}
+    let(:virtual_field) {:build_timeout_human_readable}
+    let(:default_value) { 3600 }
+
+    subject { create(:project) }
+
+    it_behaves_like 'ChronicDurationAttribute reader'
+    it_behaves_like 'ChronicDurationAttribute writer'
+  end
 end
 
 describe 'ChronicDurationAttribute - reader' do
   let(:source_field) {:timeout}
   let(:virtual_field) {:timeout_human_readable}
 
-  subject {Ci::BuildMetadata.new}
+  subject { create(:ci_build).ensure_metadata }
 
   it "doesn't contain dynamically created writer method" do
     expect(subject.class).not_to be_public_method_defined("#{virtual_field}=")
diff --git a/spec/views/projects/pipelines_settings/_show.html.haml_spec.rb b/spec/views/projects/settings/ci_cd/_form.html.haml_spec.rb
index 7b300150874..be9a4d9c57c 100644
--- a/spec/views/projects/pipelines_settings/_show.html.haml_spec.rb
+++ b/spec/views/projects/settings/ci_cd/_form.html.haml_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe 'projects/pipelines_settings/_show' do
+describe 'projects/settings/ci_cd/_form' do
   let(:project) { create(:project, :repository) }
 
   before do