refactor validations to a Entry::Retry class

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,

lib/gitlab/ci/config/entry/retry.rb

+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

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
 

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' } }

spec/lib/gitlab/ci/config/entry/retry_spec.rb

+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