Merge branch 'make-workflow-rules-to-work' into 'master'

Make `workflow:rules` work well with Merge Requests

Closes #31685

See merge request gitlab-org/gitlab!21742

app/models/ci/pipeline.rb

@@ -638,6 +638,7 @@ module Ci
         variables.append(key: 'CI_COMMIT_BEFORE_SHA', value: before_sha)
         variables.append(key: 'CI_COMMIT_REF_NAME', value: source_ref)
         variables.append(key: 'CI_COMMIT_REF_SLUG', value: source_ref_slug)
+        variables.append(key: 'CI_COMMIT_BRANCH', value: ref) if branch?
         variables.append(key: 'CI_COMMIT_TAG', value: ref) if tag?
         variables.append(key: 'CI_COMMIT_MESSAGE', value: git_commit_message.to_s)
         variables.append(key: 'CI_COMMIT_TITLE', value: git_commit_full_title.to_s)

changelogs/unreleased/make-workflow-rules-to-work.yml

+---
+title: Make `workflow:rules` to work well with Merge Requests
+merge_request: 21742
+author:
+type: changed

doc/ci/variables/predefined_variables.md

@@ -37,6 +37,7 @@ future GitLab releases.**
 | `CI_COMMIT_REF_SLUG`                          | 9.0    | all    | `$CI_COMMIT_REF_NAME` lowercased, shortened to 63 bytes, and with everything except `0-9` and `a-z` replaced with `-`. No leading / trailing `-`. Use in URLs, host names and domain names.                                                                                                                                                                |
 | `CI_COMMIT_SHA`                               | 9.0    | all    | The commit revision for which project is built                                                                                                                                                                                                                                                                                                             |
 | `CI_COMMIT_SHORT_SHA`                         | 11.7   | all    | The first eight characters of `CI_COMMIT_SHA`                                                                                                                                                                                                                                                                                                              |
+| `CI_COMMIT_BRANCH`                            | 12.6   | 0.5    | The commit branch name. Present only when building branches.                                                                                                                                                                                                                                                                                                      |
 | `CI_COMMIT_TAG`                               | 9.0    | 0.5    | The commit tag name. Present only when building tags.                                                                                                                                                                                                                                                                                                      |
 | `CI_COMMIT_TITLE`                             | 10.8   | all    | The title of the commit - the full first line of the message                                                                                                                                                                                                                                                                                               |
 | `CI_CONCURRENT_ID`                            | all    | 11.10  | Unique ID of build execution within a single executor.                                                                                                                                                                                                                                                                                                     |

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

@@ -99,11 +99,18 @@ module EE
 
             def compose!(deps = nil)
               super do
-                # This is something of a hack, see issue for details:
-                # https://gitlab.com/gitlab-org/gitlab/issues/31685
-                if !only_defined? && has_rules?
-                  @entries.delete(:only)
-                  @entries.delete(:except)
+                has_workflow_rules = deps&.workflow&.has_rules?
+
+                # If workflow:rules: or rules: are used
+                # they are considered not compatible
+                # with `only/except` defaults
+                #
+                # Context: https://gitlab.com/gitlab-org/gitlab/merge_requests/21742
+                if has_rules? || has_workflow_rules
+                  # Remove only/except defaults
+                  # defaults are not considered as defined
+                  @entries.delete(:only) unless only_defined?
+                  @entries.delete(:except) unless except_defined?
                 end
               end
             end

ee/spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -22,6 +22,7 @@ describe Gitlab::Ci::YamlProcessor do
           stage: "build",
           stage_idx: 1,
           name: "build",
+          only: { refs: %w[branches tags] },
           options: {
             script: ["test"]
           },
@@ -33,6 +34,7 @@ describe Gitlab::Ci::YamlProcessor do
           stage: "test",
           stage_idx: 2,
           name: "bridge",
+          only: { refs: %w[branches tags] },
           options: {
             bridge_needs: { pipeline: 'some/project' }
           },
@@ -52,6 +54,7 @@ describe Gitlab::Ci::YamlProcessor do
           stage: "build",
           stage_idx: 1,
           name: "build",
+          only: { refs: %w[branches tags] },
           options: {
             script: ["test"]
           },
@@ -63,6 +66,7 @@ describe Gitlab::Ci::YamlProcessor do
           stage: "test",
           stage_idx: 2,
           name: "bridge",
+          only: { refs: %w[branches tags] },
           options: {
             bridge_needs: { pipeline: 'some/project' }
           },

ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb

@@ -110,5 +110,27 @@ describe MergeRequests::CreatePipelineService, :clean_gitlab_redis_shared_state
 
       it_behaves_like 'detached merge request pipeline'
     end
+
+    context 'when workflow:rules are specified' do
+      let(:source_branch) { 'feature' }
+      let(:target_branch) { 'feature_conflict' }
+
+      context 'when bridge job is used' do
+        let(:config) do
+          {
+            workflow: {
+              rules: [
+                { if: '$CI_MERGE_REQUEST_ID' }
+              ]
+            },
+            bridge_job: {
+              needs: { pipeline: 'some/project' }
+            }
+          }
+        end
+
+        it_behaves_like 'detached merge request pipeline'
+      end
+    end
   end
 end

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

@@ -120,7 +120,7 @@ module Gitlab
 
           entry :only, Entry::Policy,
             description: 'Refs policy this job will be executed for.',
-            default: Entry::Policy::DEFAULT_ONLY,
+            default: ::Gitlab::Ci::Config::Entry::Policy::DEFAULT_ONLY,
             inherit: false
 
           entry :except, Entry::Policy,
@@ -177,11 +177,18 @@ module Gitlab
 
               @entries.delete(:type)
 
-              # This is something of a hack, see issue for details:
-              # https://gitlab.com/gitlab-org/gitlab/issues/31685
-              if !only_defined? && has_rules?
-                @entries.delete(:only)
-                @entries.delete(:except)
+              has_workflow_rules = deps&.workflow&.has_rules?
+
+              # If workflow:rules: or rules: are used
+              # they are considered not compatible
+              # with `only/except` defaults
+              #
+              # Context: https://gitlab.com/gitlab-org/gitlab/merge_requests/21742
+              if has_rules? || has_workflow_rules
+                # Remove only/except defaults
+                # defaults are not considered as defined
+                @entries.delete(:only) unless only_defined?
+                @entries.delete(:except) unless except_defined?
               end
             end
           end

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

@@ -67,7 +67,7 @@ module Gitlab
           entry :workflow, Entry::Workflow,
             description: 'List of evaluable rules to determine Pipeline status'
 
-          helpers :default, :jobs, :stages, :types, :variables
+          helpers :default, :jobs, :stages, :types, :variables, :workflow
 
           delegate :before_script_value,
                    :image_value,
@@ -106,6 +106,10 @@ module Gitlab
             self[:default]
           end
 
+          def workflow
+            self[:workflow] if workflow_defined?
+          end
+
           private
 
           # rubocop: disable CodeReuse/ActiveRecord

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

@@ -18,6 +18,10 @@ module Gitlab
           entry :rules, Entry::Rules,
             description: 'List of evaluable Rules to determine Pipeline status.',
             metadata: { allowed_when: %w[always never] }
+
+          def has_rules?
+            @config.try(:key?, :rules)
+          end
         end
       end
     end

lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules.rb

@@ -9,7 +9,13 @@ module Gitlab
           include Chain::Helpers
 
           def perform!
-            return unless Feature.enabled?(:workflow_rules, @pipeline.project)
+            unless feature_enabled?
+              if has_workflow_rules?
+                error("Workflow rules are disabled", config_error: true)
+              end
+
+              return
+            end
 
             unless workflow_passed?
               error('Pipeline filtered out by workflow rules.')
@@ -17,13 +23,15 @@ module Gitlab
           end
 
           def break?
-            return false unless Feature.enabled?(:workflow_rules, @pipeline.project)
-
-            !workflow_passed?
+            @pipeline.errors.any? || @pipeline.persisted?
           end
 
           private
 
+          def feature_enabled?
+            Feature.enabled?(:workflow_rules, @pipeline.project, default_enabled: true)
+          end
+
           def workflow_passed?
             strong_memoize(:workflow_passed) do
               workflow_rules.evaluate(@pipeline, global_context).pass?
@@ -40,6 +48,10 @@ module Gitlab
               @pipeline, yaml_variables: workflow_config[:yaml_variables])
           end
 
+          def has_workflow_rules?
+            workflow_config[:rules].present?
+          end
+
           def workflow_config
             @command.config_processor.workflow_attributes || {}
           end

lib/gitlab/ci/yaml_processor.rb

@@ -60,6 +60,8 @@ module Gitlab
           yaml_variables: transform_to_yaml_variables(job_variables(name)),
           needs_attributes: job.dig(:needs, :job),
           interruptible: job[:interruptible],
+          only: job[:only],
+          except: job[:except],
           rules: job[:rules],
           cache: job[:cache],
           resource_group_key: job[:resource_group],
@@ -90,13 +92,7 @@ module Gitlab
 
       def stages_attributes
         @stages.uniq.map do |stage|
-          seeds = stage_builds_attributes(stage).map do |attributes|
-            job = @jobs.fetch(attributes[:name].to_sym)
-
-            attributes
-              .merge(only: job.fetch(:only, {}))
-              .merge(except: job.fetch(:except, {}))
-          end
+          seeds = stage_builds_attributes(stage)
 
           { name: stage, index: @stages.index(stage), builds: seeds }
         end

lib/gitlab/config/entry/configurable.rb

@@ -25,7 +25,6 @@ module Gitlab
           end
         end
 
-        # rubocop: disable CodeReuse/ActiveRecord
         def compose!(deps = nil)
           return unless valid?
 
@@ -35,11 +34,7 @@ module Gitlab
               # we can end with different config types like String
               next unless config.is_a?(Hash)
 
-              factory
-                .value(config[key])
-                .with(key: key, parent: self)
-
-              entries[key] = factory.create!
+              entry_create!(key, config[key])
             end
 
             yield if block_given?
@@ -49,6 +44,16 @@ module Gitlab
             end
           end
         end
+
+        # rubocop: disable CodeReuse/ActiveRecord
+        def entry_create!(key, value)
+          factory = self.class
+            .nodes[key]
+            .value(value)
+            .with(key: key, parent: self)
+
+          entries[key] = factory.create!
+        end
         # rubocop: enable CodeReuse/ActiveRecord
 
         def skip_config_hash_validation?

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

@@ -461,7 +461,8 @@ describe Gitlab::Ci::Config::Entry::Job do
 
     let(:unspecified) { double('unspecified', 'specified?' => false) }
     let(:default) { double('default', '[]' => unspecified) }
-    let(:deps) { double('deps', 'default' => default, '[]' => unspecified) }
+    let(:workflow) { double('workflow', 'has_rules?' => false) }
+    let(:deps) { double('deps', 'default' => default, '[]' => unspecified, 'workflow' => workflow) }
 
     context 'when job config overrides default config' do
       before do
@@ -492,6 +493,49 @@ describe Gitlab::Ci::Config::Entry::Job do
         expect(entry[:cache].value).to eq(key: 'test', policy: 'pull-push')
       end
     end
+
+    context 'with workflow rules' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:name, :has_workflow_rules?, :only, :rules, :result) do
+        "uses default only"    | false | nil          | nil    | { refs: %w[branches tags] }
+        "uses user only"       | false | %w[branches] | nil    | { refs: %w[branches] }
+        "does not define only" | false | nil          | []     | nil
+        "does not define only" | true  | nil          | nil    | nil
+        "uses user only"       | true  | %w[branches] | nil    | { refs: %w[branches] }
+        "does not define only" | true  | nil          | []     | nil
+      end
+
+      with_them do
+        let(:config) { { script: 'ls', rules: rules, only: only }.compact }
+
+        it "#{name}" do
+          expect(workflow).to receive(:has_rules?) { has_workflow_rules? }
+
+          entry.compose!(deps)
+
+          expect(entry.only_value).to eq(result)
+        end
+      end
+    end
+
+    context 'when workflow rules is used' do
+      context 'when rules are used' do
+        let(:config) { { script: 'ls', cache: { key: 'test' }, rules: [] } }
+
+        it 'does not define only' do
+          expect(entry).not_to be_only_defined
+        end
+      end
+
+      context 'when rules are not used' do
+        let(:config) { { script: 'ls', cache: { key: 'test' }, only: [] } }
+
+        it 'does not define only' do
+          expect(entry).not_to be_only_defined
+        end
+      end
+    end
   end
 
   context 'when composed' do

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -28,6 +28,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "rspec",
+              only: { refs: %w[branches tags] },
               options: {
                 before_script: ["pwd"],
                 script: ["rspec"]
@@ -120,6 +121,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "rspec",
+              only: { refs: %w[branches tags] },
               options: { script: ["rspec"] },
               interruptible: true,
               allow_failure: false,
@@ -281,8 +283,7 @@ module Gitlab
                   when: "on_success",
                   yaml_variables: [],
                   options: { script: ["rspec"] },
-                  only: { refs: ["branches"] },
-                  except: {} }] },
+                  only: { refs: ["branches"] } }] },
            { name: "deploy",
              index: 3,
              builds:
@@ -293,8 +294,7 @@ module Gitlab
                   when: "on_success",
                   yaml_variables: [],
                   options: { script: ["cap prod"] },
-                  only: { refs: ["tags"] },
-                  except: {} }] },
+                  only: { refs: ["tags"] } }] },
            { name: ".post",
              index: 4,
              builds: [] }]
@@ -631,6 +631,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "rspec",
+              only: { refs: %w[branches tags] },
               options: {
                 before_script: ["pwd"],
                 script: ["rspec"],
@@ -662,6 +663,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "rspec",
+              only: { refs: %w[branches tags] },
               options: {
                 before_script: ["pwd"],
                 script: ["rspec"],
@@ -691,6 +693,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "rspec",
+              only: { refs: %w[branches tags] },
               options: {
                 before_script: ["pwd"],
                 script: ["rspec"],
@@ -716,6 +719,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "rspec",
+              only: { refs: %w[branches tags] },
               options: {
                 before_script: ["pwd"],
                 script: ["rspec"],
@@ -1230,6 +1234,7 @@ module Gitlab
             stage: "test",
             stage_idx: 2,
             name: "rspec",
+            only: { refs: %w[branches tags] },
             options: {
               before_script: ["pwd"],
               script: ["rspec"],
@@ -1527,6 +1532,7 @@ module Gitlab
               stage: "build",
               stage_idx: 1,
               name: "build1",
+              only: { refs: %w[branches tags] },
               options: {
                 script: ["test"]
               },
@@ -1538,6 +1544,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "test1",
+              only: { refs: %w[branches tags] },
               options: { script: ["test"] },
               needs_attributes: [
                 { name: "build1", artifacts: true },
@@ -1565,6 +1572,7 @@ module Gitlab
               stage: "build",
               stage_idx: 1,
               name: "build1",
+              only: { refs: %w[branches tags] },
               options: {
                 script: ["test"]
               },
@@ -1576,6 +1584,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "test1",
+              only: { refs: %w[branches tags] },
               options: { script: ["test"] },
               needs_attributes: [
                 { name: "parallel 1/2", artifacts: false },
@@ -1599,6 +1608,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "test1",
+              only: { refs: %w[branches tags] },
               options: { script: ["test"] },
               needs_attributes: [
                 { name: "parallel 1/2", artifacts: true },
@@ -1626,6 +1636,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "test1",
+              only: { refs: %w[branches tags] },
               options: { script: ["test"] },
               needs_attributes: [
                 { name: "build1", artifacts: true },
@@ -1733,6 +1744,7 @@ module Gitlab
               stage: "test",
               stage_idx: 2,
               name: "normal_job",
+              only: { refs: %w[branches tags] },
               options: {
                 script: ["test"]
               },
@@ -1778,6 +1790,7 @@ module Gitlab
               stage: "build",
               stage_idx: 1,
               name: "job1",
+              only: { refs: %w[branches tags] },
               options: {
                 script: ["execute-script-for-job"]
               },
@@ -1789,6 +1802,7 @@ module Gitlab
               stage: "build",
               stage_idx: 1,
               name: "job2",
+              only: { refs: %w[branches tags] },
               options: {
                 script: ["execute-script-for-job"]
               },

spec/models/ci/build_spec.rb

@@ -2366,6 +2366,7 @@ describe Ci::Build do
           { key: 'CI_COMMIT_BEFORE_SHA', value: build.before_sha, public: true, masked: false },
           { key: 'CI_COMMIT_REF_NAME', value: build.ref, public: true, masked: false },
           { key: 'CI_COMMIT_REF_SLUG', value: build.ref_slug, public: true, masked: false },
+          { key: 'CI_COMMIT_BRANCH', value: build.ref, public: true, masked: false },
           { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true, masked: false },
           { key: 'CI_COMMIT_TITLE', value: pipeline.git_commit_title, public: true, masked: false },
           { key: 'CI_COMMIT_DESCRIPTION', value: pipeline.git_commit_description, public: true, masked: false },
@@ -2589,6 +2590,19 @@ describe Ci::Build do
       it { is_expected.to include(job_variable) }
     end
 
+    context 'when build is for branch' do
+      let(:branch_variable) do
+        { key: 'CI_COMMIT_BRANCH', value: 'master', public: true, masked: false }
+      end
+
+      before do
+        build.update(tag: false)
+        pipeline.update(tag: false)
+      end
+
+      it { is_expected.to include(branch_variable) }
+    end
+
     context 'when build is for tag' do
       let(:tag_variable) do
         { key: 'CI_COMMIT_TAG', value: 'master', public: true, masked: false }

spec/models/ci/pipeline_spec.rb

@@ -599,6 +599,7 @@ describe Ci::Pipeline, :mailer do
         CI_COMMIT_BEFORE_SHA
         CI_COMMIT_REF_NAME
         CI_COMMIT_REF_SLUG
+        CI_COMMIT_BRANCH
         CI_COMMIT_MESSAGE
         CI_COMMIT_TITLE
         CI_COMMIT_DESCRIPTION

spec/services/ci/create_pipeline_service/rules_spec.rb

@@ -100,6 +100,17 @@ describe Ci::CreatePipelineService do
       stub_ci_pipeline_yaml_file(config)
     end
 
+    shared_examples 'workflow:rules feature disabled' do
+      before do
+        stub_feature_flags(workflow_rules: false)
+      end
+
+      it 'presents a message that rules are disabled' do
+        expect(pipeline.errors[:base]).to include('Workflow rules are disabled')
+        expect(pipeline).to be_persisted
+      end
+    end
+
     context 'with a single regex-matching if: clause' do
       let(:config) do
         <<-EOY
@@ -231,16 +242,7 @@ describe Ci::CreatePipelineService do
           expect(pipeline).not_to be_persisted
         end
 
-        context 'with workflow:rules shut off' do
-          before do
-            stub_feature_flags(workflow_rules: false)
-          end
-
-          it 'invalidates the pipeline with an empty jobs error' do
-            expect(pipeline.errors[:base]).to include('No stages / jobs for this pipeline.')
-            expect(pipeline).not_to be_persisted
-          end
-        end
+        it_behaves_like 'workflow:rules feature disabled'
       end
 
       context 'where workflow passes and the job passes' do
@@ -251,16 +253,7 @@ describe Ci::CreatePipelineService do
           expect(pipeline).to be_persisted
         end
 
-        context 'with workflow:rules shut off' do
-          before do
-            stub_feature_flags(workflow_rules: false)
-          end
-
-          it 'saves a pending pipeline' do
-            expect(pipeline).to be_pending
-            expect(pipeline).to be_persisted
-          end
-        end
+        it_behaves_like 'workflow:rules feature disabled'
       end
 
       context 'where workflow fails and the job fails' do
@@ -271,16 +264,7 @@ describe Ci::CreatePipelineService do
           expect(pipeline).not_to be_persisted
         end
 
-        context 'with workflow:rules shut off' do
-          before do
-            stub_feature_flags(workflow_rules: false)
-          end
-
-          it 'invalidates the pipeline with an empty jobs error' do
-            expect(pipeline.errors[:base]).to include('No stages / jobs for this pipeline.')
-            expect(pipeline).not_to be_persisted
-          end
-        end
+        it_behaves_like 'workflow:rules feature disabled'
       end
 
       context 'where workflow fails and the job passes' do
@@ -291,16 +275,7 @@ describe Ci::CreatePipelineService do
           expect(pipeline).not_to be_persisted
         end
 
-        context 'with workflow:rules shut off' do
-          before do
-            stub_feature_flags(workflow_rules: false)
-          end
-
-          it 'saves a pending pipeline' do
-            expect(pipeline).to be_pending
-            expect(pipeline).to be_persisted
-          end
-        end
+        it_behaves_like 'workflow:rules feature disabled'
       end
     end
   end

spec/services/merge_requests/create_pipeline_service_spec.rb

@@ -62,13 +62,65 @@ describe MergeRequests::CreatePipelineService do
       end
     end
 
-    context 'when .gitlab-ci.yml does not have only: [merge_requests] keyword' do
-      let(:config) do
-        { rspec: { script: 'echo' } }
+    context 'when .gitlab-ci.yml does not use workflow:rules' do
+      context 'without only: [merge_requests] keyword' do
+        let(:config) do
+          { rspec: { script: 'echo' } }
+        end
+
+        it 'does not create a pipeline' do
+          expect { subject }.not_to change { Ci::Pipeline.count }
+        end
+      end
+
+      context 'with rules that specify creation on a tag' do
+        let(:config) do
+          {
+            rspec: {
+              script: 'echo',
+              rules: [{ if: '$CI_COMMIT_TAG' }]
+            }
+          }
+        end
+
+        it 'does not create a pipeline' do
+          expect { subject }.not_to change { Ci::Pipeline.count }
+        end
+      end
+    end
+
+    context 'when workflow:rules are specified' do
+      context 'when rules request creation on merge request' do
+        let(:config) do
+          {
+            workflow: {
+              rules: [{ if: '$CI_MERGE_REQUEST_ID' }]
+            },
+            rspec: { script: 'echo' }
+          }
+        end
+
+        it 'creates a detached merge request pipeline' do
+          expect { subject }.to change { Ci::Pipeline.count }.by(1)
+
+          expect(subject).to be_persisted
+          expect(subject).to be_detached_merge_request_pipeline
+        end
       end
 
-      it 'does not create a pipeline' do
-        expect { subject }.not_to change { Ci::Pipeline.count }
+      context 'with rules do specify creation on a tag' do
+        let(:config) do
+          {
+            workflow: {
+              rules: [{ if: '$CI_COMMIT_TAG' }]
+            },
+            rspec: { script: 'echo' }
+          }
+        end
+
+        it 'does not create a pipeline' do
+          expect { subject }.not_to change { Ci::Pipeline.count }
+        end
       end
     end
   end