Merge branch '30235-support-allow-failure-for-ci-rules' into 'master'

Implement support of allow_failure keyword for CI rules

See merge request gitlab-org/gitlab!24605

changelogs/unreleased/30235-support-allow-failure-for-ci-rules.yml

+---
+title: Implement support of allow_failure keyword for CI rules
+merge_request: 24605
+author:
+type: added

doc/ci/yaml/README.md

@@ -851,7 +851,7 @@ In this example, if the first rule:
 - Matches, the job will be given the `when:always` attribute.
 - Does not match, the second and third rules will be evaluated sequentially
   until a match is found. That is, the job will be given either the:
-  - `when: manual` attribute if the second rule matches.
+  - `when: manual` attribute if the second rule matches. **The stage will not complete until this manual job is triggered and completes successfully.**
   - `when: on_success` attribute if the second rule does not match. The third
     rule will always match when reached because it has no conditional clauses.
 
@@ -937,6 +937,25 @@ NOTE: **Note:**
 For performance reasons, using `exists` with patterns is limited to 10000
 checks. After the 10000th check, rules with patterned globs will always match.
 
+#### `rules:allow_failure`
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/30235) in GitLab 12.8.
+
+You can use [`allow_failure: true`](#allow_failure) within `rules:` to allow a job to fail, or a manual job to
+wait for action, without stopping the pipeline itself. All jobs using `rules:` default to `allow_failure: false`
+if `allow_failure:` is not defined.
+
+```yaml
+job:
+  script: "echo Hello, Rules!"
+  rules:
+    - if: '$CI_MERGE_REQUEST_TARGET_BRANCH_NAME == "master"'
+      when: manual
+      allow_failure: true
+```
+
+In this example, if the first rule matches, then the job will have `when: manual` and `allow_failure: true`.
+
 #### Complex rule clauses
 
 To conjoin `if`, `changes`, and `exists` clauses with an AND, use them in the
@@ -976,6 +995,7 @@ The only job attributes currently set by `rules` are:
 
 - `when`.
 - `start_in`, if `when` is set to `delayed`.
+- `allow_failure`.
 
 A job will be included in a pipeline if `when` is evaluated to any value
 except `never`.

lib/gitlab/ci/build/rules.rb

@@ -6,11 +6,12 @@ module Gitlab
       class Rules
         include ::Gitlab::Utils::StrongMemoize
 
-        Result = Struct.new(:when, :start_in) do
+        Result = Struct.new(:when, :start_in, :allow_failure) do
           def build_attributes
             {
               when: self.when,
-              options: { start_in: start_in }.compact
+              options: { start_in: start_in }.compact,
+              allow_failure: allow_failure
             }.compact
           end
 
@@ -30,7 +31,8 @@ module Gitlab
           elsif matched_rule = match_rule(pipeline, context)
             Result.new(
               matched_rule.attributes[:when] || @default_when,
-              matched_rule.attributes[:start_in]
+              matched_rule.attributes[:start_in],
+              matched_rule.attributes[:allow_failure]
             )
           else
             Result.new('never')

lib/gitlab/ci/config/entry/rules/rule.rb

@@ -9,10 +9,10 @@ module Gitlab
           include ::Gitlab::Config::Entry::Attributable
 
           CLAUSES        = %i[if changes exists].freeze
-          ALLOWED_KEYS   = %i[if changes exists when start_in].freeze
+          ALLOWED_KEYS   = %i[if changes exists when start_in allow_failure].freeze
           ALLOWABLE_WHEN = %w[on_success on_failure always never manual delayed].freeze
 
-          attributes :if, :changes, :exists, :when, :start_in
+          attributes :if, :changes, :exists, :when, :start_in, :allow_failure
 
           validations do
             validates :config, presence: true
@@ -26,6 +26,7 @@ module Gitlab
               validates :if, expression: true
               validates :changes, :exists, array_of_strings: true, length: { maximum: 50 }
               validates :when, allowed_values: { in: ALLOWABLE_WHEN }
+              validates :allow_failure, boolean: true
             end
 
             validate do

spec/lib/gitlab/ci/build/rules_spec.rb

@@ -102,9 +102,9 @@ describe Gitlab::Ci::Build::Rules do
     end
 
     context 'with one rule without any clauses' do
-      let(:rule_list) { [{ when: 'manual' }] }
+      let(:rule_list) { [{ when: 'manual', allow_failure: true }] }
 
-      it { is_expected.to eq(described_class::Result.new('manual')) }
+      it { is_expected.to eq(described_class::Result.new('manual', nil, true)) }
     end
 
     context 'with one matching rule' do
@@ -166,5 +166,51 @@ describe Gitlab::Ci::Build::Rules do
         end
       end
     end
+
+    context 'with only allow_failure' do
+      context 'with matching rule' do
+        let(:rule_list) { [{ if: '$VAR == null', allow_failure: true }] }
+
+        it { is_expected.to eq(described_class::Result.new('on_success', nil, true)) }
+      end
+
+      context 'with non-matching rule' do
+        let(:rule_list) { [{ if: '$VAR != null', allow_failure: true }] }
+
+        it { is_expected.to eq(described_class::Result.new('never')) }
+      end
+    end
+  end
+
+  describe 'Gitlab::Ci::Build::Rules::Result' do
+    let(:when_value) { 'on_success' }
+    let(:start_in) { nil }
+    let(:allow_failure) { nil }
+
+    subject { Gitlab::Ci::Build::Rules::Result.new(when_value, start_in, allow_failure) }
+
+    describe '#build_attributes' do
+      it 'compacts nil values' do
+        expect(subject.build_attributes).to eq(options: {}, when: 'on_success')
+      end
+    end
+
+    describe '#pass?' do
+      context "'when' is 'never'" do
+        let!(:when_value) { 'never' }
+
+        it 'returns false' do
+          expect(subject.pass?).to eq(false)
+        end
+      end
+
+      context "'when' is 'on_success'" do
+        let!(:when_value) { 'on_success' }
+
+        it 'returns true' do
+          expect(subject.pass?).to eq(true)
+        end
+      end
+    end
   end
 end

spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb

@@ -27,8 +27,14 @@ describe Gitlab::Ci::Config::Entry::Rules::Rule do
       it { is_expected.to be_valid }
     end
 
+    context 'with an allow_failure: value but no clauses' do
+      let(:config) { { allow_failure: true } }
+
+      it { is_expected.to be_valid }
+    end
+
     context 'when specifying an if: clause' do
-      let(:config) { { if: '$THIS || $THAT', when: 'manual' } }
+      let(:config) { { if: '$THIS || $THAT', when: 'manual', allow_failure: true } }
 
       it { is_expected.to be_valid }
 
@@ -37,6 +43,12 @@ describe Gitlab::Ci::Config::Entry::Rules::Rule do
 
         it { is_expected.to eq('manual') }
       end
+
+      describe '#allow_failure' do
+        subject { entry.allow_failure }
+
+        it { is_expected.to eq(true) }
+      end
     end
 
     context 'using a list of multiple expressions' do
@@ -328,16 +340,43 @@ describe Gitlab::Ci::Config::Entry::Rules::Rule do
         end
       end
     end
+
+    context 'allow_failure: validation' do
+      context 'with an invalid string allow_failure:' do
+        let(:config) do
+          { if: '$THIS == "that"', allow_failure: 'always' }
+        end
+
+        it { is_expected.to be_a(described_class) }
+        it { is_expected.not_to be_valid }
+
+        it 'returns an error about invalid allow_failure:' do
+          expect(subject.errors).to include(/rule allow failure should be a boolean value/)
+        end
+
+        context 'when composed' do
+          before do
+            subject.compose!
+          end
+
+          it { is_expected.not_to be_valid }
+
+          it 'returns an error about invalid allow_failure:' do
+            expect(subject.errors).to include(/rule allow failure should be a boolean value/)
+          end
+        end
+      end
+    end
   end
 
   describe '#value' do
     subject { entry.value }
 
     context 'when specifying an if: clause' do
-      let(:config) { { if: '$THIS || $THAT', when: 'manual' } }
+      let(:config) { { if: '$THIS || $THAT', when: 'manual', allow_failure: true } }
 
       it 'stores the expression as "if"' do
-        expect(subject).to eq(if: '$THIS || $THAT', when: 'manual')
+        expect(subject).to eq(if: '$THIS || $THAT', when: 'manual', allow_failure: true)
       end
     end
 

spec/services/ci/create_pipeline_service_spec.rb

@@ -1750,9 +1750,9 @@ describe Ci::CreatePipelineService do
       let(:ref_name)    { 'refs/heads/master' }
       let(:pipeline)    { execute_service }
       let(:build_names) { pipeline.builds.pluck(:name) }
-      let(:regular_job) { pipeline.builds.find_by(name: 'regular-job') }
-      let(:rules_job)   { pipeline.builds.find_by(name: 'rules-job') }
-      let(:delayed_job) { pipeline.builds.find_by(name: 'delayed-job') }
+      let(:regular_job) { find_job('regular-job') }
+      let(:rules_job)   { find_job('rules-job') }
+      let(:delayed_job) { find_job('delayed-job') }
 
       shared_examples 'rules jobs are excluded' do
         it 'only persists the job without rules' do
@@ -1763,6 +1763,10 @@ describe Ci::CreatePipelineService do
         end
       end
 
+      def find_job(name)
+        pipeline.builds.find_by(name: name)
+      end
+
       before do
         stub_ci_pipeline_yaml_file(config)
         allow_any_instance_of(Ci::BuildScheduleWorker).to receive(:perform).and_return(true)
@@ -1782,6 +1786,12 @@ describe Ci::CreatePipelineService do
                 - if: $CI_COMMIT_REF_NAME =~ /master/
                   when: manual
 
+            negligible-job:
+              script: "exit 1"
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /master/
+                  allow_failure: true
+
             delayed-job:
               script: "echo See you later, World!"
               rules:
@@ -1800,11 +1810,23 @@ describe Ci::CreatePipelineService do
         context 'with matches' do
           it 'creates a pipeline with the vanilla and manual jobs' do
             expect(pipeline).to be_persisted
-            expect(build_names).to contain_exactly('regular-job', 'delayed-job', 'master-job')
+            expect(build_names).to contain_exactly(
+              'regular-job', 'delayed-job', 'master-job', 'negligible-job'
+            )
           end
 
           it 'assigns job:when values to the builds' do
-            expect(pipeline.builds.pluck(:when)).to contain_exactly('on_success', 'delayed', 'manual')
+            expect(find_job('regular-job').when).to eq('on_success')
+            expect(find_job('master-job').when).to eq('manual')
+            expect(find_job('negligible-job').when).to eq('on_success')
+            expect(find_job('delayed-job').when).to eq('delayed')
+          end
+
+          it 'assigns job:allow_failure values to the builds' do
+            expect(find_job('regular-job').allow_failure).to eq(false)
+            expect(find_job('master-job').allow_failure).to eq(false)
+            expect(find_job('negligible-job').allow_failure).to eq(true)
+            expect(find_job('delayed-job').allow_failure).to eq(false)
           end
 
           it 'assigns start_in for delayed jobs' do
@@ -1827,6 +1849,7 @@ describe Ci::CreatePipelineService do
               rules:
                 - if: $VAR == 'present' && $OTHER || $CI_COMMIT_REF_NAME
                   when: manual
+                  allow_failure: true
           EOY
         end
 
@@ -1834,6 +1857,7 @@ describe Ci::CreatePipelineService do
           expect(pipeline).to be_persisted
           expect(build_names).to contain_exactly('regular-job')
           expect(regular_job.when).to eq('manual')
+          expect(regular_job.allow_failure).to eq(true)
         end
       end
 
@@ -1860,6 +1884,13 @@ describe Ci::CreatePipelineService do
                   - README.md
                   when: delayed
                   start_in: 4 hours
+
+            negligible-job:
+              script: "can be failed sometimes"
+              rules:
+                - changes:
+                  - README.md
+                  allow_failure: true
           EOY
         end
 
@@ -1872,7 +1903,7 @@ describe Ci::CreatePipelineService do
           it 'creates two jobs' do
             expect(pipeline).to be_persisted
             expect(build_names)
-              .to contain_exactly('regular-job', 'rules-job', 'delayed-job')
+              .to contain_exactly('regular-job', 'rules-job', 'delayed-job', 'negligible-job')
           end
 
           it 'sets when: for all jobs' do
@@ -1881,6 +1912,10 @@ describe Ci::CreatePipelineService do
             expect(delayed_job.when).to eq('delayed')
             expect(delayed_job.options[:start_in]).to eq('4 hours')
           end
+
+          it 'sets allow_failure: for negligible job' do
+            expect(find_job('negligible-job').allow_failure).to eq(true)
+          end
         end
 
         context 'and matches the second rule' do
@@ -1922,12 +1957,14 @@ describe Ci::CreatePipelineService do
 
             rules-job:
               script: "echo hello world, $CI_COMMIT_REF_NAME"
+              allow_failure: true
               rules:
                 - changes:
                   - README.md
                   when: manual
                 - if: $CI_COMMIT_REF_NAME == "master"
                   when: on_success
+                  allow_failure: false
 
             delayed-job:
               script: "echo See you later, World!"
@@ -1936,6 +1973,7 @@ describe Ci::CreatePipelineService do
                   - README.md
                   when: delayed
                   start_in: 4 hours
+                  allow_failure: true
                 - if: $CI_COMMIT_REF_NAME == "master"
                   when: delayed
                   start_in: 1 hour
@@ -1960,6 +1998,12 @@ describe Ci::CreatePipelineService do
             expect(delayed_job.when).to eq('delayed')
             expect(delayed_job.options[:start_in]).to eq('4 hours')
           end
+
+          it 'sets allow_failure: for all jobs' do
+            expect(regular_job.allow_failure).to eq(false)
+            expect(rules_job.allow_failure).to eq(true)
+            expect(delayed_job.allow_failure).to eq(true)
+          end
         end
 
         context 'and if: matches after changes' do
@@ -1999,6 +2043,7 @@ describe Ci::CreatePipelineService do
                 - if: $CI_COMMIT_REF_NAME =~ /master/
                   changes: [README.md]
                   when: on_success
+                  allow_failure: true
                 - if: $CI_COMMIT_REF_NAME =~ /master/
                   changes: [app.rb]
                   when: manual
@@ -2016,6 +2061,7 @@ describe Ci::CreatePipelineService do
             expect(regular_job).to be_persisted
             expect(rules_job).to be_persisted
             expect(rules_job.when).to eq('manual')
+            expect(rules_job.allow_failure).to eq(false)
           end
         end
 
@@ -2040,6 +2086,150 @@ describe Ci::CreatePipelineService do
           it_behaves_like 'rules jobs are excluded'
         end
       end
+
+      context 'with complex if: allow_failure usages' do
+        let(:config) do
+          <<-EOY
+            job-1:
+              script: "exit 1"
+              allow_failure: true
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /master/
+                  allow_failure: false
+
+            job-2:
+              script: "exit 1"
+              allow_failure: true
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /nonexistant-branch/
+                  allow_failure: false
+
+            job-3:
+              script: "exit 1"
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /nonexistant-branch/
+                  allow_failure: true
+
+            job-4:
+              script: "exit 1"
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /master/
+                  allow_failure: false
+
+            job-5:
+              script: "exit 1"
+              allow_failure: false
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /master/
+                  allow_failure: true
+
+            job-6:
+              script: "exit 1"
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /nonexistant-branch/
+                  allow_failure: false
+                - allow_failure: true
+          EOY
+        end
+
+        it 'creates a pipeline' do
+          expect(pipeline).to be_persisted
+          expect(build_names).to contain_exactly('job-1', 'job-4', 'job-5', 'job-6')
+        end
+
+        it 'assigns job:allow_failure values to the builds' do
+          expect(find_job('job-1').allow_failure).to eq(false)
+          expect(find_job('job-4').allow_failure).to eq(false)
+          expect(find_job('job-5').allow_failure).to eq(true)
+          expect(find_job('job-6').allow_failure).to eq(true)
+        end
+      end
+
+      context 'with complex if: allow_failure & when usages' do
+        let(:config) do
+          <<-EOY
+            job-1:
+              script: "exit 1"
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /master/
+                  when: manual
+
+            job-2:
+              script: "exit 1"
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /master/
+                  when: manual
+                  allow_failure: true
+
+            job-3:
+              script: "exit 1"
+              allow_failure: true
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /master/
+                  when: manual
+
+            job-4:
+              script: "exit 1"
+              allow_failure: true
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /master/
+                  when: manual
+                  allow_failure: false
+
+            job-5:
+              script: "exit 1"
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /nonexistant-branch/
+                  when: manual
+                  allow_failure: false
+                - when: always
+                  allow_failure: true
+
+            job-6:
+              script: "exit 1"
+              allow_failure: false
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /master/
+                  when: manual
+
+            job-7:
+              script: "exit 1"
+              allow_failure: false
+              rules:
+                - if: $CI_COMMIT_REF_NAME =~ /nonexistant-branch/
+                  when: manual
+                - when: :on_failure
+                  allow_failure: true
+          EOY
+        end
+
+        it 'creates a pipeline' do
+          expect(pipeline).to be_persisted
+          expect(build_names).to contain_exactly(
+            'job-1', 'job-2', 'job-3', 'job-4', 'job-5', 'job-6', 'job-7'
+          )
+        end
+
+        it 'assigns job:allow_failure values to the builds' do
+          expect(find_job('job-1').allow_failure).to eq(false)
+          expect(find_job('job-2').allow_failure).to eq(true)
+          expect(find_job('job-3').allow_failure).to eq(true)
+          expect(find_job('job-4').allow_failure).to eq(false)
+          expect(find_job('job-5').allow_failure).to eq(true)
+          expect(find_job('job-6').allow_failure).to eq(false)
+          expect(find_job('job-7').allow_failure).to eq(true)
+        end
+
+        it 'assigns job:when values to the builds' do
+          expect(find_job('job-1').when).to eq('manual')
+          expect(find_job('job-2').when).to eq('manual')
+          expect(find_job('job-3').when).to eq('manual')
+          expect(find_job('job-4').when).to eq('manual')
+          expect(find_job('job-5').when).to eq('always')
+          expect(find_job('job-6').when).to eq('manual')
+          expect(find_job('job-7').when).to eq('on_failure')
+        end
+      end
     end
   end