Merge branch 'ci-add-exit-code-syntax' into 'master'

Implement allow_failure syntax with exit codes

See merge request gitlab-org/gitlab!49145

config/feature_flags/development/ci_allow_failure_with_exit_codes.yml

+---
+name: ci_allow_failure_with_exit_codes
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49145
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/292024
+milestone: '13.7'
+type: development
+group: group::pipeline authoring
+default_enabled: false

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Config
+      module Entry
+        ##
+        # Entry that represents allow_failure settings.
+        #
+        class AllowFailure < ::Gitlab::Config::Entry::Node
+          include ::Gitlab::Config::Entry::Attributable
+          include ::Gitlab::Config::Entry::Validatable
+
+          ALLOWED_KEYS = %i[exit_codes].freeze
+          attributes ALLOWED_KEYS
+
+          validations do
+            validates :config, hash_or_boolean: true
+            validates :config, allowed_keys: ALLOWED_KEYS
+            validates :exit_codes, array_of_integers_or_integer: true, allow_nil: true
+          end
+
+          def value
+            @config[:exit_codes] = Array.wrap(exit_codes) if exit_codes.present?
+            @config
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -22,6 +22,7 @@ module Gitlab
                 in: ALLOWED_WHEN,
                 message: "should be one of: #{ALLOWED_WHEN.join(', ')}"
               }
+              validates :allow_failure, boolean: true
             end
 
             validate on: :composed do
@@ -47,7 +48,7 @@ module Gitlab
             inherit: false,
             metadata: { allowed_needs: %i[job bridge] }
 
-          attributes :when
+          attributes :when, :allow_failure
 
           def self.matching?(name, config)
             !name.to_s.start_with?('.') &&
@@ -72,6 +73,10 @@ module Gitlab
           def bridge_needs
             needs_value[:bridge] if needs_value
           end
+
+          def ignored?
+            allow_failure.nil? ? manual_action? : allow_failure
+          end
         end
       end
     end

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

@@ -31,6 +31,7 @@ module Gitlab
 
               validates :dependencies, array_of_strings: true
               validates :resource_group, type: String
+              validates :allow_failure, hash_or_boolean: true
             end
 
             validates :start_in, duration: { limit: '1 week' }, if: :delayed?
@@ -117,9 +118,14 @@ module Gitlab
             description: 'Parallel configuration for this job.',
             inherit: false
 
+          entry :allow_failure, ::Gitlab::Ci::Config::Entry::AllowFailure,
+            description: 'Indicates whether this job is allowed to fail or not.',
+            inherit: false
+
           attributes :script, :tags, :when, :dependencies,
                      :needs, :retry, :parallel, :start_in,
-                     :interruptible, :timeout, :resource_group, :release
+                     :interruptible, :timeout, :resource_group,
+                     :release, :allow_failure
 
           def self.matching?(name, config)
             !name.to_s.start_with?('.') &&
@@ -166,11 +172,32 @@ module Gitlab
               release: release_value,
               after_script: after_script_value,
               ignore: ignored?,
+              allow_failure_criteria: allow_failure_criteria,
               needs: needs_defined? ? needs_value : nil,
               resource_group: resource_group,
               scheduling_type: needs_defined? ? :dag : :stage
             ).compact
           end
+
+          def ignored?
+            allow_failure_defined? ? static_allow_failure : manual_action?
+          end
+
+          private
+
+          def allow_failure_criteria
+            return unless ::Gitlab::Ci::Features.allow_failure_with_exit_codes_enabled?
+
+            if allow_failure_defined? && allow_failure_value.is_a?(Hash)
+              allow_failure_value
+            end
+          end
+
+          def static_allow_failure
+            return false if allow_failure_value.is_a?(Hash)
+
+            allow_failure_value
+          end
         end
       end
     end

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

@@ -32,7 +32,6 @@ module Gitlab
               with_options allow_nil: true do
                 validates :extends, array_of_strings_or_string: true
                 validates :rules, array_of_hashes: true
-                validates :allow_failure, boolean: true
               end
             end
 
@@ -65,7 +64,7 @@ module Gitlab
               inherit: false,
               default: {}
 
-            attributes :extends, :rules, :allow_failure
+            attributes :extends, :rules
           end
 
           def compose!(deps = nil)
@@ -141,10 +140,6 @@ module Gitlab
           def manual_action?
             self.when == 'manual'
           end
-
-          def ignored?
-            allow_failure.nil? ? manual_action? : allow_failure
-          end
         end
       end
     end

lib/gitlab/ci/features.rb

@@ -62,6 +62,10 @@ module Gitlab
       def self.ci_pipeline_editor_page_enabled?(project)
         ::Feature.enabled?(:ci_pipeline_editor_page, project, default_enabled: false)
       end
+
+      def self.allow_failure_with_exit_codes_enabled?
+        ::Feature.enabled?(:ci_allow_failure_with_exit_codes)
+      end
     end
   end
 end

lib/gitlab/ci/pipeline/seed/build.rb

@@ -60,6 +60,7 @@ module Gitlab
             @seed_attributes
               .deep_merge(pipeline_attributes)
               .deep_merge(rules_attributes)
+              .deep_merge(allow_failure_criteria_attributes)
               .deep_merge(cache_attributes)
           end
 
@@ -154,9 +155,13 @@ module Gitlab
           end
 
           def rules_attributes
-            return {} unless @using_rules
-
-            rules_result.build_attributes
+            strong_memoize(:rules_attributes) do
+              if @using_rules
+                rules_result.build_attributes
+              else
+                {}
+              end
+            end
           end
 
           def rules_result
@@ -176,6 +181,17 @@ module Gitlab
               @cache.build_attributes
             end
           end
+
+          # If a job uses `allow_failure:exit_codes` and `rules:allow_failure`
+          # we need to prevent the exit codes from being persisted because they
+          # would break the behavior defined by `rules:allow_failure`.
+          def allow_failure_criteria_attributes
+            return {} unless ::Gitlab::Ci::Features.allow_failure_with_exit_codes_enabled?
+            return {} if rules_attributes[:allow_failure].nil?
+            return {} unless @seed_attributes.dig(:options, :allow_failure_criteria)
+
+            { options: { allow_failure_criteria: nil } }
+          end
         end
       end
     end

lib/gitlab/ci/yaml_processor/result.rb

@@ -77,6 +77,7 @@ module Gitlab
             options: {
               image: job[:image],
               services: job[:services],
+              allow_failure_criteria: job[:allow_failure_criteria],
               artifacts: job[:artifacts],
               dependencies: job[:dependencies],
               cross_dependencies: job.dig(:needs, :cross_dependency),

lib/gitlab/config/entry/validators.rb

@@ -134,6 +134,16 @@ module Gitlab
           end
         end
 
+        class HashOrBooleanValidator < ActiveModel::EachValidator
+          include LegacyValidationHelpers
+
+          def validate_each(record, attribute, value)
+            unless value.is_a?(Hash) || validate_boolean(value)
+              record.errors.add(attribute, 'should be a hash or a boolean value')
+            end
+          end
+        end
+
         class KeyValidator < ActiveModel::EachValidator
           include LegacyValidationHelpers
 
@@ -158,6 +168,22 @@ module Gitlab
           end
         end
 
+        class ArrayOfIntegersOrIntegerValidator < ActiveModel::EachValidator
+          include LegacyValidationHelpers
+
+          def validate_each(record, attribute, value)
+            unless validate_integer(value) || validate_array_of_integers(value)
+              record.errors.add(attribute, 'should be an array of integers or an integer')
+            end
+          end
+
+          private
+
+          def validate_array_of_integers(values)
+            values.is_a?(Array) && values.all? { |value| validate_integer(value) }
+          end
+        end
+
         class RegexpValidator < ActiveModel::EachValidator
           include LegacyValidationHelpers
 

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

@@ -187,11 +187,17 @@ RSpec.describe Gitlab::Ci::Build::Rules do
     let(:start_in) { nil }
     let(:allow_failure) { nil }
 
-    subject { Gitlab::Ci::Build::Rules::Result.new(when_value, start_in, allow_failure) }
+    subject(:result) do
+      Gitlab::Ci::Build::Rules::Result.new(when_value, start_in, allow_failure)
+    end
 
     describe '#build_attributes' do
+      subject(:build_attributes) do
+        result.build_attributes
+      end
+
       it 'compacts nil values' do
-        expect(subject.build_attributes).to eq(options: {}, when: 'on_success')
+        is_expected.to eq(options: {}, when: 'on_success')
       end
     end
 

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Config::Entry::AllowFailure do
+  let(:entry) { described_class.new(config.deep_dup) }
+  let(:expected_config) { config }
+
+  describe 'validations' do
+    context 'when entry config value is valid' do
+      shared_examples 'valid entry' do
+        describe '#value' do
+          it 'returns key value' do
+            expect(entry.value).to eq(expected_config)
+          end
+        end
+
+        describe '#valid?' do
+          it 'is valid' do
+            expect(entry).to be_valid
+          end
+        end
+      end
+
+      context 'with boolean values' do
+        it_behaves_like 'valid entry' do
+          let(:config) { true }
+        end
+
+        it_behaves_like 'valid entry' do
+          let(:config) { false }
+        end
+      end
+
+      context 'with hash values' do
+        it_behaves_like 'valid entry' do
+          let(:config) { { exit_codes: 137 } }
+          let(:expected_config) { { exit_codes: [137] } }
+        end
+
+        it_behaves_like 'valid entry' do
+          let(:config) { { exit_codes: [42, 137] } }
+        end
+      end
+    end
+
+    context 'when entry value is not valid' do
+      shared_examples 'invalid entry' do
+        describe '#valid?' do
+          it { expect(entry).not_to be_valid }
+          it { expect(entry.errors).to include(error_message) }
+        end
+      end
+
+      context 'when it has a wrong type' do
+        let(:config) { [1] }
+        let(:error_message) do
+          'allow failure config should be a hash or a boolean value'
+        end
+
+        it_behaves_like 'invalid entry'
+      end
+
+      context 'with string exit codes' do
+        let(:config) { { exit_codes: 'string' } }
+        let(:error_message) do
+          'allow failure exit codes should be an array of integers or an integer'
+        end
+
+        it_behaves_like 'invalid entry'
+      end
+
+      context 'with array of strings as exit codes' do
+        let(:config) { { exit_codes: ['string 1', 'string 2'] } }
+        let(:error_message) do
+          'allow failure exit codes should be an array of integers or an integer'
+        end
+
+        it_behaves_like 'invalid entry'
+      end
+
+      context 'when it has an extra keys' do
+        let(:config) { { extra: true } }
+        let(:error_message) do
+          'allow failure config contains unknown keys: extra'
+        end
+
+        it_behaves_like 'invalid entry'
+      end
+    end
+  end
+end

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

@@ -227,6 +227,23 @@ RSpec.describe Gitlab::Ci::Config::Entry::Bridge do
         end
       end
     end
+
+    context 'when bridge config contains exit_codes' do
+      let(:config) do
+        { script: 'rspec', allow_failure: { exit_codes: [42] } }
+      end
+
+      describe '#valid?' do
+        it { is_expected.not_to be_valid }
+      end
+
+      describe '#errors' do
+        it 'returns an error message' do
+          expect(subject.errors)
+            .to include(/allow failure should be a boolean value/)
+        end
+      end
+    end
   end
 
   describe '#manual_action?' do

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

@@ -670,6 +670,10 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
   end
 
   describe '#ignored?' do
+    before do
+      entry.compose!
+    end
+
     context 'when job is a manual action' do
       context 'when it is not specified if job is allowed to fail' do
         let(:config) do
@@ -700,6 +704,16 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
           expect(entry).not_to be_ignored
         end
       end
+
+      context 'when job is dynamically allowed to fail' do
+        let(:config) do
+          { script: 'deploy', when: 'manual', allow_failure: { exit_codes: 42 } }
+        end
+
+        it 'is not an ignored job' do
+          expect(entry).not_to be_ignored
+        end
+      end
     end
 
     context 'when job is not a manual action' do
@@ -709,6 +723,10 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
         it 'is not an ignored job' do
           expect(entry).not_to be_ignored
         end
+
+        it 'does not return allow_failure' do
+          expect(entry.value.key?(:allow_failure_criteria)).to be_falsey
+        end
       end
 
       context 'when job is allowed to fail' do
@@ -717,6 +735,10 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
         it 'is an ignored job' do
           expect(entry).to be_ignored
         end
+
+        it 'does not return allow_failure_criteria' do
+          expect(entry.value.key?(:allow_failure_criteria)).to be_falsey
+        end
       end
 
       context 'when job is not allowed to fail' do
@@ -725,6 +747,32 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
         it 'is not an ignored job' do
           expect(entry).not_to be_ignored
         end
+
+        it 'does not return allow_failure_criteria' do
+          expect(entry.value.key?(:allow_failure_criteria)).to be_falsey
+        end
+      end
+
+      context 'when job is dynamically allowed to fail' do
+        let(:config) { { script: 'deploy', allow_failure: { exit_codes: 42 } } }
+
+        it 'is not an ignored job' do
+          expect(entry).not_to be_ignored
+        end
+
+        it 'returns allow_failure_criteria' do
+          expect(entry.value[:allow_failure_criteria]).to match(exit_codes: [42])
+        end
+
+        context 'with ci_allow_failure_with_exit_codes disabled' do
+          before do
+            stub_feature_flags(ci_allow_failure_with_exit_codes: false)
+          end
+
+          it 'does not return allow_failure_criteria' do
+            expect(entry.value.key?(:allow_failure_criteria)).to be_falsey
+          end
+        end
       end
     end
   end

spec/lib/gitlab/ci/pipeline/seed/build_spec.rb

@@ -165,6 +165,45 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
 
       it { is_expected.to include(options: {}) }
     end
+
+    context 'with allow_failure' do
+      let(:options) do
+        { allow_failure_criteria: { exit_codes: [42] } }
+      end
+
+      let(:rules) do
+        [{ if: '$VAR == null', when: 'always' }]
+      end
+
+      let(:attributes) do
+        {
+          name: 'rspec',
+          ref: 'master',
+          options: options,
+          rules: rules
+        }
+      end
+
+      context 'when rules does not override allow_failure' do
+        it { is_expected.to match a_hash_including(options: options) }
+      end
+
+      context 'when rules set allow_failure to true' do
+        let(:rules) do
+          [{ if: '$VAR == null', when: 'always', allow_failure: true }]
+        end
+
+        it { is_expected.to match a_hash_including(options: { allow_failure_criteria: nil }) }
+      end
+
+      context 'when rules set allow_failure to false' do
+        let(:rules) do
+          [{ if: '$VAR == null', when: 'always', allow_failure: false }]
+        end
+
+        it { is_expected.to match a_hash_including(options: { allow_failure_criteria: nil }) }
+      end
+    end
   end
 
   describe '#bridge?' do

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -231,6 +231,23 @@ module Gitlab
                 expect(subject[:allow_failure]).to be true
               end
             end
+
+            context 'when allow_failure has exit_codes' do
+              let(:config) do
+                YAML.dump(rspec: { script: 'rspec',
+                                   when: 'manual',
+                                   allow_failure: { exit_codes: 1 } })
+              end
+
+              it 'is not allowed to fail' do
+                expect(subject[:allow_failure]).to be false
+              end
+
+              it 'saves allow_failure_criteria into options' do
+                expect(subject[:options]).to match(
+                  a_hash_including(allow_failure_criteria: { exit_codes: [1] }))
+              end
+            end
           end
 
           context 'when job is not a manual action' do
@@ -254,6 +271,22 @@ module Gitlab
                 expect(subject[:allow_failure]).to be false
               end
             end
+
+            context 'when allow_failure is dynamically specified' do
+              let(:config) do
+                YAML.dump(rspec: { script: 'rspec',
+                                   allow_failure: { exit_codes: 1 } })
+              end
+
+              it 'is not allowed to fail' do
+                expect(subject[:allow_failure]).to be false
+              end
+
+              it 'saves allow_failure_criteria into options' do
+                expect(subject[:options]).to match(
+                  a_hash_including(allow_failure_criteria: { exit_codes: [1] }))
+              end
+            end
           end
         end
 
@@ -2494,7 +2527,13 @@ module Gitlab
         context 'returns errors if job allow_failure parameter is not an boolean' do
           let(:config) { YAML.dump({ rspec: { script: "test", allow_failure: "string" } }) }
 
-          it_behaves_like 'returns errors', 'jobs:rspec allow failure should be a boolean value'
+          it_behaves_like 'returns errors', 'jobs:rspec allow failure should be a hash or a boolean value'
+        end
+
+        context 'returns errors if job exit_code parameter from allow_failure is not an integer' do
+          let(:config) { YAML.dump({ rspec: { script: "test", allow_failure: { exit_codes: 'string' } } }) }
+
+          it_behaves_like 'returns errors', 'jobs:rspec:allow_failure exit codes should be an array of integers or an integer'
         end
 
         context 'returns errors if job stage is not a string' do

spec/services/ci/create_pipeline_service/rules_spec.rb

@@ -93,6 +93,73 @@ RSpec.describe Ci::CreatePipelineService do
         end
       end
     end
+
+    context 'with allow_failure and exit_codes', :aggregate_failures do
+      def find_job(name)
+        pipeline.builds.find_by(name: name)
+      end
+
+      let(:config) do
+        <<-EOY
+          job-1:
+            script: exit 42
+            allow_failure:
+              exit_codes: 42
+            rules:
+              - if: $CI_COMMIT_REF_NAME == "master"
+                allow_failure: false
+
+          job-2:
+            script: exit 42
+            allow_failure:
+              exit_codes: 42
+            rules:
+              - if: $CI_COMMIT_REF_NAME == "master"
+                allow_failure: true
+
+          job-3:
+            script: exit 42
+            allow_failure:
+              exit_codes: 42
+            rules:
+              - if: $CI_COMMIT_REF_NAME == "master"
+                when: manual
+        EOY
+      end
+
+      it 'creates a pipeline' do
+        expect(pipeline).to be_persisted
+        expect(build_names).to contain_exactly(
+          'job-1', 'job-2', 'job-3'
+        )
+      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(false)
+      end
+
+      it 'removes exit_codes if allow_failure is specified' do
+        expect(find_job('job-1').options.dig(:allow_failure_criteria)).to be_nil
+        expect(find_job('job-2').options.dig(:allow_failure_criteria)).to be_nil
+        expect(find_job('job-3').options.dig(:allow_failure_criteria, :exit_codes)).to eq([42])
+      end
+
+      context 'with ci_allow_failure_with_exit_codes disabled' do
+        before do
+          stub_feature_flags(ci_allow_failure_with_exit_codes: false)
+        end
+
+        it 'does not persist allow_failure_criteria' do
+          expect(pipeline).to be_persisted
+
+          expect(find_job('job-1').options.key?(:allow_failure_criteria)).to be_falsey
+          expect(find_job('job-2').options.key?(:allow_failure_criteria)).to be_falsey
+          expect(find_job('job-3').options.key?(:allow_failure_criteria)).to be_falsey
+        end
+      end
+    end
   end
 
   context 'when workflow:rules are used' do