Merge branch 'improve-error-messages-for-ci-job-entries' into 'master'

Improve error message for job configs that fail to match an entry

See merge request gitlab-org/gitlab!40120

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

@@ -122,39 +122,9 @@ module Gitlab
                      :needs, :retry, :parallel, :start_in,
                      :interruptible, :timeout, :resource_group, :release
 
-          Matcher = Struct.new(:name, :config) do
-            def applies?
-              job_is_not_hidden? &&
-              config_is_a_hash? &&
-              has_job_keys?
-            end
-
-            private
-
-            def job_is_not_hidden?
-              !name.to_s.start_with?('.')
-            end
-
-            def config_is_a_hash?
-              config.is_a?(Hash)
-            end
-
-            def has_job_keys?
-              if name == :default
-                config.key?(:script)
-              else
-                (ALLOWED_KEYS & config.keys).any?
-              end
-            end
-          end
-
           def self.matching?(name, config)
-            if Gitlab::Ci::Features.job_entry_matches_all_keys?
-              Matcher.new(name, config).applies?
-            else
-              !name.to_s.start_with?('.') &&
-                config.is_a?(Hash) && config.key?(:script)
-            end
+            !name.to_s.start_with?('.') &&
+              config.is_a?(Hash) && config.key?(:script)
           end
 
           def self.visible?

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

@@ -14,8 +14,8 @@ module Gitlab
             validates :config, type: Hash
 
             validate do
-              unless has_valid_jobs?
-                errors.add(:config, 'should contain valid jobs')
+              each_unmatched_job do |name|
+                errors.add(name, 'config should implement a script: or a trigger: keyword')
               end
 
               unless has_visible_job?
@@ -23,9 +23,9 @@ module Gitlab
               end
             end
 
-            def has_valid_jobs?
-              config.all? do |name, value|
-                Jobs.find_type(name, value)
+            def each_unmatched_job
+              config.each do |name, value|
+                yield(name) unless Jobs.find_type(name, value)
               end
             end
 

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

@@ -134,7 +134,7 @@ module Gitlab
             @jobs_config = @config
               .except(*self.class.reserved_nodes_names)
               .select do |name, config|
-              Entry::Jobs.find_type(name, config).present?
+              Entry::Jobs.find_type(name, config).present? || ALLOWED_KEYS.exclude?(name)
             end
 
             @config = @config.except(*@jobs_config.keys)

lib/gitlab/ci/features.rb

@@ -64,10 +64,6 @@ module Gitlab
         ::Feature.enabled?(:ci_plan_needs_size_limit, project, default_enabled: true)
       end
 
-      def self.job_entry_matches_all_keys?
-        ::Feature.enabled?(:ci_job_entry_matches_all_keys)
-      end
-
       def self.lint_creates_pipeline_with_dry_run?(project)
         ::Feature.enabled?(:ci_lint_creates_pipeline_with_dry_run, project, default_enabled: true)
       end

spec/controllers/projects/ci/lints_controller_spec.rb

@@ -150,7 +150,10 @@ RSpec.describe Projects::Ci::LintsController do
       it 'assigns result with errors' do
         subject
 
-        expect(assigns[:result].errors).to eq(['root config contains unknown keys: rubocop'])
+        expect(assigns[:result].errors).to match_array([
+          'jobs rubocop config should implement a script: or a trigger: keyword',
+          'jobs config should contain at least one visible job'
+        ])
       end
 
       context 'with dry_run mode' do
@@ -159,7 +162,7 @@ RSpec.describe Projects::Ci::LintsController do
         it 'assigns result with errors' do
           subject
 
-          expect(assigns[:result].errors).to eq(['root config contains unknown keys: rubocop'])
+          expect(assigns[:result].errors).to eq(['jobs rubocop config should implement a script: or a trigger: keyword'])
         end
       end
     end

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

@@ -74,16 +74,6 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
       it { is_expected.to be_falsey }
     end
 
-    context 'when config does not contain script' do
-      let(:name) { :build }
-
-      let(:config) do
-        { before_script: "cd ${PROJ_DIR} " }
-      end
-
-      it { is_expected.to be_truthy }
-    end
-
     context 'when using the default job without script' do
       let(:name) { :default }
       let(:config) do
@@ -104,14 +94,6 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
 
       it { is_expected.to be_truthy }
     end
-
-    context 'there are no shared keys between jobs and bridges' do
-      subject(:shared_values) do
-        described_class::ALLOWED_KEYS & Gitlab::Ci::Config::Entry::Bridge::ALLOWED_KEYS
-      end
-
-      it { is_expected.to be_empty }
-    end
   end
 
   describe 'validations' do

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

@@ -68,7 +68,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Jobs do
           let(:config) { { rspec: nil } }
 
           it 'reports error' do
-            expect(entry.errors).to include "jobs config should contain valid jobs"
+            expect(entry.errors).to include 'jobs rspec config should implement a script: or a trigger: keyword'
           end
         end
 

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

@@ -344,9 +344,9 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
       end
 
       describe '#errors' do
-        it 'reports errors about missing script' do
+        it 'reports errors about missing script or trigger' do
           expect(root.errors)
-            .to include "root config contains unknown keys: rspec"
+            .to include 'jobs rspec config should implement a script: or a trigger: keyword'
         end
       end
     end

spec/lib/gitlab/ci/lint_spec.rb

@@ -72,7 +72,7 @@ RSpec.describe Gitlab::Ci::Lint do
 
         it 'returns a result with errors' do
           expect(subject).not_to be_valid
-          expect(subject.errors).to include(/root config contains unknown keys/)
+          expect(subject.errors).to include(/jobs build config should implement a script: or a trigger: keyword/)
         end
       end
 

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -2409,7 +2409,7 @@ module Gitlab
         context 'returns error if job configuration is invalid' do
           let(:config) { YAML.dump({ extra: "bundle update" }) }
 
-          it_behaves_like 'returns errors', 'root config contains unknown keys: extra'
+          it_behaves_like 'returns errors', 'jobs extra config should implement a script: or a trigger: keyword'
         end
 
         context 'returns errors if services configuration is not correct' do
@@ -2427,7 +2427,7 @@ module Gitlab
         context 'returns errors if the job script is not defined' do
           let(:config) { YAML.dump({ rspec: { before_script: "test" } }) }
 
-          it_behaves_like 'returns errors', "jobs:rspec script can't be blank"
+          it_behaves_like 'returns errors', 'jobs rspec config should implement a script: or a trigger: keyword'
         end
 
         context 'returns errors if there are no visible jobs defined' do

spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb

@@ -101,7 +101,7 @@ RSpec.describe Ci::CreatePipelineService do
         end
 
         it 'contains only errors' do
-          error_message = 'root config contains unknown keys: invalid'
+          error_message = 'jobs invalid config should implement a script: or a trigger: keyword'
           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)