From fb0dec4e00f1efd637692982ba031f479103cc35 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 19 Feb 2018 16:48:01 +0100 Subject: Add new column to ci_runners table --- .../20180219153455_add_job_upper_timeout_to_ci_runners.rb | 13 +++++++++++++ db/schema.rb | 1 + 2 files changed, 14 insertions(+) create mode 100644 db/migrate/20180219153455_add_job_upper_timeout_to_ci_runners.rb diff --git a/db/migrate/20180219153455_add_job_upper_timeout_to_ci_runners.rb b/db/migrate/20180219153455_add_job_upper_timeout_to_ci_runners.rb new file mode 100644 index 00000000000..418ec7ac1d9 --- /dev/null +++ b/db/migrate/20180219153455_add_job_upper_timeout_to_ci_runners.rb @@ -0,0 +1,13 @@ +class AddJobUpperTimeoutToCiRunners < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + add_column :ci_runners, :job_upper_timeout, :integer + end + + def down + remove_column :ci_runners, :job_upper_timeout + end +end diff --git a/db/schema.rb b/db/schema.rb index 3bf42080870..413f42df40b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -459,6 +459,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do t.boolean "locked", default: false, null: false t.integer "access_level", default: 0, null: false t.string "ip_address" + t.integer "job_upper_timeout" end add_index "ci_runners", ["contacted_at"], name: "index_ci_runners_on_contacted_at", using: :btree -- cgit v1.2.1 From 7b82f4bab1661d7f7e7cb044730c977329275240 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 19 Feb 2018 17:21:00 +0100 Subject: Add support for job_upper_timeout in API --- doc/api/runners.md | 7 +++++-- lib/api/entities.rb | 1 + lib/api/runner.rb | 3 ++- lib/api/runners.rb | 1 + spec/requests/api/runner_spec.rb | 10 ++++++++++ spec/requests/api/runners_spec.rb | 5 ++++- 6 files changed, 23 insertions(+), 4 deletions(-) diff --git a/doc/api/runners.md b/doc/api/runners.md index 7495c6cdedb..4c6fc029a66 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -153,7 +153,8 @@ Example response: "mysql" ], "version": null, - "access_level": "ref_protected" + "access_level": "ref_protected", + "job_upper_timeout": 3600 } ``` @@ -174,6 +175,7 @@ PUT /runners/:id | `run_untagged` | boolean | no | Flag indicating the runner can execute untagged jobs | | `locked` | boolean | no | Flag indicating the runner is locked | | `access_level` | string | no | The access_level of the runner; `not_protected` or `ref_protected` | +| `job_upper_timeout` | integer | no | Upper timeout set when this Runner will handle the job | ``` curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/runners/6" --form "description=test-1-20150125-test" --form "tag_list=ruby,mysql,tag1,tag2" @@ -211,7 +213,8 @@ Example response: "tag2" ], "version": null, - "access_level": "ref_protected" + "access_level": "ref_protected", + "job_upper_timeout": null } ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 16147ee90c9..becd4f22a03 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -951,6 +951,7 @@ module API expose :tag_list expose :run_untagged expose :locked + expose :job_upper_timeout expose :access_level expose :version, :revision, :platform, :architecture expose :contacted_at diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 8da97a97754..14dd3b81dd0 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -14,9 +14,10 @@ module API optional :locked, type: Boolean, desc: 'Should Runner be locked for current project' optional :run_untagged, type: Boolean, desc: 'Should Runner handle untagged jobs' optional :tag_list, type: Array[String], desc: %q(List of Runner's tags) + optional :job_upper_timeout, type: Integer, desc: 'Upper timeout set when this Runner will handle the job' end post '/' do - attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list]) + attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list, :job_upper_timeout]) .merge(get_runner_details_from_request) runner = diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 996457c5dfe..bc91b7cfd54 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -57,6 +57,7 @@ module API optional :locked, type: Boolean, desc: 'Flag indicating the runner is locked' optional :access_level, type: String, values: Ci::Runner.access_levels.keys, desc: 'The access_level of the runner' + optional :job_upper_timeout, type: Integer, desc: 'Upper timeout set when this Runner will handle the job' at_least_one_of :description, :active, :tag_list, :run_untagged, :locked, :access_level end put ':id' do diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index f3dd121faa9..8c8f4bb7018 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -109,6 +109,16 @@ describe API::Runner do end end + context 'when job upper timeout is specified' do + it 'creates runner' do + post api('/runners'), token: registration_token, + job_upper_timeout: 7200 + + expect(response).to have_gitlab_http_status 201 + expect(Ci::Runner.first.job_upper_timeout).to eq(7200) + end + end + %w(name version revision platform architecture).each do |param| context "when info parameter '#{param}' info is present" do let(:value) { "#{param}_value" } diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index ec5cad4f4fd..0444880a300 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -123,6 +123,7 @@ describe API::Runners do expect(response).to have_gitlab_http_status(200) expect(json_response['description']).to eq(shared_runner.description) + expect(json_response['job_upper_timeout']).to be_nil end end @@ -192,7 +193,8 @@ describe API::Runners do tag_list: ['ruby2.1', 'pgsql', 'mysql'], run_untagged: 'false', locked: 'true', - access_level: 'ref_protected') + access_level: 'ref_protected', + job_upper_timeout: 1234 ) shared_runner.reload expect(response).to have_gitlab_http_status(200) @@ -204,6 +206,7 @@ describe API::Runners do expect(shared_runner.ref_protected?).to be_truthy expect(shared_runner.ensure_runner_queue_value) .not_to eq(runner_queue_value) + expect(shared_runner.job_upper_timeout).to eq(1234) end end -- cgit v1.2.1 From 834f473821b816515504abb7c6bc91ab2dee4450 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 19 Feb 2018 18:23:12 +0100 Subject: Override project-defined timeout with runner-defined one --- app/models/ci/build.rb | 7 +++++++ app/models/ci/runner.rb | 4 ++++ spec/models/ci/build_spec.rb | 24 +++++++++++++++++++++++- spec/models/ci/runner_spec.rb | 18 ++++++++++++++++++ spec/requests/api/runner_spec.rb | 35 +++++++++++++++++++++++++++++++++++ 5 files changed, 87 insertions(+), 1 deletion(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 08bb5915d10..e15c4bc6ceb 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -232,9 +232,16 @@ module Ci end def timeout + return runner.job_upper_timeout if should_use_runner_timeout + project.build_timeout end + def should_use_runner_timeout + runner && runner.defines_job_upper_timeout? && runner.job_upper_timeout < project.build_timeout + end + private :should_use_runner_timeout + def triggered_by?(current_user) user == current_user end diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 7173f88f1c7..b28e892dcb6 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -167,6 +167,10 @@ module Ci end end + def defines_job_upper_timeout? + job_upper_timeout && job_upper_timeout > 0 + end + private def cleanup_runner_queue diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 7d935cf8d76..d13421a107f 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1272,8 +1272,30 @@ describe Ci::Build do describe 'project settings' do describe '#timeout' do + set(:project2) { create(:project, :repository, group: group, build_timeout: 1000) } + set(:pipeline2) { create(:ci_pipeline, project: project2, sha: project2.commit.id, ref: project2.default_branch, status: 'success') } + let(:build) { create(:ci_build, :manual, pipeline: pipeline2) } + it 'returns project timeout configuration' do - expect(build.timeout).to eq(project.build_timeout) + expect(build.timeout).to eq(project2.build_timeout) + end + + context 'when runner sets timeout to bigger value' do + let(:runner2) { create(:ci_runner, job_upper_timeout: 2000) } + let(:build) { create(:ci_build, :manual, pipeline: pipeline2, runner: runner2) } + + it 'returns project timeout configuration' do + expect(build.timeout).to eq(project2.build_timeout) + end + end + + context 'when runner sets timeout to smaller value' do + let(:runner2) { create(:ci_runner, job_upper_timeout: 500) } + let(:build) { create(:ci_build, :manual, pipeline: pipeline2, runner: runner2) } + + it 'returns project timeout configuration' do + expect(build.timeout).to eq(runner2.job_upper_timeout) + end end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index ab170e6351c..80d7cd92fdb 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -556,6 +556,24 @@ describe Ci::Runner do end end + describe '#defines_job_upper_timeout?' do + context 'when job upper timeout is specified' do + subject { create(:ci_runner, job_upper_timeout: 1234) } + + it 'should return true' do + expect(subject.defines_job_upper_timeout?).to be_truthy + end + end + + context 'when job upper timeout is not specified' do + subject { create(:ci_runner) } + + it 'should return false' do + expect(subject.defines_job_upper_timeout?).to be_falsey + end + end + end + describe '.search' do let(:runner) { create(:ci_runner, token: '123abc', description: 'test runner') } diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 8c8f4bb7018..3eb0e88d095 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -658,6 +658,41 @@ describe API::Runner do end end end + + describe 'timeout support' do + context 'when project specifies job timeout' do + let(:project) { create(:project, shared_runners_enabled: false, build_timeout: 1234) } + + it 'contains info about timeout taken from project' do + request_job + + expect(response).to have_gitlab_http_status(201) + expect(json_response['runner_info']).to include({ 'timeout' => 1234 }) + end + + context 'when runner specifies lower timeout' do + let(:runner) { create(:ci_runner, job_upper_timeout: 1000) } + + it 'contains info about timeout overridden by runner' do + request_job + + expect(response).to have_gitlab_http_status(201) + expect(json_response['runner_info']).to include({ 'timeout' => 1000 }) + end + end + + context 'when runner specifies bigger timeout' do + let(:runner) { create(:ci_runner, job_upper_timeout: 2000) } + + it 'contains info about timeout not overridden by runner' do + request_job + + expect(response).to have_gitlab_http_status(201) + expect(json_response['runner_info']).to include({ 'timeout' => 1234 }) + end + end + end + end end def request_job(token = runner.token, **params) -- cgit v1.2.1 From b6d26f979c29ce594a8645b81ec0fd1028e79b64 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 19 Feb 2018 18:41:50 +0100 Subject: Add UI support for per-runner job timeout --- app/models/ci/runner.rb | 2 +- app/views/admin/runners/_runner.html.haml | 5 +++++ app/views/admin/runners/index.html.haml | 1 + app/views/projects/runners/_form.html.haml | 6 ++++++ app/views/projects/runners/_runner.html.haml | 2 ++ app/views/projects/runners/show.html.haml | 3 +++ 6 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index b28e892dcb6..be15fb0f729 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -8,7 +8,7 @@ module Ci ONLINE_CONTACT_TIMEOUT = 1.hour UPDATE_DB_RUNNER_INFO_EVERY = 40.minutes AVAILABLE_SCOPES = %w[specific shared active paused online].freeze - FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level].freeze + FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level job_upper_timeout].freeze has_many :builds has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml index e1cee584929..14aee600809 100644 --- a/app/views/admin/runners/_runner.html.haml +++ b/app/views/admin/runners/_runner.html.haml @@ -18,6 +18,11 @@ = runner.version %td = runner.ip_address + %td + - if runner.defines_job_upper_timeout? + = runner.job_upper_timeout + - else + n/a %td - if runner.shared? n/a diff --git a/app/views/admin/runners/index.html.haml b/app/views/admin/runners/index.html.haml index 9f13dbbbd82..95fd7fe7ebe 100644 --- a/app/views/admin/runners/index.html.haml +++ b/app/views/admin/runners/index.html.haml @@ -61,6 +61,7 @@ %th Description %th Version %th IP Address + %th Timeout %th Projects %th Jobs %th Tags diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 49c90869146..4fb4323dab4 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -39,6 +39,12 @@ Description .col-sm-10 = f.text_field :description, class: 'form-control' + .form-group + = label_tag :job_upper_timeout, class: 'control-label' do + Job upper timeout + .col-sm-10 + = f.text_field :job_upper_timeout, class: 'form-control' + .help-block This timeout will take precedence when lower than Project-defined timeout .form-group = label_tag :tag_list, class: 'control-label' do Tags diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index 6376496ee1a..e3107fecfad 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -36,6 +36,8 @@ - if runner.description.present? %p.runner-description = runner.description + - if runner.defines_job_upper_timeout? + %p Job upper timeout: #{runner.job_upper_timeout} - if runner.tag_list.present? %p - runner.tag_list.sort.each do |tag| diff --git a/app/views/projects/runners/show.html.haml b/app/views/projects/runners/show.html.haml index 4e57f5f844d..e0223eeb729 100644 --- a/app/views/projects/runners/show.html.haml +++ b/app/views/projects/runners/show.html.haml @@ -55,6 +55,9 @@ %tr %td Description %td= @runner.description + %tr + %td Job upper timeout + %td= @runner.job_upper_timeout %tr %td Last contact %td -- cgit v1.2.1 From 3c23cefae0a323887503101d3c227a914dd8f7c4 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 19 Feb 2018 19:18:06 +0100 Subject: Add CHANGELOG entry --- changelogs/unreleased/add-per-runner-job-timeout.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/add-per-runner-job-timeout.yml diff --git a/changelogs/unreleased/add-per-runner-job-timeout.yml b/changelogs/unreleased/add-per-runner-job-timeout.yml new file mode 100644 index 00000000000..336b4d15ddf --- /dev/null +++ b/changelogs/unreleased/add-per-runner-job-timeout.yml @@ -0,0 +1,5 @@ +--- +title: Add per-runner configured job timeout +merge_request: 17221 +author: +type: added -- cgit v1.2.1 From a4ea9a93db98461479dcb8e1d7b8425a77018f1e Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Feb 2018 01:09:59 +0100 Subject: Add ChroniDurationAttribute concern --- app/models/ci/runner.rb | 3 +++ app/models/concerns/chronic_duration_attribute.rb | 25 +++++++++++++++++++ .../concerns/chronic_duration_attribute_spec.rb | 28 ++++++++++++++++++++++ 3 files changed, 56 insertions(+) create mode 100644 app/models/concerns/chronic_duration_attribute.rb create mode 100644 spec/models/concerns/chronic_duration_attribute_spec.rb diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index be15fb0f729..cf91ed3c9dc 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -3,6 +3,7 @@ module Ci extend Gitlab::Ci::Model include Gitlab::SQL::Pattern include RedisCacheable + include ChronicDurationAttribute RUNNER_QUEUE_EXPIRY_TIME = 60.minutes ONLINE_CONTACT_TIMEOUT = 1.hour @@ -51,6 +52,8 @@ module Ci cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at, :ip_address + chronic_duration_attribute :job_upper_timeout_user_readable, :job_upper_timeout + # Searches for runners matching the given query. # # This method uses ILIKE on PostgreSQL and LIKE on MySQL. diff --git a/app/models/concerns/chronic_duration_attribute.rb b/app/models/concerns/chronic_duration_attribute.rb new file mode 100644 index 00000000000..2bf33174640 --- /dev/null +++ b/app/models/concerns/chronic_duration_attribute.rb @@ -0,0 +1,25 @@ +module ChronicDurationAttribute + extend ActiveSupport::Concern + + class_methods do + def chronic_duration_attribute(virtual_attribute, source_attribute) + chronic_duration_attribute_reader(virtual_attribute, source_attribute) + chronic_duration_attribute_writer(virtual_attribute, source_attribute) + end + + def chronic_duration_attribute_reader(virtual_attribute, source_attribute) + define_method(virtual_attribute) do + value = self.send(source_attribute) # rubocop:disable GitlabSecurity/PublicSend + ChronicDuration.output(value, format: :short) unless value.nil? + end + end + + def chronic_duration_attribute_writer(virtual_attribute, source_attribute) + define_method("#{virtual_attribute}=") do |value| + new_value = ChronicDuration.parse(value).to_i + self.send("#{source_attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend + new_value + end + end + end +end diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb new file mode 100644 index 00000000000..1a352537aaf --- /dev/null +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' + +shared_examples 'ChronicDurationAttribute' do + describe 'dynamically defined methods' do + it { expect(subject.class).to be_public_method_defined(virtual_field) } + it { expect(subject.class).to be_public_method_defined("#{virtual_field}=") } + + it 'parses chronic duration input' do + subject.send("#{virtual_field}=", "10m") + + expect(subject.send(source_field)).to eq(600) + end + + it 'outputs chronic duration formated value' do + subject.send("#{source_field}=", 120) + + expect(subject.send(virtual_field)).to eq('2m') + end + end +end + +describe 'ChronicDurationAttribute' do + let(:source_field) { :maximum_job_timeout } + let(:virtual_field) { :maximum_job_timeout_user_readable } + subject { Ci::Runner.new } + + it_behaves_like 'ChronicDurationAttribute' +end -- cgit v1.2.1 From d633bc8134fe472137fb668c1eb78de45dc9bb57 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Feb 2018 01:21:59 +0100 Subject: Rename job_upper_timeout to maximum_job_timeout --- app/models/ci/build.rb | 4 ++-- app/models/ci/runner.rb | 8 ++++---- app/views/admin/runners/_runner.html.haml | 4 ++-- app/views/admin/runners/index.html.haml | 2 +- app/views/projects/runners/_form.html.haml | 6 +++--- app/views/projects/runners/_runner.html.haml | 4 ++-- app/views/projects/runners/show.html.haml | 4 ++-- .../20180219153455_add_job_upper_timeout_to_ci_runners.rb | 13 ------------- .../20180219153455_add_maximum_job_timeout_to_ci_runners.rb | 13 +++++++++++++ db/schema.rb | 2 +- doc/api/runners.md | 6 +++--- lib/api/entities.rb | 2 +- lib/api/runner.rb | 4 ++-- lib/api/runners.rb | 2 +- spec/models/ci/build_spec.rb | 6 +++--- spec/models/ci/runner_spec.rb | 12 ++++++------ spec/requests/api/runner_spec.rb | 10 +++++----- spec/requests/api/runners_spec.rb | 6 +++--- 18 files changed, 54 insertions(+), 54 deletions(-) delete mode 100644 db/migrate/20180219153455_add_job_upper_timeout_to_ci_runners.rb create mode 100644 db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index e15c4bc6ceb..61a0ef08dde 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -232,13 +232,13 @@ module Ci end def timeout - return runner.job_upper_timeout if should_use_runner_timeout + return runner.maximum_job_timeout if should_use_runner_timeout project.build_timeout end def should_use_runner_timeout - runner && runner.defines_job_upper_timeout? && runner.job_upper_timeout < project.build_timeout + !runner.nil? && runner.defines_maximum_job_timeout? && runner.maximum_job_timeout < project.build_timeout end private :should_use_runner_timeout diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index cf91ed3c9dc..3d83e00ccfe 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -9,7 +9,7 @@ module Ci ONLINE_CONTACT_TIMEOUT = 1.hour UPDATE_DB_RUNNER_INFO_EVERY = 40.minutes AVAILABLE_SCOPES = %w[specific shared active paused online].freeze - FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level job_upper_timeout].freeze + FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_job_timeout_user_readable].freeze has_many :builds has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -52,7 +52,7 @@ module Ci cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at, :ip_address - chronic_duration_attribute :job_upper_timeout_user_readable, :job_upper_timeout + chronic_duration_attribute :maximum_job_timeout_user_readable, :maximum_job_timeout # Searches for runners matching the given query. # @@ -170,8 +170,8 @@ module Ci end end - def defines_job_upper_timeout? - job_upper_timeout && job_upper_timeout > 0 + def defines_maximum_job_timeout? + !maximum_job_timeout.nil? && maximum_job_timeout > 0 end private diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml index 14aee600809..5f0fb5079d9 100644 --- a/app/views/admin/runners/_runner.html.haml +++ b/app/views/admin/runners/_runner.html.haml @@ -19,8 +19,8 @@ %td = runner.ip_address %td - - if runner.defines_job_upper_timeout? - = runner.job_upper_timeout + - if runner.defines_maximum_job_timeout? + = runner.maximum_job_timeout_user_readable - else n/a %td diff --git a/app/views/admin/runners/index.html.haml b/app/views/admin/runners/index.html.haml index 95fd7fe7ebe..a0c7d8f5fa9 100644 --- a/app/views/admin/runners/index.html.haml +++ b/app/views/admin/runners/index.html.haml @@ -61,7 +61,7 @@ %th Description %th Version %th IP Address - %th Timeout + %th Maximum timeout %th Projects %th Jobs %th Tags diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 4fb4323dab4..8fb8e6e0ebf 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -40,10 +40,10 @@ .col-sm-10 = f.text_field :description, class: 'form-control' .form-group - = label_tag :job_upper_timeout, class: 'control-label' do - Job upper timeout + = label_tag :maximum_job_timeout_user_readable, class: 'control-label' do + Maximum job timeout .col-sm-10 - = f.text_field :job_upper_timeout, class: 'form-control' + = f.text_field :maximum_job_timeout_user_readable, class: 'form-control' .help-block This timeout will take precedence when lower than Project-defined timeout .form-group = label_tag :tag_list, class: 'control-label' do diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index e3107fecfad..f7c41fe44c0 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -36,8 +36,8 @@ - if runner.description.present? %p.runner-description = runner.description - - if runner.defines_job_upper_timeout? - %p Job upper timeout: #{runner.job_upper_timeout} + - if runner.defines_maximum_job_timeout? + %p Maximum job timeout: #{runner.maximum_job_timeout_user_readable} - if runner.tag_list.present? %p - runner.tag_list.sort.each do |tag| diff --git a/app/views/projects/runners/show.html.haml b/app/views/projects/runners/show.html.haml index e0223eeb729..0d39236680c 100644 --- a/app/views/projects/runners/show.html.haml +++ b/app/views/projects/runners/show.html.haml @@ -56,8 +56,8 @@ %td Description %td= @runner.description %tr - %td Job upper timeout - %td= @runner.job_upper_timeout + %td Maximum job timeout + %td= @runner.maximum_job_timeout_user_readable %tr %td Last contact %td diff --git a/db/migrate/20180219153455_add_job_upper_timeout_to_ci_runners.rb b/db/migrate/20180219153455_add_job_upper_timeout_to_ci_runners.rb deleted file mode 100644 index 418ec7ac1d9..00000000000 --- a/db/migrate/20180219153455_add_job_upper_timeout_to_ci_runners.rb +++ /dev/null @@ -1,13 +0,0 @@ -class AddJobUpperTimeoutToCiRunners < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - def up - add_column :ci_runners, :job_upper_timeout, :integer - end - - def down - remove_column :ci_runners, :job_upper_timeout - end -end diff --git a/db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb b/db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb new file mode 100644 index 00000000000..5656970ede9 --- /dev/null +++ b/db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb @@ -0,0 +1,13 @@ +class AddMaximumJobTimeoutToCiRunners < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + add_column :ci_runners, :maximum_job_timeout, :integer + end + + def down + remove_column :ci_runners, :maximum_job_timeout + end +end diff --git a/db/schema.rb b/db/schema.rb index 413f42df40b..806e829dcbd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -459,7 +459,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do t.boolean "locked", default: false, null: false t.integer "access_level", default: 0, null: false t.string "ip_address" - t.integer "job_upper_timeout" + t.integer "maximum_job_timeout" end add_index "ci_runners", ["contacted_at"], name: "index_ci_runners_on_contacted_at", using: :btree diff --git a/doc/api/runners.md b/doc/api/runners.md index 4c6fc029a66..348fd499af2 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -154,7 +154,7 @@ Example response: ], "version": null, "access_level": "ref_protected", - "job_upper_timeout": 3600 + "maximum_job_timeout": 3600 } ``` @@ -175,7 +175,7 @@ PUT /runners/:id | `run_untagged` | boolean | no | Flag indicating the runner can execute untagged jobs | | `locked` | boolean | no | Flag indicating the runner is locked | | `access_level` | string | no | The access_level of the runner; `not_protected` or `ref_protected` | -| `job_upper_timeout` | integer | no | Upper timeout set when this Runner will handle the job | +| `maximum_job_timeout` | integer | no | Maximum timeout set when this Runner will handle the job | ``` curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/runners/6" --form "description=test-1-20150125-test" --form "tag_list=ruby,mysql,tag1,tag2" @@ -214,7 +214,7 @@ Example response: ], "version": null, "access_level": "ref_protected", - "job_upper_timeout": null + "maximum_job_timeout": null } ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index becd4f22a03..bb18fa00dc6 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -951,7 +951,7 @@ module API expose :tag_list expose :run_untagged expose :locked - expose :job_upper_timeout + expose :maximum_job_timeout expose :access_level expose :version, :revision, :platform, :architecture expose :contacted_at diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 14dd3b81dd0..3a26155be6d 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -14,10 +14,10 @@ module API optional :locked, type: Boolean, desc: 'Should Runner be locked for current project' optional :run_untagged, type: Boolean, desc: 'Should Runner handle untagged jobs' optional :tag_list, type: Array[String], desc: %q(List of Runner's tags) - optional :job_upper_timeout, type: Integer, desc: 'Upper timeout set when this Runner will handle the job' + optional :maximum_job_timeout, type: Integer, desc: 'Maximum timeout set when this Runner will handle the job' end post '/' do - attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list, :job_upper_timeout]) + attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list, :maximum_job_timeout]) .merge(get_runner_details_from_request) runner = diff --git a/lib/api/runners.rb b/lib/api/runners.rb index bc91b7cfd54..b3037235353 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -57,7 +57,7 @@ module API optional :locked, type: Boolean, desc: 'Flag indicating the runner is locked' optional :access_level, type: String, values: Ci::Runner.access_levels.keys, desc: 'The access_level of the runner' - optional :job_upper_timeout, type: Integer, desc: 'Upper timeout set when this Runner will handle the job' + optional :maximum_job_timeout, type: Integer, desc: 'Maximum timeout set when this Runner will handle the job' at_least_one_of :description, :active, :tag_list, :run_untagged, :locked, :access_level end put ':id' do diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index d13421a107f..115106548f0 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1281,7 +1281,7 @@ describe Ci::Build do end context 'when runner sets timeout to bigger value' do - let(:runner2) { create(:ci_runner, job_upper_timeout: 2000) } + let(:runner2) { create(:ci_runner, maximum_job_timeout: 2000) } let(:build) { create(:ci_build, :manual, pipeline: pipeline2, runner: runner2) } it 'returns project timeout configuration' do @@ -1290,11 +1290,11 @@ describe Ci::Build do end context 'when runner sets timeout to smaller value' do - let(:runner2) { create(:ci_runner, job_upper_timeout: 500) } + let(:runner2) { create(:ci_runner, maximum_job_timeout: 500) } let(:build) { create(:ci_build, :manual, pipeline: pipeline2, runner: runner2) } it 'returns project timeout configuration' do - expect(build.timeout).to eq(runner2.job_upper_timeout) + expect(build.timeout).to eq(runner2.maximum_job_timeout) end end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 80d7cd92fdb..5b5fa7fac01 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -556,20 +556,20 @@ describe Ci::Runner do end end - describe '#defines_job_upper_timeout?' do - context 'when job upper timeout is specified' do - subject { create(:ci_runner, job_upper_timeout: 1234) } + describe '#defines_maximum_job_timeout?' do + context 'when maximum job timeout is specified' do + subject { create(:ci_runner, maximum_job_timeout: 1234) } it 'should return true' do - expect(subject.defines_job_upper_timeout?).to be_truthy + expect(subject.defines_maximum_job_timeout?).to be_truthy end end - context 'when job upper timeout is not specified' do + context 'when maximum job timeout is not specified' do subject { create(:ci_runner) } it 'should return false' do - expect(subject.defines_job_upper_timeout?).to be_falsey + expect(subject.defines_maximum_job_timeout?).to be_falsey end end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 3eb0e88d095..a6a4f510406 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -109,13 +109,13 @@ describe API::Runner do end end - context 'when job upper timeout is specified' do + context 'when maximum job timeout is specified' do it 'creates runner' do post api('/runners'), token: registration_token, - job_upper_timeout: 7200 + maximum_job_timeout: 7200 expect(response).to have_gitlab_http_status 201 - expect(Ci::Runner.first.job_upper_timeout).to eq(7200) + expect(Ci::Runner.first.maximum_job_timeout).to eq(7200) end end @@ -671,7 +671,7 @@ describe API::Runner do end context 'when runner specifies lower timeout' do - let(:runner) { create(:ci_runner, job_upper_timeout: 1000) } + let(:runner) { create(:ci_runner, maximum_job_timeout: 1000) } it 'contains info about timeout overridden by runner' do request_job @@ -682,7 +682,7 @@ describe API::Runner do end context 'when runner specifies bigger timeout' do - let(:runner) { create(:ci_runner, job_upper_timeout: 2000) } + let(:runner) { create(:ci_runner, maximum_job_timeout: 2000) } it 'contains info about timeout not overridden by runner' do request_job diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 0444880a300..836b22f5657 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -123,7 +123,7 @@ describe API::Runners do expect(response).to have_gitlab_http_status(200) expect(json_response['description']).to eq(shared_runner.description) - expect(json_response['job_upper_timeout']).to be_nil + expect(json_response['maximum_job_timeout']).to be_nil end end @@ -194,7 +194,7 @@ describe API::Runners do run_untagged: 'false', locked: 'true', access_level: 'ref_protected', - job_upper_timeout: 1234 ) + maximum_job_timeout: 1234 ) shared_runner.reload expect(response).to have_gitlab_http_status(200) @@ -206,7 +206,7 @@ describe API::Runners do expect(shared_runner.ref_protected?).to be_truthy expect(shared_runner.ensure_runner_queue_value) .not_to eq(runner_queue_value) - expect(shared_runner.job_upper_timeout).to eq(1234) + expect(shared_runner.maximum_job_timeout).to eq(1234) end end -- cgit v1.2.1 From 78a4189ece4f8d125eefbfdf6619d3452820bb8e Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Feb 2018 04:03:12 +0100 Subject: Show timeout information on job's page --- .../jobs/components/sidebar_details_block.vue | 15 +++++++ app/models/ci/build.rb | 12 ++++-- app/serializers/build_details_entity.rb | 5 +++ ...eout_and_timeout_source_columns_to_ci_builds.rb | 15 +++++++ db/schema.rb | 2 + .../gitlab/import_export/safe_model_attributes.yml | 2 + .../concerns/chronic_duration_attribute_spec.rb | 50 +++++++++++++++------- spec/services/ci/retry_build_service_spec.rb | 3 +- 8 files changed, 84 insertions(+), 20 deletions(-) create mode 100644 db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb diff --git a/app/assets/javascripts/jobs/components/sidebar_details_block.vue b/app/assets/javascripts/jobs/components/sidebar_details_block.vue index 56814a52525..94c2084623b 100644 --- a/app/assets/javascripts/jobs/components/sidebar_details_block.vue +++ b/app/assets/javascripts/jobs/components/sidebar_details_block.vue @@ -39,6 +39,15 @@ runnerId() { return `#${this.job.runner.id}`; }, + timeout() { + let t = `${this.job.timeout.value}`; + + if (this.job.timeout.source != null) { + t += ` (from ${this.job.timeout.source})`; + } + + return t; + }, renderBlock() { return this.job.merge_request || this.job.duration || @@ -114,6 +123,12 @@ title="Queued" :value="queued" /> + (*) { !build.used_timeout.nil? } do |build| + { value: build.used_timeout_user_readable, + source: build.timeout_source } + end + expose :erased_by, if: -> (*) { build.erased? }, using: UserEntity expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :erase_build, build) } do |build| erase_project_job_path(project, build) diff --git a/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb b/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb new file mode 100644 index 00000000000..cb8651b1cfd --- /dev/null +++ b/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb @@ -0,0 +1,15 @@ +class AddUsedTimeoutAndTimeoutSourceColumnsToCiBuilds < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + add_column :ci_builds, :used_timeout, :integer + add_column :ci_builds, :timeout_source, :string + end + + def down + remove_column :ci_builds, :used_timeout + remove_column :ci_builds, :timeout_source + end +end diff --git a/db/schema.rb b/db/schema.rb index 806e829dcbd..61f15944f5a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -311,6 +311,8 @@ ActiveRecord::Schema.define(version: 20180327101207) do t.integer "artifacts_metadata_store" t.boolean "protected" t.integer "failure_reason" + t.integer "used_timeout" + t.string "timeout_source" end add_index "ci_builds", ["artifacts_expire_at"], name: "index_ci_builds_on_artifacts_expire_at", where: "(artifacts_file <> ''::text)", using: :btree diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 0716852f57f..bd7e60a5d9e 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -283,6 +283,8 @@ CommitStatus: - retried - protected - failure_reason +- used_timeout +- timeout_source Ci::Variable: - id - project_id diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb index 1a352537aaf..85adfaf4487 100644 --- a/spec/models/concerns/chronic_duration_attribute_spec.rb +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -1,28 +1,46 @@ require 'spec_helper' -shared_examples 'ChronicDurationAttribute' do - describe 'dynamically defined methods' do - it { expect(subject.class).to be_public_method_defined(virtual_field) } - it { expect(subject.class).to be_public_method_defined("#{virtual_field}=") } +shared_examples 'ChronicDurationAttribute reader' do + it 'contains dynamically created reader method' do + expect(subject.class).to be_public_method_defined(virtual_field) + end - it 'parses chronic duration input' do - subject.send("#{virtual_field}=", "10m") + it 'outputs chronic duration formated value' do + subject.send("#{source_field}=", 120) - expect(subject.send(source_field)).to eq(600) - end + expect(subject.send(virtual_field)).to eq('2m') + end +end + +shared_examples 'ChronicDurationAttribute writer' do + it 'contains dynamically created writer method' do + expect(subject.class).to be_public_method_defined("#{virtual_field}=") + end - it 'outputs chronic duration formated value' do - subject.send("#{source_field}=", 120) + it 'parses chronic duration input' do + subject.send("#{virtual_field}=", "10m") - expect(subject.send(virtual_field)).to eq('2m') - end + expect(subject.send(source_field)).to eq(600) end end describe 'ChronicDurationAttribute' do - let(:source_field) { :maximum_job_timeout } - let(:virtual_field) { :maximum_job_timeout_user_readable } - subject { Ci::Runner.new } + let(:source_field) {:maximum_job_timeout} + let(:virtual_field) {:maximum_job_timeout_user_readable} + subject {Ci::Runner.new} + + it_behaves_like 'ChronicDurationAttribute reader' + it_behaves_like 'ChronicDurationAttribute writer' +end + +describe 'ChronicDurationAttribute - reader' do + let(:source_field) {:used_timeout} + let(:virtual_field) {:used_timeout_user_readable} + subject {Ci::Build.new} + + it "doesn't contain dynamically created writer method" do + expect(subject.class).not_to be_public_method_defined("#{virtual_field}=") + end - it_behaves_like 'ChronicDurationAttribute' + it_behaves_like 'ChronicDurationAttribute reader' end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index b86a3d72bb4..e425e80e51e 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -29,7 +29,8 @@ describe Ci::RetryBuildService do commit_id deployments erased_by_id last_deployment project_id runner_id tag_taggings taggings tags trigger_request_id user_id auto_canceled_by_id retried failure_reason - artifacts_file_store artifacts_metadata_store].freeze + artifacts_file_store artifacts_metadata_store + used_timeout timeout_source].freeze shared_examples 'build duplication' do let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } -- cgit v1.2.1 From 42d2551dda0caf22c642fd8a85948fe77d5483d5 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Feb 2018 05:28:08 +0100 Subject: Update documentation --- doc/ci/runners/README.md | 45 ++++++++++++++++++++++++---- doc/ci/runners/img/shared_runners_admin.png | Bin 29192 -> 87490 bytes doc/user/project/pipelines/settings.md | 9 ++++++ 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/doc/ci/runners/README.md b/doc/ci/runners/README.md index 7a7b50b294d..b91aa334ff3 100644 --- a/doc/ci/runners/README.md +++ b/doc/ci/runners/README.md @@ -231,6 +231,38 @@ To make a Runner pick tagged/untagged jobs: 1. Check the **Run untagged jobs** option 1. Click **Save changes** for the changes to take effect +### Setting maximum job timeout for a Runner + +For each Runner you can specify a _maximum job timeout_. Such timeout, +if smaller than [project defined timeout], will take the precedence. This +feature can be used to prevent Shared Runner from being appropriated +by a project by setting a ridiculous big timeout (e.g. one week). + +When not configured, Runner will not override project timeout. + +How this feature will work: + +**Example 1 - Runner timeout bigger than project timeout** + +1. You set the _maximum job timeout_ for a Runner to 24 hours +1. You set the _CI/CD Timeout_ for a project to **2 hours** +1. You start a job +1. The job, if running longer, will be timeouted after **2 hours** + +**Example 2 - Runner timeout not configured** + +1. You remove the _maximum job timeout_ configuration from a Runner +1. You set the _CI/CD Timeout_ for a project to **2 hours** +1. You start a job +1. The job, if running longer, will be timeouted after **2 hours** + +**Example 3 - Runner timeout smaller than project timeout** + +1. You set the _maximum job timeout_ for a Runner to **30 minutes** +1. You set the _CI/CD Timeout_ for a project to 2 hours +1. You start a job +1. The job, if running longer, will be timeouted after **30 minutes** + ### Be careful with sensitive information With some [Runner Executors](https://docs.gitlab.com/runner/executors/README.html), @@ -259,12 +291,6 @@ Mentioned briefly earlier, but the following things of Runners can be exploited. We're always looking for contributions that can mitigate these [Security Considerations](https://docs.gitlab.com/runner/security/). -[install]: http://docs.gitlab.com/runner/install/ -[fifo]: https://en.wikipedia.org/wiki/FIFO_(computing_and_electronics) -[register]: http://docs.gitlab.com/runner/register/ -[protected branches]: ../../user/project/protected_branches.md -[protected tags]: ../../user/project/protected_tags.md - ## Determining the IP address of a Runner > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/17286) in GitLab 10.6. @@ -297,3 +323,10 @@ You can find the IP address of a Runner for a specific project by: 1. On the details page you should see a row for "IP Address" ![specific Runner IP address](img/specific_runner_ip_address.png) + +[install]: http://docs.gitlab.com/runner/install/ +[fifo]: https://en.wikipedia.org/wiki/FIFO_(computing_and_electronics) +[register]: http://docs.gitlab.com/runner/register/ +[protected branches]: ../../user/project/protected_branches.md +[protected tags]: ../../user/project/protected_tags.md +[project defined timeout]: ../../user/project/pipelines/settings.html#timeout diff --git a/doc/ci/runners/img/shared_runners_admin.png b/doc/ci/runners/img/shared_runners_admin.png index e049b339b36..4145cee354b 100644 Binary files a/doc/ci/runners/img/shared_runners_admin.png and b/doc/ci/runners/img/shared_runners_admin.png differ diff --git a/doc/user/project/pipelines/settings.md b/doc/user/project/pipelines/settings.md index 43451844f2d..1052c9efa25 100644 --- a/doc/user/project/pipelines/settings.md +++ b/doc/user/project/pipelines/settings.md @@ -27,6 +27,13 @@ The default value is 60 minutes. Decrease the time limit if you want to impose a hard limit on your jobs' running time or increase it otherwise. In any case, if the job surpasses the threshold, it is marked as failed. +### Timeout overriding on Runner level + +> - [Introduced][ce-17221] in GitLab 10.6. + +Project defined timeout (either specific timeout set by user or the default +60 minutes timeout) may be [overridden on Runner level][timeout overriding]. + ## Custom CI config path > - [Introduced][ce-12509] in GitLab 9.4. @@ -152,5 +159,7 @@ into your `README.md`: [var]: ../../../ci/yaml/README.md#git-strategy [coverage report]: #test-coverage-parsing +[timeout overriding]: ../../../ci/runners/README.html#setting-maximum-job-timeout-for-a-runner [ce-9362]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9362 [ce-12509]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12509 +[ce-17221]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/17221 -- cgit v1.2.1 From c21e817a9081e01bdbf27f4a28bdb7af791a7e8a Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Feb 2018 05:28:31 +0100 Subject: Add link to timeout overriding documentation from job page sidebar --- .../jobs/components/sidebar_detail_row.vue | 20 ++++++++++++++++++++ .../jobs/components/sidebar_details_block.vue | 1 + 2 files changed, 21 insertions(+) diff --git a/app/assets/javascripts/jobs/components/sidebar_detail_row.vue b/app/assets/javascripts/jobs/components/sidebar_detail_row.vue index a6819aaeb12..a24a0c5e779 100644 --- a/app/assets/javascripts/jobs/components/sidebar_detail_row.vue +++ b/app/assets/javascripts/jobs/components/sidebar_detail_row.vue @@ -11,11 +11,19 @@ type: String, required: true, }, + helpUrl: { + type: String, + required: false, + default: '', + }, }, computed: { hasTitle() { return this.title.length > 0; }, + hasHelpURL() { + return this.helpUrl.length > 0; + }, }, }; @@ -28,5 +36,17 @@ {{ title }}: {{ value }} + + + + + +

diff --git a/app/assets/javascripts/jobs/components/sidebar_details_block.vue b/app/assets/javascripts/jobs/components/sidebar_details_block.vue index 94c2084623b..c979e11b469 100644 --- a/app/assets/javascripts/jobs/components/sidebar_details_block.vue +++ b/app/assets/javascripts/jobs/components/sidebar_details_block.vue @@ -127,6 +127,7 @@ class="js-job-timeout" v-if="job.timeout" title="Timeout" + help-url="/help/ci/runners/README.html#setting-maximum-job-timeout-for-a-runner" :value="timeout" /> Date: Mon, 26 Feb 2018 16:26:39 +0100 Subject: Rename chronic_duration_attribute* to chronic_duration_attr* --- app/models/ci/build.rb | 2 +- app/models/ci/runner.rb | 2 +- app/models/concerns/chronic_duration_attribute.rb | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 3072817f443..5b9e06ab203 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -91,7 +91,7 @@ module Ci after_commit :update_project_statistics_after_save, on: [:create, :update] after_commit :update_project_statistics, on: :destroy - chronic_duration_attribute_reader :used_timeout_user_readable, :used_timeout + chronic_duration_attr_reader :used_timeout_user_readable, :used_timeout class << self # This is needed for url_for to work, diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 3d83e00ccfe..f95afd4c40c 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -52,7 +52,7 @@ module Ci cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at, :ip_address - chronic_duration_attribute :maximum_job_timeout_user_readable, :maximum_job_timeout + chronic_duration_attr :maximum_job_timeout_user_readable, :maximum_job_timeout # Searches for runners matching the given query. # diff --git a/app/models/concerns/chronic_duration_attribute.rb b/app/models/concerns/chronic_duration_attribute.rb index 2bf33174640..ae3aeda1709 100644 --- a/app/models/concerns/chronic_duration_attribute.rb +++ b/app/models/concerns/chronic_duration_attribute.rb @@ -2,19 +2,19 @@ module ChronicDurationAttribute extend ActiveSupport::Concern class_methods do - def chronic_duration_attribute(virtual_attribute, source_attribute) - chronic_duration_attribute_reader(virtual_attribute, source_attribute) - chronic_duration_attribute_writer(virtual_attribute, source_attribute) + def chronic_duration_attr(virtual_attribute, source_attribute) + chronic_duration_attr_reader(virtual_attribute, source_attribute) + chronic_duration_attr_writer(virtual_attribute, source_attribute) end - def chronic_duration_attribute_reader(virtual_attribute, source_attribute) + def chronic_duration_attr_reader(virtual_attribute, source_attribute) define_method(virtual_attribute) do value = self.send(source_attribute) # rubocop:disable GitlabSecurity/PublicSend ChronicDuration.output(value, format: :short) unless value.nil? end end - def chronic_duration_attribute_writer(virtual_attribute, source_attribute) + def chronic_duration_attr_writer(virtual_attribute, source_attribute) define_method("#{virtual_attribute}=") do |value| new_value = ChronicDuration.parse(value).to_i self.send("#{source_attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend -- cgit v1.2.1 From 36753b78c065a54d7501f37f69fb49506f26688c Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Feb 2018 16:35:08 +0100 Subject: Replace user_readable with human_readable --- app/models/ci/build.rb | 2 +- app/models/ci/runner.rb | 4 ++-- app/serializers/build_details_entity.rb | 2 +- app/views/admin/runners/_runner.html.haml | 2 +- app/views/projects/runners/_form.html.haml | 4 ++-- app/views/projects/runners/_runner.html.haml | 2 +- app/views/projects/runners/show.html.haml | 2 +- spec/models/concerns/chronic_duration_attribute_spec.rb | 4 ++-- 8 files changed, 11 insertions(+), 11 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 5b9e06ab203..f47cbe0a206 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -91,7 +91,7 @@ module Ci after_commit :update_project_statistics_after_save, on: [:create, :update] after_commit :update_project_statistics, on: :destroy - chronic_duration_attr_reader :used_timeout_user_readable, :used_timeout + chronic_duration_attr_reader :used_timeout_human_readable, :used_timeout class << self # This is needed for url_for to work, diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index f95afd4c40c..baf57423682 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -9,7 +9,7 @@ module Ci ONLINE_CONTACT_TIMEOUT = 1.hour UPDATE_DB_RUNNER_INFO_EVERY = 40.minutes AVAILABLE_SCOPES = %w[specific shared active paused online].freeze - FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_job_timeout_user_readable].freeze + FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_job_timeout_human_readable].freeze has_many :builds has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -52,7 +52,7 @@ module Ci cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at, :ip_address - chronic_duration_attr :maximum_job_timeout_user_readable, :maximum_job_timeout + chronic_duration_attr :maximum_job_timeout_human_readable, :maximum_job_timeout # Searches for runners matching the given query. # diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb index d1a4a9561d2..17769790371 100644 --- a/app/serializers/build_details_entity.rb +++ b/app/serializers/build_details_entity.rb @@ -6,7 +6,7 @@ class BuildDetailsEntity < JobEntity expose :pipeline, using: PipelineEntity expose :timeout, if: -> (*) { !build.used_timeout.nil? } do |build| - { value: build.used_timeout_user_readable, + { value: build.used_timeout_human_readable, source: build.timeout_source } end diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml index 5f0fb5079d9..fc6ad6dfe95 100644 --- a/app/views/admin/runners/_runner.html.haml +++ b/app/views/admin/runners/_runner.html.haml @@ -20,7 +20,7 @@ = runner.ip_address %td - if runner.defines_maximum_job_timeout? - = runner.maximum_job_timeout_user_readable + = runner.maximum_job_timeout_human_readable - else n/a %td diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 8fb8e6e0ebf..7e9435e0110 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -40,10 +40,10 @@ .col-sm-10 = f.text_field :description, class: 'form-control' .form-group - = label_tag :maximum_job_timeout_user_readable, class: 'control-label' do + = label_tag :maximum_job_timeout_human_readable, class: 'control-label' do Maximum job timeout .col-sm-10 - = f.text_field :maximum_job_timeout_user_readable, class: 'form-control' + = f.text_field :maximum_job_timeout_human_readable, class: 'form-control' .help-block This timeout will take precedence when lower than Project-defined timeout .form-group = label_tag :tag_list, class: 'control-label' do diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index f7c41fe44c0..91d6b172566 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -37,7 +37,7 @@ %p.runner-description = runner.description - if runner.defines_maximum_job_timeout? - %p Maximum job timeout: #{runner.maximum_job_timeout_user_readable} + %p Maximum job timeout: #{runner.maximum_job_timeout_human_readable} - if runner.tag_list.present? %p - runner.tag_list.sort.each do |tag| diff --git a/app/views/projects/runners/show.html.haml b/app/views/projects/runners/show.html.haml index 0d39236680c..67084e3d66a 100644 --- a/app/views/projects/runners/show.html.haml +++ b/app/views/projects/runners/show.html.haml @@ -57,7 +57,7 @@ %td= @runner.description %tr %td Maximum job timeout - %td= @runner.maximum_job_timeout_user_readable + %td= @runner.maximum_job_timeout_human_readable %tr %td Last contact %td diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb index 85adfaf4487..0d9b45e36e8 100644 --- a/spec/models/concerns/chronic_duration_attribute_spec.rb +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -26,7 +26,7 @@ end describe 'ChronicDurationAttribute' do let(:source_field) {:maximum_job_timeout} - let(:virtual_field) {:maximum_job_timeout_user_readable} + let(:virtual_field) {:maximum_job_timeout_human_readable} subject {Ci::Runner.new} it_behaves_like 'ChronicDurationAttribute reader' @@ -35,7 +35,7 @@ end describe 'ChronicDurationAttribute - reader' do let(:source_field) {:used_timeout} - let(:virtual_field) {:used_timeout_user_readable} + let(:virtual_field) {:used_timeout_human_readable} subject {Ci::Build.new} it "doesn't contain dynamically created writer method" do -- cgit v1.2.1 From 27266db06b90ac97f75a902a7255185838d2104d Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Feb 2018 16:38:11 +0100 Subject: Remove information about maximum_job_timeout from runners lists --- app/views/admin/runners/_runner.html.haml | 5 ----- app/views/admin/runners/index.html.haml | 1 - app/views/projects/runners/_runner.html.haml | 2 -- 3 files changed, 8 deletions(-) diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml index fc6ad6dfe95..e1cee584929 100644 --- a/app/views/admin/runners/_runner.html.haml +++ b/app/views/admin/runners/_runner.html.haml @@ -18,11 +18,6 @@ = runner.version %td = runner.ip_address - %td - - if runner.defines_maximum_job_timeout? - = runner.maximum_job_timeout_human_readable - - else - n/a %td - if runner.shared? n/a diff --git a/app/views/admin/runners/index.html.haml b/app/views/admin/runners/index.html.haml index a0c7d8f5fa9..9f13dbbbd82 100644 --- a/app/views/admin/runners/index.html.haml +++ b/app/views/admin/runners/index.html.haml @@ -61,7 +61,6 @@ %th Description %th Version %th IP Address - %th Maximum timeout %th Projects %th Jobs %th Tags diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index 91d6b172566..6376496ee1a 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -36,8 +36,6 @@ - if runner.description.present? %p.runner-description = runner.description - - if runner.defines_maximum_job_timeout? - %p Maximum job timeout: #{runner.maximum_job_timeout_human_readable} - if runner.tag_list.present? %p - runner.tag_list.sort.each do |tag| -- cgit v1.2.1 From 40f57b9b62f768f5aa66ef6d3e8d0922ffe7ec0f Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Feb 2018 16:45:12 +0100 Subject: Use change instead of up/down in simple migrations --- db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb | 6 +----- ...556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb | 7 +------ 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb b/db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb index 5656970ede9..1ad7bb86e72 100644 --- a/db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb +++ b/db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb @@ -3,11 +3,7 @@ class AddMaximumJobTimeoutToCiRunners < ActiveRecord::Migration DOWNTIME = false - def up + def change add_column :ci_runners, :maximum_job_timeout, :integer end - - def down - remove_column :ci_runners, :maximum_job_timeout - end end diff --git a/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb b/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb index cb8651b1cfd..435fba712aa 100644 --- a/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb +++ b/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb @@ -3,13 +3,8 @@ class AddUsedTimeoutAndTimeoutSourceColumnsToCiBuilds < ActiveRecord::Migration DOWNTIME = false - def up + def change add_column :ci_builds, :used_timeout, :integer add_column :ci_builds, :timeout_source, :string end - - def down - remove_column :ci_builds, :used_timeout - remove_column :ci_builds, :timeout_source - end end -- cgit v1.2.1 From f18eb6dae43b8407ceb8bcbdecd6df30af2a66ac Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Feb 2018 16:53:23 +0100 Subject: Add styling improvements in specs --- spec/models/ci/build_spec.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 115106548f0..cceb2033cbb 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1274,10 +1274,13 @@ describe Ci::Build do describe '#timeout' do set(:project2) { create(:project, :repository, group: group, build_timeout: 1000) } set(:pipeline2) { create(:ci_pipeline, project: project2, sha: project2.commit.id, ref: project2.default_branch, status: 'success') } + let(:build) { create(:ci_build, :manual, pipeline: pipeline2) } + subject { build.timeout } + it 'returns project timeout configuration' do - expect(build.timeout).to eq(project2.build_timeout) + is_expected.to eq(project2.build_timeout) end context 'when runner sets timeout to bigger value' do @@ -1285,7 +1288,7 @@ describe Ci::Build do let(:build) { create(:ci_build, :manual, pipeline: pipeline2, runner: runner2) } it 'returns project timeout configuration' do - expect(build.timeout).to eq(project2.build_timeout) + is_expected.to eq(project2.build_timeout) end end @@ -1294,7 +1297,7 @@ describe Ci::Build do let(:build) { create(:ci_build, :manual, pipeline: pipeline2, runner: runner2) } it 'returns project timeout configuration' do - expect(build.timeout).to eq(runner2.maximum_job_timeout) + is_expected.to eq(runner2.maximum_job_timeout) end end end -- cgit v1.2.1 From e75614e8cb391f0e499b71b261de7c8148f1b6a4 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Feb 2018 17:33:43 +0100 Subject: Add few frontend improvements --- app/assets/javascripts/jobs/components/sidebar_detail_row.vue | 6 +++++- app/assets/javascripts/jobs/components/sidebar_details_block.vue | 7 ++++++- app/assets/javascripts/jobs/job_details_bundle.js | 1 + app/views/projects/jobs/show.html.haml | 2 +- 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/jobs/components/sidebar_detail_row.vue b/app/assets/javascripts/jobs/components/sidebar_detail_row.vue index a24a0c5e779..dfe87d89a39 100644 --- a/app/assets/javascripts/jobs/components/sidebar_detail_row.vue +++ b/app/assets/javascripts/jobs/components/sidebar_detail_row.vue @@ -44,8 +44,12 @@ - +

diff --git a/app/assets/javascripts/jobs/components/sidebar_details_block.vue b/app/assets/javascripts/jobs/components/sidebar_details_block.vue index c979e11b469..ad859679a1e 100644 --- a/app/assets/javascripts/jobs/components/sidebar_details_block.vue +++ b/app/assets/javascripts/jobs/components/sidebar_details_block.vue @@ -22,6 +22,11 @@ type: Boolean, required: true, }, + runnerHelpUrl: { + type: String, + required: false, + default: '', + }, }, computed: { shouldRenderContent() { @@ -127,7 +132,7 @@ class="js-job-timeout" v-if="job.timeout" title="Timeout" - help-url="/help/ci/runners/README.html#setting-maximum-job-timeout-for-a-runner" + :help-url="runnerHelpUrl" :value="timeout" /> { props: { isLoading: this.mediator.state.isLoading, job: this.mediator.store.state.job, + runnerHelpUrl: dataset.runnerHelpUrl, }, }); }, diff --git a/app/views/projects/jobs/show.html.haml b/app/views/projects/jobs/show.html.haml index 849c273db8c..4d2bc3a04a4 100644 --- a/app/views/projects/jobs/show.html.haml +++ b/app/views/projects/jobs/show.html.haml @@ -111,4 +111,4 @@ .js-build-options{ data: javascript_build_options } -#js-job-details-vue{ data: { endpoint: project_job_path(@project, @build, format: :json) } } +#js-job-details-vue{ data: { endpoint: project_job_path(@project, @build, format: :json), runner_help_url: '/help/ci/runners/README.html#setting-maximum-job-timeout-for-a-runner' } } -- cgit v1.2.1 From 3e7f3bc7980018910f6c6e7a19f706ad18ab2793 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Feb 2018 18:51:56 +0100 Subject: Refactor timeout selection mechanism --- app/models/ci/build.rb | 10 ++-------- spec/models/ci/build_spec.rb | 46 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index f47cbe0a206..f9a83965199 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -124,7 +124,7 @@ module Ci after_transition pending: :running do |build| build.used_timeout = build.timeout - build.timeout_source = build.should_use_runner_timeout? ? 'Runner' : 'Project' + build.timeout_source = build.timeout < build.project.build_timeout ? 'Runner' : 'Project' build.save! build.run_after_commit do @@ -239,13 +239,7 @@ module Ci end def timeout - return runner.maximum_job_timeout if should_use_runner_timeout? - - project.build_timeout - end - - def should_use_runner_timeout? - !runner.nil? && runner.defines_maximum_job_timeout? && runner.maximum_job_timeout < project.build_timeout + [project.build_timeout, runner&.maximum_job_timeout].compact.min end def triggered_by?(current_user) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index cceb2033cbb..78842bd8a92 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2036,6 +2036,52 @@ describe Ci::Build do end end + describe 'state transition: pending: :running' do + let(:runner) { create(:ci_runner) } + let(:job) { create(:ci_build, :pending, runner: runner) } + + before do + job.project.build_timeout = 1800 + job.project.save! + end + + shared_examples 'saves data on transition' do + it 'saves used_timeout and timeout_source on transition' do + expect(job.used_timeout).to be_nil + expect(job.timeout_source).to be_nil + + job.run! + + expect(job.used_timeout).to eq(expected_timeout) + expect(job.timeout_source).to eq(expected_timeout_source) + end + end + + context 'when runner timeout overrides project timeout' do + let(:expected_timeout) { 900 } + let(:expected_timeout_source) { 'Runner' } + + before do + runner.maximum_job_timeout = 900 + runner.save! + end + + it_behaves_like 'saves data on transition' + end + + context "when runner timeout doesn't override project timeout" do + let(:expected_timeout) { 1800 } + let(:expected_timeout_source) { 'Project' } + + before do + runner.maximum_job_timeout = 3600 + runner.save! + end + + it_behaves_like 'saves data on transition' + end + end + describe 'state transition: any => [:running]' do shared_examples 'validation is active' do context 'when depended job has not been completed yet' do -- cgit v1.2.1 From cb502b626659001af1e0ca82288a2b427e346d18 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Feb 2018 18:56:39 +0100 Subject: Remove unused method --- app/models/ci/runner.rb | 4 ---- spec/models/ci/runner_spec.rb | 18 ------------------ 2 files changed, 22 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index baf57423682..413607e0eff 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -170,10 +170,6 @@ module Ci end end - def defines_maximum_job_timeout? - !maximum_job_timeout.nil? && maximum_job_timeout > 0 - end - private def cleanup_runner_queue diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 5b5fa7fac01..ab170e6351c 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -556,24 +556,6 @@ describe Ci::Runner do end end - describe '#defines_maximum_job_timeout?' do - context 'when maximum job timeout is specified' do - subject { create(:ci_runner, maximum_job_timeout: 1234) } - - it 'should return true' do - expect(subject.defines_maximum_job_timeout?).to be_truthy - end - end - - context 'when maximum job timeout is not specified' do - subject { create(:ci_runner) } - - it 'should return false' do - expect(subject.defines_maximum_job_timeout?).to be_falsey - end - end - end - describe '.search' do let(:runner) { create(:ci_runner, token: '123abc', description: 'test runner') } -- cgit v1.2.1 From 14549703bf2c6e1a1ac1c23327c452e5e68ee959 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Feb 2018 19:18:01 +0100 Subject: Revert change in doc/ci/runners/img/shared_runners_admin.png --- doc/ci/runners/img/shared_runners_admin.png | Bin 87490 -> 29192 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/doc/ci/runners/img/shared_runners_admin.png b/doc/ci/runners/img/shared_runners_admin.png index 4145cee354b..e049b339b36 100644 Binary files a/doc/ci/runners/img/shared_runners_admin.png and b/doc/ci/runners/img/shared_runners_admin.png differ -- cgit v1.2.1 From 7ef24a47714802edd8ad5470a6147b60172a0a28 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 28 Feb 2018 20:56:14 +0100 Subject: Properly handle empty value for chronic duration attribute --- app/models/concerns/chronic_duration_attribute.rb | 8 +++++++- spec/models/concerns/chronic_duration_attribute_spec.rb | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/chronic_duration_attribute.rb b/app/models/concerns/chronic_duration_attribute.rb index ae3aeda1709..87c60a2b7d5 100644 --- a/app/models/concerns/chronic_duration_attribute.rb +++ b/app/models/concerns/chronic_duration_attribute.rb @@ -10,14 +10,20 @@ module ChronicDurationAttribute def chronic_duration_attr_reader(virtual_attribute, source_attribute) define_method(virtual_attribute) do value = self.send(source_attribute) # rubocop:disable GitlabSecurity/PublicSend - ChronicDuration.output(value, format: :short) unless value.nil? + + return '' if value.nil? + + ChronicDuration.output(value, format: :short) end end def chronic_duration_attr_writer(virtual_attribute, source_attribute) define_method("#{virtual_attribute}=") do |value| new_value = ChronicDuration.parse(value).to_i + new_value = nil if new_value <= 0 + self.send("#{source_attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend + new_value end end diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb index 0d9b45e36e8..31db7d055cc 100644 --- a/spec/models/concerns/chronic_duration_attribute_spec.rb +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -5,11 +5,17 @@ shared_examples 'ChronicDurationAttribute reader' do expect(subject.class).to be_public_method_defined(virtual_field) end - it 'outputs chronic duration formated value' do + it 'outputs chronic duration formatted value' do subject.send("#{source_field}=", 120) expect(subject.send(virtual_field)).to eq('2m') end + + it 'outputs empty string when value set to nil' do + subject.send("#{source_field}=", nil) + + expect(subject.send(virtual_field)).to be_empty + end end shared_examples 'ChronicDurationAttribute writer' do @@ -18,10 +24,16 @@ shared_examples 'ChronicDurationAttribute writer' do end it 'parses chronic duration input' do - subject.send("#{virtual_field}=", "10m") + subject.send("#{virtual_field}=", '10m') expect(subject.send(source_field)).to eq(600) end + + it 'writes null when empty input is used' do + subject.send("#{virtual_field}=", '') + + expect(subject.send(source_field)).to be_nil + end end describe 'ChronicDurationAttribute' do -- cgit v1.2.1 From dbd7455583a10679f0e365cfeacd4729a4b543ec Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 28 Feb 2018 20:56:35 +0100 Subject: Use _human_readable for Runner's registration API --- lib/api/runner.rb | 4 ++-- spec/requests/api/runner_spec.rb | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 3a26155be6d..74ddaf26bb6 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -14,10 +14,10 @@ module API optional :locked, type: Boolean, desc: 'Should Runner be locked for current project' optional :run_untagged, type: Boolean, desc: 'Should Runner handle untagged jobs' optional :tag_list, type: Array[String], desc: %q(List of Runner's tags) - optional :maximum_job_timeout, type: Integer, desc: 'Maximum timeout set when this Runner will handle the job' + optional :maximum_job_timeout_human_readable, type: String, desc: 'Maximum timeout set when this Runner will handle the job' end post '/' do - attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list, :maximum_job_timeout]) + attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list, :maximum_job_timeout_human_readable]) .merge(get_runner_details_from_request) runner = diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index a6a4f510406..55c4150a393 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -112,10 +112,20 @@ describe API::Runner do context 'when maximum job timeout is specified' do it 'creates runner' do post api('/runners'), token: registration_token, - maximum_job_timeout: 7200 + maximum_job_timeout_human_readable: '2h 30m' expect(response).to have_gitlab_http_status 201 - expect(Ci::Runner.first.maximum_job_timeout).to eq(7200) + expect(Ci::Runner.first.maximum_job_timeout).to eq(9000) + end + + context 'when maximum job timeout is empty' do + it 'creates runner' do + post api('/runners'), token: registration_token, + maximum_job_timeout_human_readable: '' + + expect(response).to have_gitlab_http_status 201 + expect(Ci::Runner.first.maximum_job_timeout).to be_nil + end end end -- cgit v1.2.1 From 8ffa48098b77e1f35f9c01f6977d9ccf1d166a24 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 28 Feb 2018 21:07:57 +0100 Subject: Downcase timeout_source value --- app/models/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index f9a83965199..2f01a098b0e 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -124,7 +124,7 @@ module Ci after_transition pending: :running do |build| build.used_timeout = build.timeout - build.timeout_source = build.timeout < build.project.build_timeout ? 'Runner' : 'Project' + build.timeout_source = build.timeout < build.project.build_timeout ? 'runner' : 'project' build.save! build.run_after_commit do -- cgit v1.2.1 From 1b0b8b9c02642ac19b9f5019cdd38fcec280c2a7 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 28 Feb 2018 21:36:01 +0100 Subject: Change timeout_source to enum --- app/models/ci/build.rb | 15 +++++++++++---- app/presenters/ci/build_presenter.rb | 14 ++++++++++++++ app/serializers/build_details_entity.rb | 2 +- ...ed_timeout_and_timeout_source_columns_to_ci_builds.rb | 2 +- db/schema.rb | 2 +- spec/models/ci/build_spec.rb | 16 +++++++--------- 6 files changed, 35 insertions(+), 16 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 2f01a098b0e..cbd2cc6c58f 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -93,6 +93,12 @@ module Ci chronic_duration_attr_reader :used_timeout_human_readable, :used_timeout + enum timeout_source: { + unknown_timeout_source: nil, + project_timeout_source: 1, + runner_timeout_source: 2 + } + class << self # This is needed for url_for to work, # as the controller is JobsController @@ -123,10 +129,6 @@ module Ci end after_transition pending: :running do |build| - build.used_timeout = build.timeout - build.timeout_source = build.timeout < build.project.build_timeout ? 'runner' : 'project' - build.save! - build.run_after_commit do BuildHooksWorker.perform_async(id) end @@ -160,6 +162,11 @@ module Ci before_transition any => [:running] do |build| build.validates_dependencies! unless Feature.enabled?('ci_disable_validates_dependencies') end + + before_transition pending: :running do |build| + build.used_timeout = build.timeout + build.timeout_source = build.timeout < build.project.build_timeout ? :runner_timeout_source : :project_timeout_source + end end def detailed_status(current_user) diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index 255475e1fe6..be6cc2e70b1 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -1,5 +1,12 @@ module Ci class BuildPresenter < Gitlab::View::Presenter::Delegated + + TIMEOUT_SOURCES = { + unknown_timeout_source: nil, + project_timeout_source: 'project', + runner_timeout_source: 'runner' + }.freeze + presents :build def erased_by_user? @@ -18,6 +25,13 @@ module Ci end end + def timeout_source + return unless build.timeout_source? + + TIMEOUT_SOURCES[build.timeout_source.to_sym] || + build.timeout_source + end + def trigger_variables return [] unless trigger_request diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb index 17769790371..0ffc537dfd8 100644 --- a/app/serializers/build_details_entity.rb +++ b/app/serializers/build_details_entity.rb @@ -7,7 +7,7 @@ class BuildDetailsEntity < JobEntity expose :timeout, if: -> (*) { !build.used_timeout.nil? } do |build| { value: build.used_timeout_human_readable, - source: build.timeout_source } + source: build.present.timeout_source } end expose :erased_by, if: -> (*) { build.erased? }, using: UserEntity diff --git a/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb b/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb index 435fba712aa..18c4fd5bae4 100644 --- a/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb +++ b/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb @@ -5,6 +5,6 @@ class AddUsedTimeoutAndTimeoutSourceColumnsToCiBuilds < ActiveRecord::Migration def change add_column :ci_builds, :used_timeout, :integer - add_column :ci_builds, :timeout_source, :string + add_column :ci_builds, :timeout_source, :integer end end diff --git a/db/schema.rb b/db/schema.rb index 61f15944f5a..9b126385045 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -312,7 +312,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do t.boolean "protected" t.integer "failure_reason" t.integer "used_timeout" - t.string "timeout_source" + t.integer "timeout_source" end add_index "ci_builds", ["artifacts_expire_at"], name: "index_ci_builds_on_artifacts_expire_at", where: "(artifacts_file <> ''::text)", using: :btree diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 78842bd8a92..c3624158b76 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2046,20 +2046,18 @@ describe Ci::Build do end shared_examples 'saves data on transition' do - it 'saves used_timeout and timeout_source on transition' do - expect(job.used_timeout).to be_nil - expect(job.timeout_source).to be_nil - - job.run! + it 'saves used_timeout' do + expect { job.run! }.to change { job.reload.used_timeout }.from(nil).to(expected_timeout) + end - expect(job.used_timeout).to eq(expected_timeout) - expect(job.timeout_source).to eq(expected_timeout_source) + it 'saves timeout_source' do + expect { job.run! }.to change { job.reload.timeout_source }.from('unknown_timeout_source').to(expected_timeout_source) end end context 'when runner timeout overrides project timeout' do let(:expected_timeout) { 900 } - let(:expected_timeout_source) { 'Runner' } + let(:expected_timeout_source) { 'runner_timeout_source' } before do runner.maximum_job_timeout = 900 @@ -2071,7 +2069,7 @@ describe Ci::Build do context "when runner timeout doesn't override project timeout" do let(:expected_timeout) { 1800 } - let(:expected_timeout_source) { 'Project' } + let(:expected_timeout_source) { 'project_timeout_source' } before do runner.maximum_job_timeout = 3600 -- cgit v1.2.1 From d34e937b93b103435a59e6759a9f30e9f8addc11 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 28 Feb 2018 22:30:06 +0100 Subject: Add tests for frontend changes --- spec/javascripts/jobs/mock_data.js | 4 ++++ spec/javascripts/jobs/sidebar_detail_row_spec.js | 17 +++++++++++++++++ spec/javascripts/jobs/sidebar_details_block_spec.js | 6 ++++++ 3 files changed, 27 insertions(+) diff --git a/spec/javascripts/jobs/mock_data.js b/spec/javascripts/jobs/mock_data.js index 43589d54be4..6c3f39f0193 100644 --- a/spec/javascripts/jobs/mock_data.js +++ b/spec/javascripts/jobs/mock_data.js @@ -115,6 +115,10 @@ export default { commit_path: '/root/ci-mock/commit/c58647773a6b5faf066d4ad6ff2c9fbba5f180f6', }, }, + timeout: { + value: '1m 40s', + source: 'runner', + }, merge_request: { iid: 2, path: '/root/ci-mock/merge_requests/2', diff --git a/spec/javascripts/jobs/sidebar_detail_row_spec.js b/spec/javascripts/jobs/sidebar_detail_row_spec.js index 3ac65709c4a..e1f0fe334ac 100644 --- a/spec/javascripts/jobs/sidebar_detail_row_spec.js +++ b/spec/javascripts/jobs/sidebar_detail_row_spec.js @@ -37,4 +37,21 @@ describe('Sidebar detail row', () => { vm.$el.textContent.replace(/\s+/g, ' ').trim(), ).toEqual('this is the title: this is the value'); }); + + it('should not render help when helpUrl not provided', () => { + expect(vm.$el.querySelector('.help-button')).toBeUndefined(); + }); + + beforeEach(() => { + vm = new SidebarDetailRow({ + propsData: { + helpUrl: 'help url', + }, + }).$mount(); + }); + + it('should render help when helpUrl is provided', () => { + expect(vm.$el.querySelector('.help-button a').getAttribute('href')).toEqual('help url'); + }); + }); diff --git a/spec/javascripts/jobs/sidebar_details_block_spec.js b/spec/javascripts/jobs/sidebar_details_block_spec.js index 95532ef5382..602dae514b1 100644 --- a/spec/javascripts/jobs/sidebar_details_block_spec.js +++ b/spec/javascripts/jobs/sidebar_details_block_spec.js @@ -96,6 +96,12 @@ describe('Sidebar details block', () => { ).toEqual('Runner: #1'); }); + it('should render timeout information', () => { + expect( + trimWhitespace(vm.$el.querySelector('.js-job-timeout')), + ).toEqual('Timeout: 1m 40s (from runner)'); + }); + it('should render coverage', () => { expect( trimWhitespace(vm.$el.querySelector('.js-job-coverage')), -- cgit v1.2.1 From 1dde609ca6b130aa0a3d39e929edee7e770e62fc Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 1 Mar 2018 03:12:32 +0100 Subject: Move job timeout information to new ci_builds_metadata table --- .../jobs/components/sidebar_details_block.vue | 8 +++---- app/models/ci/build.rb | 18 ++++++---------- app/models/ci/build_metadata.rb | 25 ++++++++++++++++++++++ app/presenters/ci/build_metadata_presenter.rb | 20 +++++++++++++++++ app/presenters/ci/build_presenter.rb | 13 ----------- app/serializers/build_details_entity.rb | 5 +---- app/serializers/build_metadata_entity.rb | 10 +++++++++ ...eout_and_timeout_source_columns_to_ci_builds.rb | 10 --------- ...180301010859_create_ci_builds_metadata_table.rb | 13 +++++++++++ ..._add_build_foreign_key_to_ci_builds_metadata.rb | 15 +++++++++++++ db/schema.rb | 11 +++++++--- spec/javascripts/jobs/mock_data.js | 6 +++--- spec/models/ci/build_spec.rb | 4 ++-- .../concerns/chronic_duration_attribute_spec.rb | 2 +- spec/services/ci/retry_build_service_spec.rb | 2 +- 15 files changed, 110 insertions(+), 52 deletions(-) create mode 100644 app/models/ci/build_metadata.rb create mode 100644 app/presenters/ci/build_metadata_presenter.rb create mode 100644 app/serializers/build_metadata_entity.rb delete mode 100644 db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb create mode 100644 db/migrate/20180301010859_create_ci_builds_metadata_table.rb create mode 100644 db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb diff --git a/app/assets/javascripts/jobs/components/sidebar_details_block.vue b/app/assets/javascripts/jobs/components/sidebar_details_block.vue index ad859679a1e..15584922d1f 100644 --- a/app/assets/javascripts/jobs/components/sidebar_details_block.vue +++ b/app/assets/javascripts/jobs/components/sidebar_details_block.vue @@ -45,10 +45,10 @@ return `#${this.job.runner.id}`; }, timeout() { - let t = `${this.job.timeout.value}`; + let t = `${this.job.metadata.used_timeout_human_readable}`; - if (this.job.timeout.source != null) { - t += ` (from ${this.job.timeout.source})`; + if (this.job.metadata.timeout_source != null) { + t += ` (from ${this.job.metadata.timeout_source})`; } return t; @@ -130,7 +130,7 @@ /> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id + has_one :metadata, class_name: 'Ci::BuildMetadata' + # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @persisted_environment ||= Environment.find_by( @@ -84,6 +85,10 @@ module Ci before_save :ensure_token before_destroy { unscoped_project } + before_create do |build| + build.metadata = Ci::BuildMetadata.new + end + after_create unless: :importing? do |build| run_after_commit { BuildHooksWorker.perform_async(build.id) } end @@ -91,14 +96,6 @@ module Ci after_commit :update_project_statistics_after_save, on: [:create, :update] after_commit :update_project_statistics, on: :destroy - chronic_duration_attr_reader :used_timeout_human_readable, :used_timeout - - enum timeout_source: { - unknown_timeout_source: nil, - project_timeout_source: 1, - runner_timeout_source: 2 - } - class << self # This is needed for url_for to work, # as the controller is JobsController @@ -164,8 +161,7 @@ module Ci end before_transition pending: :running do |build| - build.used_timeout = build.timeout - build.timeout_source = build.timeout < build.project.build_timeout ? :runner_timeout_source : :project_timeout_source + build.metadata.save_timeout_state! end end diff --git a/app/models/ci/build_metadata.rb b/app/models/ci/build_metadata.rb new file mode 100644 index 00000000000..7a1315dfcf9 --- /dev/null +++ b/app/models/ci/build_metadata.rb @@ -0,0 +1,25 @@ +module Ci + class BuildMetadata < ActiveRecord::Base + extend Gitlab::Ci::Model + include Presentable + include ChronicDurationAttribute + + self.table_name = 'ci_builds_metadata' + + belongs_to :build, class_name: 'Ci::Build' + + chronic_duration_attr_reader :used_timeout_human_readable, :used_timeout + + enum timeout_source: { + unknown_timeout_source: nil, + project_timeout_source: 1, + runner_timeout_source: 2 + } + + def save_timeout_state! + self.used_timeout = build.timeout + self.timeout_source = build.timeout < build.project.build_timeout ? :runner_timeout_source : :project_timeout_source + save! + end + end +end diff --git a/app/presenters/ci/build_metadata_presenter.rb b/app/presenters/ci/build_metadata_presenter.rb new file mode 100644 index 00000000000..e80eb19ea19 --- /dev/null +++ b/app/presenters/ci/build_metadata_presenter.rb @@ -0,0 +1,20 @@ +module Ci + class BuildMetadataPresenter < Gitlab::View::Presenter::Delegated + + TIMEOUT_SOURCES = { + unknown_timeout_source: nil, + project_timeout_source: 'project', + runner_timeout_source: 'runner' + }.freeze + + presents :metadata + + def timeout_source + return unless metadata.timeout_source? + + TIMEOUT_SOURCES[metadata.timeout_source.to_sym] || + metadata.timeout_source + end + + end +end diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index be6cc2e70b1..9345e5069bc 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -1,12 +1,6 @@ module Ci class BuildPresenter < Gitlab::View::Presenter::Delegated - TIMEOUT_SOURCES = { - unknown_timeout_source: nil, - project_timeout_source: 'project', - runner_timeout_source: 'runner' - }.freeze - presents :build def erased_by_user? @@ -25,13 +19,6 @@ module Ci end end - def timeout_source - return unless build.timeout_source? - - TIMEOUT_SOURCES[build.timeout_source.to_sym] || - build.timeout_source - end - def trigger_variables return [] unless trigger_request diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb index 0ffc537dfd8..ca4480fe2b1 100644 --- a/app/serializers/build_details_entity.rb +++ b/app/serializers/build_details_entity.rb @@ -5,10 +5,7 @@ class BuildDetailsEntity < JobEntity expose :runner, using: RunnerEntity expose :pipeline, using: PipelineEntity - expose :timeout, if: -> (*) { !build.used_timeout.nil? } do |build| - { value: build.used_timeout_human_readable, - source: build.present.timeout_source } - end + expose :metadata, using: BuildMetadataEntity expose :erased_by, if: -> (*) { build.erased? }, using: UserEntity expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :erase_build, build) } do |build| diff --git a/app/serializers/build_metadata_entity.rb b/app/serializers/build_metadata_entity.rb new file mode 100644 index 00000000000..71c7295ba9f --- /dev/null +++ b/app/serializers/build_metadata_entity.rb @@ -0,0 +1,10 @@ +class BuildMetadataEntity < Grape::Entity + + expose :used_timeout_human_readable do |metadata| + metadata.used_timeout_human_readable unless metadata.used_timeout.nil? + end + + expose :timeout_source do |metadata| + metadata.present.timeout_source + end +end diff --git a/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb b/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb deleted file mode 100644 index 18c4fd5bae4..00000000000 --- a/db/migrate/20180221022556_add_used_timeout_and_timeout_source_columns_to_ci_builds.rb +++ /dev/null @@ -1,10 +0,0 @@ -class AddUsedTimeoutAndTimeoutSourceColumnsToCiBuilds < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - def change - add_column :ci_builds, :used_timeout, :integer - add_column :ci_builds, :timeout_source, :integer - end -end diff --git a/db/migrate/20180301010859_create_ci_builds_metadata_table.rb b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb new file mode 100644 index 00000000000..9d5e9c1779b --- /dev/null +++ b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb @@ -0,0 +1,13 @@ +class CreateCiBuildsMetadataTable < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + create_table :ci_builds_metadata do |t| + t.integer :build_id, null: false + t.integer :used_timeout + t.integer :timeout_source + end + end +end diff --git a/db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb b/db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb new file mode 100644 index 00000000000..feda2d6e9c9 --- /dev/null +++ b/db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb @@ -0,0 +1,15 @@ +class AddBuildForeignKeyToCiBuildsMetadata < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key(:ci_builds_metadata, :ci_builds, column: :build_id) + end + + def down + remove_foreign_key(:ci_builds_metadata, column: :build_id) + end +end diff --git a/db/schema.rb b/db/schema.rb index 9b126385045..5463b3f1219 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -311,8 +311,6 @@ ActiveRecord::Schema.define(version: 20180327101207) do t.integer "artifacts_metadata_store" t.boolean "protected" t.integer "failure_reason" - t.integer "used_timeout" - t.integer "timeout_source" end add_index "ci_builds", ["artifacts_expire_at"], name: "index_ci_builds_on_artifacts_expire_at", where: "(artifacts_file <> ''::text)", using: :btree @@ -331,6 +329,12 @@ ActiveRecord::Schema.define(version: 20180327101207) do add_index "ci_builds", ["updated_at"], name: "index_ci_builds_on_updated_at", using: :btree add_index "ci_builds", ["user_id"], name: "index_ci_builds_on_user_id", using: :btree + create_table "ci_builds_metadata", force: :cascade do |t| + t.integer "build_id", null: false + t.integer "used_timeout" + t.integer "timeout_source" + end + create_table "ci_group_variables", force: :cascade do |t| t.string "key", null: false t.text "value" @@ -460,8 +464,8 @@ ActiveRecord::Schema.define(version: 20180327101207) do t.boolean "run_untagged", default: true, null: false t.boolean "locked", default: false, null: false t.integer "access_level", default: 0, null: false - t.string "ip_address" t.integer "maximum_job_timeout" + t.string "ip_address" end add_index "ci_runners", ["contacted_at"], name: "index_ci_runners_on_contacted_at", using: :btree @@ -2031,6 +2035,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do add_foreign_key "ci_builds", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_a2141b1522", on_delete: :nullify add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade + add_foreign_key "ci_builds_metadata", "ci_builds", column: "build_id", name: "fk_e20479742e", on_delete: :cascade add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade add_foreign_key "ci_job_artifacts", "ci_builds", column: "job_id", on_delete: :cascade add_foreign_key "ci_job_artifacts", "projects", on_delete: :cascade diff --git a/spec/javascripts/jobs/mock_data.js b/spec/javascripts/jobs/mock_data.js index 6c3f39f0193..8a7ba50830c 100644 --- a/spec/javascripts/jobs/mock_data.js +++ b/spec/javascripts/jobs/mock_data.js @@ -115,9 +115,9 @@ export default { commit_path: '/root/ci-mock/commit/c58647773a6b5faf066d4ad6ff2c9fbba5f180f6', }, }, - timeout: { - value: '1m 40s', - source: 'runner', + metadata: { + used_timeout_human_readable: '1m 40s', + timeout_source: 'runner', }, merge_request: { iid: 2, diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index c3624158b76..1da84d6caa9 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2047,11 +2047,11 @@ describe Ci::Build do shared_examples 'saves data on transition' do it 'saves used_timeout' do - expect { job.run! }.to change { job.reload.used_timeout }.from(nil).to(expected_timeout) + expect { job.run! }.to change { job.reload.metadata.used_timeout }.from(nil).to(expected_timeout) end it 'saves timeout_source' do - expect { job.run! }.to change { job.reload.timeout_source }.from('unknown_timeout_source').to(expected_timeout_source) + expect { job.run! }.to change { job.reload.metadata.timeout_source }.from('unknown_timeout_source').to(expected_timeout_source) end end diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb index 31db7d055cc..fbbbcd374ee 100644 --- a/spec/models/concerns/chronic_duration_attribute_spec.rb +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -48,7 +48,7 @@ end describe 'ChronicDurationAttribute - reader' do let(:source_field) {:used_timeout} let(:virtual_field) {:used_timeout_human_readable} - subject {Ci::Build.new} + subject {Ci::BuildMetadata.new} it "doesn't contain dynamically created writer method" do expect(subject.class).not_to be_public_method_defined("#{virtual_field}=") diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index e425e80e51e..8de0bdf92e2 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -30,7 +30,7 @@ describe Ci::RetryBuildService do runner_id tag_taggings taggings tags trigger_request_id user_id auto_canceled_by_id retried failure_reason artifacts_file_store artifacts_metadata_store - used_timeout timeout_source].freeze + metadata].freeze shared_examples 'build duplication' do let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } -- cgit v1.2.1 From 2ed1d5418b61f09e27173b364b9ad5f064a07823 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 1 Mar 2018 19:43:10 +0100 Subject: Fix static analysis offences --- app/presenters/ci/build_metadata_presenter.rb | 4 +--- app/presenters/ci/build_presenter.rb | 1 - app/serializers/build_metadata_entity.rb | 1 - spec/javascripts/jobs/sidebar_detail_row_spec.js | 1 - spec/models/concerns/chronic_duration_attribute_spec.rb | 3 ++- 5 files changed, 3 insertions(+), 7 deletions(-) diff --git a/app/presenters/ci/build_metadata_presenter.rb b/app/presenters/ci/build_metadata_presenter.rb index e80eb19ea19..5048f967ea8 100644 --- a/app/presenters/ci/build_metadata_presenter.rb +++ b/app/presenters/ci/build_metadata_presenter.rb @@ -1,6 +1,5 @@ module Ci class BuildMetadataPresenter < Gitlab::View::Presenter::Delegated - TIMEOUT_SOURCES = { unknown_timeout_source: nil, project_timeout_source: 'project', @@ -13,8 +12,7 @@ module Ci return unless metadata.timeout_source? TIMEOUT_SOURCES[metadata.timeout_source.to_sym] || - metadata.timeout_source + metadata.timeout_source end - end end diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index 9345e5069bc..255475e1fe6 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -1,6 +1,5 @@ module Ci class BuildPresenter < Gitlab::View::Presenter::Delegated - presents :build def erased_by_user? diff --git a/app/serializers/build_metadata_entity.rb b/app/serializers/build_metadata_entity.rb index 71c7295ba9f..4b6a538665d 100644 --- a/app/serializers/build_metadata_entity.rb +++ b/app/serializers/build_metadata_entity.rb @@ -1,5 +1,4 @@ class BuildMetadataEntity < Grape::Entity - expose :used_timeout_human_readable do |metadata| metadata.used_timeout_human_readable unless metadata.used_timeout.nil? end diff --git a/spec/javascripts/jobs/sidebar_detail_row_spec.js b/spec/javascripts/jobs/sidebar_detail_row_spec.js index e1f0fe334ac..e66cb4d9809 100644 --- a/spec/javascripts/jobs/sidebar_detail_row_spec.js +++ b/spec/javascripts/jobs/sidebar_detail_row_spec.js @@ -53,5 +53,4 @@ describe('Sidebar detail row', () => { it('should render help when helpUrl is provided', () => { expect(vm.$el.querySelector('.help-button a').getAttribute('href')).toEqual('help url'); }); - }); diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb index fbbbcd374ee..b1ffeb0c74f 100644 --- a/spec/models/concerns/chronic_duration_attribute_spec.rb +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -39,7 +39,8 @@ end describe 'ChronicDurationAttribute' do let(:source_field) {:maximum_job_timeout} let(:virtual_field) {:maximum_job_timeout_human_readable} - subject {Ci::Runner.new} + + subject { Ci::Runner.new } it_behaves_like 'ChronicDurationAttribute reader' it_behaves_like 'ChronicDurationAttribute writer' -- cgit v1.2.1 From 27c4cdbe288e3dba35eebdd50dbc6e103346a8c4 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 1 Mar 2018 20:03:48 +0100 Subject: Update tests - remove unneeded change --- spec/lib/gitlab/import_export/safe_model_attributes.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index bd7e60a5d9e..0716852f57f 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -283,8 +283,6 @@ CommitStatus: - retried - protected - failure_reason -- used_timeout -- timeout_source Ci::Variable: - id - project_id -- cgit v1.2.1 From 9ef86b1b12e80bcc3a6277e2b6a36b793c828489 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 1 Mar 2018 20:04:15 +0100 Subject: Fix transition failure when metadata not exists --- app/models/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 4b4a988d600..cc29c6150d3 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -161,7 +161,7 @@ module Ci end before_transition pending: :running do |build| - build.metadata.save_timeout_state! + build.metadata.save_timeout_state! unless build.metadata.nil? end end -- cgit v1.2.1 From bb64b20a1f7dd831e4c200343b531f9d048c02a8 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 5 Mar 2018 17:35:06 +0100 Subject: Refactorize Ci::Build and Ci::BuildMetadata models --- app/models/ci/build.rb | 14 +++++++------- app/models/ci/build_metadata.rb | 16 +++++++++++----- .../20180301010859_create_ci_builds_metadata_table.rb | 7 +++++-- ...011323_add_build_foreign_key_to_ci_builds_metadata.rb | 15 --------------- db/schema.rb | 7 +++---- spec/models/ci/build_spec.rb | 10 +++++++--- 6 files changed, 33 insertions(+), 36 deletions(-) delete mode 100644 db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index cc29c6150d3..7a12d7a3deb 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -24,7 +24,7 @@ module Ci has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id - has_one :metadata, class_name: 'Ci::BuildMetadata' + has_one :build_metadata, class_name: 'Ci::BuildMetadata' # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @@ -85,10 +85,6 @@ module Ci before_save :ensure_token before_destroy { unscoped_project } - before_create do |build| - build.metadata = Ci::BuildMetadata.new - end - after_create unless: :importing? do |build| run_after_commit { BuildHooksWorker.perform_async(build.id) } end @@ -161,10 +157,14 @@ module Ci end before_transition pending: :running do |build| - build.metadata.save_timeout_state! unless build.metadata.nil? + build.metadata.save_timeout_state! end end + def metadata + self.build_metadata ||= Ci::BuildMetadata.new + end + def detailed_status(current_user) Gitlab::Ci::Status::Build::Factory .new(self, current_user) @@ -242,7 +242,7 @@ module Ci end def timeout - [project.build_timeout, runner&.maximum_job_timeout].compact.min + metadata.used_timeout end def triggered_by?(current_user) diff --git a/app/models/ci/build_metadata.rb b/app/models/ci/build_metadata.rb index 7a1315dfcf9..9043bed86bf 100644 --- a/app/models/ci/build_metadata.rb +++ b/app/models/ci/build_metadata.rb @@ -1,4 +1,6 @@ module Ci + # The purpose of this class is to store Build related data that can be disposed. + # Data that should be persisted forever, should be stored with Ci::Build model. class BuildMetadata < ActiveRecord::Base extend Gitlab::Ci::Model include Presentable @@ -11,14 +13,18 @@ module Ci chronic_duration_attr_reader :used_timeout_human_readable, :used_timeout enum timeout_source: { - unknown_timeout_source: nil, - project_timeout_source: 1, - runner_timeout_source: 2 + unknown_timeout_source: 1, + project_timeout_source: 2, + runner_timeout_source: 3 } def save_timeout_state! - self.used_timeout = build.timeout - self.timeout_source = build.timeout < build.project.build_timeout ? :runner_timeout_source : :project_timeout_source + project_timeout = build.project&.build_timeout + timeout = [project_timeout, build.runner&.maximum_job_timeout].compact.min + + self.used_timeout = timeout + self.timeout_source = timeout < project_timeout ? :runner_timeout_source : :project_timeout_source + save! end end diff --git a/db/migrate/20180301010859_create_ci_builds_metadata_table.rb b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb index 9d5e9c1779b..cd7824d7788 100644 --- a/db/migrate/20180301010859_create_ci_builds_metadata_table.rb +++ b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb @@ -4,10 +4,13 @@ class CreateCiBuildsMetadataTable < ActiveRecord::Migration DOWNTIME = false def change - create_table :ci_builds_metadata do |t| + create_table :ci_builds_metadata, id: false do |t| t.integer :build_id, null: false t.integer :used_timeout - t.integer :timeout_source + t.integer :timeout_source, null: false, default: 1 + + t.primary_key :build_id + t.foreign_key :ci_builds, column: :build_id, on_delete: :cascade end end end diff --git a/db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb b/db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb deleted file mode 100644 index feda2d6e9c9..00000000000 --- a/db/migrate/20180301011323_add_build_foreign_key_to_ci_builds_metadata.rb +++ /dev/null @@ -1,15 +0,0 @@ -class AddBuildForeignKeyToCiBuildsMetadata < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_concurrent_foreign_key(:ci_builds_metadata, :ci_builds, column: :build_id) - end - - def down - remove_foreign_key(:ci_builds_metadata, column: :build_id) - end -end diff --git a/db/schema.rb b/db/schema.rb index 5463b3f1219..920d71e0110 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -329,10 +329,9 @@ ActiveRecord::Schema.define(version: 20180327101207) do add_index "ci_builds", ["updated_at"], name: "index_ci_builds_on_updated_at", using: :btree add_index "ci_builds", ["user_id"], name: "index_ci_builds_on_user_id", using: :btree - create_table "ci_builds_metadata", force: :cascade do |t| - t.integer "build_id", null: false + create_table "ci_builds_metadata", primary_key: "build_id", force: :cascade do |t| t.integer "used_timeout" - t.integer "timeout_source" + t.integer "timeout_source", default: 1, null: false end create_table "ci_group_variables", force: :cascade do |t| @@ -2035,7 +2034,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do add_foreign_key "ci_builds", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_a2141b1522", on_delete: :nullify add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade - add_foreign_key "ci_builds_metadata", "ci_builds", column: "build_id", name: "fk_e20479742e", on_delete: :cascade + add_foreign_key "ci_builds_metadata", "ci_builds", column: "build_id", on_delete: :cascade add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade add_foreign_key "ci_job_artifacts", "ci_builds", column: "job_id", on_delete: :cascade add_foreign_key "ci_job_artifacts", "projects", on_delete: :cascade diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 1da84d6caa9..670803a0883 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1275,17 +1275,21 @@ describe Ci::Build do set(:project2) { create(:project, :repository, group: group, build_timeout: 1000) } set(:pipeline2) { create(:ci_pipeline, project: project2, sha: project2.commit.id, ref: project2.default_branch, status: 'success') } - let(:build) { create(:ci_build, :manual, pipeline: pipeline2) } + let(:build) { create(:ci_build, :pending, pipeline: pipeline2) } subject { build.timeout } + before do + build.run! + end + it 'returns project timeout configuration' do is_expected.to eq(project2.build_timeout) end context 'when runner sets timeout to bigger value' do let(:runner2) { create(:ci_runner, maximum_job_timeout: 2000) } - let(:build) { create(:ci_build, :manual, pipeline: pipeline2, runner: runner2) } + let(:build) { create(:ci_build, :pending, pipeline: pipeline2, runner: runner2) } it 'returns project timeout configuration' do is_expected.to eq(project2.build_timeout) @@ -1294,7 +1298,7 @@ describe Ci::Build do context 'when runner sets timeout to smaller value' do let(:runner2) { create(:ci_runner, maximum_job_timeout: 500) } - let(:build) { create(:ci_build, :manual, pipeline: pipeline2, runner: runner2) } + let(:build) { create(:ci_build, :pending, pipeline: pipeline2, runner: runner2) } it 'returns project timeout configuration' do is_expected.to eq(runner2.maximum_job_timeout) -- cgit v1.2.1 From d02694c5c53265ccd9e8a51a52dc8bb8bcbff656 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 5 Mar 2018 17:38:58 +0100 Subject: Use help_page_path helper to generate runner_help_url --- app/views/projects/jobs/show.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/jobs/show.html.haml b/app/views/projects/jobs/show.html.haml index 4d2bc3a04a4..fa27ded7cc2 100644 --- a/app/views/projects/jobs/show.html.haml +++ b/app/views/projects/jobs/show.html.haml @@ -111,4 +111,4 @@ .js-build-options{ data: javascript_build_options } -#js-job-details-vue{ data: { endpoint: project_job_path(@project, @build, format: :json), runner_help_url: '/help/ci/runners/README.html#setting-maximum-job-timeout-for-a-runner' } } +#js-job-details-vue{ data: { endpoint: project_job_path(@project, @build, format: :json), runner_help_url: help_page_path('ci/runners/README.html', anchor: 'setting-maximum-job-timeout-for-a-runner') } } -- cgit v1.2.1 From 62f053e4e50dd04933d49622a74dcb89ebe8174e Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 5 Mar 2018 17:49:40 +0100 Subject: Update runner registration API --- lib/api/runner.rb | 5 +++-- spec/requests/api/runner_spec.rb | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 74ddaf26bb6..73d5993701c 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -14,11 +14,12 @@ module API optional :locked, type: Boolean, desc: 'Should Runner be locked for current project' optional :run_untagged, type: Boolean, desc: 'Should Runner handle untagged jobs' optional :tag_list, type: Array[String], desc: %q(List of Runner's tags) - optional :maximum_job_timeout_human_readable, type: String, desc: 'Maximum timeout set when this Runner will handle the job' + optional :maximum_job_timeout, type: String, desc: 'Maximum timeout set when this Runner will handle the job' end post '/' do - attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list, :maximum_job_timeout_human_readable]) + attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list]) .merge(get_runner_details_from_request) + .merge(maximum_job_timeout_human_readable: params[:maximum_job_timeout]) runner = if runner_registration_token_valid? diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 55c4150a393..0ca7ba7154e 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -112,7 +112,7 @@ describe API::Runner do context 'when maximum job timeout is specified' do it 'creates runner' do post api('/runners'), token: registration_token, - maximum_job_timeout_human_readable: '2h 30m' + maximum_job_timeout: '2h 30m' expect(response).to have_gitlab_http_status 201 expect(Ci::Runner.first.maximum_job_timeout).to eq(9000) @@ -121,7 +121,7 @@ describe API::Runner do context 'when maximum job timeout is empty' do it 'creates runner' do post api('/runners'), token: registration_token, - maximum_job_timeout_human_readable: '' + maximum_job_timeout: '' expect(response).to have_gitlab_http_status 201 expect(Ci::Runner.first.maximum_job_timeout).to be_nil -- cgit v1.2.1 From 1e138767a652d86458d38665b98c9c2e5d4c3cb8 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 5 Mar 2018 20:22:00 +0100 Subject: Fix tests failures --- app/models/concerns/chronic_duration_attribute.rb | 4 ++-- spec/javascripts/jobs/sidebar_detail_row_spec.js | 1 + .../concerns/chronic_duration_attribute_spec.rb | 20 +++++++++++++++++++- spec/services/ci/retry_build_service_spec.rb | 2 +- 4 files changed, 23 insertions(+), 4 deletions(-) diff --git a/app/models/concerns/chronic_duration_attribute.rb b/app/models/concerns/chronic_duration_attribute.rb index 87c60a2b7d5..15095d0b758 100644 --- a/app/models/concerns/chronic_duration_attribute.rb +++ b/app/models/concerns/chronic_duration_attribute.rb @@ -19,8 +19,8 @@ module ChronicDurationAttribute def chronic_duration_attr_writer(virtual_attribute, source_attribute) define_method("#{virtual_attribute}=") do |value| - new_value = ChronicDuration.parse(value).to_i - new_value = nil if new_value <= 0 + new_value = ChronicDuration.parse(value).to_i unless value.nil? + new_value = nil if !new_value.nil? && new_value <= 0 self.send("#{source_attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend diff --git a/spec/javascripts/jobs/sidebar_detail_row_spec.js b/spec/javascripts/jobs/sidebar_detail_row_spec.js index e66cb4d9809..9fe5cf78b8b 100644 --- a/spec/javascripts/jobs/sidebar_detail_row_spec.js +++ b/spec/javascripts/jobs/sidebar_detail_row_spec.js @@ -46,6 +46,7 @@ describe('Sidebar detail row', () => { vm = new SidebarDetailRow({ propsData: { helpUrl: 'help url', + value: 'foo', }, }).$mount(); }); diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb index b1ffeb0c74f..b25475d4fdb 100644 --- a/spec/models/concerns/chronic_duration_attribute_spec.rb +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -29,11 +29,29 @@ shared_examples 'ChronicDurationAttribute writer' do expect(subject.send(source_field)).to eq(600) end - it 'writes null when empty input is used' do + it 'writes nil when empty input is used' do subject.send("#{virtual_field}=", '') expect(subject.send(source_field)).to be_nil end + + it 'writes nil when negative input is used' do + allow(ChronicDuration).to receive(:parse).and_return(-10) + + subject.send("#{virtual_field}=", '-10m') + + expect(subject.send(source_field)).to be_nil + end + + it 'writes nil when nil input is used' do + subject.send("#{virtual_field}=", nil) + + expect(subject.send(source_field)).to be_nil + end + + it "doesn't raise exception when nil input is used" do + expect { subject.send("#{virtual_field}=", nil) }.not_to raise_error(NoMethodError) + end end describe 'ChronicDurationAttribute' do diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 8de0bdf92e2..415302e84f2 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -30,7 +30,7 @@ describe Ci::RetryBuildService do runner_id tag_taggings taggings tags trigger_request_id user_id auto_canceled_by_id retried failure_reason artifacts_file_store artifacts_metadata_store - metadata].freeze + build_metadata].freeze shared_examples 'build duplication' do let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } -- cgit v1.2.1 From f5e602ee0f8d95617adf6fb9b5a1a132a471fb12 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 6 Mar 2018 16:14:23 +0100 Subject: Rename maximum_job_timeout to maximum_timeout --- app/models/ci/build_metadata.rb | 2 +- app/models/ci/runner.rb | 4 ++-- app/views/projects/runners/_form.html.haml | 4 ++-- app/views/projects/runners/show.html.haml | 2 +- .../20180219153455_add_maximum_job_timeout_to_ci_runners.rb | 9 --------- .../20180219153455_add_maximum_timeout_to_ci_runners.rb | 9 +++++++++ db/schema.rb | 2 +- doc/api/runners.md | 6 +++--- lib/api/entities.rb | 2 +- lib/api/runner.rb | 4 ++-- lib/api/runners.rb | 2 +- spec/models/ci/build_spec.rb | 10 +++++----- spec/models/concerns/chronic_duration_attribute_spec.rb | 4 ++-- spec/requests/api/runner_spec.rb | 12 ++++++------ spec/requests/api/runners_spec.rb | 6 +++--- 15 files changed, 39 insertions(+), 39 deletions(-) delete mode 100644 db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb create mode 100644 db/migrate/20180219153455_add_maximum_timeout_to_ci_runners.rb diff --git a/app/models/ci/build_metadata.rb b/app/models/ci/build_metadata.rb index 9043bed86bf..89fee37b1b6 100644 --- a/app/models/ci/build_metadata.rb +++ b/app/models/ci/build_metadata.rb @@ -20,7 +20,7 @@ module Ci def save_timeout_state! project_timeout = build.project&.build_timeout - timeout = [project_timeout, build.runner&.maximum_job_timeout].compact.min + timeout = [project_timeout, build.runner&.maximum_timeout].compact.min self.used_timeout = timeout self.timeout_source = timeout < project_timeout ? :runner_timeout_source : :project_timeout_source diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 413607e0eff..62cc636db22 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -9,7 +9,7 @@ module Ci ONLINE_CONTACT_TIMEOUT = 1.hour UPDATE_DB_RUNNER_INFO_EVERY = 40.minutes AVAILABLE_SCOPES = %w[specific shared active paused online].freeze - FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_job_timeout_human_readable].freeze + FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze has_many :builds has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -52,7 +52,7 @@ module Ci cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at, :ip_address - chronic_duration_attr :maximum_job_timeout_human_readable, :maximum_job_timeout + chronic_duration_attr :maximum_timeout_human_readable, :maximum_timeout # Searches for runners matching the given query. # diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 7e9435e0110..6a681736b6f 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -40,10 +40,10 @@ .col-sm-10 = f.text_field :description, class: 'form-control' .form-group - = label_tag :maximum_job_timeout_human_readable, class: 'control-label' do + = label_tag :maximum_timeout_human_readable, class: 'control-label' do Maximum job timeout .col-sm-10 - = f.text_field :maximum_job_timeout_human_readable, class: 'form-control' + = f.text_field :maximum_timeout_human_readable, class: 'form-control' .help-block This timeout will take precedence when lower than Project-defined timeout .form-group = label_tag :tag_list, class: 'control-label' do diff --git a/app/views/projects/runners/show.html.haml b/app/views/projects/runners/show.html.haml index 67084e3d66a..f33e7e25b68 100644 --- a/app/views/projects/runners/show.html.haml +++ b/app/views/projects/runners/show.html.haml @@ -57,7 +57,7 @@ %td= @runner.description %tr %td Maximum job timeout - %td= @runner.maximum_job_timeout_human_readable + %td= @runner.maximum_timeout_human_readable %tr %td Last contact %td diff --git a/db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb b/db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb deleted file mode 100644 index 1ad7bb86e72..00000000000 --- a/db/migrate/20180219153455_add_maximum_job_timeout_to_ci_runners.rb +++ /dev/null @@ -1,9 +0,0 @@ -class AddMaximumJobTimeoutToCiRunners < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - def change - add_column :ci_runners, :maximum_job_timeout, :integer - end -end diff --git a/db/migrate/20180219153455_add_maximum_timeout_to_ci_runners.rb b/db/migrate/20180219153455_add_maximum_timeout_to_ci_runners.rb new file mode 100644 index 00000000000..072e696a43e --- /dev/null +++ b/db/migrate/20180219153455_add_maximum_timeout_to_ci_runners.rb @@ -0,0 +1,9 @@ +class AddMaximumTimeoutToCiRunners < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_runners, :maximum_timeout, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 920d71e0110..a2967409703 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -463,7 +463,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do t.boolean "run_untagged", default: true, null: false t.boolean "locked", default: false, null: false t.integer "access_level", default: 0, null: false - t.integer "maximum_job_timeout" + t.integer "maximum_timeout" t.string "ip_address" end diff --git a/doc/api/runners.md b/doc/api/runners.md index 348fd499af2..f384ac57bfe 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -154,7 +154,7 @@ Example response: ], "version": null, "access_level": "ref_protected", - "maximum_job_timeout": 3600 + "maximum_timeout": 3600 } ``` @@ -175,7 +175,7 @@ PUT /runners/:id | `run_untagged` | boolean | no | Flag indicating the runner can execute untagged jobs | | `locked` | boolean | no | Flag indicating the runner is locked | | `access_level` | string | no | The access_level of the runner; `not_protected` or `ref_protected` | -| `maximum_job_timeout` | integer | no | Maximum timeout set when this Runner will handle the job | +| `maximum_timeout` | integer | no | Maximum timeout set when this Runner will handle the job | ``` curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/runners/6" --form "description=test-1-20150125-test" --form "tag_list=ruby,mysql,tag1,tag2" @@ -214,7 +214,7 @@ Example response: ], "version": null, "access_level": "ref_protected", - "maximum_job_timeout": null + "maximum_timeout": null } ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index bb18fa00dc6..324e14f5654 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -951,7 +951,7 @@ module API expose :tag_list expose :run_untagged expose :locked - expose :maximum_job_timeout + expose :maximum_timeout expose :access_level expose :version, :revision, :platform, :architecture expose :contacted_at diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 73d5993701c..95f57e6402d 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -14,12 +14,12 @@ module API optional :locked, type: Boolean, desc: 'Should Runner be locked for current project' optional :run_untagged, type: Boolean, desc: 'Should Runner handle untagged jobs' optional :tag_list, type: Array[String], desc: %q(List of Runner's tags) - optional :maximum_job_timeout, type: String, desc: 'Maximum timeout set when this Runner will handle the job' + optional :maximum_timeout, type: String, desc: 'Maximum timeout set when this Runner will handle the job' end post '/' do attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list]) .merge(get_runner_details_from_request) - .merge(maximum_job_timeout_human_readable: params[:maximum_job_timeout]) + .merge(maximum_timeout_human_readable: params[:maximum_timeout]) runner = if runner_registration_token_valid? diff --git a/lib/api/runners.rb b/lib/api/runners.rb index b3037235353..5f2a9567605 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -57,7 +57,7 @@ module API optional :locked, type: Boolean, desc: 'Flag indicating the runner is locked' optional :access_level, type: String, values: Ci::Runner.access_levels.keys, desc: 'The access_level of the runner' - optional :maximum_job_timeout, type: Integer, desc: 'Maximum timeout set when this Runner will handle the job' + optional :maximum_timeout, type: Integer, desc: 'Maximum timeout set when this Runner will handle the job' at_least_one_of :description, :active, :tag_list, :run_untagged, :locked, :access_level end put ':id' do diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 670803a0883..abb2d0b5ecb 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1288,7 +1288,7 @@ describe Ci::Build do end context 'when runner sets timeout to bigger value' do - let(:runner2) { create(:ci_runner, maximum_job_timeout: 2000) } + let(:runner2) { create(:ci_runner, maximum_timeout: 2000) } let(:build) { create(:ci_build, :pending, pipeline: pipeline2, runner: runner2) } it 'returns project timeout configuration' do @@ -1297,11 +1297,11 @@ describe Ci::Build do end context 'when runner sets timeout to smaller value' do - let(:runner2) { create(:ci_runner, maximum_job_timeout: 500) } + let(:runner2) { create(:ci_runner, maximum_timeout: 500) } let(:build) { create(:ci_build, :pending, pipeline: pipeline2, runner: runner2) } it 'returns project timeout configuration' do - is_expected.to eq(runner2.maximum_job_timeout) + is_expected.to eq(runner2.maximum_timeout) end end end @@ -2064,7 +2064,7 @@ describe Ci::Build do let(:expected_timeout_source) { 'runner_timeout_source' } before do - runner.maximum_job_timeout = 900 + runner.maximum_timeout = 900 runner.save! end @@ -2076,7 +2076,7 @@ describe Ci::Build do let(:expected_timeout_source) { 'project_timeout_source' } before do - runner.maximum_job_timeout = 3600 + runner.maximum_timeout = 3600 runner.save! end diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb index b25475d4fdb..c991d12a5fc 100644 --- a/spec/models/concerns/chronic_duration_attribute_spec.rb +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -55,8 +55,8 @@ shared_examples 'ChronicDurationAttribute writer' do end describe 'ChronicDurationAttribute' do - let(:source_field) {:maximum_job_timeout} - let(:virtual_field) {:maximum_job_timeout_human_readable} + let(:source_field) {:maximum_timeout} + let(:virtual_field) {:maximum_timeout_human_readable} subject { Ci::Runner.new } diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 0ca7ba7154e..6676ceaab66 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -112,19 +112,19 @@ describe API::Runner do context 'when maximum job timeout is specified' do it 'creates runner' do post api('/runners'), token: registration_token, - maximum_job_timeout: '2h 30m' + maximum_timeout: '2h 30m' expect(response).to have_gitlab_http_status 201 - expect(Ci::Runner.first.maximum_job_timeout).to eq(9000) + expect(Ci::Runner.first.maximum_timeout).to eq(9000) end context 'when maximum job timeout is empty' do it 'creates runner' do post api('/runners'), token: registration_token, - maximum_job_timeout: '' + maximum_timeout: '' expect(response).to have_gitlab_http_status 201 - expect(Ci::Runner.first.maximum_job_timeout).to be_nil + expect(Ci::Runner.first.maximum_timeout).to be_nil end end end @@ -681,7 +681,7 @@ describe API::Runner do end context 'when runner specifies lower timeout' do - let(:runner) { create(:ci_runner, maximum_job_timeout: 1000) } + let(:runner) { create(:ci_runner, maximum_timeout: 1000) } it 'contains info about timeout overridden by runner' do request_job @@ -692,7 +692,7 @@ describe API::Runner do end context 'when runner specifies bigger timeout' do - let(:runner) { create(:ci_runner, maximum_job_timeout: 2000) } + let(:runner) { create(:ci_runner, maximum_timeout: 2000) } it 'contains info about timeout not overridden by runner' do request_job diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 836b22f5657..247e8795ed4 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -123,7 +123,7 @@ describe API::Runners do expect(response).to have_gitlab_http_status(200) expect(json_response['description']).to eq(shared_runner.description) - expect(json_response['maximum_job_timeout']).to be_nil + expect(json_response['maximum_timeout']).to be_nil end end @@ -194,7 +194,7 @@ describe API::Runners do run_untagged: 'false', locked: 'true', access_level: 'ref_protected', - maximum_job_timeout: 1234 ) + maximum_timeout: 1234 ) shared_runner.reload expect(response).to have_gitlab_http_status(200) @@ -206,7 +206,7 @@ describe API::Runners do expect(shared_runner.ref_protected?).to be_truthy expect(shared_runner.ensure_runner_queue_value) .not_to eq(runner_queue_value) - expect(shared_runner.maximum_job_timeout).to eq(1234) + expect(shared_runner.maximum_timeout).to eq(1234) end end -- cgit v1.2.1 From d58d3098f159a17fbcf1ae27165c249722990988 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 6 Mar 2018 16:25:13 +0100 Subject: Rename used_timeout to timeout --- app/assets/javascripts/jobs/components/sidebar_details_block.vue | 4 ++-- app/models/ci/build.rb | 2 +- app/models/ci/build_metadata.rb | 4 ++-- app/serializers/build_metadata_entity.rb | 4 ++-- db/migrate/20180301010859_create_ci_builds_metadata_table.rb | 2 +- db/schema.rb | 4 ++-- spec/javascripts/jobs/mock_data.js | 2 +- spec/models/ci/build_spec.rb | 4 ++-- spec/models/concerns/chronic_duration_attribute_spec.rb | 4 ++-- 9 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/jobs/components/sidebar_details_block.vue b/app/assets/javascripts/jobs/components/sidebar_details_block.vue index 15584922d1f..6ff3fa6e099 100644 --- a/app/assets/javascripts/jobs/components/sidebar_details_block.vue +++ b/app/assets/javascripts/jobs/components/sidebar_details_block.vue @@ -45,7 +45,7 @@ return `#${this.job.runner.id}`; }, timeout() { - let t = `${this.job.metadata.used_timeout_human_readable}`; + let t = `${this.job.metadata.timeout_human_readable}`; if (this.job.metadata.timeout_source != null) { t += ` (from ${this.job.metadata.timeout_source})`; @@ -130,7 +130,7 @@ /> Date: Tue, 6 Mar 2018 16:43:44 +0100 Subject: BuildMetadata styling improvements --- app/models/ci/build_metadata.rb | 10 +++++----- spec/models/ci/build_spec.rb | 15 +++++++-------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/app/models/ci/build_metadata.rb b/app/models/ci/build_metadata.rb index 335209e8ec2..424b6da1014 100644 --- a/app/models/ci/build_metadata.rb +++ b/app/models/ci/build_metadata.rb @@ -19,13 +19,13 @@ module Ci } def save_timeout_state! - project_timeout = build.project&.build_timeout - timeout = [project_timeout, build.runner&.maximum_timeout].compact.min + return unless build.runner.present? - self.timeout = timeout - self.timeout_source = timeout < project_timeout ? :runner_timeout_source : :project_timeout_source + project_timeout = build.project&.build_timeout + timeout = [project_timeout, build.runner.maximum_timeout].compact.min + timeout_source = timeout < project_timeout ? :runner_timeout_source : :project_timeout_source - save! + update_attributes(timeout: timeout, timeout_source: timeout_source) end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index cdc2c5a8679..6affd05d8aa 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1283,8 +1283,10 @@ describe Ci::Build do build.run! end - it 'returns project timeout configuration' do - is_expected.to eq(project2.build_timeout) + context 'when runner is not assigned' do + it 'returns project timeout configuration' do + is_expected.to be_nil + end end context 'when runner sets timeout to bigger value' do @@ -2045,8 +2047,7 @@ describe Ci::Build do let(:job) { create(:ci_build, :pending, runner: runner) } before do - job.project.build_timeout = 1800 - job.project.save! + job.project.update_attribute(:build_timeout, 1800) end shared_examples 'saves data on transition' do @@ -2064,8 +2065,7 @@ describe Ci::Build do let(:expected_timeout_source) { 'runner_timeout_source' } before do - runner.maximum_timeout = 900 - runner.save! + runner.update_attribute(:maximum_timeout, 900) end it_behaves_like 'saves data on transition' @@ -2076,8 +2076,7 @@ describe Ci::Build do let(:expected_timeout_source) { 'project_timeout_source' } before do - runner.maximum_timeout = 3600 - runner.save! + runner.update_attribute(:maximum_timeout, 3600) end it_behaves_like 'saves data on transition' -- cgit v1.2.1 From 4c3482345a3bcc9ac5d29f5dbc55c966ca5a0c72 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 7 Mar 2018 23:53:15 +0100 Subject: Fix sidebar_detail_row_spec.js --- spec/javascripts/jobs/sidebar_detail_row_spec.js | 34 +++++++++++++----------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/spec/javascripts/jobs/sidebar_detail_row_spec.js b/spec/javascripts/jobs/sidebar_detail_row_spec.js index 9fe5cf78b8b..e6bfb0c4adc 100644 --- a/spec/javascripts/jobs/sidebar_detail_row_spec.js +++ b/spec/javascripts/jobs/sidebar_detail_row_spec.js @@ -38,20 +38,24 @@ describe('Sidebar detail row', () => { ).toEqual('this is the title: this is the value'); }); - it('should not render help when helpUrl not provided', () => { - expect(vm.$el.querySelector('.help-button')).toBeUndefined(); - }); - - beforeEach(() => { - vm = new SidebarDetailRow({ - propsData: { - helpUrl: 'help url', - value: 'foo', - }, - }).$mount(); - }); - - it('should render help when helpUrl is provided', () => { - expect(vm.$el.querySelector('.help-button a').getAttribute('href')).toEqual('help url'); + describe('when helpUrl not provided', () => { + it('should not render help', () => { + expect(vm.$el.querySelector('.help-button')).toBeNull(); + }); + }); + + describe('when helpUrl provided', () => { + beforeEach(() => { + vm = new SidebarDetailRow({ + propsData: { + helpUrl: 'help url', + value: 'foo', + }, + }).$mount(); + }); + + it('should render help', () => { + expect(vm.$el.querySelector('.help-button a').getAttribute('href')).toEqual('help url'); + }); }); }); -- cgit v1.2.1 From afcc57abfdcb11001803655f938187cbdc96b67c Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 20 Mar 2018 23:21:17 +0100 Subject: Rename metadata relation and methods --- app/models/ci/build.rb | 10 +++++----- app/serializers/build_details_entity.rb | 4 +++- spec/models/ci/build_spec.rb | 4 ++-- spec/services/ci/retry_build_service_spec.rb | 2 +- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 405c89d0103..0b3c6ac4fee 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -24,7 +24,7 @@ module Ci has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id - has_one :build_metadata, class_name: 'Ci::BuildMetadata' + has_one :metadata, class_name: 'Ci::BuildMetadata' # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @@ -157,12 +157,12 @@ module Ci end before_transition pending: :running do |build| - build.metadata.save_timeout_state! + build.ensure_metadata.save_timeout_state! end end - def metadata - self.build_metadata ||= Ci::BuildMetadata.new + def ensure_metadata + metadata || build_metadata end def detailed_status(current_user) @@ -242,7 +242,7 @@ module Ci end def timeout - metadata.timeout + ensure_metadata.timeout end def triggered_by?(current_user) diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb index ca4480fe2b1..99ca0bd158b 100644 --- a/app/serializers/build_details_entity.rb +++ b/app/serializers/build_details_entity.rb @@ -5,7 +5,9 @@ class BuildDetailsEntity < JobEntity expose :runner, using: RunnerEntity expose :pipeline, using: PipelineEntity - expose :metadata, using: BuildMetadataEntity + expose :metadata, using: BuildMetadataEntity do |build| + build.ensure_metadata + end expose :erased_by, if: -> (*) { build.erased? }, using: UserEntity expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :erase_build, build) } do |build| diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 6affd05d8aa..b209d53ae08 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2052,11 +2052,11 @@ describe Ci::Build do shared_examples 'saves data on transition' do it 'saves timeout' do - expect { job.run! }.to change { job.reload.metadata.timeout }.from(nil).to(expected_timeout) + expect { job.run! }.to change { job.reload.ensure_metadata.timeout }.from(nil).to(expected_timeout) end it 'saves timeout_source' do - expect { job.run! }.to change { job.reload.metadata.timeout_source }.from('unknown_timeout_source').to(expected_timeout_source) + expect { job.run! }.to change { job.reload.ensure_metadata.timeout_source }.from('unknown_timeout_source').to(expected_timeout_source) end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 415302e84f2..8de0bdf92e2 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -30,7 +30,7 @@ describe Ci::RetryBuildService do runner_id tag_taggings taggings tags trigger_request_id user_id auto_canceled_by_id retried failure_reason artifacts_file_store artifacts_metadata_store - build_metadata].freeze + metadata].freeze shared_examples 'build duplication' do let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } -- cgit v1.2.1 From a1cd3390e550c0008e83146e8627fdee3f58e422 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Mar 2018 01:20:15 +0100 Subject: Add project_id column to Ci::BuildMetadata --- app/models/ci/build_metadata.rb | 11 +++++++++++ db/migrate/20180301010859_create_ci_builds_metadata_table.rb | 4 ++++ db/schema.rb | 4 ++++ 3 files changed, 19 insertions(+) diff --git a/app/models/ci/build_metadata.rb b/app/models/ci/build_metadata.rb index 424b6da1014..c221f43384b 100644 --- a/app/models/ci/build_metadata.rb +++ b/app/models/ci/build_metadata.rb @@ -9,9 +9,12 @@ module Ci self.table_name = 'ci_builds_metadata' belongs_to :build, class_name: 'Ci::Build' + belongs_to :project chronic_duration_attr_reader :timeout_human_readable, :timeout + after_initialize :set_project_id + enum timeout_source: { unknown_timeout_source: 1, project_timeout_source: 2, @@ -27,5 +30,13 @@ module Ci update_attributes(timeout: timeout, timeout_source: timeout_source) end + + private + + def set_project_id + return unless self.project_id.nil? + + self.project_id = build&.project&.id + end end end diff --git a/db/migrate/20180301010859_create_ci_builds_metadata_table.rb b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb index 72c204026d8..650bf43835d 100644 --- a/db/migrate/20180301010859_create_ci_builds_metadata_table.rb +++ b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb @@ -6,11 +6,15 @@ class CreateCiBuildsMetadataTable < ActiveRecord::Migration def change create_table :ci_builds_metadata, id: false do |t| t.integer :build_id, null: false + t.integer :project_id, null: false t.integer :timeout t.integer :timeout_source, null: false, default: 1 t.primary_key :build_id t.foreign_key :ci_builds, column: :build_id, on_delete: :cascade + t.foreign_key :projects, column: :project_id, on_delete: :cascade + + t.index :project_id end end end diff --git a/db/schema.rb b/db/schema.rb index a14823dfa15..56541aa4ecd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -330,10 +330,13 @@ ActiveRecord::Schema.define(version: 20180327101207) do add_index "ci_builds", ["user_id"], name: "index_ci_builds_on_user_id", using: :btree create_table "ci_builds_metadata", primary_key: "build_id", force: :cascade do |t| + t.integer "project_id", null: false t.integer "timeout" t.integer "timeout_source", default: 1, null: false end + add_index "ci_builds_metadata", ["project_id"], name: "index_ci_builds_metadata_on_project_id", using: :btree + create_table "ci_group_variables", force: :cascade do |t| t.string "key", null: false t.text "value" @@ -2035,6 +2038,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade add_foreign_key "ci_builds_metadata", "ci_builds", column: "build_id", on_delete: :cascade + add_foreign_key "ci_builds_metadata", "projects", on_delete: :cascade add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade add_foreign_key "ci_job_artifacts", "ci_builds", column: "job_id", on_delete: :cascade add_foreign_key "ci_job_artifacts", "projects", on_delete: :cascade -- cgit v1.2.1 From 9ccde9cc1f1ccb70cc455686a5c7aed94adb8876 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Mar 2018 01:23:59 +0100 Subject: Fix style problem in spec/requests/api/runners_spec.rb --- spec/requests/api/runners_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 247e8795ed4..d30f0cf36e2 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -194,7 +194,7 @@ describe API::Runners do run_untagged: 'false', locked: 'true', access_level: 'ref_protected', - maximum_timeout: 1234 ) + maximum_timeout: 1234) shared_runner.reload expect(response).to have_gitlab_http_status(200) -- cgit v1.2.1 From 557c85a79f141e435c56d971d813f492978bad61 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Mar 2018 02:02:38 +0100 Subject: Use raw value of maximum_timeout for Runner registration API --- lib/api/runner.rb | 5 ++--- spec/requests/api/runner_spec.rb | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 95f57e6402d..57c0a729535 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -14,12 +14,11 @@ module API optional :locked, type: Boolean, desc: 'Should Runner be locked for current project' optional :run_untagged, type: Boolean, desc: 'Should Runner handle untagged jobs' optional :tag_list, type: Array[String], desc: %q(List of Runner's tags) - optional :maximum_timeout, type: String, desc: 'Maximum timeout set when this Runner will handle the job' + optional :maximum_timeout, type: Integer, desc: 'Maximum timeout set when this Runner will handle the job' end post '/' do - attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list]) + attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list, :maximum_timeout]) .merge(get_runner_details_from_request) - .merge(maximum_timeout_human_readable: params[:maximum_timeout]) runner = if runner_registration_token_valid? diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 6676ceaab66..4703c64c534 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -112,7 +112,7 @@ describe API::Runner do context 'when maximum job timeout is specified' do it 'creates runner' do post api('/runners'), token: registration_token, - maximum_timeout: '2h 30m' + maximum_timeout: 9000 expect(response).to have_gitlab_http_status 201 expect(Ci::Runner.first.maximum_timeout).to eq(9000) -- cgit v1.2.1 From ed86344f4941ce8b30546260b8437451026c8d97 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Mar 2018 02:58:43 +0100 Subject: Add tests for Ci::BuildMetadata --- spec/models/ci/build_metadata_spec.rb | 69 +++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 spec/models/ci/build_metadata_spec.rb diff --git a/spec/models/ci/build_metadata_spec.rb b/spec/models/ci/build_metadata_spec.rb new file mode 100644 index 00000000000..4758738cdd0 --- /dev/null +++ b/spec/models/ci/build_metadata_spec.rb @@ -0,0 +1,69 @@ +require 'spec_helper' + +describe Ci::BuildMetadata do + set(:user) { create(:user) } + set(:group) { create(:group, :access_requestable) } + set(:project) { create(:project, :repository, group: group, build_timeout: 2000) } + + set(:pipeline) do + create(:ci_pipeline, project: project, + sha: project.commit.id, + ref: project.default_branch, + status: 'success') + end + + let(:build) { create(:ci_build, pipeline: pipeline) } + let(:build_metadata) { described_class.create(build: build) } + + context 'when creating' do + subject { build_metadata.project_id } + + it 'saves project_id' do + is_expected.to eq(project.id) + end + end + + describe '#save_timeout_state!' do + subject { build_metadata } + + context 'when runner is not assigned to the job' do + it "doesn't change timeout value" do + expect { subject.save_timeout_state! }.not_to change { subject.reload.timeout } + end + + it "doesn't change timeout_source value" do + expect { subject.save_timeout_state! }.not_to change { subject.reload.timeout_source } + end + end + + context 'when runner is assigned to the job' do + before do + build.update_attributes(runner: runner) + end + + context 'when runner timeout is lower than project timeout' do + let(:runner) { create(:ci_runner, maximum_timeout: 1900) } + + it 'sets runner timeout' do + expect { subject.save_timeout_state! }.to change { subject.reload.timeout }.to(1900) + end + + it 'sets runner_timeout_source' do + expect { subject.save_timeout_state! }.to change { subject.reload.timeout_source }.to('runner_timeout_source') + end + end + + context 'when runner timeout is higher than project timeout' do + let(:runner) { create(:ci_runner, maximum_timeout: 2100) } + + it 'sets project timeout' do + expect { subject.save_timeout_state! }.to change { subject.reload.timeout }.to(2000) + end + + it 'sets project_timeout_source' do + expect { subject.save_timeout_state! }.to change { subject.reload.timeout_source }.to('project_timeout_source') + end + end + end + end +end -- cgit v1.2.1 From c747d9bc0b35d2982f45e2e8a0bdaa305f504f38 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Mar 2018 17:22:52 +0100 Subject: Add validation for chronic_duration_attr_writer --- app/models/concerns/chronic_duration_attribute.rb | 27 +++++++-- .../concerns/chronic_duration_attribute_spec.rb | 66 +++++++++++++++++----- 2 files changed, 74 insertions(+), 19 deletions(-) diff --git a/app/models/concerns/chronic_duration_attribute.rb b/app/models/concerns/chronic_duration_attribute.rb index 15095d0b758..ad8934f58d8 100644 --- a/app/models/concerns/chronic_duration_attribute.rb +++ b/app/models/concerns/chronic_duration_attribute.rb @@ -18,13 +18,32 @@ module ChronicDurationAttribute end def chronic_duration_attr_writer(virtual_attribute, source_attribute) + virtual_attribute_validator = "#{virtual_attribute}_validator".to_sym + validation_error = "#{virtual_attribute}_error".to_sym + + validate virtual_attribute_validator + attr_accessor validation_error + define_method("#{virtual_attribute}=") do |value| - new_value = ChronicDuration.parse(value).to_i unless value.nil? - new_value = nil if !new_value.nil? && new_value <= 0 + begin + self.send("#{validation_error}=", '') # rubocop:disable GitlabSecurity/PublicSend - self.send("#{source_attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend + new_value = + if value.blank? + nil + else + ChronicDuration.parse(value).to_i + end + + self.send("#{source_attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend + rescue ChronicDuration::DurationParseError => ex + self.send("#{validation_error}=", ex.message) # rubocop:disable GitlabSecurity/PublicSend + end + end - new_value + define_method(virtual_attribute_validator) do + error = self.send(validation_error) # rubocop:disable GitlabSecurity/PublicSend + self.send('errors').add(source_attribute, error) unless error.blank? # rubocop:disable GitlabSecurity/PublicSend end end end diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb index cfbf83dab54..fea3e752ab5 100644 --- a/spec/models/concerns/chronic_duration_attribute_spec.rb +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -11,10 +11,12 @@ shared_examples 'ChronicDurationAttribute reader' do expect(subject.send(virtual_field)).to eq('2m') end - it 'outputs empty string when value set to nil' do - subject.send("#{source_field}=", nil) + context 'when value is set to nil' do + it 'outputs empty string' do + subject.send("#{source_field}=", nil) - expect(subject.send(virtual_field)).to be_empty + expect(subject.send(virtual_field)).to be_empty + end end end @@ -29,28 +31,62 @@ shared_examples 'ChronicDurationAttribute writer' do expect(subject.send(source_field)).to eq(600) end - it 'writes nil when empty input is used' do - subject.send("#{virtual_field}=", '') + it 'passes validation' do + subject.send("#{virtual_field}=", '10m') - expect(subject.send(source_field)).to be_nil + expect(subject.valid?).to be_truthy end - it 'writes nil when negative input is used' do - allow(ChronicDuration).to receive(:parse).and_return(-10) + context 'when negative input is used' do + before do + subject.send("#{source_field}=", 3600) + end + + it "doesn't raise exception" do + expect { subject.send("#{virtual_field}=", '-10m') }.not_to raise_error(ChronicDuration::DurationParseError) + end + + it "doesn't change value" do + expect { subject.send("#{virtual_field}=", '-10m') }.not_to change { subject.send(source_field) } + end - subject.send("#{virtual_field}=", '-10m') + it "doesn't pass validation" do + subject.send("#{virtual_field}=", '-10m') - expect(subject.send(source_field)).to be_nil + expect(subject.valid?).to be_falsey + end end - it 'writes nil when nil input is used' do - subject.send("#{virtual_field}=", nil) + context 'when empty input is used' do + it 'writes nil' do + subject.send("#{virtual_field}=", '') + + expect(subject.send(source_field)).to be_nil + end + + it 'passes validation' do + subject.send("#{virtual_field}=", '') - expect(subject.send(source_field)).to be_nil + expect(subject.valid?).to be_truthy + end end - it "doesn't raise exception when nil input is used" do - expect { subject.send("#{virtual_field}=", nil) }.not_to raise_error(NoMethodError) + context 'when nil input is used' do + it 'writes nil' do + subject.send("#{virtual_field}=", nil) + + expect(subject.send(source_field)).to be_nil + end + + it 'passes validation' do + subject.send("#{virtual_field}=", nil) + + expect(subject.valid?).to be_truthy + end + + it "doesn't raise exception" do + expect { subject.send("#{virtual_field}=", nil) }.not_to raise_error(NoMethodError) + end end end -- cgit v1.2.1 From 7d7b0688b846e346a0799340875d459d26c1718d Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 21 Mar 2018 17:34:55 +0100 Subject: Add validation for max_timeout in Ci::Runner --- app/models/ci/runner.rb | 4 ++++ spec/models/ci/build_spec.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 62cc636db22..5a4c56ec0dc 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -54,6 +54,10 @@ module Ci chronic_duration_attr :maximum_timeout_human_readable, :maximum_timeout + validates :maximum_timeout, allow_nil: true, + numericality: { greater_than_or_equal_to: 600, + message: 'needs to be at least 10 minutes' } + # Searches for runners matching the given query. # # This method uses ILIKE on PostgreSQL and LIKE on MySQL. diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index b209d53ae08..14dec2d4fc9 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1299,7 +1299,7 @@ describe Ci::Build do end context 'when runner sets timeout to smaller value' do - let(:runner2) { create(:ci_runner, maximum_timeout: 500) } + let(:runner2) { create(:ci_runner, maximum_timeout: 600) } let(:build) { create(:ci_build, :pending, pipeline: pipeline2, runner: runner2) } it 'returns project timeout configuration' do -- cgit v1.2.1 From 973e4030b13adbcc4eb7fad347b928a5164a04ff Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Thu, 22 Mar 2018 17:52:28 +0100 Subject: Refactor build_metadata --- .../jobs/components/sidebar_details_block.vue | 12 +++++++++--- app/models/ci/build.rb | 2 +- app/models/ci/build_metadata.rb | 16 ++++------------ app/serializers/build_details_entity.rb | 4 +--- spec/factories/ci/build_metadata.rb | 9 +++++++++ spec/models/ci/build_metadata_spec.rb | 10 +--------- 6 files changed, 25 insertions(+), 28 deletions(-) create mode 100644 spec/factories/ci/build_metadata.rb diff --git a/app/assets/javascripts/jobs/components/sidebar_details_block.vue b/app/assets/javascripts/jobs/components/sidebar_details_block.vue index 6ff3fa6e099..172de6b3679 100644 --- a/app/assets/javascripts/jobs/components/sidebar_details_block.vue +++ b/app/assets/javascripts/jobs/components/sidebar_details_block.vue @@ -44,10 +44,16 @@ runnerId() { return `#${this.job.runner.id}`; }, + hasTimeout() { + return this.job.metadata != null && this.job.metadata.timeout_human_readable !== ''; + }, timeout() { - let t = `${this.job.metadata.timeout_human_readable}`; + if (this.job.metadata == null) { + return ''; + } - if (this.job.metadata.timeout_source != null) { + let t = this.job.metadata.timeout_human_readable; + if (this.job.metadata.timeout_source !== '') { t += ` (from ${this.job.metadata.timeout_source})`; } @@ -130,7 +136,7 @@ /> (*) { build.erased? }, using: UserEntity expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :erase_build, build) } do |build| diff --git a/spec/factories/ci/build_metadata.rb b/spec/factories/ci/build_metadata.rb new file mode 100644 index 00000000000..66bbd977b88 --- /dev/null +++ b/spec/factories/ci/build_metadata.rb @@ -0,0 +1,9 @@ +FactoryBot.define do + factory :ci_build_metadata, class: Ci::BuildMetadata do + build factory: :ci_build + + after(:build) do |build_metadata, _| + build_metadata.project ||= build_metadata.build.project + end + end +end diff --git a/spec/models/ci/build_metadata_spec.rb b/spec/models/ci/build_metadata_spec.rb index 4758738cdd0..d21e9600e42 100644 --- a/spec/models/ci/build_metadata_spec.rb +++ b/spec/models/ci/build_metadata_spec.rb @@ -13,15 +13,7 @@ describe Ci::BuildMetadata do end let(:build) { create(:ci_build, pipeline: pipeline) } - let(:build_metadata) { described_class.create(build: build) } - - context 'when creating' do - subject { build_metadata.project_id } - - it 'saves project_id' do - is_expected.to eq(project.id) - end - end + let(:build_metadata) { create(:ci_build_metadata, build: build) } describe '#save_timeout_state!' do subject { build_metadata } -- cgit v1.2.1 From c2bc153314f14566abfdcbf92020cff41fc05a7e Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Fri, 23 Mar 2018 00:12:15 +0100 Subject: Refactorize ChronicDurationAttribute concern --- app/models/concerns/chronic_duration_attribute.rb | 51 +++++++++------------- .../concerns/chronic_duration_attribute_spec.rb | 24 +++++----- 2 files changed, 33 insertions(+), 42 deletions(-) diff --git a/app/models/concerns/chronic_duration_attribute.rb b/app/models/concerns/chronic_duration_attribute.rb index ad8934f58d8..fa1eafb1d7a 100644 --- a/app/models/concerns/chronic_duration_attribute.rb +++ b/app/models/concerns/chronic_duration_attribute.rb @@ -2,49 +2,38 @@ module ChronicDurationAttribute extend ActiveSupport::Concern class_methods do - def chronic_duration_attr(virtual_attribute, source_attribute) - chronic_duration_attr_reader(virtual_attribute, source_attribute) - chronic_duration_attr_writer(virtual_attribute, source_attribute) - end - def chronic_duration_attr_reader(virtual_attribute, source_attribute) define_method(virtual_attribute) do - value = self.send(source_attribute) # rubocop:disable GitlabSecurity/PublicSend - - return '' if value.nil? - - ChronicDuration.output(value, format: :short) + chronic_duration_attributes[virtual_attribute] || output_chronic_duration_attribute(source_attribute) end end def chronic_duration_attr_writer(virtual_attribute, source_attribute) - virtual_attribute_validator = "#{virtual_attribute}_validator".to_sym - validation_error = "#{virtual_attribute}_error".to_sym - - validate virtual_attribute_validator - attr_accessor validation_error + chronic_duration_attr_reader(virtual_attribute, source_attribute) define_method("#{virtual_attribute}=") do |value| + chronic_duration_attributes[virtual_attribute] = value.presence || '' + begin - self.send("#{validation_error}=", '') # rubocop:disable GitlabSecurity/PublicSend - - new_value = - if value.blank? - nil - else - ChronicDuration.parse(value).to_i - end - - self.send("#{source_attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend - rescue ChronicDuration::DurationParseError => ex - self.send("#{validation_error}=", ex.message) # rubocop:disable GitlabSecurity/PublicSend + new_value = ChronicDuration.parse(value).to_i if value.present? + assign_attributes(source_attribute => new_value) + rescue ChronicDuration::DurationParseError + # ignore error as it will be caught by validation end end - define_method(virtual_attribute_validator) do - error = self.send(validation_error) # rubocop:disable GitlabSecurity/PublicSend - self.send('errors').add(source_attribute, error) unless error.blank? # rubocop:disable GitlabSecurity/PublicSend - end + validates virtual_attribute, allow_nil: true, duration: true end + + alias_method :chronic_duration_attr, :chronic_duration_attr_writer + end + + def chronic_duration_attributes + @chronic_duration_attributes ||= {} + end + + def output_chronic_duration_attribute(source_attribute) + value = attributes[source_attribute.to_s] + ChronicDuration.output(value, format: :short) if value end end diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb index fea3e752ab5..27c86e60e60 100644 --- a/spec/models/concerns/chronic_duration_attribute_spec.rb +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -12,10 +12,10 @@ shared_examples 'ChronicDurationAttribute reader' do end context 'when value is set to nil' do - it 'outputs empty string' do + it 'outputs nil' do subject.send("#{source_field}=", nil) - expect(subject.send(virtual_field)).to be_empty + expect(subject.send(virtual_field)).to be_nil end end end @@ -25,15 +25,15 @@ shared_examples 'ChronicDurationAttribute writer' do expect(subject.class).to be_public_method_defined("#{virtual_field}=") end - it 'parses chronic duration input' do + before do subject.send("#{virtual_field}=", '10m') + end + it 'parses chronic duration input' do expect(subject.send(source_field)).to eq(600) end it 'passes validation' do - subject.send("#{virtual_field}=", '10m') - expect(subject.valid?).to be_truthy end @@ -54,33 +54,34 @@ shared_examples 'ChronicDurationAttribute writer' do subject.send("#{virtual_field}=", '-10m') expect(subject.valid?).to be_falsey + expect(subject.errors&.messages).to include(virtual_field => ['is not a correct duration']) end end context 'when empty input is used' do - it 'writes nil' do + before do subject.send("#{virtual_field}=", '') + end + it 'writes nil' do expect(subject.send(source_field)).to be_nil end it 'passes validation' do - subject.send("#{virtual_field}=", '') - expect(subject.valid?).to be_truthy end end context 'when nil input is used' do - it 'writes nil' do + before do subject.send("#{virtual_field}=", nil) + end + it 'writes nil' do expect(subject.send(source_field)).to be_nil end it 'passes validation' do - subject.send("#{virtual_field}=", nil) - expect(subject.valid?).to be_truthy end @@ -103,6 +104,7 @@ end describe 'ChronicDurationAttribute - reader' do let(:source_field) {:timeout} let(:virtual_field) {:timeout_human_readable} + subject {Ci::BuildMetadata.new} it "doesn't contain dynamically created writer method" do -- cgit v1.2.1 From 7008ed1e33bff510126a0eb0e4f7bf1a7adb02bd Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Mar 2018 17:47:46 +0200 Subject: Change and rename behavior of save_timeout_state! --- app/models/ci/build.rb | 2 +- app/models/ci/build_metadata.rb | 4 ++-- spec/models/ci/build_metadata_spec.rb | 14 +++++++------- spec/models/ci/build_spec.rb | 23 +++++++++++++++++++++++ 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 355d1c0523f..073d73f0426 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -157,7 +157,7 @@ module Ci end before_transition pending: :running do |build| - build.ensure_metadata.save_timeout_state! + build.ensure_metadata.update_timeout_state end end diff --git a/app/models/ci/build_metadata.rb b/app/models/ci/build_metadata.rb index 6d8a895d509..de5b4170201 100644 --- a/app/models/ci/build_metadata.rb +++ b/app/models/ci/build_metadata.rb @@ -21,14 +21,14 @@ module Ci runner_timeout_source: 3 } - def save_timeout_state! + def update_timeout_state return unless build.runner.present? project_timeout = project&.build_timeout timeout = [project_timeout, build.runner.maximum_timeout].compact.min timeout_source = timeout < project_timeout ? :runner_timeout_source : :project_timeout_source - update!(timeout: timeout, timeout_source: timeout_source) + update(timeout: timeout, timeout_source: timeout_source) end end end diff --git a/spec/models/ci/build_metadata_spec.rb b/spec/models/ci/build_metadata_spec.rb index d21e9600e42..268561ee941 100644 --- a/spec/models/ci/build_metadata_spec.rb +++ b/spec/models/ci/build_metadata_spec.rb @@ -15,16 +15,16 @@ describe Ci::BuildMetadata do let(:build) { create(:ci_build, pipeline: pipeline) } let(:build_metadata) { create(:ci_build_metadata, build: build) } - describe '#save_timeout_state!' do + describe '#update_timeout_state' do subject { build_metadata } context 'when runner is not assigned to the job' do it "doesn't change timeout value" do - expect { subject.save_timeout_state! }.not_to change { subject.reload.timeout } + expect { subject.update_timeout_state }.not_to change { subject.reload.timeout } end it "doesn't change timeout_source value" do - expect { subject.save_timeout_state! }.not_to change { subject.reload.timeout_source } + expect { subject.update_timeout_state }.not_to change { subject.reload.timeout_source } end end @@ -37,11 +37,11 @@ describe Ci::BuildMetadata do let(:runner) { create(:ci_runner, maximum_timeout: 1900) } it 'sets runner timeout' do - expect { subject.save_timeout_state! }.to change { subject.reload.timeout }.to(1900) + expect { subject.update_timeout_state }.to change { subject.reload.timeout }.to(1900) end it 'sets runner_timeout_source' do - expect { subject.save_timeout_state! }.to change { subject.reload.timeout_source }.to('runner_timeout_source') + expect { subject.update_timeout_state }.to change { subject.reload.timeout_source }.to('runner_timeout_source') end end @@ -49,11 +49,11 @@ describe Ci::BuildMetadata do let(:runner) { create(:ci_runner, maximum_timeout: 2100) } it 'sets project timeout' do - expect { subject.save_timeout_state! }.to change { subject.reload.timeout }.to(2000) + expect { subject.update_timeout_state }.to change { subject.reload.timeout }.to(2000) end it 'sets project_timeout_source' do - expect { subject.save_timeout_state! }.to change { subject.reload.timeout_source }.to('project_timeout_source') + expect { subject.update_timeout_state }.to change { subject.reload.timeout_source }.to('project_timeout_source') end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 14dec2d4fc9..920b2284eb1 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2050,6 +2050,11 @@ describe Ci::Build do job.project.update_attribute(:build_timeout, 1800) end + def run_job_without_exception + job.run! + rescue StateMachines::InvalidTransition + end + shared_examples 'saves data on transition' do it 'saves timeout' do expect { job.run! }.to change { job.reload.ensure_metadata.timeout }.from(nil).to(expected_timeout) @@ -2058,6 +2063,24 @@ describe Ci::Build do it 'saves timeout_source' do expect { job.run! }.to change { job.reload.ensure_metadata.timeout_source }.from('unknown_timeout_source').to(expected_timeout_source) end + + context 'when Ci::BuildMetadata#update_timeout_state fails update' do + before do + allow_any_instance_of(Ci::BuildMetadata).to receive(:update_timeout_state).and_return(false) + end + + it "doesn't save timeout" do + expect { run_job_without_exception }.not_to change { job.reload.ensure_metadata.timeout_source } + end + + it "doesn't save timeout_source" do + expect { run_job_without_exception }.not_to change { job.reload.ensure_metadata.timeout_source } + end + + it 'raises an exception' do + expect { job.run! }.to raise_error(StateMachines::InvalidTransition) + end + end end context 'when runner timeout overrides project timeout' do -- cgit v1.2.1 From 403decbbad26bea620125aed34b440a9b6611172 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Mar 2018 18:58:35 +0200 Subject: Add explicit primary key for ci_builds_metadata table --- app/models/ci/build_metadata.rb | 1 + db/migrate/20180301010859_create_ci_builds_metadata_table.rb | 4 ++-- db/schema.rb | 4 +++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/models/ci/build_metadata.rb b/app/models/ci/build_metadata.rb index de5b4170201..96762f8845c 100644 --- a/app/models/ci/build_metadata.rb +++ b/app/models/ci/build_metadata.rb @@ -11,6 +11,7 @@ module Ci belongs_to :build, class_name: 'Ci::Build' belongs_to :project + validates :build, presence: true validates :project, presence: true chronic_duration_attr_reader :timeout_human_readable, :timeout diff --git a/db/migrate/20180301010859_create_ci_builds_metadata_table.rb b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb index 650bf43835d..ce737444092 100644 --- a/db/migrate/20180301010859_create_ci_builds_metadata_table.rb +++ b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb @@ -4,16 +4,16 @@ class CreateCiBuildsMetadataTable < ActiveRecord::Migration DOWNTIME = false def change - create_table :ci_builds_metadata, id: false do |t| + create_table :ci_builds_metadata do |t| t.integer :build_id, null: false t.integer :project_id, null: false t.integer :timeout t.integer :timeout_source, null: false, default: 1 - t.primary_key :build_id t.foreign_key :ci_builds, column: :build_id, on_delete: :cascade t.foreign_key :projects, column: :project_id, on_delete: :cascade + t.index :build_id, unique: true t.index :project_id end end diff --git a/db/schema.rb b/db/schema.rb index 56541aa4ecd..3d3a4c0ce4a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -329,12 +329,14 @@ ActiveRecord::Schema.define(version: 20180327101207) do add_index "ci_builds", ["updated_at"], name: "index_ci_builds_on_updated_at", using: :btree add_index "ci_builds", ["user_id"], name: "index_ci_builds_on_user_id", using: :btree - create_table "ci_builds_metadata", primary_key: "build_id", force: :cascade do |t| + create_table "ci_builds_metadata", force: :cascade do |t| + t.integer "build_id", null: false t.integer "project_id", null: false t.integer "timeout" t.integer "timeout_source", default: 1, null: false end + add_index "ci_builds_metadata", ["build_id"], name: "index_ci_builds_metadata_on_build_id", unique: true, using: :btree add_index "ci_builds_metadata", ["project_id"], name: "index_ci_builds_metadata_on_project_id", using: :btree create_table "ci_group_variables", force: :cascade do |t| -- cgit v1.2.1 From 6ecde0076afa83e30608ea9caba924bbab66a123 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Mon, 26 Mar 2018 19:26:52 +0200 Subject: Remove Ci::Build#timeout --- app/models/ci/build.rb | 6 ++---- lib/api/entities.rb | 2 +- lib/gitlab/ci/build/step.rb | 4 ++-- spec/lib/gitlab/ci/build/step_spec.rb | 12 ++++++++++-- spec/models/ci/build_spec.rb | 37 ----------------------------------- spec/requests/api/runner_spec.rb | 4 ++-- 6 files changed, 17 insertions(+), 48 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 073d73f0426..ed02af05e3d 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -26,6 +26,8 @@ module Ci has_one :metadata, class_name: 'Ci::BuildMetadata' + delegate :timeout, to: :metadata, prefix: true, allow_nil: true + # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @persisted_environment ||= Environment.find_by( @@ -241,10 +243,6 @@ module Ci latest_builds.where('stage_idx < ?', stage_idx) end - def timeout - ensure_metadata.timeout - end - def triggered_by?(current_user) user == current_user end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 324e14f5654..38161d1f127 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1120,7 +1120,7 @@ module API end class RunnerInfo < Grape::Entity - expose :timeout + expose :metadata_timeout, as: :timeout end class Step < Grape::Entity diff --git a/lib/gitlab/ci/build/step.rb b/lib/gitlab/ci/build/step.rb index 411f67f8ce7..0b1ebe4e048 100644 --- a/lib/gitlab/ci/build/step.rb +++ b/lib/gitlab/ci/build/step.rb @@ -14,7 +14,7 @@ module Gitlab self.new(:script).tap do |step| step.script = job.options[:before_script].to_a + job.options[:script].to_a step.script = job.commands.split("\n") if step.script.empty? - step.timeout = job.timeout + step.timeout = job.metadata_timeout step.when = WHEN_ON_SUCCESS end end @@ -25,7 +25,7 @@ module Gitlab self.new(:after_script).tap do |step| step.script = after_script - step.timeout = job.timeout + step.timeout = job.metadata_timeout step.when = WHEN_ALWAYS step.allow_failure = true end diff --git a/spec/lib/gitlab/ci/build/step_spec.rb b/spec/lib/gitlab/ci/build/step_spec.rb index 5a21282712a..cce4efaa069 100644 --- a/spec/lib/gitlab/ci/build/step_spec.rb +++ b/spec/lib/gitlab/ci/build/step_spec.rb @@ -5,10 +5,14 @@ describe Gitlab::Ci::Build::Step do shared_examples 'has correct script' do subject { described_class.from_commands(job) } + before do + job.run! + end + it 'fabricates an object' do expect(subject.name).to eq(:script) expect(subject.script).to eq(script) - expect(subject.timeout).to eq(job.timeout) + expect(subject.timeout).to eq(job.metadata_timeout) expect(subject.when).to eq('on_success') expect(subject.allow_failure).to be_falsey end @@ -47,6 +51,10 @@ describe Gitlab::Ci::Build::Step do subject { described_class.from_after_script(job) } + before do + job.run! + end + context 'when after_script is empty' do it 'doesn not fabricate an object' do is_expected.to be_nil @@ -59,7 +67,7 @@ describe Gitlab::Ci::Build::Step do it 'fabricates an object' do expect(subject.name).to eq(:after_script) expect(subject.script).to eq(['ls -la', 'date']) - expect(subject.timeout).to eq(job.timeout) + expect(subject.timeout).to eq(job.metadata_timeout) expect(subject.when).to eq('always') expect(subject.allow_failure).to be_truthy end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 920b2284eb1..f5534d22a54 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1271,43 +1271,6 @@ describe Ci::Build do end describe 'project settings' do - describe '#timeout' do - set(:project2) { create(:project, :repository, group: group, build_timeout: 1000) } - set(:pipeline2) { create(:ci_pipeline, project: project2, sha: project2.commit.id, ref: project2.default_branch, status: 'success') } - - let(:build) { create(:ci_build, :pending, pipeline: pipeline2) } - - subject { build.timeout } - - before do - build.run! - end - - context 'when runner is not assigned' do - it 'returns project timeout configuration' do - is_expected.to be_nil - end - end - - context 'when runner sets timeout to bigger value' do - let(:runner2) { create(:ci_runner, maximum_timeout: 2000) } - let(:build) { create(:ci_build, :pending, pipeline: pipeline2, runner: runner2) } - - it 'returns project timeout configuration' do - is_expected.to eq(project2.build_timeout) - end - end - - context 'when runner sets timeout to smaller value' do - let(:runner2) { create(:ci_runner, maximum_timeout: 600) } - let(:build) { create(:ci_build, :pending, pipeline: pipeline2, runner: runner2) } - - it 'returns project timeout configuration' do - is_expected.to eq(runner2.maximum_timeout) - end - end - end - describe '#allow_git_fetch' do it 'return project allow_git_fetch configuration' do expect(build.allow_git_fetch).to eq(project.build_allow_git_fetch) diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 4703c64c534..5084b36c761 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -360,12 +360,12 @@ describe API::Runner do let(:expected_steps) do [{ 'name' => 'script', 'script' => %w(ls date), - 'timeout' => job.timeout, + 'timeout' => job.metadata_timeout, 'when' => 'on_success', 'allow_failure' => false }, { 'name' => 'after_script', 'script' => %w(ls date), - 'timeout' => job.timeout, + 'timeout' => job.metadata_timeout, 'when' => 'always', 'allow_failure' => true }] end -- cgit v1.2.1