Improve code, remove unused validator, improve names

11 files changed, 22 insertions, 36 deletions

diff --git a/lib/gitlab/ci/config/node/cache.rb b/lib/gitlab/ci/config/node/cache.rb
index 21d96b220b8..b4bda2841ac 100644
--- a/lib/gitlab/ci/config/node/cache.rb
+++ b/lib/gitlab/ci/config/node/cache.rb
@@ -8,8 +8,10 @@ module Gitlab
         class Cache < Entry
           include Configurable
 
+          ALLOWED_KEYS = %i[key untracked paths]
+
           validations do
-            validates :config, allowed_keys: %i[key untracked paths]
+            validates :config, allowed_keys: ALLOWED_KEYS
           end
 
           node :key, Node::Key,
diff --git a/lib/gitlab/ci/config/node/commands.rb b/lib/gitlab/ci/config/node/commands.rb
index f7e6950001e..d7657ae314b 100644
--- a/lib/gitlab/ci/config/node/commands.rb
+++ b/lib/gitlab/ci/config/node/commands.rb
@@ -11,22 +11,20 @@ module Gitlab
           validations do
             include LegacyValidationHelpers
 
-            validate :string_or_array_of_strings
-
-            def string_or_array_of_strings
-              unless config_valid?
+            validate do
+              unless string_or_array_of_strings?(config)
                 errors.add(:config,
                            'should be a string or an array of strings')
               end
             end
 
-            def config_valid?
-              validate_string(config) || validate_array_of_strings(config)
+            def string_or_array_of_strings?(field)
+              validate_string(field) || validate_array_of_strings(field)
             end
           end
 
           def value
-            [@config].flatten
+            Array(@config)
           end
         end
       end
diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb
index 93a9a253322..aedc28fe1d0 100644
--- a/lib/gitlab/ci/config/node/configurable.rb
+++ b/lib/gitlab/ci/config/node/configurable.rb
@@ -56,10 +56,9 @@ module Gitlab
                 end
 
                 define_method("#{symbol}_value") do
-                  if @entries[symbol]
-                    return unless @entries[symbol].valid?
-                    @entries[symbol].value
-                  end
+                  return unless @entries[symbol] && @entries[symbol].valid?
+
+                  @entries[symbol].value
                 end
 
                 alias_method symbol.to_sym, "#{symbol}_value".to_sym
diff --git a/lib/gitlab/ci/config/node/global.rb b/lib/gitlab/ci/config/node/global.rb
index b545b78a940..ccd539fb003 100644
--- a/lib/gitlab/ci/config/node/global.rb
+++ b/lib/gitlab/ci/config/node/global.rb
@@ -42,7 +42,7 @@ module Gitlab
             super
 
             compose_jobs!
-            compose_stages!
+            compose_deprecated_entries!
           end
 
           def compose_jobs!
@@ -54,7 +54,7 @@ module Gitlab
             @entries[:jobs] = factory.create!
           end
 
-          def compose_stages!
+          def compose_deprecated_entries!
             ##
             # Deprecated `:types` key workaround - if types are defined and
             # stages are not defined we use types definition as stages.
diff --git a/lib/gitlab/ci/config/node/job.rb b/lib/gitlab/ci/config/node/job.rb
index ace79d829f2..e84737acbb9 100644
--- a/lib/gitlab/ci/config/node/job.rb
+++ b/lib/gitlab/ci/config/node/job.rb
@@ -66,10 +66,10 @@ module Gitlab
           node :services, Services,
             description: 'Services that will be used to execute this job.'
 
-          node :only, While,
+          node :only, Trigger,
             description: 'Refs policy this job will be executed for.'
 
-          node :except, While,
+          node :except, Trigger,
             description: 'Refs policy this job will be executed for.'
 
           node :variables, Variables,
diff --git a/lib/gitlab/ci/config/node/null.rb b/lib/gitlab/ci/config/node/null.rb
index 880d29f663d..88a5f53f13c 100644
--- a/lib/gitlab/ci/config/node/null.rb
+++ b/lib/gitlab/ci/config/node/null.rb
@@ -3,7 +3,7 @@ module Gitlab
     class Config
       module Node
         ##
-        # This class represents an undefined and unspecified node.
+        # This class represents an undefined node.
         #
         # Implements the Null Object pattern.
         #
diff --git a/lib/gitlab/ci/config/node/while.rb b/lib/gitlab/ci/config/node/trigger.rb
index 84d4352624d..d8b31975088 100644
--- a/lib/gitlab/ci/config/node/while.rb
+++ b/lib/gitlab/ci/config/node/trigger.rb
@@ -3,9 +3,9 @@ module Gitlab
     class Config
       module Node
         ##
-        # Entry that represents a ref and trigger policy for the job.
+        # Entry that represents a trigger policy for the job.
         #
-        class While < Entry
+        class Trigger < Entry
           include Validatable
 
           validations do
diff --git a/lib/gitlab/ci/config/node/undefined.rb b/lib/gitlab/ci/config/node/undefined.rb
index 84dab61e7e9..45fef8c3ae5 100644
--- a/lib/gitlab/ci/config/node/undefined.rb
+++ b/lib/gitlab/ci/config/node/undefined.rb
@@ -3,16 +3,12 @@ module Gitlab
     class Config
       module Node
         ##
-        # This class represents an undefined and unspecified entry node.
+        # This class represents an unspecified entry node.
         #
         # It decorates original entry adding method that indicates it is
         # unspecified.
         #
         class Undefined < SimpleDelegator
-          def initialize(entry)
-            super
-          end
-
           def specified?
             false
           end
diff --git a/lib/gitlab/ci/config/node/validators.rb b/lib/gitlab/ci/config/node/validators.rb
index 23d5faf6f07..e20908ad3cb 100644
--- a/lib/gitlab/ci/config/node/validators.rb
+++ b/lib/gitlab/ci/config/node/validators.rb
@@ -44,15 +44,6 @@ module Gitlab
             end
           end
 
-          class RequiredValidator < ActiveModel::EachValidator
-            def validate_each(record, attribute, value)
-              if value.nil?
-                raise Entry::InvalidError,
-                  "Entry needs #{attribute} attribute set internally."
-              end
-            end
-          end
-
           class KeyValidator < ActiveModel::EachValidator
             include LegacyValidationHelpers
 
diff --git a/spec/lib/gitlab/ci/config/node/artifacts_spec.rb b/spec/lib/gitlab/ci/config/node/artifacts_spec.rb
index beed29b18ae..c09a0a9c793 100644
--- a/spec/lib/gitlab/ci/config/node/artifacts_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/artifacts_spec.rb
@@ -8,7 +8,7 @@ describe Gitlab::Ci::Config::Node::Artifacts do
       let(:config) { { paths: %w[public/] } }
 
       describe '#value' do
-        it 'returns image string' do
+        it 'returns artifacs configuration' do
           expect(entry.value).to eq config
         end
       end
diff --git a/spec/lib/gitlab/ci/config/node/while_spec.rb b/spec/lib/gitlab/ci/config/node/trigger_spec.rb
index aac2ed7b3db..a4a3e36754e 100644
--- a/spec/lib/gitlab/ci/config/node/while_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/trigger_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Ci::Config::Node::While do
+describe Gitlab::Ci::Config::Node::Trigger do
   let(:entry) { described_class.new(config) }
 
   describe 'validations' do
@@ -48,7 +48,7 @@ describe Gitlab::Ci::Config::Node::While do
       describe '#errors' do
         it 'saves errors' do
           expect(entry.errors)
-            .to include 'while config should be an array of strings or regexps'
+            .to include 'trigger config should be an array of strings or regexps'
         end
       end
     end