From d71a779740d1f667892e9bef798961e2a39a3a9c Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Sat, 15 Sep 2018 12:58:44 +0200
Subject: [PATCH 01/29] allow retries to be a hash

- when it is a hash, retries max count is assumed to be at hash[:max]
- when it is an integer, this is the max count (as before)
---
 app/models/ci/build.rb       |  3 ++-
 spec/models/ci/build_spec.rb | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 25596581d0f..09b03278939 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -320,7 +320,8 @@ module Ci
     end
 
     def retries_max
-      self.options.to_h.fetch(:retry, 0).to_i
+      retries = self.options[:retry]
+      retries.is_a?(Hash) && retries.fetch(:max, 0) || retries || 0
     end
 
     def latest?
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 5bd2f096656..88a466317a5 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1472,7 +1472,7 @@ describe Ci::Build do
     end
 
     describe '#retries_max' do
-      context 'when max retries value is defined' do
+      context 'when max retries value is defined as an integer' do
         subject { create(:ci_build, options: { retry: 1 }) }
 
         it 'returns a number of configured max retries' do
@@ -1480,6 +1480,22 @@ describe Ci::Build do
         end
       end
 
+      context 'when retries value is defined as a hash' do
+        subject { create(:ci_build, options: { retry: { max: 1 } }) }
+
+        it 'returns a number of configured max retries' do
+          expect(subject.retries_max).to eq 1
+        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 a number of configured max retries' do
+          expect(subject.retries_max).to eq 0
+        end
+      end
+
       context 'when max retries value is not defined' do
         subject { create(:ci_build) }
 

From 473b52b2837bfffc634f574f148d949e218137ed Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Sat, 15 Sep 2018 13:39:55 +0200
Subject: [PATCH 02/29] add an option when to retry a build (unused yet)

---
 app/models/ci/build.rb       |  5 +++++
 spec/models/ci/build_spec.rb | 26 ++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 09b03278939..b133c756fcf 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -324,6 +324,11 @@ module Ci
       retries.is_a?(Hash) && retries.fetch(:max, 0) || retries || 0
     end
 
+    def retry_when
+      retries = self.options[:retry]
+      retries.is_a?(Hash) && retries.fetch(:when, 'always').to_s || 'always'
+    end
+
     def latest?
       !retried?
     end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 88a466317a5..ba1de3612ad 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1512,6 +1512,32 @@ describe Ci::Build do
         end
       end
     end
+
+    describe '#retry_when' do
+      context 'when value is defined' do
+        subject { create(:ci_build, options: { retry: { when: :something } }) }
+
+        it 'returns the configured value' do
+          expect(subject.retry_when).to eq :something
+        end
+      end
+
+      context 'when value is not defined' 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
+          expect(subject.retry_when).to eq :always
+        end
+      end
+    end
   end
 
   describe '#keep_artifacts!' do

From 007db85dd4e4c92e060160427429c4fb2ad5cb32 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Sat, 15 Sep 2018 14:02:43 +0200
Subject: [PATCH 03/29] add `retry_failure?` option and use it to decide if to
 retry a build failure

---
 app/models/ci/build.rb       |  17 +++-
 spec/models/ci/build_spec.rb | 150 +++++++++++++++++++++++++++++++++++
 2 files changed, 164 insertions(+), 3 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index b133c756fcf..592b784807c 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -220,9 +220,7 @@ module Ci
 
         build.deployment&.drop
 
-        next if build.retries_max.zero?
-
-        if build.retries_count < build.retries_max
+        if build.retry_failure?
           begin
             Ci::Build.retry(build, build.user)
           rescue Gitlab::Access::AccessDeniedError => ex
@@ -329,6 +327,19 @@ module Ci
       retries.is_a?(Hash) && retries.fetch(:when, 'always').to_s || 'always'
     end
 
+    def retry_failure?
+      return false if retries_max.zero? || retries_count >= retries_max
+
+      case failure_reason.to_s
+      when 'runner_system_failure'
+        %['always', 'system failure'].include?(retry_when)
+      when 'script_failure'
+        %['always', 'script failure'].include?(retry_when)
+      else
+        retry_when == 'always'
+      end
+    end
+
     def latest?
       !retried?
     end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index ba1de3612ad..3085cf7b271 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1538,6 +1538,156 @@ describe Ci::Build do
         end
       end
     end
+
+    describe '#retry_failure?' do
+      subject { create(:ci_build) }
+
+      context 'when retries max is zero' do
+        before do
+          expect(subject).to receive(:retries_max).at_least(:once).and_return(0)
+        end
+
+        it 'returns false' do
+          expect(subject.retry_failure?).to eq false
+        end
+      end
+
+      context 'when retries max equals retries count' do
+        before do
+          expect(subject).to receive(:retries_max).at_least(:once).and_return(1)
+          expect(subject).to receive(:retries_count).at_least(:once).and_return(1)
+        end
+
+        it 'returns false' do
+          expect(subject.retry_failure?).to eq false
+        end
+      end
+
+      context 'when retries max is higher than retries count' do
+        before do
+          expect(subject).to receive(:retries_max).at_least(:once).and_return(2)
+          expect(subject).to receive(:retries_count).at_least(:once).and_return(1)
+        end
+
+        context 'and retry when is always' do
+          before do
+            expect(subject).to receive(:retry_when).at_least(:once).and_return('always')
+          end
+
+          it 'returns true' do
+            expect(subject.retry_failure?).to eq true
+          end
+        end
+
+        context 'and failure was a system runner failure' do
+          before do
+            expect(subject).to receive(:failure_reason).at_least(:once).and_return('runner_system_failure')
+          end
+
+          context 'and retry when is always' do
+            before do
+              expect(subject).to receive(:retry_when).at_least(:once).and_return('always')
+            end
+
+            it 'returns true' do
+              expect(subject.retry_failure?).to eq true
+            end
+          end
+
+          context 'and retry when is system failure' do
+            before do
+              expect(subject).to receive(:retry_when).at_least(:once).and_return('system failure')
+            end
+
+            it 'returns true' do
+              expect(subject.retry_failure?).to eq true
+            end
+          end
+
+          context 'and retry when is script failure' do
+            before do
+              expect(subject).to receive(:retry_when).at_least(:once).and_return('script failure')
+            end
+
+            it 'returns false' do
+              expect(subject.retry_failure?).to eq false
+            end
+          end
+        end
+
+        context 'and failure was a script failure' do
+          before do
+            expect(subject).to receive(:failure_reason).at_least(:once).and_return('script_failure')
+          end
+
+          context 'and retry when is always' do
+            before do
+              expect(subject).to receive(:retry_when).at_least(:once).and_return('always')
+            end
+
+            it 'returns true' do
+              expect(subject.retry_failure?).to eq true
+            end
+          end
+
+          context 'and retry when is system failure' do
+            before do
+              expect(subject).to receive(:retry_when).at_least(:once).and_return('system failure')
+            end
+
+            it 'returns false' do
+              expect(subject.retry_failure?).to eq false
+            end
+          end
+
+          context 'and retry when is script failure' do
+            before do
+              expect(subject).to receive(:retry_when).at_least(:once).and_return('script failure')
+            end
+
+            it 'returns true' do
+              expect(subject.retry_failure?).to eq true
+            end
+          end
+        end
+
+        context 'and failure was some other failure' do
+          before do
+            expect(subject).to receive(:failure_reason).at_least(:once).and_return('some other reason')
+          end
+
+          context 'and retry when is always' do
+            before do
+              expect(subject).to receive(:retry_when).at_least(:once).and_return('always')
+            end
+
+            it 'returns true' do
+              expect(subject.retry_failure?).to eq true
+            end
+          end
+
+          context 'and retry when is system failure' do
+            before do
+              expect(subject).to receive(:retry_when).at_least(:once).and_return('system failure')
+            end
+
+            it 'returns false' do
+              expect(subject.retry_failure?).to eq false
+            end
+          end
+
+          context 'and retry when is script failure' do
+            before do
+              expect(subject).to receive(:retry_when).at_least(:once).and_return('script failure')
+            end
+
+            it 'returns false' do
+              expect(subject.retry_failure?).to eq false
+            end
+          end
+        end
+      end
+    end
   end
 
   describe '#keep_artifacts!' do

From 39204d8c6184328b21e4057aaa78d698abd56e29 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Sat, 15 Sep 2018 14:31:36 +0200
Subject: [PATCH 04/29] refactor retry logic to define any reason and more than
 one reason to retry

---
 app/models/ci/build.rb       |  13 ++--
 spec/models/ci/build_spec.rb | 124 +++++++----------------------------
 2 files changed, 27 insertions(+), 110 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 592b784807c..08d6e669cf9 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -324,20 +324,15 @@ module Ci
 
     def retry_when
       retries = self.options[:retry]
-      retries.is_a?(Hash) && retries.fetch(:when, 'always').to_s || 'always'
+      Array.wrap(
+        retries.is_a?(Hash) && retries.fetch(:when, 'always') || 'always'
+      )
     end
 
     def retry_failure?
       return false if retries_max.zero? || retries_count >= retries_max
 
-      case failure_reason.to_s
-      when 'runner_system_failure'
-        %['always', 'system failure'].include?(retry_when)
-      when 'script_failure'
-        %['always', 'script failure'].include?(retry_when)
-      else
-        retry_when == 'always'
-      end
+      retry_when.include?('always') || retry_when.include?(failure_reason.to_s)
     end
 
     def latest?
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 3085cf7b271..4f113d2e653 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1514,11 +1514,19 @@ describe Ci::Build do
     end
 
     describe '#retry_when' do
-      context 'when value is defined' do
-        subject { create(:ci_build, options: { retry: { when: :something } }) }
+      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 without as an array' do
+        subject { create(:ci_build, options: { retry: { when: %w[something more] } }) }
 
         it 'returns the configured value' do
-          expect(subject.retry_when).to eq :something
+          expect(subject.retry_when).to eq %w[something more]
         end
       end
 
@@ -1526,7 +1534,7 @@ describe Ci::Build do
         subject { create(:ci_build) }
 
         it 'returns `always`' do
-          expect(subject.retry_when).to eq :always
+          expect(subject.retry_when).to eq ['always']
         end
       end
 
@@ -1534,7 +1542,7 @@ describe Ci::Build do
         subject { create(:ci_build, options: { retry: 1 }) }
 
         it 'returns `always`' do
-          expect(subject.retry_when).to eq :always
+          expect(subject.retry_when).to eq ['always']
         end
       end
     end
@@ -1579,111 +1587,25 @@ describe Ci::Build do
           end
         end
 
-        context 'and failure was a system runner failure' do
+        context 'and retry when includes the failure_reason' do
           before do
-            expect(subject).to receive(:failure_reason).at_least(:once).and_return('runner_system_failure')
+            expect(subject).to receive(:failure_reason).at_least(:once).and_return('some_reason')
+            expect(subject).to receive(:retry_when).at_least(:once).and_return(['some_reason'])
           end
 
-          context 'and retry when is always' do
-            before do
-              expect(subject).to receive(:retry_when).at_least(:once).and_return('always')
-            end
-
-            it 'returns true' do
-              expect(subject.retry_failure?).to eq true
-            end
-          end
-
-          context 'and retry when is system failure' do
-            before do
-              expect(subject).to receive(:retry_when).at_least(:once).and_return('system failure')
-            end
-
-            it 'returns true' do
-              expect(subject.retry_failure?).to eq true
-            end
-          end
-
-          context 'and retry when is script failure' do
-            before do
-              expect(subject).to receive(:retry_when).at_least(:once).and_return('script failure')
-            end
-
-            it 'returns false' do
-              expect(subject.retry_failure?).to eq false
-            end
+          it 'returns true' do
+            expect(subject.retry_failure?).to eq true
           end
         end
 
-        context 'and failure was a script failure' do
+        context 'and retry when does not include failure_reason' do
           before do
-            expect(subject).to receive(:failure_reason).at_least(:once).and_return('script_failure')
+            expect(subject).to receive(:failure_reason).at_least(:once).and_return('some_reason')
+            expect(subject).to receive(:retry_when).at_least(:once).and_return(['some', 'other failure'])
           end
 
-          context 'and retry when is always' do
-            before do
-              expect(subject).to receive(:retry_when).at_least(:once).and_return('always')
-            end
-
-            it 'returns true' do
-              expect(subject.retry_failure?).to eq true
-            end
-          end
-
-          context 'and retry when is system failure' do
-            before do
-              expect(subject).to receive(:retry_when).at_least(:once).and_return('system failure')
-            end
-
-            it 'returns false' do
-              expect(subject.retry_failure?).to eq false
-            end
-          end
-
-          context 'and retry when is script failure' do
-            before do
-              expect(subject).to receive(:retry_when).at_least(:once).and_return('script failure')
-            end
-
-            it 'returns true' do
-              expect(subject.retry_failure?).to eq true
-            end
-          end
-        end
-
-        context 'and failure was some other failure' do
-          before do
-            expect(subject).to receive(:failure_reason).at_least(:once).and_return('some other reason')
-          end
-
-          context 'and retry when is always' do
-            before do
-              expect(subject).to receive(:retry_when).at_least(:once).and_return('always')
-            end
-
-            it 'returns true' do
-              expect(subject.retry_failure?).to eq true
-            end
-          end
-
-          context 'and retry when is system failure' do
-            before do
-              expect(subject).to receive(:retry_when).at_least(:once).and_return('system failure')
-            end
-
-            it 'returns false' do
-              expect(subject.retry_failure?).to eq false
-            end
-          end
-
-          context 'and retry when is script failure' do
-            before do
-              expect(subject).to receive(:retry_when).at_least(:once).and_return('script failure')
-            end
-
-            it 'returns false' do
-              expect(subject.retry_failure?).to eq false
-            end
+          it 'returns false' do
+            expect(subject.retry_failure?).to eq false
           end
         end
       end

From 93705f9e72cd14c89d6cf2d3e364611f51c8c038 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Mon, 17 Sep 2018 12:53:00 +0200
Subject: [PATCH 05/29] fix some spec wording and values

---
 spec/models/ci/build_spec.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 4f113d2e653..2b4923926db 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1475,7 +1475,7 @@ describe Ci::Build do
       context 'when max retries value is defined as an integer' do
         subject { create(:ci_build, options: { retry: 1 }) }
 
-        it 'returns a number of configured max retries' do
+        it 'returns the number of configured max retries' do
           expect(subject.retries_max).to eq 1
         end
       end
@@ -1483,7 +1483,7 @@ describe Ci::Build do
       context 'when retries value is defined as a hash' do
         subject { create(:ci_build, options: { retry: { max: 1 } }) }
 
-        it 'returns a number of configured max retries' do
+        it 'returns the number of configured max retries' do
           expect(subject.retries_max).to eq 1
         end
       end
@@ -1491,7 +1491,7 @@ describe Ci::Build do
       context 'when retries value is defined as a hash without max key' do
         subject { create(:ci_build, options: { retry: { something: :else } }) }
 
-        it 'returns a number of configured max retries' do
+        it 'returns zero' do
           expect(subject.retries_max).to eq 0
         end
       end
@@ -1522,7 +1522,7 @@ describe Ci::Build do
         end
       end
 
-      context 'when value is defined without as an array' do
+      context 'when value is defined as an array' do
         subject { create(:ci_build, options: { retry: { when: %w[something more] } }) }
 
         it 'returns the configured value' do
@@ -1579,7 +1579,7 @@ describe Ci::Build do
 
         context 'and retry when is always' do
           before do
-            expect(subject).to receive(:retry_when).at_least(:once).and_return('always')
+            expect(subject).to receive(:retry_when).at_least(:once).and_return(['always'])
           end
 
           it 'returns true' do

From 42f36954348c3f09dc31e4c7697f857f9dc63111 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Wed, 19 Sep 2018 14:08:28 +0200
Subject: [PATCH 06/29] add changelog entry

---
 changelogs/unreleased/max_retries_when.yml | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/max_retries_when.yml

diff --git a/changelogs/unreleased/max_retries_when.yml b/changelogs/unreleased/max_retries_when.yml
new file mode 100644
index 00000000000..dad3cd8a123
--- /dev/null
+++ b/changelogs/unreleased/max_retries_when.yml
@@ -0,0 +1,5 @@
+---
+title: Allow to configure when to retry failed CI jobs
+merge_request: 21758
+author: Markus Doits
+type: added

From 0db50a808d15e7a3d93552da8a0051ed5245f461 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Wed, 19 Sep 2018 17:27:31 +0200
Subject: [PATCH 07/29] update job config validator to validate new retry
 syntax

---
 lib/gitlab/ci/config/entry/job.rb           |  55 +++++++-
 lib/gitlab/ci/config/entry/validators.rb    |  12 +-
 spec/lib/gitlab/ci/config/entry/job_spec.rb | 149 +++++++++++++++++---
 3 files changed, 191 insertions(+), 25 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index 362014b1a09..765053d8c33 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/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,
diff --git a/lib/gitlab/ci/config/entry/validators.rb b/lib/gitlab/ci/config/entry/validators.rb
index 805d26ca8d8..137694ff93d 100644
--- a/lib/gitlab/ci/config/entry/validators.rb
+++ b/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
 
diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index f1a2946acda..9941c975517 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/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
+
+          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 'when it is not an integer' do
-          let(:config) { { retry: 1.5 } }
+        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 wrong value' do
-            expect(entry).not_to be_valid
-            expect(entry.errors).to include 'job retry must be an integer'
+            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
-        end
 
-        context 'when the value is too high' do
-          let(:config) { { retry: 10 } }
+          context 'when max is lower than zero' do
+            let(:config) { { retry: { max: -1 } } }
 
-          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 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

From 452a4399be2a2a869b2e3f7487669e0c5c74dcb2 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Wed, 19 Sep 2018 17:42:24 +0200
Subject: [PATCH 08/29] add specs for create pipeline service and new retry
 when feature

---
 .../ci/create_pipeline_service_spec.rb        | 32 +++++++++++++++----
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 054b7b1561c..5c87ed5c3c6 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -435,16 +435,34 @@ describe Ci::CreatePipelineService do
     end
 
     context 'when builds with auto-retries are configured' do
-      before do
-        config = YAML.dump(rspec: { script: 'rspec', retry: 2 })
-        stub_ci_pipeline_yaml_file(config)
+      context 'as an integer' do
+        before do
+          config = YAML.dump(rspec: { script: 'rspec', retry: 2 })
+          stub_ci_pipeline_yaml_file(config)
+        end
+
+        it 'correctly creates builds with auto-retry value configured' do
+          pipeline = execute_service
+
+          expect(pipeline).to be_persisted
+          expect(pipeline.builds.find_by(name: 'rspec').retries_max).to eq 2
+          expect(pipeline.builds.find_by(name: 'rspec').retry_when).to eq ['always']
+        end
       end
 
-      it 'correctly creates builds with auto-retry value configured' do
-        pipeline = execute_service
+      context 'as hash' do
+        before do
+          config = YAML.dump(rspec: { script: 'rspec', retry: { max: 2, when: 'runner_system_failure' } })
+          stub_ci_pipeline_yaml_file(config)
+        end
 
-        expect(pipeline).to be_persisted
-        expect(pipeline.builds.find_by(name: 'rspec').retries_max).to eq 2
+        it 'correctly creates builds with auto-retry value configured' do
+          pipeline = execute_service
+
+          expect(pipeline).to be_persisted
+          expect(pipeline.builds.find_by(name: 'rspec').retries_max).to eq 2
+          expect(pipeline.builds.find_by(name: 'rspec').retry_when).to eq ['runner_system_failure']
+        end
       end
     end
 

From ffcc200a68fd6d08707a8fd70b128a79206fd69e Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Wed, 19 Sep 2018 18:03:12 +0200
Subject: [PATCH 09/29] fix allowed keys validator to use correct attribute as
 error base?

---
 lib/gitlab/ci/config/entry/validators.rb    | 4 ++--
 spec/lib/gitlab/ci/config/entry/job_spec.rb | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/validators.rb b/lib/gitlab/ci/config/entry/validators.rb
index 137694ff93d..af902eb0ee8 100644
--- a/lib/gitlab/ci/config/entry/validators.rb
+++ b/lib/gitlab/ci/config/entry/validators.rb
@@ -10,8 +10,8 @@ module Gitlab
               unknown_keys = value.try(:keys).to_a - options[:in]
 
               if unknown_keys.any?
-                record.errors.add(:config, "#{attribute} contains unknown keys: " +
-                                            unknown_keys.join(', '))
+                record.errors.add(attribute, "contains unknown keys: " +
+                                             unknown_keys.join(', '))
               end
             end
           end
diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index 9941c975517..9054e909a44 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb
@@ -187,7 +187,7 @@ describe Gitlab::Ci::Config::Entry::Job do
             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'
+                .to include 'job retry contains unknown keys: unknown_key, one_more'
             end
           end
 

From fdb5a51352fdb4796d26ab2c9a1daaa5a89a0283 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Wed, 19 Sep 2018 19:38:19 +0200
Subject: [PATCH 10/29] refactor for hopefully lower cognitive complexity

before:

- Method `validate_retry` has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.
- Method `validate_retry_max` has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.
- Method `validate_retry_when` has a Cognitive Complexity of 14 (exceeds 5 allowed). Consider refactoring.
---
 lib/gitlab/ci/config/entry/job.rb           | 79 ++++++++++++---------
 spec/lib/gitlab/ci/config/entry/job_spec.rb |  2 +-
 2 files changed, 45 insertions(+), 36 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index 765053d8c33..76b417c4277 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -26,23 +26,36 @@ module Gitlab
             validates :name, type: Symbol
 
             validates :retry, hash_or_integer: true, allowed_keys: ALLOWED_KEYS_RETRY, allow_nil: true
-            validate :validate_retry
+            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 :when,
+                inclusion: { in: %w[on_success on_failure always manual delayed],
+                             message: 'should be on_success, on_failure, ' \
+                                      'always, manual or delayed' }
+
+              validates :dependencies, array_of_strings: true
+              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
-              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])
+              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)
@@ -57,34 +70,30 @@ module Gitlab
             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
+                validate_retry_when_string(retry_when)
               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
+                validate_retry_when_array(retry_when)
               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 :parallel, numericality: { only_integer: true,
-                                                   greater_than_or_equal_to: 2 }
-              validates :when,
-                inclusion: { in: %w[on_success on_failure always manual delayed],
-                             message: 'should be on_success, on_failure, ' \
-                                      'always, manual or delayed' }
+            def possible_retry_when_options
+              @possible_retry_when_options ||= Gitlab::Ci::Status::Build::Failed.reasons.keys.map(&:to_s) + ['always']
+            end
 
-              validates :dependencies, array_of_strings: true
-              validates :extends, type: String
+            def validate_retry_when_string(retry_when)
+              unless possible_retry_when_options.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_options
+              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?
diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index 9054e909a44..7d0a5b81084 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb
@@ -242,7 +242,7 @@ describe Gitlab::Ci::Config::Entry::Job do
 
             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'
+              expect(entry.errors).to include 'job retry when contains unknown values: unknown_reason'
             end
           end
         end

From b734e3a139fb69fcc79b7c7e44bcd29ff7fc3495 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Wed, 19 Sep 2018 21:07:46 +0200
Subject: [PATCH 11/29] one more try to reduce cognitive overhead

before:

- Method `validate_retry_max` has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.
- Method `validate_retry_when` has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
---
 lib/gitlab/ci/config/entry/job.rb | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index 76b417c4277..5a91d5574f0 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -59,38 +59,44 @@ module Gitlab
             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
+              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?
 
-              if retry_when.is_a?(String)
+              case retry_when
+              when String
                 validate_retry_when_string(retry_when)
-              elsif retry_when.is_a?(Array)
+              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_options
-              @possible_retry_when_options ||= Gitlab::Ci::Status::Build::Failed.reasons.keys.map(&:to_s) + ['always']
+            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_options.include?(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_options
+              unknown_whens = retry_when - possible_retry_when_values
               unless unknown_whens.empty?
                 errors[:base] << "retry when contains unknown values: #{unknown_whens.join(', ')}"
               end

From 95236f539a2a1734db24226682965164a3db0a8e Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Thu, 20 Sep 2018 16:04:00 +0200
Subject: [PATCH 12/29] add documentation for new retry when feature

---
 doc/ci/yaml/README.md | 49 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index b3a55e48f4e..58ab0838222 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -74,7 +74,7 @@ A job is defined by a list of parameters that define the job behavior.
 | after_script  | no       | Override a set of commands that are executed after job |
 | environment   | no       | Defines a name of environment to which deployment is done by this job |
 | coverage      | no       | Define code coverage settings for a given job |
-| retry         | no       | Define how many times a job can be auto-retried in case of a failure |
+| retry         | no       | Define when and how many times a job can be auto-retried in case of a failure |
 | parallel      | no       | Defines how many instances of a job should be run in parallel |
 
 ### `extends`
@@ -1433,18 +1433,19 @@ job1:
 ## `retry`
 
 > [Introduced][ce-12909] in GitLab 9.5.
+> [Behaviour expanded][ce-21758] in GitLab 11.4 to control on which failures to retry.
 
 `retry` allows you to configure how many times a job is going to be retried in
 case of a failure.
 
-When a job fails, and has `retry` configured it is going to be processed again
+When a job fails and has `retry` configured it is going to be processed again
 up to the amount of times specified by the `retry` keyword.
 
 If `retry` is set to 2, and a job succeeds in a second run (first retry), it won't be retried
 again. `retry` value has to be a positive integer, equal or larger than 0, but
 lower or equal to 2 (two retries maximum, three runs in total).
 
-A simple example:
+A simple example to retry in all failure cases:
 
 ```yaml
 test:
@@ -1452,6 +1453,47 @@ test:
   retry: 2
 ```
 
+By default a job will be retried on all failure cases. To have a better control
+on which failures to retry, `retry` can be a hash with `when` and `max` keys. `max`
+specifies how many times to retry, `when` the failure cases to retry.
+
+To retry only runner system failures at maximum two times:
+
+```yaml
+test:
+  script: rspec
+  retry:
+    max: 2
+    when: runner_system_failure
+```
+
+If there is another failure than a runner system failure, the job will not be
+retried.
+
+To retry on multiple failure cases `when` can also be an array of failures:
+
+```yaml
+test:
+  script: rspec
+  retry:
+    max: 2
+    when:
+      - runner_system_failure
+      - stuck_or_timeout_failure
+```
+
+Possible values for `when` are:
+
+- `always`: retry on any failure (default)
+- `unknown_failure`: retry when the failure reason is unknown
+- `script_failure`: retry when the script failed
+- `api_failure`: retry on api failure
+- `stuck_or_timeout_failure`: retry when the job got stuck or timed out
+- `runner_system_failure`: retry if there was a runner system failure (e.g. setting up the job failed)
+- `missing_dependency_failure`: retry if a dependency was missing
+- `runner_unsupported`: retry if the runner was unsupported
+
+
 ## `parallel`
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/22631) in GitLab 11.5.
@@ -2052,6 +2094,7 @@ CI with various languages.
 [ce-7983]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7983
 [ce-7447]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7447
 [ce-12909]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12909
+[ce-21758]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21758
 [schedules]: ../../user/project/pipelines/schedules.md
 [variables-expressions]: ../variables/README.md#variables-expressions
 [ee]: https://about.gitlab.com/gitlab-ee/

From 48f37a92be9b9b3e21cce6772c147a303a881ca5 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Thu, 20 Sep 2018 16:25:37 +0200
Subject: [PATCH 13/29] add a test that checks that retry when values in
 documentation are valid

---
 doc/ci/yaml/README.md                       |  8 ++++++
 spec/lib/gitlab/ci/config/entry/job_spec.rb | 28 ++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index 58ab0838222..7be00f0bfc8 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -1484,6 +1484,14 @@ test:
 
 Possible values for `when` are:
 
+<!--
+  Please make sure to update `RETRY_WHEN_IN_DOCUMENTATION` array in
+  `spec/lib/gitlab/ci/config/entry/job_spec.rb` if you change any of
+  the documented values below. The test there makes sure that all documented
+  values are really valid as a config option and therefore should always
+  stay in sync with this documentation.
+ -->
+
 - `always`: retry on any failure (default)
 - `unknown_failure`: retry when the failure reason is unknown
 - `script_failure`: retry when the script failed
diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index 7d0a5b81084..36ff4519bcb 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb
@@ -138,6 +138,32 @@ describe Gitlab::Ci::Config::Entry::Job 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
@@ -181,7 +207,7 @@ describe Gitlab::Ci::Config::Entry::Job do
         end
 
         context 'defined as a hash' do
-          context 'with unkown keys' 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

From b7ee04d4564deaf30cf3c8ead34701ac6d746c05 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Thu, 18 Oct 2018 18:48:44 +0200
Subject: [PATCH 14/29] refactor validations to a Entry::Retry class

---
 lib/gitlab/ci/config/entry/job.rb             |  68 +------
 lib/gitlab/ci/config/entry/retry.rb           |  64 ++++++
 lib/gitlab/ci/config/entry/validators.rb      |  10 +
 spec/lib/gitlab/ci/config/entry/job_spec.rb   | 182 ++++++++++++++++-
 spec/lib/gitlab/ci/config/entry/retry_spec.rb | 183 ++++++++++++++++++
 5 files changed, 442 insertions(+), 65 deletions(-)
 create mode 100644 lib/gitlab/ci/config/entry/retry.rb
 create mode 100644 spec/lib/gitlab/ci/config/entry/retry_spec.rb

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

From 7c5c1e9240f8ced4f569b0b97b4c87a53657fb99 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Thu, 18 Oct 2018 19:05:11 +0200
Subject: [PATCH 15/29] fix wrong retry error messages

---
 lib/gitlab/ci/config/entry/retry.rb           | 12 ++++++++++++
 spec/lib/gitlab/ci/config/entry/retry_spec.rb |  4 ++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/retry.rb b/lib/gitlab/ci/config/entry/retry.rb
index 140201447d1..1f9539dafdb 100644
--- a/lib/gitlab/ci/config/entry/retry.rb
+++ b/lib/gitlab/ci/config/entry/retry.rb
@@ -17,6 +17,10 @@ module Gitlab
                                                  greater_than_or_equal_to: 0,
                                                  less_than_or_equal_to: 2 }
             end
+
+            def location
+              'retry'
+            end
           end
 
           class FullRetry < Entry::Node
@@ -47,12 +51,20 @@ module Gitlab
             def self.possible_retry_when_values
               @possible_retry_when_values ||= Gitlab::Ci::Status::Build::Failed.reasons.keys.map(&:to_s) + ['always']
             end
+
+            def location
+              'retry'
+            end
           end
 
           class UnknownStrategy < Entry::Node
             def errors
               ["#{location} has to be either an integer or a hash"]
             end
+
+            def location
+              'retry config'
+            end
           end
 
           def self.default
diff --git a/spec/lib/gitlab/ci/config/entry/retry_spec.rb b/spec/lib/gitlab/ci/config/entry/retry_spec.rb
index 17ffa51106f..cc5dfb56655 100644
--- a/spec/lib/gitlab/ci/config/entry/retry_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/retry_spec.rb
@@ -78,7 +78,7 @@ describe Gitlab::Ci::Config::Entry::Retry do
 
         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'
+          expect(entry.errors).to include 'retry config has to be either an integer or a hash'
         end
       end
 
@@ -98,7 +98,7 @@ describe Gitlab::Ci::Config::Entry::Retry do
 
           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'
+            expect(entry.errors).to include 'retry config has to be either an integer or a hash'
           end
         end
 

From ff3484a0b331456b36f82b0d6cda2afd56da566e Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Thu, 18 Oct 2018 19:10:29 +0200
Subject: [PATCH 16/29] remove now unneeded validation

---
 lib/gitlab/ci/config/entry/job.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index 5ff03d803dc..6907fad8e8a 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -28,7 +28,6 @@ module Gitlab
               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],

From 9818bb561889579ed1031e90b7879ec1c3f0e25a Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Fri, 19 Oct 2018 16:47:18 +0200
Subject: [PATCH 17/29] refactoring after latest feedback

---
 app/models/ci/build.rb                        |   8 +-
 lib/gitlab/ci/config/entry/retry.rb           |  16 +-
 spec/lib/gitlab/ci/config/entry/retry_spec.rb | 193 +++++++++++-------
 spec/models/ci/build_spec.rb                  |  50 +----
 4 files changed, 149 insertions(+), 118 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 08d6e669cf9..7b220487495 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 1f9539dafdb..a89d9f05117 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 cc5dfb56655..4feddafb580 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) }
 
+  shared_context 'when retry value is a numeric', :numeric do
+    let(:config) { max }
+    let(:max) {}
+  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 retry value is a hash', :hash do
+      context 'and `when` is a string' do
+        let(:when) { 'unknown_failure' }
+
+        it 'returns when wrapped in an array' do
+          expect(value).to eq(when: ['unknown_failure'])
+        end
+      end
+
+      context 'and `when` is an array' do
+        let(:when) { %w[unknown_failure runner_system_failure] }
+
+        it 'returns when as it was passed' do
+          expect(value).to eq(when: %w[unknown_failure runner_system_failure])
+        end
+      end
+    end
+  end
+
   describe 'validation' do
     context 'when retry value is correct' do
-      context 'when it is a numeric' do
-        let(:config) { 2 }
+      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 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 } }
+      context 'when it is a hash', :hash do
+        context 'with max' do
+          let(:max) { 2 }
 
           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
+
+        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 2b4923926db..8a7b06ad21c 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' } }) }
+      context 'with retries when config option' do
+        subject { create(:ci_build, options: { retry: { when: ['some_reason'] } }) }
 
-        it 'returns the configured value inside an array' do
-          expect(subject.retry_when).to eq ['something']
+        it 'returns the configured when' do
+          expect(subject.retry_when).to eq ['some_reason']
         end
       end
 
-      context 'when value is defined as an array' do
-        subject { create(:ci_build, options: { retry: { when: %w[something more] } }) }
-
-        it 'returns the configured value' do
-          expect(subject.retry_when).to eq %w[something more]
-        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)

From 63aa35cb206d265faeebfaf2cc71156c69077ce8 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Fri, 19 Oct 2018 16:54:29 +0200
Subject: [PATCH 18/29] small fixes to doc and remove on whitespace noise

---
 doc/ci/yaml/README.md             | 2 +-
 lib/gitlab/ci/config/entry/job.rb | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index 7be00f0bfc8..056e52843e7 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -1433,7 +1433,7 @@ job1:
 ## `retry`
 
 > [Introduced][ce-12909] in GitLab 9.5.
-> [Behaviour expanded][ce-21758] in GitLab 11.4 to control on which failures to retry.
+> [Behaviour expanded][ce-21758] in GitLab 11.5 to control on which failures to retry.
 
 `retry` allows you to configure how many times a job is going to be retried in
 case of a failure.
diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index 6907fad8e8a..8e8c979f973 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -28,12 +28,10 @@ module Gitlab
               validates :allow_failure, boolean: true
               validates :parallel, numericality: { only_integer: true,
                                                    greater_than_or_equal_to: 2 }
-
               validates :when,
                 inclusion: { in: %w[on_success on_failure always manual delayed],
                              message: 'should be on_success, on_failure, ' \
                                       'always, manual or delayed' }
-
               validates :dependencies, array_of_strings: true
               validates :extends, type: String
             end

From c68f52703596ccbdb513a5649be7144c5ad6e767 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Fri, 19 Oct 2018 17:45:42 +0200
Subject: [PATCH 19/29] fix failing specs due to latest changes

---
 spec/lib/gitlab/ci/yaml_processor_spec.rb         | 4 ++--
 spec/services/ci/process_pipeline_service_spec.rb | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb
index dcfd54107a3..441e8214181 100644
--- a/spec/lib/gitlab/ci/yaml_processor_spec.rb
+++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb
@@ -53,11 +53,11 @@ module Gitlab
         describe 'retry entry' do
           context 'when retry count is specified' do
             let(:config) do
-              YAML.dump(rspec: { script: 'rspec', retry: 1 })
+              YAML.dump(rspec: { script: 'rspec', retry: { max: 1 } })
             end
 
             it 'includes retry count in build options attribute' do
-              expect(subject[:options]).to include(retry: 1)
+              expect(subject[:options]).to include(retry: { max: 1 })
             end
           end
 
diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb
index 8c7258c42ad..538992b621e 100644
--- a/spec/services/ci/process_pipeline_service_spec.rb
+++ b/spec/services/ci/process_pipeline_service_spec.rb
@@ -671,9 +671,9 @@ describe Ci::ProcessPipelineService, '#execute' do
 
   context 'when builds with auto-retries are configured' do
     before do
-      create_build('build:1', stage_idx: 0, user: user, options: { retry: 2 })
+      create_build('build:1', stage_idx: 0, user: user, options: { retry: { max: 2 } })
       create_build('test:1', stage_idx: 1, user: user, when: :on_failure)
-      create_build('test:2', stage_idx: 1, user: user, options: { retry: 1 })
+      create_build('test:2', stage_idx: 1, user: user, options: { retry: { max: 1 } })
     end
 
     it 'automatically retries builds in a valid order' do

From d131b3e2697654a59d57a70fbaec519dfa9a79e0 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Mon, 22 Oct 2018 10:54:43 +0200
Subject: [PATCH 20/29] fix location of spec file in doc comment [CI SKIP]

---
 doc/ci/yaml/README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index 056e52843e7..74b30b93fa9 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -1486,7 +1486,7 @@ Possible values for `when` are:
 
 <!--
   Please make sure to update `RETRY_WHEN_IN_DOCUMENTATION` array in
-  `spec/lib/gitlab/ci/config/entry/job_spec.rb` if you change any of
+  `spec/lib/gitlab/ci/config/entry/retry_spec.rb` if you change any of
   the documented values below. The test there makes sure that all documented
   values are really valid as a config option and therefore should always
   stay in sync with this documentation.

From b1c15f2bd4b085c8d3826185891d517923fa890e Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Mon, 22 Oct 2018 21:29:45 +0200
Subject: [PATCH 21/29] use Ci::Build instead of CommitStatus as per feedback

---
 lib/gitlab/ci/config/entry/retry.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gitlab/ci/config/entry/retry.rb b/lib/gitlab/ci/config/entry/retry.rb
index a89d9f05117..e39cc5de229 100644
--- a/lib/gitlab/ci/config/entry/retry.rb
+++ b/lib/gitlab/ci/config/entry/retry.rb
@@ -55,7 +55,7 @@ module Gitlab
             end
 
             def self.possible_retry_when_values
-              @possible_retry_when_values ||= CommitStatus.failure_reasons.keys.map(&:to_s) + ['always']
+              @possible_retry_when_values ||= ::Ci::Build.failure_reasons.keys.map(&:to_s) + ['always']
             end
 
             def value

From 9b327ffdbd3c5c845edcd3cdc82f45468899a22f Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Tue, 23 Oct 2018 10:36:23 +0200
Subject: [PATCH 22/29] doc updates as per feedback

---
 doc/ci/yaml/README.md | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index 74b30b93fa9..624a4bda2c3 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -1433,12 +1433,13 @@ job1:
 ## `retry`
 
 > [Introduced][ce-12909] in GitLab 9.5.
-> [Behaviour expanded][ce-21758] in GitLab 11.5 to control on which failures to retry.
+> [Behaviour expanded](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21758)
+> in GitLab 11.5 to control on which failures to retry.
 
 `retry` allows you to configure how many times a job is going to be retried in
 case of a failure.
 
-When a job fails and has `retry` configured it is going to be processed again
+When a job fails and has `retry` configured, it is going to be processed again
 up to the amount of times specified by the `retry` keyword.
 
 If `retry` is set to 2, and a job succeeds in a second run (first retry), it won't be retried
@@ -1453,9 +1454,11 @@ test:
   retry: 2
 ```
 
-By default a job will be retried on all failure cases. To have a better control
-on which failures to retry, `retry` can be a hash with `when` and `max` keys. `max`
-specifies how many times to retry, `when` the failure cases to retry.
+By default, a job will be retried on all failure cases. To have a better control
+on which failures to retry, `retry` can be a hash with with the following keys:
+
+- `max`: The maximum number of retries.
+- `when`: The failure cases to retry.
 
 To retry only runner system failures at maximum two times:
 
@@ -1467,10 +1470,10 @@ test:
     when: runner_system_failure
 ```
 
-If there is another failure than a runner system failure, the job will not be
-retried.
+If there is another failure, other than a runner system failure, the job will
+not be retried.
 
-To retry on multiple failure cases `when` can also be an array of failures:
+To retry on multiple failure cases, `when` can also be an array of failures:
 
 ```yaml
 test:
@@ -1492,14 +1495,14 @@ Possible values for `when` are:
   stay in sync with this documentation.
  -->
 
-- `always`: retry on any failure (default)
-- `unknown_failure`: retry when the failure reason is unknown
-- `script_failure`: retry when the script failed
-- `api_failure`: retry on api failure
-- `stuck_or_timeout_failure`: retry when the job got stuck or timed out
-- `runner_system_failure`: retry if there was a runner system failure (e.g. setting up the job failed)
-- `missing_dependency_failure`: retry if a dependency was missing
-- `runner_unsupported`: retry if the runner was unsupported
+- `always`: Retry on any failure (default).
+- `unknown_failure`: Retry when the failure reason is unknown.
+- `script_failure`: Retry when the script failed.
+- `api_failure`: Retry on api failure.
+- `stuck_or_timeout_failure`: Retry when the job got stuck or timed out.
+- `runner_system_failure`: Retry if there was a runner system failure (e.g. setting up the job failed).
+- `missing_dependency_failure`: Retry if a dependency was missing.
+- `runner_unsupported`: Retry if the runner was unsupported.
 
 
 ## `parallel`
@@ -2102,7 +2105,6 @@ CI with various languages.
 [ce-7983]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7983
 [ce-7447]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7447
 [ce-12909]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12909
-[ce-21758]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21758
 [schedules]: ../../user/project/pipelines/schedules.md
 [variables-expressions]: ../variables/README.md#variables-expressions
 [ee]: https://about.gitlab.com/gitlab-ee/

From 293c7b9e4164a2b224d6fcf80c402757130cff1f Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Wed, 24 Oct 2018 21:57:19 +0200
Subject: [PATCH 23/29] small doc fix [CI SKIP]

---
 doc/ci/yaml/README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index 624a4bda2c3..c827faace33 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -1498,7 +1498,7 @@ Possible values for `when` are:
 - `always`: Retry on any failure (default).
 - `unknown_failure`: Retry when the failure reason is unknown.
 - `script_failure`: Retry when the script failed.
-- `api_failure`: Retry on api failure.
+- `api_failure`: Retry on API failure.
 - `stuck_or_timeout_failure`: Retry when the job got stuck or timed out.
 - `runner_system_failure`: Retry if there was a runner system failure (e.g. setting up the job failed).
 - `missing_dependency_failure`: Retry if a dependency was missing.

From dfa8ab6f4219774104412c76b76c2716c44ccf3c Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Wed, 24 Oct 2018 22:22:10 +0200
Subject: [PATCH 24/29] handle old retry format in build (possibly saved in
 database)

---
 app/models/ci/build.rb       | 15 +++++++++++++--
 spec/models/ci/build_spec.rb | 16 ++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 7b220487495..bebf356d1bf 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -317,12 +317,23 @@ module Ci
       pipeline.builds.retried.where(name: self.name).count
     end
 
+    # The format of the retry option changed in GitLab 11.5. Before it was an
+    # integer only, after it is a hash. New builds always created have the
+    # correct format, but builds created before GitLab 11.5 and saved in
+    # database still have the old integer only format. This helper method makes
+    # sure that the format is always correct when accessing the retry options,
+    # even on old builds.
+    def sanitized_retry_option
+      value = options&.[](:retry)
+      value.is_a?(Integer) ? { max: value } : value
+    end
+
     def retries_max
-      options&.dig(:retry, :max) || 0
+      sanitized_retry_option&.[](:max) || 0
     end
 
     def retry_when
-      options&.dig(:retry, :when) || ['always']
+      sanitized_retry_option&.[](:when) || ['always']
     end
 
     def retry_failure?
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 8a7b06ad21c..6849bc6db7a 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1495,6 +1495,14 @@ describe Ci::Build do
           expect(subject.retries_max).to eq 0
         end
       end
+
+      context 'with integer only config option' 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
     end
 
     describe '#retry_when' do
@@ -1513,6 +1521,14 @@ describe Ci::Build do
           expect(subject.retry_when).to eq ['always']
         end
       end
+
+      context 'with integer only config option' do
+        subject { create(:ci_build, options: { retry: 1 }) }
+
+        it 'returns always array' do
+          expect(subject.retry_when).to eq ['always']
+        end
+      end
     end
 
     describe '#retry_failure?' do

From 3aa85715dd5d1274866b32beeb903bf2a62e066e Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Mon, 29 Oct 2018 17:11:26 +0100
Subject: [PATCH 25/29] change forgotten constant in spec to match code

---
 spec/lib/gitlab/ci/config/entry/retry_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/lib/gitlab/ci/config/entry/retry_spec.rb b/spec/lib/gitlab/ci/config/entry/retry_spec.rb
index 4feddafb580..164a9ed4c3d 100644
--- a/spec/lib/gitlab/ci/config/entry/retry_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/retry_spec.rb
@@ -113,7 +113,7 @@ describe Gitlab::Ci::Config::Entry::Retry do
           end
         end
 
-        CommitStatus.failure_reasons.each_key do |reason|
+        ::Ci::Build.failure_reasons.each_key do |reason|
           context "with when from CommitStatus.failure_reasons `#{reason}`" do
             let(:when) { reason }
 

From 024523a3b5385c5ee678c695de5a1aed6f537441 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Mon, 5 Nov 2018 17:28:25 +0100
Subject: [PATCH 26/29] one more refactor after feedback

---
 app/models/ci/build.rb | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index bebf356d1bf..392cf5bb721 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -317,23 +317,12 @@ module Ci
       pipeline.builds.retried.where(name: self.name).count
     end
 
-    # The format of the retry option changed in GitLab 11.5. Before it was an
-    # integer only, after it is a hash. New builds always created have the
-    # correct format, but builds created before GitLab 11.5 and saved in
-    # database still have the old integer only format. This helper method makes
-    # sure that the format is always correct when accessing the retry options,
-    # even on old builds.
-    def sanitized_retry_option
-      value = options&.[](:retry)
-      value.is_a?(Integer) ? { max: value } : value
-    end
-
     def retries_max
-      sanitized_retry_option&.[](:max) || 0
+      normalized_retry.fetch(:max, 0)
     end
 
     def retry_when
-      sanitized_retry_option&.[](:when) || ['always']
+      normalized_retry.fetch(:when, ['always'])
     end
 
     def retry_failure?
@@ -904,6 +893,17 @@ module Ci
       options&.dig(:environment, :url) || persisted_environment&.external_url
     end
 
+    # The format of the retry option changed in GitLab 11.5. Before it was an
+    # integer only, after it is a hash. New builds always created have the
+    # correct format, but builds created before GitLab 11.5 and saved in
+    # database still have the old integer only format. This helper method makes
+    # sure that the format is always correct when accessing the retry options,
+    # even on old builds.
+    def normalized_retry
+      value = options[:retry]
+      value.is_a?(Integer) ? { max: value } : value.to_h
+    end
+
     def build_attributes_from_config
       return {} unless pipeline.config_processor
 

From 65e2731f8316d614b9002f5c80f6afbd08925543 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Mon, 5 Nov 2018 19:18:16 +0100
Subject: [PATCH 27/29] fix failure in case of missing options

---
 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 392cf5bb721..9d6bfbb0db7 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -900,7 +900,7 @@ module Ci
     # sure that the format is always correct when accessing the retry options,
     # even on old builds.
     def normalized_retry
-      value = options[:retry]
+      value = options&.dig(:retry)
       value.is_a?(Integer) ? { max: value } : value.to_h
     end
 

From 1c807d01d461e391d1d0ef56d198784c1884eacc Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Mon, 5 Nov 2018 19:36:51 +0100
Subject: [PATCH 28/29] amend method description a little bit

---
 app/models/ci/build.rb | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 9d6bfbb0db7..045228b4226 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -893,12 +893,11 @@ module Ci
       options&.dig(:environment, :url) || persisted_environment&.external_url
     end
 
-    # The format of the retry option changed in GitLab 11.5. Before it was an
-    # integer only, after it is a hash. New builds always created have the
-    # correct format, but builds created before GitLab 11.5 and saved in
-    # database still have the old integer only format. This helper method makes
-    # sure that the format is always correct when accessing the retry options,
-    # even on old builds.
+    # The format of the retry option changed in GitLab 11.5: Before it was
+    # integer only, after it is a hash. New builds are created with the new
+    # format, but builds created before GitLab 11.5 and saved in database still
+    # have the old integer only format. This method returns the retry option
+    # normalized as a hash in 11.5+ format.
     def normalized_retry
       value = options&.dig(:retry)
       value.is_a?(Integer) ? { max: value } : value.to_h

From 0c59fdaab69e93e2f00fdf70f2a31bb095e3d8d0 Mon Sep 17 00:00:00 2001
From: Markus Doits <markus.doits@stellenticket.de>
Date: Wed, 7 Nov 2018 14:21:44 +0100
Subject: [PATCH 29/29] fix merge conflict

---
 spec/lib/gitlab/ci/config/entry/job_spec.rb | 356 --------------------
 1 file changed, 356 deletions(-)

diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index 79e50d7152d..ac9b0c674a5 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb
@@ -98,183 +98,6 @@ 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 } }
-
-          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
-
       context 'when parallel value is not correct' do
         context 'when it is not a numeric value' do
           let(:config) { { parallel: true } }
@@ -305,185 +128,6 @@ 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' } }