From 583544d0899c691d5f46712ad576bdacc18259e5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Tue, 13 Aug 2019 11:47:30 +0200
Subject: [PATCH] Require `stage:` to be set with `needs:`

Since we are unsure what would be the behavior of `stage:`
when we work on DAG. Let's make `stage:` to be required
today with `needs:`.
---
 lib/gitlab/ci/config/entry/job.rb             |  3 ++
 lib/gitlab/config/entry/attributable.rb       |  4 ++
 lib/gitlab/config/entry/validators.rb         | 11 +++++
 spec/lib/gitlab/ci/config/entry/job_spec.rb   | 40 +++++++++++++++++--
 .../gitlab/config/entry/attributable_spec.rb  |  3 ++
 5 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index 2fd76bc3690..29a52b9da17 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -16,8 +16,11 @@ module Gitlab
                             dependencies needs before_script after_script variables
                             environment coverage retry parallel extends].freeze
 
+          REQUIRED_BY_NEEDS = %i[stage].freeze
+
           validations do
             validates :config, allowed_keys: ALLOWED_KEYS
+            validates :config, required_keys: REQUIRED_BY_NEEDS, if: :has_needs?
             validates :config, presence: true
             validates :script, presence: true
             validates :name, presence: true
diff --git a/lib/gitlab/config/entry/attributable.rb b/lib/gitlab/config/entry/attributable.rb
index 560fe63df0e..87bd257f69a 100644
--- a/lib/gitlab/config/entry/attributable.rb
+++ b/lib/gitlab/config/entry/attributable.rb
@@ -18,6 +18,10 @@ module Gitlab
 
                 config[attribute]
               end
+
+              define_method("has_#{attribute}?") do
+                config.is_a?(Hash) && config.key?(attribute)
+              end
             end
           end
         end
diff --git a/lib/gitlab/config/entry/validators.rb b/lib/gitlab/config/entry/validators.rb
index 6796fcce75f..0289e675c6b 100644
--- a/lib/gitlab/config/entry/validators.rb
+++ b/lib/gitlab/config/entry/validators.rb
@@ -26,6 +26,17 @@ module Gitlab
           end
         end
 
+        class RequiredKeysValidator < ActiveModel::EachValidator
+          def validate_each(record, attribute, value)
+            present_keys = options[:in] - value.try(:keys).to_a
+
+            if present_keys.any?
+              record.errors.add(attribute, "missing required keys: " +
+                present_keys.join(', '))
+            end
+          end
+        end
+
         class AllowedValuesValidator < ActiveModel::EachValidator
           def validate_each(record, attribute, value)
             unless options[:in].include?(value.to_s)
diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index 800ef122203..415ade7a096 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb
@@ -89,14 +89,23 @@ describe Gitlab::Ci::Config::Entry::Job do
 
       context 'when has needs' do
         let(:config) do
-          { script: 'echo', needs: ['another-job'] }
+          {
+            stage: 'test',
+            script: 'echo',
+            needs: ['another-job']
+          }
         end
 
         it { expect(entry).to be_valid }
 
         context 'when has dependencies' do
           let(:config) do
-            { script: 'echo', dependencies: ['another-job'], needs: ['another-job'] }
+            {
+              stage: 'test',
+              script: 'echo',
+              dependencies: ['another-job'],
+              needs: ['another-job']
+            }
           end
 
           it { expect(entry).to be_valid }
@@ -256,7 +265,11 @@ describe Gitlab::Ci::Config::Entry::Job do
       context 'when has needs' do
         context 'that are not a array of strings' do
           let(:config) do
-            { script: 'echo', needs: 'build-job' }
+            {
+              stage: 'test',
+              script: 'echo',
+              needs: 'build-job'
+            }
           end
 
           it 'returns error about invalid type' do
@@ -267,7 +280,12 @@ describe Gitlab::Ci::Config::Entry::Job do
 
         context 'when have dependencies that are not subset of needs' do
           let(:config) do
-            { script: 'echo', dependencies: ['another-job'], needs: ['build-job'] }
+            {
+              stage: 'test',
+              script: 'echo',
+              dependencies: ['another-job'],
+              needs: ['build-job']
+            }
           end
 
           it 'returns error about invalid data' do
@@ -275,6 +293,20 @@ describe Gitlab::Ci::Config::Entry::Job do
             expect(entry.errors).to include 'job dependencies the another-job should be part of needs'
           end
         end
+
+        context 'when stage: is missing' do
+          let(:config) do
+            {
+              script: 'echo',
+              needs: ['build-job']
+            }
+          end
+
+          it 'returns error about invalid data' do
+            expect(entry).not_to be_valid
+            expect(entry.errors).to include 'job config missing required keys: stage'
+          end
+        end
       end
     end
   end
diff --git a/spec/lib/gitlab/config/entry/attributable_spec.rb b/spec/lib/gitlab/config/entry/attributable_spec.rb
index 82614bb8238..6b548d5c4a8 100644
--- a/spec/lib/gitlab/config/entry/attributable_spec.rb
+++ b/spec/lib/gitlab/config/entry/attributable_spec.rb
@@ -25,7 +25,9 @@ describe Gitlab::Config::Entry::Attributable do
     end
 
     it 'returns the value of config' do
+      expect(instance).to have_name
       expect(instance.name).to eq 'some name'
+      expect(instance).to have_test
       expect(instance.test).to eq 'some test'
     end
 
@@ -42,6 +44,7 @@ describe Gitlab::Config::Entry::Attributable do
     end
 
     it 'returns nil' do
+      expect(instance).not_to have_test
       expect(instance.test).to be_nil
     end
   end