update job config validator to validate new retry syntax

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

@@ -16,6 +16,8 @@ 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
@@ -23,12 +25,57 @@ 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
+
+            def validate_retry
+              return if !config ||
+                  !config.is_a?(Hash) ||
+                  config[:retry].nil? ||
+                  !config[:retry].is_a?(Integer) && !config[:retry].is_a?(Hash)
+
+              check =
+                if config[:retry].is_a?(Integer)
+                  { max: config[:retry] }
+                else
+                  config[:retry]
+                end
+
+              validate_retry_max(check[:max])
+              validate_retry_when(check[:when])
+            end
+
+            def validate_retry_max(retry_max)
+              if retry_max.is_a?(Integer)
+                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
+              else
+                errors[:base] << "retry max #{::I18n.t('errors.messages.not_an_integer')}"
+              end
+            end
+
+            def validate_retry_when(retry_when)
+              return if retry_when.blank?
+
+              possible_failures = Gitlab::Ci::Status::Build::Failed.reasons.keys.map(&:to_s) + ['always']
+
+              if retry_when.is_a?(String)
+                unless possible_failures.include?(retry_when)
+                  errors[:base] << 'retry when is unknown'
+                end
+              elsif retry_when.is_a?(Array)
+                unknown_whens = retry_when - possible_failures
+                unless unknown_whens.empty?
+                  errors[:base] << "retry when cannot have unknown failures #{unknown_whens.join(', ')}"
+                end
+              else
+                errors[:base] << 'retry when should be an array of strings or a string'
+              end
+            end
+
             with_options allow_nil: true do
               validates :tags, array_of_strings: true
               validates :allow_failure, boolean: true
-              validates :retry, numericality: { only_integer: true,
-                                                greater_than_or_equal_to: 0,
-                                                less_than_or_equal_to: 2 }
               validates :parallel, numericality: { only_integer: true,
                                                    greater_than_or_equal_to: 2 }
               validates :when,
@@ -160,7 +207,7 @@ module Gitlab
               environment: environment_defined? ? environment_value : nil,
               environment_name: environment_defined? ? environment_value[:name] : nil,
               coverage: coverage_defined? ? coverage_value : nil,
-              retry: retry_defined? ? retry_value.to_i : nil,
+              retry: retry_defined? ? retry_value : nil,
               parallel: parallel_defined? ? parallel_value.to_i : nil,
               artifacts: artifacts_value,
               after_script: after_script_value,

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

@@ -7,10 +7,10 @@ module Gitlab
         module Validators
           class AllowedKeysValidator < ActiveModel::EachValidator
             def validate_each(record, attribute, value)
-              unknown_keys = record.config.try(:keys).to_a - options[:in]
+              unknown_keys = value.try(:keys).to_a - options[:in]
 
               if unknown_keys.any?
-                record.errors.add(:config, 'contains unknown keys: ' +
+                record.errors.add(:config, "#{attribute} contains unknown keys: " +
                                             unknown_keys.join(', '))
               end
             end
@@ -68,6 +68,14 @@ module Gitlab
             end
           end
 
+          class HashOrIntegerValidator < ActiveModel::EachValidator
+            def validate_each(record, attribute, value)
+              unless value.is_a?(Hash) || value.is_a?(Integer)
+                record.errors.add(attribute, 'should be a hash or an integer')
+              end
+            end
+          end
+
           class KeyValidator < ActiveModel::EachValidator
             include LegacyValidationHelpers
 

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

@@ -98,41 +98,152 @@ describe Gitlab::Ci::Config::Entry::Job do
         end
       end
 
+      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
+      end
+
       context 'when retry value is not correct' do
-        context 'when it is not a numeric value' 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 is not a number'
+            expect(entry.errors).to include 'job retry should be a hash or an integer'
           end
         end
 
-        context 'when it is lower than zero' do
-          let(:config) { { retry: -1 } }
+        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 must be greater than or equal to 0'
+            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
-        end
 
-        context 'when it is not an integer' do
-          let(:config) { { retry: 1.5 } }
+          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 must be an integer'
+            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 'when the value is too high' do
-          let(:config) { { retry: 10 } }
+        context 'defined as a hash' do
+          context 'with unkown keys' do
+            let(:config) { { retry: { max: 2, unknown_key: :something, one_more: :key } } }
 
-          it 'returns error about value too high' do
-            expect(entry).not_to be_valid
-            expect(entry.errors).to include 'job retry must be less than or equal to 2'
+            it 'returns error about the unknown key' do
+              expect(entry).not_to be_valid
+              expect(entry.errors)
+                .to include 'job config 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 cannot have unknown failures unknown_reason'
+            end
           end
         end
       end