Merge branch 'warn-on-missing-workflow-rules' into 'master'

Warn on missing workflow:rules when job:rules is used

See merge request gitlab-org/gitlab!34085

app/controllers/projects/ci/lints_controller.rb

@@ -14,7 +14,7 @@ class Projects::Ci::LintsController < Projects::ApplicationController
     @errors = result.errors
 
     if result.valid?
-      @config_processor = result.content
+      @config_processor = result.config
       @stages = @config_processor.stages
       @builds = @config_processor.builds
       @jobs = @config_processor.jobs

app/models/ci/pipeline.rb

@@ -641,9 +641,22 @@ module Ci
     end
 
     def add_error_message(content)
-      return unless Gitlab::Ci::Features.store_pipeline_messages?(project)
+      add_message(:error, content)
+    end
+
+    def add_warning_message(content)
+      add_message(:warning, content)
+    end
+
+    # We can't use `messages.error` scope here because messages should also be
+    # read when the pipeline is not persisted. Using the scope will return no
+    # results as it would query persisted data.
+    def error_messages
+      messages.select(&:error?)
+    end
 
-      messages.error.build(content: content)
+    def warning_messages
+      messages.select(&:warning?)
     end
 
     # Manually set the notes for a Ci::Pipeline
@@ -1019,6 +1032,12 @@ module Ci
 
     private
 
+    def add_message(severity, content)
+      return unless Gitlab::Ci::Features.store_pipeline_messages?(project)
+
+      messages.build(severity: severity, content: content)
+    end
+
     def pipeline_data
       Gitlab::DataBuilder::Pipeline.build(self)
     end

lib/gitlab/ci/config.rb

@@ -39,6 +39,10 @@ module Gitlab
         @root.errors
       end
 
+      def warnings
+        @root.warnings
+      end
+
       def to_hash
         @config
       end

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

@@ -82,6 +82,10 @@ 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`')
+              end
+
               # inherit root variables
               @root_variables_value = deps&.variables_value # rubocop:disable Gitlab/ModuleWithInstanceVariables
 

lib/gitlab/ci/features.rb

@@ -47,6 +47,12 @@ module Gitlab
       def self.variables_api_filter_environment_scope?
         ::Feature.enabled?(:ci_variables_api_filter_environment_scope, default_enabled: false)
       end
+
+      # This FF is only used for development purpose to test that warnings can be
+      # raised and propagated to the UI.
+      def self.raise_job_rules_without_workflow_rules_warning?
+        ::Feature.enabled?(:ci_raise_job_rules_without_workflow_rules_warning)
+      end
     end
   end
 end

lib/gitlab/ci/pipeline/chain/config/process.rb

@@ -19,7 +19,11 @@ module Gitlab
                   parent_pipeline: parent_pipeline
                 }
               )
+
+              add_warnings_to_pipeline(@command.config_processor.warnings)
             rescue Gitlab::Ci::YamlProcessor::ValidationError => ex
+              add_warnings_to_pipeline(ex.warnings)
+
               error(ex.message, config_error: true)
             rescue => ex
               Gitlab::ErrorTracking.track_exception(ex,
@@ -34,6 +38,14 @@ module Gitlab
             def break?
               @pipeline.errors.any? || @pipeline.persisted?
             end
+
+            private
+
+            def add_warnings_to_pipeline(warnings)
+              return unless warnings.present?
+
+              warnings.each { |message| warning(message) }
+            end
           end
         end
       end

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

@@ -19,6 +19,10 @@ module Gitlab
             # https://gitlab.com/gitlab-org/gitlab/-/issues/220823
             pipeline.errors.add(:base, message)
           end
+
+          def warning(message)
+            pipeline.add_warning_message(message)
+          end
         end
       end
     end

lib/gitlab/ci/yaml_processor.rb

@@ -3,15 +3,33 @@
 module Gitlab
   module Ci
     class YamlProcessor
-      ValidationError = Class.new(StandardError)
+      # ValidationError is treated like a result object in the form of an exception.
+      # We can return any warnings, raised during the config validation, along with
+      # the error object until we support multiple messages to be returned.
+      class ValidationError < StandardError
+        attr_reader :warnings
+
+        def initialize(message, warnings: [])
+          @warnings = warnings
+          super(message)
+        end
+      end
 
       include Gitlab::Config::Entry::LegacyValidationHelpers
 
       attr_reader :stages, :jobs
 
-      ResultWithErrors = Struct.new(:content, :errors) do
+      class Result
+        attr_reader :config, :errors, :warnings
+
+        def initialize(config: nil, errors: [], warnings: [])
+          @config = config
+          @errors = errors
+          @warnings = warnings
+        end
+
         def valid?
-          errors.empty?
+          config.present? && errors.empty?
         end
       end
 
@@ -20,24 +38,32 @@ module Gitlab
         @config = @ci_config.to_hash
 
         unless @ci_config.valid?
-          raise ValidationError, @ci_config.errors.first
+          error!(@ci_config.errors.first)
         end
 
         initial_parsing
       rescue Gitlab::Ci::Config::ConfigError => e
-        raise ValidationError, e.message
+        error!(e.message)
       end
 
       def self.new_with_validation_errors(content, opts = {})
-        return ResultWithErrors.new('', ['Please provide content of .gitlab-ci.yml']) if content.blank?
+        return Result.new(errors: ['Please provide content of .gitlab-ci.yml']) if content.blank?
 
         config = Gitlab::Ci::Config.new(content, **opts)
-        return ResultWithErrors.new("", config.errors) unless config.valid?
+        return Result.new(errors: config.errors, warnings: config.warnings) unless config.valid?
 
         config = Gitlab::Ci::YamlProcessor.new(content, opts)
-        ResultWithErrors.new(config, [])
-      rescue ValidationError, Gitlab::Ci::Config::ConfigError => e
-        ResultWithErrors.new('', [e.message])
+        Result.new(config: config, warnings: config.warnings)
+
+      rescue ValidationError => e
+        Result.new(errors: [e.message], warnings: e.warnings)
+
+      rescue Gitlab::Ci::Config::ConfigError => e
+        Result.new(errors: [e.message])
+      end
+
+      def warnings
+        @ci_config&.warnings || []
       end
 
       def builds
@@ -157,10 +183,14 @@ module Gitlab
         return unless job[:stage]
 
         unless job[:stage].is_a?(String) && job[:stage].in?(@stages)
-          raise ValidationError, "#{name} job: chosen stage does not exist; available stages are #{@stages.join(", ")}"
+          error!("#{name} job: chosen stage does not exist; available stages are #{@stages.join(", ")}")
         end
       end
 
+      def error!(message)
+        raise ValidationError.new(message, warnings: warnings)
+      end
+
       def validate_job_dependencies!(name, job)
         return unless job[:dependencies]
 
@@ -190,7 +220,7 @@ module Gitlab
 
       def validate_job_dependency!(name, dependency, dependency_type = 'dependency')
         unless @jobs[dependency.to_sym]
-          raise ValidationError, "#{name} job: undefined #{dependency_type}: #{dependency}"
+          error!("#{name} job: undefined #{dependency_type}: #{dependency}")
         end
 
         job_stage_index = stage_index(name)
@@ -199,7 +229,7 @@ module Gitlab
         # A dependency might be defined later in the configuration
         # with a stage that does not exist
         unless dependency_stage_index.present? && dependency_stage_index < job_stage_index
-          raise ValidationError, "#{name} job: #{dependency_type} #{dependency} is not defined in prior stages"
+          error!("#{name} job: #{dependency_type} #{dependency} is not defined in prior stages")
         end
       end
 
@@ -221,19 +251,19 @@ module Gitlab
 
         on_stop_job = @jobs[on_stop.to_sym]
         unless on_stop_job
-          raise ValidationError, "#{name} job: on_stop job #{on_stop} is not defined"
+          error!("#{name} job: on_stop job #{on_stop} is not defined")
         end
 
         unless on_stop_job[:environment]
-          raise ValidationError, "#{name} job: on_stop job #{on_stop} does not have environment defined"
+          error!("#{name} job: on_stop job #{on_stop} does not have environment defined")
         end
 
         unless on_stop_job[:environment][:name] == environment[:name]
-          raise ValidationError, "#{name} job: on_stop job #{on_stop} have different environment name"
+          error!("#{name} job: on_stop job #{on_stop} have different environment name")
         end
 
         unless on_stop_job[:environment][:action] == 'stop'
-          raise ValidationError, "#{name} job: on_stop job #{on_stop} needs to have action stop defined"
+          error!("#{name} job: on_stop job #{on_stop} needs to have action stop defined")
         end
       end
     end

lib/gitlab/config/entry/node.rb

@@ -16,6 +16,7 @@ module Gitlab
           @config = config
           @metadata = metadata
           @entries = {}
+          @warnings = []
 
           yield(self) if block_given?
 
@@ -60,6 +61,14 @@ module Gitlab
           []
         end
 
+        def warnings
+          @warnings + descendants.flat_map(&:warnings)
+        end
+
+        def add_warning(message)
+          @warnings << "#{location} #{message}"
+        end
+
         def value
           if leaf?
             @config

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

@@ -231,6 +231,12 @@ RSpec.describe Gitlab::Ci::Config::Entry::Processable do
     end
 
     context 'when workflow rules is used' do
+      let(:workflow) { double('workflow', 'has_rules?' => true) }
+
+      before do
+        entry.compose!(deps)
+      end
+
       context 'when rules are used' do
         let(:config) { { script: 'ls', cache: { key: 'test' }, rules: [] } }
 
@@ -239,11 +245,11 @@ RSpec.describe Gitlab::Ci::Config::Entry::Processable do
         end
       end
 
-      context 'when rules are not used' do
+      context 'when rules are not used and only is defined' do
         let(:config) { { script: 'ls', cache: { key: 'test' }, only: [] } }
 
-        it 'does not define only' do
-          expect(entry).not_to be_only_defined
+        it 'keeps only entry' do
+          expect(entry).to be_only_defined
         end
       end
     end

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -435,6 +435,153 @@ module Gitlab
         end
       end
 
+      describe '#warnings' do
+        before do
+          stub_feature_flags(ci_raise_job_rules_without_workflow_rules_warning: true)
+        end
+
+        context 'when a warning is raised in a given entry' do
+          let(:config) do
+            <<-EOYML
+              rspec:
+                script: rspec
+                rules:
+                  - if: '$VAR == "value"'
+            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`')
+          end
+        end
+
+        context 'when a warning is raised together with errors' do
+          let(:config) do
+            <<-EOYML
+              rspec:
+                script: rspec
+                rules:
+                  - if: '$VAR == "value"'
+              invalid:
+                script: echo
+                artifacts:
+                  - wrong_key: value
+            EOYML
+          end
+
+          it 'is propagated all the way up into the raised exception' do
+            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`')
+            end
+          end
+        end
+
+        context 'when error is raised before composing the config' do
+          let(:config) do
+            <<-EOYML
+              include: unknown/file.yml
+              rspec:
+                script: rspec
+                rules:
+                  - if: '$VAR == "value"'
+            EOYML
+          end
+
+          it 'raises an exception with empty warnings array' do
+            expect { subject }.to raise_error do |error|
+              expect(error).to be_a(described_class::ValidationError)
+              expect(error.message).to eq('Local file `unknown/file.yml` does not have project!')
+              expect(error.warnings).to be_empty
+            end
+          end
+        end
+
+        context 'when error is raised after composing the config with warnings' do
+          shared_examples 'has warnings and expected error' do |error_message|
+            it 'raises an exception including warnings' do
+              expect { subject }.to raise_error do |error|
+                expect(error).to be_a(described_class::ValidationError)
+                expect(error.message).to match(error_message)
+                expect(error.warnings).to be_present
+              end
+            end
+          end
+
+          context 'when stage does not exist' do
+            let(:config) do
+              <<-EOYML
+                rspec:
+                  stage: custom_stage
+                  script: rspec
+                  rules:
+                    - if: '$VAR == "value"'
+              EOYML
+            end
+
+            it_behaves_like 'has warnings and expected error', /rspec job: chosen stage does not exist/
+          end
+
+          context 'job dependency does not exist' do
+            let(:config) do
+              <<-EOYML
+                build:
+                  stage: build
+                  script: echo
+                  rules:
+                    - if: '$VAR == "value"'
+                test:
+                  stage: test
+                  script: echo
+                  needs: [unknown_job]
+              EOYML
+            end
+
+            it_behaves_like 'has warnings and expected error', /test job: undefined need: unknown_job/
+          end
+
+          context 'job dependency defined in later stage' do
+            let(:config) do
+              <<-EOYML
+                build:
+                  stage: build
+                  script: echo
+                  needs: [test]
+                  rules:
+                    - if: '$VAR == "value"'
+                test:
+                  stage: test
+                  script: echo
+              EOYML
+            end
+
+            it_behaves_like 'has warnings and expected error', /build job: need test is not defined in prior stages/
+          end
+        end
+
+        context 'when feature flag is disabled' do
+          before do
+            stub_feature_flags(ci_raise_job_rules_without_workflow_rules_warning: false)
+          end
+
+          context 'job rules used without workflow rules' do
+            let(:config) do
+              <<-EOYML
+                rspec:
+                  script: rspec
+                  rules:
+                    - if: '$VAR == "value"'
+              EOYML
+            end
+
+            it 'does not raise the warning' do
+              expect(subject.warnings).to be_empty
+            end
+          end
+        end
+      end
+
       describe 'only / except policies validations' do
         context 'when `only` has an invalid value' do
           let(:config) { { rspec: { script: "rspec", type: "test", only: only } } }
@@ -2517,7 +2664,7 @@ module Gitlab
           it 'returns errors and empty configuration' do
             expect(subject.valid?).to eq(false)
             expect(subject.errors).to eq(['Invalid configuration format'])
-            expect(subject.content).to be_blank
+            expect(subject.config).to be_blank
           end
         end
 
@@ -2527,7 +2674,7 @@ module Gitlab
           it 'returns errors and empty configuration' do
             expect(subject.valid?).to eq(false)
             expect(subject.errors).to eq(['jobs:rspec:tags config should be an array of strings'])
-            expect(subject.content).to be_blank
+            expect(subject.config).to be_blank
           end
         end
 
@@ -2539,7 +2686,7 @@ module Gitlab
             expect(subject.errors).to contain_exactly(
               'jobs:rspec config contains unknown keys: bad_tags',
               'jobs:rspec rules should be an array of hashes')
-            expect(subject.content).to be_blank
+            expect(subject.config).to be_blank
           end
         end
 
@@ -2549,7 +2696,7 @@ module Gitlab
           it 'returns errors and empty configuration' do
             expect(subject.valid?).to eq(false)
             expect(subject.errors).to eq(['Please provide content of .gitlab-ci.yml'])
-            expect(subject.content).to be_blank
+            expect(subject.config).to be_blank
           end
         end
 
@@ -2559,7 +2706,7 @@ module Gitlab
           it 'returns errors and empty configuration' do
             expect(subject.valid?).to eq(false)
             expect(subject.errors).to eq(['Unknown alias: bad_alias'])
-            expect(subject.content).to be_blank
+            expect(subject.config).to be_blank
           end
         end
 
@@ -2569,7 +2716,7 @@ module Gitlab
           it 'returns errors and empty configuration' do
             expect(subject.valid?).to eq(true)
             expect(subject.errors).to be_empty
-            expect(subject.content).to be_present
+            expect(subject.config).to be_present
           end
         end
       end

spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::CreatePipelineService do
+  describe 'creation errors and warnings' do
+    let_it_be(:user)    { create(:admin) }
+    let_it_be(:project) { create(:project, :repository, creator: user) }
+
+    let(:ref)      { 'refs/heads/master' }
+    let(:source)   { :push }
+    let(:service)  { described_class.new(project, user, { ref: ref }) }
+    let(:pipeline) { service.execute(source) }
+
+    before do
+      stub_ci_pipeline_yaml_file(config)
+      stub_feature_flags(ci_raise_job_rules_without_workflow_rules_warning: true)
+    end
+
+    context 'when created successfully' do
+      context 'when warnings are raised' do
+        let(:config) do
+          <<~YAML
+            test:
+              script: rspec
+              rules:
+                - if: '$CI_COMMIT_BRANCH'
+          YAML
+        end
+
+        it 'contains only warnings' 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`'
+          )
+        end
+
+        context 'when feature flag is disabled for the particular warning' do
+          before do
+            stub_feature_flags(ci_raise_job_rules_without_workflow_rules_warning: false)
+          end
+
+          it 'does not contain warnings' do
+            expect(pipeline.error_messages.map(&:content)).to be_empty
+
+            expect(pipeline.warning_messages.map(&:content)).to be_empty
+          end
+        end
+      end
+
+      context 'when no warnings are raised' do
+        let(:config) do
+          <<~YAML
+            test:
+              script: rspec
+          YAML
+        end
+
+        it 'contains no warnings' do
+          expect(pipeline.error_messages).to be_empty
+
+          expect(pipeline.warning_messages).to be_empty
+        end
+      end
+    end
+
+    context 'when failed to create the pipeline' do
+      context 'when warnings are raised' do
+        let(:config) do
+          <<~YAML
+            build:
+              stage: build
+              script: echo
+              needs: [test]
+            test:
+              stage: test
+              script: echo
+              rules:
+                - if: '$CI_COMMIT_BRANCH'
+          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`'
+
+          expect(pipeline.yaml_errors).to eq(error_message)
+          expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message)
+          expect(pipeline.errors.full_messages).to contain_exactly(error_message)
+
+          expect(pipeline.warning_messages.map(&:content)).to contain_exactly(warning_message)
+        end
+      end
+
+      context 'when no warnings are raised' do
+        let(:config) do
+          <<~YAML
+            invalid: yaml
+          YAML
+        end
+
+        it 'contains only errors' do
+          error_message = 'root config contains unknown keys: invalid'
+          expect(pipeline.yaml_errors).to eq(error_message)
+          expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message)
+          expect(pipeline.errors.full_messages).to contain_exactly(error_message)
+
+          expect(pipeline.warning_messages).to be_empty
+        end
+      end
+    end
+  end
+end