Merge branch 'fix/require-build-script-configuration-entry' into 'master' issues-copy

Make job script a required configuration entry

## What does this MR do?

This MR makes a job script a required configuration entry.

## Does this MR meet the acceptance criteria?

- [x] [Changelog entry](https://docs.gitlab.com/ce/development/changelog.html) added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing

## What are the relevant issue numbers?

Closes #24575

See merge request !7566

7 files changed, 97 insertions, 58 deletions

diff --git a/changelogs/unreleased/fix-require-build-script-configuration-entry.yml b/changelogs/unreleased/fix-require-build-script-configuration-entry.yml
new file mode 100644
index 00000000000..00b3fd2681f
--- /dev/null
+++ b/changelogs/unreleased/fix-require-build-script-configuration-entry.yml
@@ -0,0 +1,4 @@
+---
+title: Make job script a required configuration entry
+merge_request: 7566
+author: 
diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb
index 06599238d22..f7ff7ea212e 100644
--- a/lib/gitlab/ci/config.rb
+++ b/lib/gitlab/ci/config.rb
@@ -4,12 +4,6 @@ module Gitlab
     # Base GitLab CI Configuration facade
     #
     class Config
-      ##
-      # Temporary delegations that should be removed after refactoring
-      #
-      delegate :before_script, :image, :services, :after_script, :variables,
-               :stages, :cache, :jobs, to: :@global
-
       def initialize(config)
         @config = Loader.new(config).load!
 
@@ -28,6 +22,41 @@ module Gitlab
       def to_hash
         @config
       end
+
+      ##
+      # Temporary method that should be removed after refactoring
+      #
+      def before_script
+        @global.before_script_value
+      end
+
+      def image
+        @global.image_value
+      end
+
+      def services
+        @global.services_value
+      end
+
+      def after_script
+        @global.after_script_value
+      end
+
+      def variables
+        @global.variables_value
+      end
+
+      def stages
+        @global.stages_value
+      end
+
+      def cache
+        @global.cache_value
+      end
+
+      def jobs
+        @global.jobs_value
+      end
     end
   end
 end
diff --git a/lib/gitlab/ci/config/entry/configurable.rb b/lib/gitlab/ci/config/entry/configurable.rb
index 0f438faeda2..833ae4a0ff3 100644
--- a/lib/gitlab/ci/config/entry/configurable.rb
+++ b/lib/gitlab/ci/config/entry/configurable.rb
@@ -66,8 +66,6 @@ module Gitlab
 
                   @entries[symbol].value
                 end
-
-                alias_method symbol.to_sym, "#{symbol}_value".to_sym
               end
             end
           end
diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index ab4ef333629..20dcc024b4e 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -13,12 +13,10 @@ module Gitlab
                             type stage when artifacts cache dependencies before_script
                             after_script variables environment]
 
-          attributes :tags, :allow_failure, :when, :dependencies
-
           validations do
             validates :config, allowed_keys: ALLOWED_KEYS
-
             validates :config, presence: true
+            validates :script, presence: true
             validates :name, presence: true
             validates :name, type: Symbol
 
@@ -77,6 +75,8 @@ module Gitlab
                   :cache, :image, :services, :only, :except, :variables,
                   :artifacts, :commands, :environment
 
+          attributes :script, :tags, :allow_failure, :when, :dependencies
+
           def compose!(deps = nil)
             super do
               if type_defined? && !stage_defined?
@@ -118,20 +118,20 @@ module Gitlab
 
           def to_hash
             { name: name,
-              before_script: before_script,
-              script: script,
+              before_script: before_script_value,
+              script: script_value,
               commands: commands,
-              image: image,
-              services: services,
-              stage: stage,
-              cache: cache,
-              only: only,
-              except: except,
-              variables: variables_defined? ? variables : nil,
-              environment: environment_defined? ? environment : nil,
-              environment_name: environment_defined? ? environment[:name] : nil,
-              artifacts: artifacts,
-              after_script: after_script }
+              image: image_value,
+              services: services_value,
+              stage: stage_value,
+              cache: cache_value,
+              only: only_value,
+              except: except_value,
+              variables: variables_defined? ? variables_value : nil,
+              environment: environment_defined? ? environment_value : nil,
+              environment_name: environment_defined? ? environment_value[:name] : nil,
+              artifacts: artifacts_value,
+              after_script: after_script_value }
           end
         end
       end
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index 84f21631719..ff5dcc06ab3 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -1124,8 +1124,8 @@ EOT
         end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:extra config should be a hash")
       end
 
-      it "returns errors if there are unknown parameters that are hashes, but doesn't have a script" do
-        config = YAML.dump({ extra: { services: "test" } })
+      it "returns errors if services configuration is not correct" do
+        config = YAML.dump({ extra: { script: 'rspec', services: "test" } })
         expect do
           GitlabCiYamlProcessor.new(config, path)
         end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:extra:services config should be an array of strings")
diff --git a/spec/lib/gitlab/ci/config/entry/global_spec.rb b/spec/lib/gitlab/ci/config/entry/global_spec.rb
index c7726adfd27..5e5c5dcc385 100644
--- a/spec/lib/gitlab/ci/config/entry/global_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/global_spec.rb
@@ -60,9 +60,9 @@ describe Gitlab::Ci::Config::Entry::Global do
       end
 
       context 'when not composed' do
-        describe '#before_script' do
+        describe '#before_script_value' do
           it 'returns nil' do
-            expect(global.before_script).to be nil
+            expect(global.before_script_value).to be nil
           end
         end
 
@@ -82,40 +82,40 @@ describe Gitlab::Ci::Config::Entry::Global do
           end
         end
 
-        describe '#before_script' do
+        describe '#before_script_value' do
           it 'returns correct script' do
-            expect(global.before_script).to eq ['ls', 'pwd']
+            expect(global.before_script_value).to eq ['ls', 'pwd']
           end
         end
 
-        describe '#image' do
+        describe '#image_value' do
           it 'returns valid image' do
-            expect(global.image).to eq 'ruby:2.2'
+            expect(global.image_value).to eq 'ruby:2.2'
           end
         end
 
-        describe '#services' do
+        describe '#services_value' do
           it 'returns array of services' do
-            expect(global.services).to eq ['postgres:9.1', 'mysql:5.5']
+            expect(global.services_value).to eq ['postgres:9.1', 'mysql:5.5']
           end
         end
 
-        describe '#after_script' do
+        describe '#after_script_value' do
           it 'returns after script' do
-            expect(global.after_script).to eq ['make clean']
+            expect(global.after_script_value).to eq ['make clean']
           end
         end
 
-        describe '#variables' do
+        describe '#variables_value' do
           it 'returns variables' do
-            expect(global.variables).to eq(VAR: 'value')
+            expect(global.variables_value).to eq(VAR: 'value')
           end
         end
 
-        describe '#stages' do
+        describe '#stages_value' do
           context 'when stages key defined' do
             it 'returns array of stages' do
-              expect(global.stages).to eq %w[build pages]
+              expect(global.stages_value).to eq %w[build pages]
             end
           end
 
@@ -126,21 +126,21 @@ describe Gitlab::Ci::Config::Entry::Global do
             end
 
             it 'returns array of types as stages' do
-              expect(global.stages).to eq %w[test deploy]
+              expect(global.stages_value).to eq %w[test deploy]
             end
           end
         end
 
-        describe '#cache' do
+        describe '#cache_value' do
           it 'returns cache configuration' do
-            expect(global.cache)
+            expect(global.cache_value)
               .to eq(key: 'k', untracked: true, paths: ['public/'])
           end
         end
 
-        describe '#jobs' do
+        describe '#jobs_value' do
           it 'returns jobs configuration' do
-            expect(global.jobs).to eq(
+            expect(global.jobs_value).to eq(
               rspec: { name: :rspec,
                        script: %w[rspec ls],
                        before_script: ['ls', 'pwd'],
@@ -185,21 +185,21 @@ describe Gitlab::Ci::Config::Entry::Global do
         end
       end
 
-      describe '#variables' do
+      describe '#variables_value' do
         it 'returns default value for variables' do
-          expect(global.variables).to eq({})
+          expect(global.variables_value).to eq({})
         end
       end
 
-      describe '#stages' do
+      describe '#stages_value' do
         it 'returns an array of default stages' do
-          expect(global.stages).to eq %w[build test deploy]
+          expect(global.stages_value).to eq %w[build test deploy]
         end
       end
 
-      describe '#cache' do
+      describe '#cache_value' do
         it 'returns correct cache definition' do
-          expect(global.cache).to eq(key: 'a')
+          expect(global.cache_value).to eq(key: 'a')
         end
       end
     end
@@ -217,9 +217,9 @@ describe Gitlab::Ci::Config::Entry::Global do
         { variables: nil, rspec: { script: 'rspec' } }
       end
 
-      describe '#variables' do
+      describe '#variables_value' do
         it 'undefined entry returns a default value' do
-          expect(global.variables).to eq({})
+          expect(global.variables_value).to eq({})
         end
       end
     end
@@ -245,9 +245,9 @@ describe Gitlab::Ci::Config::Entry::Global do
       end
     end
 
-    describe '#before_script' do
+    describe '#before_script_value' do
       it 'returns nil' do
-        expect(global.before_script).to be_nil
+        expect(global.before_script_value).to be_nil
       end
     end
   end
diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index c05711b6338..fc9b8b86dc4 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb
@@ -19,8 +19,7 @@ describe Gitlab::Ci::Config::Entry::Job do
         let(:entry) { described_class.new(config, name: ''.to_sym) }
 
         it 'reports error' do
-          expect(entry.errors)
-            .to include "job name can't be blank"
+          expect(entry.errors).to include "job name can't be blank"
         end
       end
     end
@@ -56,6 +55,15 @@ describe Gitlab::Ci::Config::Entry::Job do
           end
         end
       end
+
+      context 'when script is not provided' do
+        let(:config) { { stage: 'test' } }
+
+        it 'returns error about missing script entry' do
+          expect(entry).not_to be_valid
+          expect(entry.errors).to include "job script can't be blank"
+        end
+      end
     end
   end
 
@@ -78,7 +86,7 @@ describe Gitlab::Ci::Config::Entry::Job do
       before { entry.compose!(deps) }
 
       let(:config) do
-        { image: 'some_image', cache: { key: 'test' } }
+        { script: 'rspec', image: 'some_image', cache: { key: 'test' } }
       end
 
       it 'overrides global config' do