diff options
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 |