diff options
author | Markus Doits <markus.doits@stellenticket.de> | 2018-10-18 18:48:44 +0200 |
---|---|---|
committer | Markus Doits <markus.doits@stellenticket.de> | 2018-11-07 13:04:21 +0100 |
commit | b7ee04d4564deaf30cf3c8ead34701ac6d746c05 (patch) | |
tree | 00ea07bb86c0b362e53cf34c41271c90068c54a9 | |
parent | 48f37a92be9b9b3e21cce6772c147a303a881ca5 (diff) | |
download | gitlab-ce-b7ee04d4564deaf30cf3c8ead34701ac6d746c05.tar.gz |
refactor validations to a Entry::Retry class
-rw-r--r-- | lib/gitlab/ci/config/entry/job.rb | 68 | ||||
-rw-r--r-- | lib/gitlab/ci/config/entry/retry.rb | 64 | ||||
-rw-r--r-- | lib/gitlab/ci/config/entry/validators.rb | 10 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/config/entry/job_spec.rb | 182 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/config/entry/retry_spec.rb | 183 |
5 files changed, 442 insertions, 65 deletions
diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index 5a91d5574f0..5ff03d803dc 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -16,8 +16,6 @@ module Gitlab dependencies before_script after_script variables environment coverage retry parallel extends].freeze - ALLOWED_KEYS_RETRY = %i[max when].freeze - validations do validates :config, allowed_keys: ALLOWED_KEYS validates :config, presence: true @@ -25,14 +23,12 @@ module Gitlab validates :name, presence: true validates :name, type: Symbol - validates :retry, hash_or_integer: true, allowed_keys: ALLOWED_KEYS_RETRY, allow_nil: true - validate :validate_retry, if: :validate_retry? - with_options allow_nil: true do validates :tags, array_of_strings: true validates :allow_failure, boolean: true validates :parallel, numericality: { only_integer: true, greater_than_or_equal_to: 2 } + validates :retry, hash_or_integer: true validates :when, inclusion: { in: %w[on_success on_failure always manual delayed], @@ -43,65 +39,6 @@ module Gitlab validates :extends, type: String end - def validate_retry? - config&.is_a?(Hash) && - config[:retry].present? && - (config[:retry].is_a?(Integer) || config[:retry].is_a?(Hash)) - end - - def validate_retry - if config[:retry].is_a?(Integer) - validate_retry_max(config[:retry]) - else - validate_retry_max(config[:retry][:max]) - validate_retry_when(config[:retry][:when]) - end - end - - def validate_retry_max(retry_max) - case retry_max - when Integer - validate_retry_max_integer(retry_max) - else - errors[:base] << "retry max #{::I18n.t('errors.messages.not_an_integer')}" - end - end - - def validate_retry_max_integer(retry_max) - errors[:base] << "retry max #{::I18n.t('errors.messages.less_than_or_equal_to', count: 2)}" if retry_max > 2 - errors[:base] << "retry max #{::I18n.t('errors.messages.greater_than_or_equal_to', count: 0)}" if retry_max < 0 - end - - def validate_retry_when(retry_when) - return if retry_when.blank? - - case retry_when - when String - validate_retry_when_string(retry_when) - when Array - validate_retry_when_array(retry_when) - else - errors[:base] << 'retry when should be an array of strings or a string' - end - end - - def possible_retry_when_values - @possible_retry_when_values ||= Gitlab::Ci::Status::Build::Failed.reasons.keys.map(&:to_s) + ['always'] - end - - def validate_retry_when_string(retry_when) - unless possible_retry_when_values.include?(retry_when) - errors[:base] << 'retry when is unknown' - end - end - - def validate_retry_when_array(retry_when) - unknown_whens = retry_when - possible_retry_when_values - unless unknown_whens.empty? - errors[:base] << "retry when contains unknown values: #{unknown_whens.join(', ')}" - end - end - validates :start_in, duration: { limit: '1 day' }, if: :delayed? validates :start_in, absence: true, unless: :delayed? end @@ -148,6 +85,9 @@ module Gitlab entry :coverage, Entry::Coverage, description: 'Coverage configuration for this job.' + entry :retry, Entry::Retry, + description: 'Retry configuration for this job.' + helpers :before_script, :script, :stage, :type, :after_script, :cache, :image, :services, :only, :except, :variables, :artifacts, :commands, :environment, :coverage, :retry, diff --git a/lib/gitlab/ci/config/entry/retry.rb b/lib/gitlab/ci/config/entry/retry.rb new file mode 100644 index 00000000000..140201447d1 --- /dev/null +++ b/lib/gitlab/ci/config/entry/retry.rb @@ -0,0 +1,64 @@ +module Gitlab + module Ci + class Config + module Entry + ## + # Entry that represents a retry config for a job. + # + class Retry < Simplifiable + strategy :SimpleRetry, if: -> (config) { config.is_a?(Integer) } + strategy :FullRetry, if: -> (config) { config.is_a?(Hash) } + + class SimpleRetry < Entry::Node + include Entry::Validatable + + validations do + validates :config, numericality: { only_integer: true, + greater_than_or_equal_to: 0, + less_than_or_equal_to: 2 } + end + end + + class FullRetry < Entry::Node + include Entry::Validatable + include Entry::Attributable + + ALLOWED_KEYS = %i[max when].freeze + attributes :max, :when + + validations do + validates :config, allowed_keys: ALLOWED_KEYS + + with_options allow_nil: true do + validates :max, numericality: { only_integer: true, + greater_than_or_equal_to: 0, + less_than_or_equal_to: 2 } + + validates :when, array_of_strings_or_string: true + validates :when, + allowed_array_values: { in: FullRetry.possible_retry_when_values }, + if: -> (config) { config.when.is_a?(Array) } + validates :when, + inclusion: { in: FullRetry.possible_retry_when_values }, + if: -> (config) { config.when.is_a?(String) } + end + end + + def self.possible_retry_when_values + @possible_retry_when_values ||= Gitlab::Ci::Status::Build::Failed.reasons.keys.map(&:to_s) + ['always'] + end + end + + class UnknownStrategy < Entry::Node + def errors + ["#{location} has to be either an integer or a hash"] + end + end + + def self.default + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/entry/validators.rb b/lib/gitlab/ci/config/entry/validators.rb index af902eb0ee8..a1d552fb2e5 100644 --- a/lib/gitlab/ci/config/entry/validators.rb +++ b/lib/gitlab/ci/config/entry/validators.rb @@ -24,6 +24,16 @@ module Gitlab end end + class AllowedArrayValuesValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + unkown_values = value - options[:in] + unless unkown_values.empty? + record.errors.add(attribute, "contains unknown values: " + + unkown_values.join(', ')) + end + end + end + class ArrayOfStringsValidator < ActiveModel::EachValidator include LegacyValidationHelpers diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb index 36ff4519bcb..79e50d7152d 100644 --- a/spec/lib/gitlab/ci/config/entry/job_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb @@ -10,7 +10,7 @@ describe Gitlab::Ci::Config::Entry::Job do let(:result) do %i[before_script script stage type after_script cache image services only except variables artifacts - environment coverage] + environment coverage retry] end it { is_expected.to match_array result } @@ -98,6 +98,7 @@ describe Gitlab::Ci::Config::Entry::Job do end end +<<<<<<< HEAD context 'when retry value is correct' do context 'when it is a numeric' do let(:config) { { script: 'rspec', retry: 2 } } @@ -304,6 +305,185 @@ describe Gitlab::Ci::Config::Entry::Job do end end +||||||| merged common ancestors + context 'when retry value is correct' do + context 'when it is a numeric' do + let(:config) { { script: 'rspec', retry: 2 } } + + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'when it is a hash without when' do + let(:config) { { script: 'rspec', retry: { max: 2 } } } + + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'when it is a hash with string when' do + let(:config) { { script: 'rspec', retry: { max: 2, when: 'unknown_failure' } } } + + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'when it is a hash with string when always' do + let(:config) { { script: 'rspec', retry: { max: 2, when: 'always' } } } + + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'when it is a hash with array when' do + let(:config) { { script: 'rspec', retry: { max: 2, 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 "when it is a hash with value from documentation `#{reason}`" do + let(:config) { { script: 'rspec', retry: { max: 2, when: reason } } } + + it 'is valid' do + expect(entry).to be_valid + end + end + end + end + + context 'when retry value is not correct' do + context 'when it is not a numeric nor an array' do + let(:config) { { retry: true } } + + it 'returns error about invalid type' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'job retry should be a hash or an integer' + end + end + + context 'not defined as a hash' do + context 'when it is lower than zero' do + let(:config) { { retry: -1 } } + + it 'returns error about value too low' do + expect(entry).not_to be_valid + expect(entry.errors) + .to include 'job retry max must be greater than or equal to 0' + end + end + + context 'when it is not an integer' do + let(:config) { { retry: 1.5 } } + + it 'returns error about wrong value' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'job retry should be a hash or an integer' + end + end + + context 'when the value is too high' do + let(:config) { { retry: 10 } } + + it 'returns error about value too high' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'job retry max must be less than or equal to 2' + end + end + end + + context 'defined as a hash' do + context 'with unknown keys' do + let(:config) { { retry: { max: 2, unknown_key: :something, one_more: :key } } } + + it 'returns error about the unknown key' do + expect(entry).not_to be_valid + expect(entry.errors) + .to include 'job retry contains unknown keys: unknown_key, one_more' + end + end + + context 'when max is lower than zero' do + let(:config) { { retry: { max: -1 } } } + + it 'returns error about value too low' do + expect(entry).not_to be_valid + expect(entry.errors) + .to include 'job retry max must be greater than or equal to 0' + end + end + + context 'when max is not an integer' do + let(:config) { { retry: { max: 1.5 } } } + + it 'returns error about wrong value' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'job retry max must be an integer' + end + end + + context 'when max is too high' do + let(:config) { { retry: { max: 10 } } } + + it 'returns error about value too high' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'job retry max must be less than or equal to 2' + end + end + + context 'when when has the wrong format' do + let(:config) { { retry: { when: true } } } + + it 'returns error about the wrong format' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'job retry max must be an integer' + end + end + + context 'when when is a string and unknown' do + let(:config) { { retry: { when: 'unknown_reason' } } } + + it 'returns error about the wrong format' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'job retry when is unknown' + end + end + + context 'when when is an array and includes unknown failures' do + let(:config) { { retry: { when: %w[unknown_reason runner_system_failure] } } } + + it 'returns error about the wrong format' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'job retry when contains unknown values: unknown_reason' + end + end + end + end + +======= +>>>>>>> refactor validations to a Entry::Retry class context 'when delayed job' do context 'when start_in is specified' do let(:config) { { script: 'echo', when: 'delayed', start_in: '1 day' } } diff --git a/spec/lib/gitlab/ci/config/entry/retry_spec.rb b/spec/lib/gitlab/ci/config/entry/retry_spec.rb new file mode 100644 index 00000000000..17ffa51106f --- /dev/null +++ b/spec/lib/gitlab/ci/config/entry/retry_spec.rb @@ -0,0 +1,183 @@ +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 } + + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'when it is a hash without when' do + let(:config) { { max: 2 } } + + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'when it is a hash with string when' do + let(:config) { { max: 2, when: 'unknown_failure' } } + + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'when it is a hash with string when always' do + let(:config) { { max: 2, when: 'always' } } + + 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] } } + + 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 "when it is a hash with value from documentation `#{reason}`" do + let(:config) { { max: 2, when: reason } } + + it 'is valid' do + expect(entry).to be_valid + end + end + end + end + + context 'when retry value is not correct' do + context 'when it is not a numeric nor an array' do + let(:config) { true } + + it 'returns error about invalid type' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'job retry should be a hash or an integer' + end + end + + context 'not defined as a hash' do + context 'when it is lower than zero' do + let(:config) { -1 } + + it 'returns error about value too low' do + expect(entry).not_to be_valid + expect(entry.errors) + .to include 'retry config must be greater than or equal to 0' + end + end + + context 'when it is not an integer' do + let(:config) { 1.5 } + + it 'returns error about wrong value' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'job retry should be a hash or an integer' + end + end + + context 'when the value is too high' do + let(:config) { 10 } + + it 'returns error about value too high' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'retry config must be less than or equal to 2' + end + end + end + + context 'defined as a hash' do + context 'with unknown keys' do + let(:config) { { max: 2, unknown_key: :something, one_more: :key } } + + it 'returns error about the unknown key' do + expect(entry).not_to be_valid + expect(entry.errors) + .to include 'retry config contains unknown keys: unknown_key, one_more' + end + end + + context 'when max is lower than zero' do + let(:config) { { max: -1 } } + + it 'returns error about value too low' do + expect(entry).not_to be_valid + expect(entry.errors) + .to include 'retry max must be greater than or equal to 0' + end + end + + context 'when max is not an integer' do + let(:config) { { max: 1.5 } } + + it 'returns error about wrong value' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'retry max must be an integer' + end + end + + context 'when max is too high' do + let(:config) { { max: 10 } } + + it 'returns error about value too high' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'retry max must be less than or equal to 2' + end + end + + context 'when when has the wrong format' do + let(:config) { { when: true } } + + it 'returns error about the wrong format' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'retry when should be an array of strings or a string' + end + end + + context 'when when is a string and unknown' do + let(:config) { { when: 'unknown_reason' } } + + it 'returns error about the wrong format' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'retry when is not included in the list' + end + end + + context 'when when is an array and includes unknown failures' do + let(:config) { { when: %w[unknown_reason runner_system_failure] } } + + it 'returns error about the wrong format' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'retry when contains unknown values: unknown_reason' + end + end + end + end + end +end |