Merge branch 'improve-matching-for-ci-job-entries' into 'master'

Show relevant errors when failing to match a CI job entry

See merge request gitlab-org/gitlab!36536

changelogs/unreleased/improve-matching-for-ci-job-entries.yml

+---
+title: Show relevant error messages when failing to match a CI job entry
+merge_request: 36536
+author:
+type: fixed

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

@@ -11,7 +11,7 @@ module Gitlab
         class Bridge < ::Gitlab::Config::Entry::Node
           include ::Gitlab::Ci::Config::Entry::Processable
 
-          ALLOWED_KEYS = %i[trigger allow_failure when needs].freeze
+          ALLOWED_KEYS = %i[trigger].freeze
 
           validations do
             validates :config, allowed_keys: ALLOWED_KEYS + PROCESSABLE_ALLOWED_KEYS

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

@@ -11,9 +11,8 @@ module Gitlab
           include ::Gitlab::Ci::Config::Entry::Processable
 
           ALLOWED_WHEN = %w[on_success on_failure always manual delayed].freeze
-          ALLOWED_KEYS = %i[tags script type image services
-                            allow_failure type when start_in artifacts cache
-                            dependencies before_script needs after_script
+          ALLOWED_KEYS = %i[tags script type image services start_in artifacts
+                            cache dependencies before_script after_script
                             environment coverage retry parallel interruptible timeout
                             resource_group release secrets].freeze
 
@@ -129,9 +128,39 @@ 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)
-            !name.to_s.start_with?('.') &&
-              config.is_a?(Hash) && config.key?(:script)
+            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
           end
 
           def self.visible?

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

@@ -14,7 +14,8 @@ module Gitlab
           include ::Gitlab::Config::Entry::Attributable
           include ::Gitlab::Config::Entry::Inheritable
 
-          PROCESSABLE_ALLOWED_KEYS = %i[extends stage only except rules variables inherit].freeze
+          PROCESSABLE_ALLOWED_KEYS = %i[extends stage only except rules variables
+                                        inherit allow_failure when needs].freeze
 
           included do
             validations do

lib/gitlab/ci/features.rb

@@ -77,6 +77,10 @@ module Gitlab
       def self.allow_to_create_merge_request_pipelines_in_target_project?(target_project)
         ::Feature.enabled?(:ci_allow_to_create_merge_request_pipelines_in_target_project, target_project)
       end
+
+      def self.job_entry_matches_all_keys?
+        ::Feature.enabled?(:ci_job_entry_matches_all_keys)
+      end
     end
   end
 end

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

@@ -73,6 +73,45 @@ 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
+        { before_script: "cd ${PROJ_DIR} " }
+      end
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when using the default job with script' do
+      let(:name) { :default }
+      let(:config) do
+        {
+          before_script: "cd ${PROJ_DIR} ",
+          script: "ls"
+        }
+      end
+
+      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/yaml_processor_spec.rb

@@ -2484,6 +2484,14 @@ module Gitlab
           end.to raise_error(Gitlab::Ci::YamlProcessor::ValidationError, "jobs config should contain at least one visible job")
         end
 
+        it "returns errors if the job script is not defined" do
+          config = YAML.dump({ rspec: { before_script: "test" } })
+
+          expect do
+            Gitlab::Ci::YamlProcessor.new(config)
+          end.to raise_error(Gitlab::Ci::YamlProcessor::ValidationError, "jobs:rspec script can't be blank")
+        end
+
         it "returns errors if there are no visible jobs defined" do
           config = YAML.dump({ before_script: ["bundle update"], '.hidden'.to_sym => { script: 'ls' } })
           expect do