Simplify behaviour of rules

This makes the logic when to remove only/except
much simpler

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

@@ -65,6 +65,7 @@ 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,
@@ -98,12 +99,18 @@ module EE
 
             def compose!(deps = nil)
               super do
-                # If workflow:rules:, rules: and only: are undefined
-                # do create default `only:`
+                has_workflow_rules = deps&.workflow&.has_rules?
+
+                # If workflow:rules: or rules: are used
+                # they are considered not compatible
+                # with `only/except` defaults
                 #
-                # 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)
+                # 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/job.rb

@@ -120,6 +120,7 @@ module Gitlab
 
           entry :only, Entry::Policy,
             description: 'Refs policy this job will be executed for.',
+            default: ::Gitlab::Ci::Config::Entry::Policy::DEFAULT_ONLY,
             inherit: false
 
           entry :except, Entry::Policy,
@@ -176,12 +177,18 @@ module Gitlab
 
               @entries.delete(:type)
 
-              # If workflow:rules:, rules: and only: are undefined
-              # do create default `only:`
+              has_workflow_rules = deps&.workflow&.has_rules?
+
+              # If workflow:rules: or rules: are used
+              # they are considered not compatible
+              # with `only/except` defaults
               #
-              # 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)
+              # 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/pipeline/chain/evaluate_workflow_rules.rb

@@ -9,7 +9,13 @@ module Gitlab
           include Chain::Helpers
 
           def perform!
-            return unless feature_enabled?
+            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,15 +23,13 @@ module Gitlab
           end
 
           def break?
-            return false unless feature_enabled?
-
-            !workflow_passed?
+            @pipeline.errors.any? || @pipeline.persisted?
           end
 
           private
 
           def feature_enabled?
-            Feature.enabled?(:workflow_rules, default_enabled: true)
+            Feature.enabled?(:workflow_rules, @pipeline.project, default_enabled: true)
           end
 
           def workflow_passed?
@@ -44,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

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 },

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