Make `workflow:rules` to work well with Merge Requests

This improves `workflow:rules` behavior to allow
workflow rules to control when pipeline is created.

For, example it allows to define a global workflow
to create pipelines on a branch, and on a merge request
in an easy way without the need to re-invent that multiple
times for each job.

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

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

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

@@ -65,7 +65,6 @@ module EE
 
             entry :only, ::Gitlab::Ci::Config::Entry::Policy,
               description: 'Refs policy this job will be executed for.',
-              default: ::Gitlab::Ci::Config::Entry::Policy::DEFAULT_ONLY,
               inherit: false
 
             entry :except, ::Gitlab::Ci::Config::Entry::Policy,
@@ -99,11 +98,12 @@ 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)
+                # If workflow:rules:, rules: and only: are undefined
+                # do create default `only:`
+                #
+                # This is to make `only:` to be backward compatible
+                if !deps&.workflow&.has_rules? && !has_rules? && !only_defined?
+                  entry_create!(:only, ::Gitlab::Ci::Config::Entry::Policy::DEFAULT_ONLY)
                 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,6 @@ module Gitlab
 
           entry :only, Entry::Policy,
             description: 'Refs policy this job will be executed for.',
-            default: Entry::Policy::DEFAULT_ONLY,
             inherit: false
 
           entry :except, Entry::Policy,
@@ -177,11 +176,12 @@ 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)
+              # If workflow:rules:, rules: and only: are undefined
+              # do create default `only:`
+              #
+              # This is to make `only:` to be backward compatible
+              if !deps&.workflow&.has_rules? && !has_rules? && !only_defined?
+                entry_create!(:only, Entry::Policy::DEFAULT_ONLY)
               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,7 @@ module Gitlab
           include Chain::Helpers
 
           def perform!
-            return unless Feature.enabled?(:workflow_rules, @pipeline.project)
+            return unless feature_enabled?
 
             unless workflow_passed?
               error('Pipeline filtered out by workflow rules.')
@@ -17,13 +17,17 @@ module Gitlab
           end
 
           def break?
-            return false unless Feature.enabled?(:workflow_rules, @pipeline.project)
+            return false unless feature_enabled?
 
             !workflow_passed?
           end
 
           private
 
+          def feature_enabled?
+            Feature.enabled?(:workflow_rules, default_enabled: true)
+          end
+
           def workflow_passed?
             strong_memoize(:workflow_passed) do
               workflow_rules.evaluate(@pipeline, global_context).pass?

lib/gitlab/ci/yaml_processor.rb

@@ -42,6 +42,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],
@@ -72,13 +74,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/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