Merge branch 'raise-warning-on-multiple-pipelines' into 'master'

Raise warning when job:rules can trigger multiple pipelines

See merge request gitlab-org/gitlab!38387

app/controllers/projects/ci/lints_controller.rb

@@ -12,6 +12,7 @@ class Projects::Ci::LintsController < Projects::ApplicationController
 
     @status = result.valid?
     @errors = result.errors
+    @warnings = result.warnings
 
     if result.valid?
       @config_processor = result.config

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

@@ -83,8 +83,8 @@ module Gitlab
                 @entries.delete(:except) unless except_defined? # rubocop:disable Gitlab/ModuleWithInstanceVariables
               end
 
-              if has_rules? && !has_workflow_rules && Gitlab::Ci::Features.raise_job_rules_without_workflow_rules_warning?
-                add_warning('uses `rules` without defining `workflow:rules`')
+              unless has_workflow_rules
+                validate_against_warnings
               end
 
               # inherit root variables
@@ -94,6 +94,19 @@ module Gitlab
             end
           end
 
+          def validate_against_warnings
+            # If rules are valid format and workflow rules are not specified
+            return unless rules_value
+            return unless Gitlab::Ci::Features.raise_job_rules_without_workflow_rules_warning?
+
+            last_rule = rules_value.last
+
+            if last_rule&.keys == [:when] && last_rule[:when] != 'never'
+              docs_url = 'read more: https://docs.gitlab.com/ee/ci/yaml/README.html#differences-between-rules-and-onlyexcept'
+              add_warning("may allow multiple pipelines to run for a single action due to `rules:when` clause with no `workflow:rules` - #{docs_url}")
+            end
+          end
+
           def name
             metadata[:name]
           end

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

@@ -230,6 +230,12 @@ RSpec.describe Gitlab::Ci::Config::Entry::Processable do
       end
     end
 
+    shared_examples 'has no warnings' do
+      it 'does not raise the warning' do
+        expect(entry.warnings).to be_empty
+      end
+    end
+
     context 'when workflow rules is used' do
       let(:workflow) { double('workflow', 'has_rules?' => true) }
 
@@ -254,6 +260,86 @@ RSpec.describe Gitlab::Ci::Config::Entry::Processable do
       end
     end
 
+    context 'when workflow rules is not used' do
+      let(:workflow) { double('workflow', 'has_rules?' => false) }
+      let(:feature_flag_value) { true }
+
+      before do
+        stub_feature_flags(ci_raise_job_rules_without_workflow_rules_warning: feature_flag_value)
+        entry.compose!(deps)
+      end
+
+      context 'when rules are valid' do
+        let(:config) do
+          {
+            script: 'ls',
+            rules: [
+              { if: '$CI_COMMIT_BRANCH', when: 'on_success' },
+              last_rule
+            ]
+          }
+        end
+
+        context 'when last rule contains only `when`' do
+          let(:last_rule) { { when: when_value } }
+
+          context 'and its value is not `never`' do
+            let(:when_value) { 'on_success' }
+
+            it 'raises a warning' do
+              expect(entry.warnings).to contain_exactly(/may allow multiple pipelines/)
+            end
+
+            context 'when feature flag is disabled' do
+              let(:feature_flag_value) { false }
+
+              it_behaves_like 'has no warnings'
+            end
+          end
+
+          context 'and its value is `never`' do
+            let(:when_value) { 'never' }
+
+            it_behaves_like 'has no warnings'
+          end
+        end
+
+        context 'when last rule does not contain only `when`' do
+          let(:last_rule) { { if: '$CI_MERGE_REQUEST_ID', when: 'always' } }
+
+          it_behaves_like 'has no warnings'
+        end
+      end
+
+      context 'when rules are invalid' do
+        let(:config) { { script: 'ls', rules: { when: 'always' } } }
+
+        it_behaves_like 'has no warnings'
+      end
+    end
+
+    context 'when workflow rules is used' do
+      let(:workflow) { double('workflow', 'has_rules?' => true) }
+
+      before do
+        entry.compose!(deps)
+      end
+
+      context 'when last rule contains only `when' do
+        let(:config) do
+          {
+            script: 'ls',
+            rules: [
+              { if: '$CI_COMMIT_BRANCH', when: 'on_success' },
+              { when: 'always' }
+            ]
+          }
+        end
+
+        it_behaves_like 'has no warnings'
+      end
+    end
+
     context 'with inheritance' do
       context 'of variables' do
         let(:config) do

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -443,15 +443,15 @@ module Gitlab
         context 'when a warning is raised in a given entry' do
           let(:config) do
             <<-EOYML
-              rspec:
-                script: rspec
-                rules:
-                  - if: '$VAR == "value"'
+            rspec:
+              script: echo
+              rules:
+                - when: always
             EOYML
           end
 
           it 'is propagated all the way up to the processor' do
-            expect(subject.warnings).to contain_exactly('jobs:rspec uses `rules` without defining `workflow:rules`')
+            expect(subject.warnings).to contain_exactly(/jobs:rspec may allow multiple pipelines to run/)
           end
         end
 
@@ -461,7 +461,7 @@ module Gitlab
               rspec:
                 script: rspec
                 rules:
-                  - if: '$VAR == "value"'
+                  - when: always
               invalid:
                 script: echo
                 artifacts:
@@ -473,7 +473,7 @@ module Gitlab
             expect { subject }.to raise_error do |error|
               expect(error).to be_a(described_class::ValidationError)
               expect(error.message).to eq('jobs:invalid:artifacts config should be a hash')
-              expect(error.warnings).to contain_exactly('jobs:rspec uses `rules` without defining `workflow:rules`')
+              expect(error.warnings).to contain_exactly(/jobs:rspec may allow multiple pipelines to run/)
             end
           end
         end
@@ -485,7 +485,7 @@ module Gitlab
               rspec:
                 script: rspec
                 rules:
-                  - if: '$VAR == "value"'
+                  - when: always
             EOYML
           end
 
@@ -516,7 +516,7 @@ module Gitlab
                   stage: custom_stage
                   script: rspec
                   rules:
-                    - if: '$VAR == "value"'
+                    - when: always
               EOYML
             end
 
@@ -530,7 +530,7 @@ module Gitlab
                   stage: build
                   script: echo
                   rules:
-                    - if: '$VAR == "value"'
+                    - when: always
                 test:
                   stage: test
                   script: echo
@@ -549,7 +549,7 @@ module Gitlab
                   script: echo
                   needs: [test]
                   rules:
-                    - if: '$VAR == "value"'
+                    - when: always
                 test:
                   stage: test
                   script: echo
@@ -571,7 +571,7 @@ module Gitlab
                 rspec:
                   script: rspec
                   rules:
-                    - if: '$VAR == "value"'
+                    - when: always
               EOYML
             end
 

spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe Ci::CreatePipelineService do
             test:
               script: rspec
               rules:
-                - if: '$CI_COMMIT_BRANCH'
+                - when: always
           YAML
         end
 
@@ -32,7 +32,7 @@ RSpec.describe Ci::CreatePipelineService do
           expect(pipeline.error_messages.map(&:content)).to be_empty
 
           expect(pipeline.warning_messages.map(&:content)).to contain_exactly(
-            'jobs:test uses `rules` without defining `workflow:rules`'
+            /jobs:test may allow multiple pipelines to run/
           )
         end
 
@@ -77,13 +77,13 @@ RSpec.describe Ci::CreatePipelineService do
               stage: test
               script: echo
               rules:
-                - if: '$CI_COMMIT_BRANCH'
+                - when: on_success
           YAML
         end
 
         it 'contains both errors and warnings' do
           error_message = 'build job: need test is not defined in prior stages'
-          warning_message = 'jobs:test uses `rules` without defining `workflow:rules`'
+          warning_message = /jobs:test may allow multiple pipelines to run/
 
           expect(pipeline.yaml_errors).to eq(error_message)
           expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message)