refactoring after latest feedback

4 files changed, 137 insertions, 106 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) }
 
-  describe 'validation' do
-    context 'when retry value is correct' do
-      context 'when it is a numeric' do
-        let(:config) { 2 }
+  shared_context 'when retry value is a numeric', :numeric do
+    let(:config) { max }
+    let(:max) {}
+  end
 
-        it 'is valid' do
-          expect(entry).to be_valid
-        end
+  shared_context 'when retry value is a hash', :hash do
+    let(:config) { { max: max, when: public_send(:when) }.compact }
+    let(:when) {}
+    let(:max) {}
+  end
+
+  describe '#value' do
+    subject(:value) { entry.value }
+
+    context 'when retry value is a numeric', :numeric do
+      let(:max) { 2 }
+
+      it 'is returned as a hash with max key' do
+        expect(value).to eq(max: 2)
       end
+    end
 
-      context 'when it is a hash without when' do
-        let(:config) { { max: 2 } }
+    context 'when retry value is a hash', :hash do
+      context 'and `when` is a string' do
+        let(:when) { 'unknown_failure' }
 
-        it 'is valid' do
-          expect(entry).to be_valid
+        it 'returns when wrapped in an array' do
+          expect(value).to eq(when: ['unknown_failure'])
         end
       end
 
-      context 'when it is a hash with string when' do
-        let(:config) { { max: 2, when: 'unknown_failure' } }
+      context 'and `when` is an array' do
+        let(:when) { %w[unknown_failure runner_system_failure] }
 
-        it 'is valid' do
-          expect(entry).to be_valid
+        it 'returns when as it was passed' do
+          expect(value).to eq(when: %w[unknown_failure runner_system_failure])
         end
       end
+    end
+  end
 
-      context 'when it is a hash with string when always' do
-        let(:config) { { max: 2, when: 'always' } }
+  describe 'validation' do
+    context 'when retry value is correct' do
+      context 'when it is a numeric', :numeric do
+        let(:max) { 2 }
 
         it 'is valid' do
           expect(entry).to be_valid
         end
       end
 
-      context 'when it is a hash with array when' do
-        let(:config) { { max: 2, when: %w[unknown_failure runner_system_failure] } }
+      context 'when it is a hash', :hash do
+        context 'with max' do
+          let(:max) { 2 }
 
-        it 'is valid' do
-          expect(entry).to be_valid
+          it 'is valid' do
+            expect(entry).to be_valid
+          end
+        end
+
+        context 'with string when' do
+          let(:when) { 'unknown_failure' }
+
+          it 'is valid' do
+            expect(entry).to be_valid
+          end
+        end
+
+        context 'with string when always' do
+          let(:when) { 'always' }
+
+          it 'is valid' do
+            expect(entry).to be_valid
+          end
         end
-      end
 
-      # Those values are documented at `doc/ci/yaml/README.md`. If any of
-      # those values gets invalid, documentation must be updated. To make
-      # sure this is catched, check explicitly that all of the documented
-      # values are valid. If they are not it means the documentation and this
-      # array must be updated.
-      RETRY_WHEN_IN_DOCUMENTATION = %w[
-          always
-          unknown_failure
-          script_failure
-          api_failure
-          stuck_or_timeout_failure
-          runner_system_failure
-          missing_dependency_failure
-          runner_unsupported
-      ].freeze
-
-      RETRY_WHEN_IN_DOCUMENTATION.each do |reason|
-        context "when it is a hash with value from documentation `#{reason}`" do
-          let(:config) { { max: 2, when: reason } }
+        context 'with array when' do
+          let(:when) { %w[unknown_failure runner_system_failure] }
 
           it 'is valid' do
             expect(entry).to be_valid
           end
         end
+
+        # Those values are documented at `doc/ci/yaml/README.md`. If any of
+        # those values gets invalid, documentation must be updated. To make
+        # sure this is catched, check explicitly that all of the documented
+        # values are valid. If they are not it means the documentation and this
+        # array must be updated.
+        RETRY_WHEN_IN_DOCUMENTATION = %w[
+            always
+            unknown_failure
+            script_failure
+            api_failure
+            stuck_or_timeout_failure
+            runner_system_failure
+            missing_dependency_failure
+            runner_unsupported
+        ].freeze
+
+        RETRY_WHEN_IN_DOCUMENTATION.each do |reason|
+          context "with when from documentation `#{reason}`" do
+            let(:when) { reason }
+
+            it 'is valid' do
+              expect(entry).to be_valid
+            end
+          end
+        end
+
+        CommitStatus.failure_reasons.each_key do |reason|
+          context "with when from CommitStatus.failure_reasons `#{reason}`" do
+            let(:when) { reason }
+
+            it 'is valid' do
+              expect(entry).to be_valid
+            end
+          end
+        end
       end
     end
 
@@ -82,9 +135,9 @@ describe Gitlab::Ci::Config::Entry::Retry do
         end
       end
 
-      context 'not defined as a hash' do
+      context 'when it is a numeric', :numeric do
         context 'when it is lower than zero' do
-          let(:config) { -1 }
+          let(:max) { -1 }
 
           it 'returns error about value too low' do
             expect(entry).not_to be_valid
@@ -94,7 +147,7 @@ describe Gitlab::Ci::Config::Entry::Retry do
         end
 
         context 'when it is not an integer' do
-          let(:config) { 1.5 }
+          let(:max) { 1.5 }
 
           it 'returns error about wrong value' do
             expect(entry).not_to be_valid
@@ -103,7 +156,7 @@ describe Gitlab::Ci::Config::Entry::Retry do
         end
 
         context 'when the value is too high' do
-          let(:config) { 10 }
+          let(:max) { 10 }
 
           it 'returns error about value too high' do
             expect(entry).not_to be_valid
@@ -112,7 +165,7 @@ describe Gitlab::Ci::Config::Entry::Retry do
         end
       end
 
-      context 'defined as a hash' do
+      context 'when it is a hash', :hash do
         context 'with unknown keys' do
           let(:config) { { max: 2, unknown_key: :something, one_more: :key } }
 
@@ -123,8 +176,8 @@ describe Gitlab::Ci::Config::Entry::Retry do
           end
         end
 
-        context 'when max is lower than zero' do
-          let(:config) { { max: -1 } }
+        context 'with max lower than zero' do
+          let(:max) { -1 }
 
           it 'returns error about value too low' do
             expect(entry).not_to be_valid
@@ -133,8 +186,8 @@ describe Gitlab::Ci::Config::Entry::Retry do
           end
         end
 
-        context 'when max is not an integer' do
-          let(:config) { { max: 1.5 } }
+        context 'with max not an integer' do
+          let(:max) { 1.5 }
 
           it 'returns error about wrong value' do
             expect(entry).not_to be_valid
@@ -142,8 +195,8 @@ describe Gitlab::Ci::Config::Entry::Retry do
           end
         end
 
-        context 'when max is too high' do
-          let(:config) { { max: 10 } }
+        context 'iwth max too high' do
+          let(:max) { 10 }
 
           it 'returns error about value too high' do
             expect(entry).not_to be_valid
@@ -151,8 +204,8 @@ describe Gitlab::Ci::Config::Entry::Retry do
           end
         end
 
-        context 'when when has the wrong format' do
-          let(:config) { { when: true } }
+        context 'with when in wrong format' do
+          let(:when) { true }
 
           it 'returns error about the wrong format' do
             expect(entry).not_to be_valid
@@ -160,8 +213,8 @@ describe Gitlab::Ci::Config::Entry::Retry do
           end
         end
 
-        context 'when when is a string and unknown' do
-          let(:config) { { when: 'unknown_reason' } }
+        context 'with an unknown when string' do
+          let(:when) { 'unknown_reason' }
 
           it 'returns error about the wrong format' do
             expect(entry).not_to be_valid
@@ -169,8 +222,8 @@ describe Gitlab::Ci::Config::Entry::Retry do
           end
         end
 
-        context 'when when is an array and includes unknown failures' do
-          let(:config) { { when: %w[unknown_reason runner_system_failure] } }
+        context 'with an unknown failure reason in a when array' do
+          let(:when) { %w[unknown_reason runner_system_failure] }
 
           it 'returns error about the wrong format' do
             expect(entry).not_to be_valid
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 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' } }) }
-
-        it 'returns the configured value inside an array' do
-          expect(subject.retry_when).to eq ['something']
-        end
-      end
-
-      context 'when value is defined as an array' do
-        subject { create(:ci_build, options: { retry: { when: %w[something more] } }) }
+      context 'with retries when config option' do
+        subject { create(:ci_build, options: { retry: { when: ['some_reason'] } }) }
 
-        it 'returns the configured value' do
-          expect(subject.retry_when).to eq %w[something more]
+        it 'returns the configured when' do
+          expect(subject.retry_when).to eq ['some_reason']
         end
       end
 
-      context 'when value is not defined' do
+      context 'without retries when config option' do
         subject { create(:ci_build) }
 
-        it 'returns `always`' do
-          expect(subject.retry_when).to eq ['always']
-        end
-      end
-
-      context 'when retry is only defined as an integer' do
-        subject { create(:ci_build, options: { retry: 1 }) }
-
-        it 'returns `always`' do
+        it 'returns always array' do
           expect(subject.retry_when).to eq ['always']
         end
       end
@@ -3001,7 +2969,7 @@ describe Ci::Build do
     end
 
     context 'when build is configured to be retried' do
-      subject { create(:ci_build, :running, options: { retry: 3 }, project: project, user: user) }
+      subject { create(:ci_build, :running, options: { retry: { max: 3 } }, project: project, user: user) }
 
       it 'retries build and assigns the same user to it' do
         expect(described_class).to receive(:retry)