diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 08d6e669cf9ff9a3c0d6a565003483d09fa10272..7b220487495ac22b0966f8ad28459c8e616314dd 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -318,15 +318,11 @@ module Ci end def retries_max - retries = self.options[:retry] - retries.is_a?(Hash) && retries.fetch(:max, 0) || retries || 0 + options&.dig(:retry, :max) || 0 end def retry_when - retries = self.options[:retry] - Array.wrap( - retries.is_a?(Hash) && retries.fetch(:when, 'always') || 'always' - ) + options&.dig(:retry, :when) || ['always'] end def retry_failure? diff --git a/lib/gitlab/ci/config/entry/retry.rb b/lib/gitlab/ci/config/entry/retry.rb index 1f9539dafdb44bc4c9cea3e0316f256c6c99fee4..a89d9f051176b78691fed519fdcb3e0d70a83b60 100644 --- a/lib/gitlab/ci/config/entry/retry.rb +++ b/lib/gitlab/ci/config/entry/retry.rb @@ -18,6 +18,12 @@ module Gitlab less_than_or_equal_to: 2 } end + def value + { + max: config + } + end + def location 'retry' end @@ -49,7 +55,15 @@ module Gitlab end def self.possible_retry_when_values - @possible_retry_when_values ||= Gitlab::Ci::Status::Build::Failed.reasons.keys.map(&:to_s) + ['always'] + @possible_retry_when_values ||= CommitStatus.failure_reasons.keys.map(&:to_s) + ['always'] + end + + def value + super.tap do |config| + # make sure that `when` is an array, because we allow it to + # be passed as a String in config for simplicity + config[:when] = Array.wrap(config[:when]) if config[:when] + end end def location diff --git a/spec/lib/gitlab/ci/config/entry/retry_spec.rb b/spec/lib/gitlab/ci/config/entry/retry_spec.rb index cc5dfb56655ddff8acb5aeb71ed525355161e985..4feddafb58015e703d39f72125c58467ed26dd7b 100644 --- a/spec/lib/gitlab/ci/config/entry/retry_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/retry_spec.rb @@ -3,72 +3,125 @@ require 'spec_helper' describe Gitlab::Ci::Config::Entry::Retry do let(:entry) { described_class.new(config) } - describe 'validation' do - context 'when retry value is correct' do - context 'when it is a numeric' do - let(:config) { 2 } + shared_context 'when retry value is a numeric', :numeric do + let(:config) { max } + let(:max) {} + end - it 'is valid' do - expect(entry).to be_valid - end + shared_context 'when retry value is a hash', :hash do + let(:config) { { max: max, when: public_send(:when) }.compact } + let(:when) {} + let(:max) {} + end + + describe '#value' do + subject(:value) { entry.value } + + context 'when retry value is a numeric', :numeric do + let(:max) { 2 } + + it 'is returned as a hash with max key' do + expect(value).to eq(max: 2) end + end - context 'when it is a hash without when' do - let(:config) { { max: 2 } } + context 'when retry value is a hash', :hash do + context 'and `when` is a string' do + let(:when) { 'unknown_failure' } - it 'is valid' do - expect(entry).to be_valid + it 'returns when wrapped in an array' do + expect(value).to eq(when: ['unknown_failure']) end end - context 'when it is a hash with string when' do - let(:config) { { max: 2, when: 'unknown_failure' } } + context 'and `when` is an array' do + let(:when) { %w[unknown_failure runner_system_failure] } - it 'is valid' do - expect(entry).to be_valid + it 'returns when as it was passed' do + expect(value).to eq(when: %w[unknown_failure runner_system_failure]) end end + end + end - context 'when it is a hash with string when always' do - let(:config) { { max: 2, when: 'always' } } + describe 'validation' do + context 'when retry value is correct' do + context 'when it is a numeric', :numeric do + let(:max) { 2 } it 'is valid' do expect(entry).to be_valid end end - context 'when it is a hash with array when' do - let(:config) { { max: 2, when: %w[unknown_failure runner_system_failure] } } + context 'when it is a hash', :hash do + context 'with max' do + let(:max) { 2 } - it 'is valid' do - expect(entry).to be_valid + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'with string when' do + let(:when) { 'unknown_failure' } + + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'with string when always' do + let(:when) { 'always' } + + it 'is valid' do + expect(entry).to be_valid + end end - end - # Those values are documented at `doc/ci/yaml/README.md`. If any of - # those values gets invalid, documentation must be updated. To make - # sure this is catched, check explicitly that all of the documented - # values are valid. If they are not it means the documentation and this - # array must be updated. - RETRY_WHEN_IN_DOCUMENTATION = %w[ - always - unknown_failure - script_failure - api_failure - stuck_or_timeout_failure - runner_system_failure - missing_dependency_failure - runner_unsupported - ].freeze - - RETRY_WHEN_IN_DOCUMENTATION.each do |reason| - context "when it is a hash with value from documentation `#{reason}`" do - let(:config) { { max: 2, when: reason } } + context 'with array when' do + let(:when) { %w[unknown_failure runner_system_failure] } it 'is valid' do expect(entry).to be_valid end end + + # Those values are documented at `doc/ci/yaml/README.md`. If any of + # those values gets invalid, documentation must be updated. To make + # sure this is catched, check explicitly that all of the documented + # values are valid. If they are not it means the documentation and this + # array must be updated. + RETRY_WHEN_IN_DOCUMENTATION = %w[ + always + unknown_failure + script_failure + api_failure + stuck_or_timeout_failure + runner_system_failure + missing_dependency_failure + runner_unsupported + ].freeze + + RETRY_WHEN_IN_DOCUMENTATION.each do |reason| + context "with when from documentation `#{reason}`" do + let(:when) { reason } + + it 'is valid' do + expect(entry).to be_valid + end + end + end + + CommitStatus.failure_reasons.each_key do |reason| + context "with when from CommitStatus.failure_reasons `#{reason}`" do + let(:when) { reason } + + it 'is valid' do + expect(entry).to be_valid + end + end + end end end @@ -82,9 +135,9 @@ describe Gitlab::Ci::Config::Entry::Retry do end end - context 'not defined as a hash' do + context 'when it is a numeric', :numeric do context 'when it is lower than zero' do - let(:config) { -1 } + let(:max) { -1 } it 'returns error about value too low' do expect(entry).not_to be_valid @@ -94,7 +147,7 @@ describe Gitlab::Ci::Config::Entry::Retry do end context 'when it is not an integer' do - let(:config) { 1.5 } + let(:max) { 1.5 } it 'returns error about wrong value' do expect(entry).not_to be_valid @@ -103,7 +156,7 @@ describe Gitlab::Ci::Config::Entry::Retry do end context 'when the value is too high' do - let(:config) { 10 } + let(:max) { 10 } it 'returns error about value too high' do expect(entry).not_to be_valid @@ -112,7 +165,7 @@ describe Gitlab::Ci::Config::Entry::Retry do end end - context 'defined as a hash' do + context 'when it is a hash', :hash do context 'with unknown keys' do let(:config) { { max: 2, unknown_key: :something, one_more: :key } } @@ -123,8 +176,8 @@ describe Gitlab::Ci::Config::Entry::Retry do end end - context 'when max is lower than zero' do - let(:config) { { max: -1 } } + context 'with max lower than zero' do + let(:max) { -1 } it 'returns error about value too low' do expect(entry).not_to be_valid @@ -133,8 +186,8 @@ describe Gitlab::Ci::Config::Entry::Retry do end end - context 'when max is not an integer' do - let(:config) { { max: 1.5 } } + context 'with max not an integer' do + let(:max) { 1.5 } it 'returns error about wrong value' do expect(entry).not_to be_valid @@ -142,8 +195,8 @@ describe Gitlab::Ci::Config::Entry::Retry do end end - context 'when max is too high' do - let(:config) { { max: 10 } } + context 'iwth max too high' do + let(:max) { 10 } it 'returns error about value too high' do expect(entry).not_to be_valid @@ -151,8 +204,8 @@ describe Gitlab::Ci::Config::Entry::Retry do end end - context 'when when has the wrong format' do - let(:config) { { when: true } } + context 'with when in wrong format' do + let(:when) { true } it 'returns error about the wrong format' do expect(entry).not_to be_valid @@ -160,8 +213,8 @@ describe Gitlab::Ci::Config::Entry::Retry do end end - context 'when when is a string and unknown' do - let(:config) { { when: 'unknown_reason' } } + context 'with an unknown when string' do + let(:when) { 'unknown_reason' } it 'returns error about the wrong format' do expect(entry).not_to be_valid @@ -169,8 +222,8 @@ describe Gitlab::Ci::Config::Entry::Retry do end end - context 'when when is an array and includes unknown failures' do - let(:config) { { when: %w[unknown_reason runner_system_failure] } } + context 'with an unknown failure reason in a when array' do + let(:when) { %w[unknown_reason runner_system_failure] } it 'returns error about the wrong format' do expect(entry).not_to be_valid diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 2b4923926db03e639b304b1572a77c03ee80f8e5..8a7b06ad21cc84faaabf4edf861a10aef8466fc0 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1472,15 +1472,7 @@ describe Ci::Build do end describe '#retries_max' do - context 'when max retries value is defined as an integer' do - subject { create(:ci_build, options: { retry: 1 }) } - - it 'returns the number of configured max retries' do - expect(subject.retries_max).to eq 1 - end - end - - context 'when retries value is defined as a hash' do + context 'with retries max config option' do subject { create(:ci_build, options: { retry: { max: 1 } }) } it 'returns the number of configured max retries' do @@ -1488,15 +1480,7 @@ describe Ci::Build do end end - context 'when retries value is defined as a hash without max key' do - subject { create(:ci_build, options: { retry: { something: :else } }) } - - it 'returns zero' do - expect(subject.retries_max).to eq 0 - end - end - - context 'when max retries value is not defined' do + context 'without retries max config option' do subject { create(:ci_build) } it 'returns zero' do @@ -1514,34 +1498,18 @@ describe Ci::Build do end describe '#retry_when' do - context 'when value is defined without an array' do - subject { create(:ci_build, options: { retry: { when: 'something' } }) } - - it 'returns the configured value inside an array' do - expect(subject.retry_when).to eq ['something'] - end - end - - context 'when value is defined as an array' do - subject { create(:ci_build, options: { retry: { when: %w[something more] } }) } + context 'with retries when config option' do + subject { create(:ci_build, options: { retry: { when: ['some_reason'] } }) } - it 'returns the configured value' do - expect(subject.retry_when).to eq %w[something more] + it 'returns the configured when' do + expect(subject.retry_when).to eq ['some_reason'] end end - context 'when value is not defined' do + context 'without retries when config option' do subject { create(:ci_build) } - it 'returns `always`' do - expect(subject.retry_when).to eq ['always'] - end - end - - context 'when retry is only defined as an integer' do - subject { create(:ci_build, options: { retry: 1 }) } - - it 'returns `always`' do + it 'returns always array' do expect(subject.retry_when).to eq ['always'] end end @@ -3001,7 +2969,7 @@ describe Ci::Build do end context 'when build is configured to be retried' do - subject { create(:ci_build, :running, options: { retry: 3 }, project: project, user: user) } + subject { create(:ci_build, :running, options: { retry: { max: 3 } }, project: project, user: user) } it 'retries build and assigns the same user to it' do expect(described_class).to receive(:retry)