From 87637693acbd443c674bcbf26d87281156a70761 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 22 Sep 2017 12:17:01 +0200
Subject: [PATCH 01/19] Introduce CI/CD variables collection class

---
 .../project_services/kubernetes_service.rb    | 18 +++---
 lib/gitlab/ci/variables/collection.rb         | 60 ++++++++++++++++++
 .../gitlab/ci/variables/collection_spec.rb    | 63 +++++++++++++++++++
 3 files changed, 131 insertions(+), 10 deletions(-)
 create mode 100644 lib/gitlab/ci/variables/collection.rb
 create mode 100644 spec/lib/gitlab/ci/variables/collection_spec.rb

diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb
index 8ba07173c74..5c1a1063baa 100644
--- a/app/models/project_services/kubernetes_service.rb
+++ b/app/models/project_services/kubernetes_service.rb
@@ -100,19 +100,17 @@ class KubernetesService < DeploymentService
   def predefined_variables
     config = YAML.dump(kubeconfig)
 
-    variables = [
-      { key: 'KUBE_URL', value: api_url, public: true },
-      { key: 'KUBE_TOKEN', value: token, public: false },
-      { key: 'KUBE_NAMESPACE', value: actual_namespace, public: true },
-      { key: 'KUBECONFIG', value: config, public: false, file: true }
-    ]
+    variables = Gitlab::Ci::Variables::Collection.new.tap do |collection|
+      collection.append(key: 'KUBE_URL', value: api_url, public: true)
+      collection.append(key: 'KUBE_TOKEN', value: token, public: false)
+      collection.append(key: 'KUBE_NAMESPACE', value: actual_namespace, public: true)
+      collection.append(key: 'KUBECONFIG', value: config, public: false, file: true)
 
-    if ca_pem.present?
-      variables << { key: 'KUBE_CA_PEM', value: ca_pem, public: true }
-      variables << { key: 'KUBE_CA_PEM_FILE', value: ca_pem, public: true, file: true }
+      collection.append(key: 'KUBE_CA_PEM', value: ca_pem, public: true)
+      collection.append(key: 'KUBE_CA_PEM_FILE', value: ca_pem, public: true, file: true)
     end
 
-    variables
+    variables.to_hash
   end
 
   # Constructs a list of terminals from the reactive cache
diff --git a/lib/gitlab/ci/variables/collection.rb b/lib/gitlab/ci/variables/collection.rb
new file mode 100644
index 00000000000..0d830293c3c
--- /dev/null
+++ b/lib/gitlab/ci/variables/collection.rb
@@ -0,0 +1,60 @@
+module Gitlab
+  module Ci
+    module Variables
+      class Collection
+        include Enumerable
+
+        Variable = Struct.new(:key, :value, :public, :file)
+
+        def initialize(variables = [])
+          @variables = []
+
+          variables.each { |variable| append(variable) }
+        end
+
+        def append(resource)
+          @variables.append(fabricate(resource))
+        end
+
+        def each
+          @variables.each { |variable| yield variable }
+        end
+
+        def +(other)
+          self.class.new.tap do |collection|
+            self.each { |variable| collection.append(variable) }
+            other.each { |variable| collection.append(variable) }
+          end
+        end
+
+        def to_h
+          self.map do |variable|
+            variable.to_h.reject do |key, value|
+              key == :file && value == false
+            end
+          end
+        end
+
+        alias_method :to_hash, :to_h
+
+        private
+
+        def fabricate(resource)
+          case resource
+          when Hash
+            Variable.new(resource.fetch(:key),
+                         resource.fetch(:value),
+                         resource.fetch(:public, false),
+                         resource.fetch(:file, false))
+          when ::Ci::Variable
+            Variable.new(resource.key, resource.value, false, false)
+          when Collection::Variable
+            resource.dup
+          else
+            raise ArgumentError, 'Unknown CI/CD variable resource!'
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb
new file mode 100644
index 00000000000..0ded8581a86
--- /dev/null
+++ b/spec/lib/gitlab/ci/variables/collection_spec.rb
@@ -0,0 +1,63 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Variables::Collection do
+  describe '.new' do
+    it 'can be initialized with an array' do
+      variable = { key: 'SOME_VAR', value: 'Some Value'}
+      collection = described_class.new([variable])
+
+      expect(collection.first.to_h).to include variable
+    end
+
+    it 'can be initialized without an argument' do
+      expect(subject).to be_none
+    end
+  end
+
+  describe '#append' do
+    it 'appends a hash' do
+      subject.append(key: 'VARIABLE', value: 'something')
+
+      expect(subject).to be_one
+    end
+
+    it 'appends a Ci::Variable' do
+      subject.append(build(:ci_variable))
+
+      expect(subject).to be_one
+    end
+
+    it 'appends an internal resource' do
+      collection = described_class.new([{ key: 'TEST', value: 1 }])
+
+      subject.append(collection.first)
+
+      expect(subject).to be_one
+    end
+  end
+
+  describe '#+' do
+    it 'makes it possible to combine with an array' do
+      collection = described_class.new([{ key: 'TEST', value: 1 }])
+      variables = [{ key: 'TEST', value: 'something'}]
+
+      expect((collection + variables).count).to eq 2
+    end
+
+    it 'makes it possible to combine with another collection' do
+      collection = described_class.new([{ key: 'TEST', value: 1 }])
+      other = described_class.new([{ key: 'TEST', value: 2 }])
+
+      expect((collection + other).count).to eq 2
+    end
+  end
+
+  describe '#to_hash' do
+    it 'creates a hash / value mapping' do
+      collection = described_class.new([{ key: 'TEST', value: 1 }])
+
+      expect(collection.to_hash)
+        .to eq [{ key: 'TEST', value: 1, public: false }]
+    end
+  end
+end

From e85e1dbb57af835945b37dc03d1f850cbdaf4d82 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 22 Sep 2017 13:46:20 +0200
Subject: [PATCH 02/19] Fix hash syntax in variables collection class

---
 spec/lib/gitlab/ci/variables/collection_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb
index 0ded8581a86..39ed1cb4c26 100644
--- a/spec/lib/gitlab/ci/variables/collection_spec.rb
+++ b/spec/lib/gitlab/ci/variables/collection_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe Gitlab::Ci::Variables::Collection do
   describe '.new' do
     it 'can be initialized with an array' do
-      variable = { key: 'SOME_VAR', value: 'Some Value'}
+      variable = { key: 'SOME_VAR', value: 'Some Value' }
       collection = described_class.new([variable])
 
       expect(collection.first.to_h).to include variable
@@ -39,7 +39,7 @@ describe Gitlab::Ci::Variables::Collection do
   describe '#+' do
     it 'makes it possible to combine with an array' do
       collection = described_class.new([{ key: 'TEST', value: 1 }])
-      variables = [{ key: 'TEST', value: 'something'}]
+      variables = [{ key: 'TEST', value: 'something' }]
 
       expect((collection + variables).count).to eq 2
     end

From a194b1982426b5f2411ee4a62ac3432de293528b Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 7 Mar 2018 10:09:53 +0100
Subject: [PATCH 03/19] Make variables collection to runner mapping explicit

---
 app/models/project_services/kubernetes_service.rb | 2 +-
 lib/gitlab/ci/variables/collection.rb             | 4 +---
 spec/lib/gitlab/ci/variables/collection_spec.rb   | 6 +++---
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb
index c1c68be5ed9..edf80bbd509 100644
--- a/app/models/project_services/kubernetes_service.rb
+++ b/app/models/project_services/kubernetes_service.rb
@@ -115,7 +115,7 @@ class KubernetesService < DeploymentService
       collection.append(key: 'KUBE_CA_PEM_FILE', value: ca_pem, public: true, file: true)
     end
 
-    variables.to_hash
+    variables.to_runner_variables
   end
 
   # Constructs a list of terminals from the reactive cache
diff --git a/lib/gitlab/ci/variables/collection.rb b/lib/gitlab/ci/variables/collection.rb
index 0d830293c3c..e9aa7f9a31c 100644
--- a/lib/gitlab/ci/variables/collection.rb
+++ b/lib/gitlab/ci/variables/collection.rb
@@ -27,7 +27,7 @@ module Gitlab
           end
         end
 
-        def to_h
+        def to_runner_variables
           self.map do |variable|
             variable.to_h.reject do |key, value|
               key == :file && value == false
@@ -35,8 +35,6 @@ module Gitlab
           end
         end
 
-        alias_method :to_hash, :to_h
-
         private
 
         def fabricate(resource)
diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb
index 39ed1cb4c26..16c23ae450a 100644
--- a/spec/lib/gitlab/ci/variables/collection_spec.rb
+++ b/spec/lib/gitlab/ci/variables/collection_spec.rb
@@ -52,11 +52,11 @@ describe Gitlab::Ci::Variables::Collection do
     end
   end
 
-  describe '#to_hash' do
-    it 'creates a hash / value mapping' do
+  describe '#to_runner_variables' do
+    it 'creates an array of hashes in a runner-compatible format' do
       collection = described_class.new([{ key: 'TEST', value: 1 }])
 
-      expect(collection.to_hash)
+      expect(collection.to_runner_variables)
         .to eq [{ key: 'TEST', value: 1, public: false }]
     end
   end

From 9d4c9272e016442cd84fbada82493b03b350bb8e Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 12 Mar 2018 11:43:24 +0100
Subject: [PATCH 04/19] Document runner's expecations regarding variable
 collection

---
 lib/gitlab/ci/variables/collection.rb | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lib/gitlab/ci/variables/collection.rb b/lib/gitlab/ci/variables/collection.rb
index e9aa7f9a31c..2f5987f2846 100644
--- a/lib/gitlab/ci/variables/collection.rb
+++ b/lib/gitlab/ci/variables/collection.rb
@@ -27,10 +27,15 @@ module Gitlab
           end
         end
 
+        ##
+        # If `file: true` has been provided we expose it, otherwise we
+        # don't expose `file` attribute at all (stems from what the runner
+        # expects).
+        #
         def to_runner_variables
           self.map do |variable|
-            variable.to_h.reject do |key, value|
-              key == :file && value == false
+            variable.to_h.reject do |component, value|
+              component == :file && value == false
             end
           end
         end
@@ -40,10 +45,10 @@ module Gitlab
         def fabricate(resource)
           case resource
           when Hash
-            Variable.new(resource.fetch(:key),
-                         resource.fetch(:value),
-                         resource.fetch(:public, false),
-                         resource.fetch(:file, false))
+            Collection::Variable.new(resource.fetch(:key),
+                                     resource.fetch(:value),
+                                     resource.fetch(:public, false),
+                                     resource.fetch(:file, false))
           when ::Ci::Variable
             Variable.new(resource.key, resource.value, false, false)
           when Collection::Variable

From 5ed8286e742597720d48e3bf6fc56938ba717f5e Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 12 Mar 2018 13:58:54 +0100
Subject: [PATCH 05/19] Extract variables collection item to a separate class

---
 app/models/ci/variable.rb                     |  4 ++
 .../project_services/kubernetes_service.rb    |  4 +-
 lib/gitlab/ci/variables/collection.rb         | 33 +------------
 lib/gitlab/ci/variables/collection/item.rb    | 46 ++++++++++++++++++
 .../ci/variables/collection/item_spec.rb      | 47 +++++++++++++++++++
 .../gitlab/ci/variables/collection_spec.rb    |  5 +-
 6 files changed, 103 insertions(+), 36 deletions(-)
 create mode 100644 lib/gitlab/ci/variables/collection/item.rb
 create mode 100644 spec/lib/gitlab/ci/variables/collection/item_spec.rb

diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb
index 7c71291de84..51389fb82bf 100644
--- a/app/models/ci/variable.rb
+++ b/app/models/ci/variable.rb
@@ -12,5 +12,9 @@ module Ci
     }
 
     scope :unprotected, -> { where(protected: false) }
+
+    def to_hash
+      { key: key, value: value, public: false }
+    end
   end
 end
diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb
index edf80bbd509..347c9f34033 100644
--- a/app/models/project_services/kubernetes_service.rb
+++ b/app/models/project_services/kubernetes_service.rb
@@ -105,7 +105,7 @@ class KubernetesService < DeploymentService
   def predefined_variables
     config = YAML.dump(kubeconfig)
 
-    variables = Gitlab::Ci::Variables::Collection.new.tap do |collection|
+    Gitlab::Ci::Variables::Collection.new.tap do |collection|
       collection.append(key: 'KUBE_URL', value: api_url, public: true)
       collection.append(key: 'KUBE_TOKEN', value: token, public: false)
       collection.append(key: 'KUBE_NAMESPACE', value: actual_namespace, public: true)
@@ -114,8 +114,6 @@ class KubernetesService < DeploymentService
       collection.append(key: 'KUBE_CA_PEM', value: ca_pem, public: true)
       collection.append(key: 'KUBE_CA_PEM_FILE', value: ca_pem, public: true, file: true)
     end
-
-    variables.to_runner_variables
   end
 
   # Constructs a list of terminals from the reactive cache
diff --git a/lib/gitlab/ci/variables/collection.rb b/lib/gitlab/ci/variables/collection.rb
index 2f5987f2846..83db5492d43 100644
--- a/lib/gitlab/ci/variables/collection.rb
+++ b/lib/gitlab/ci/variables/collection.rb
@@ -4,8 +4,6 @@ module Gitlab
       class Collection
         include Enumerable
 
-        Variable = Struct.new(:key, :value, :public, :file)
-
         def initialize(variables = [])
           @variables = []
 
@@ -13,7 +11,7 @@ module Gitlab
         end
 
         def append(resource)
-          @variables.append(fabricate(resource))
+          @variables.append(Collection::Item.fabricate(resource))
         end
 
         def each
@@ -27,35 +25,8 @@ module Gitlab
           end
         end
 
-        ##
-        # If `file: true` has been provided we expose it, otherwise we
-        # don't expose `file` attribute at all (stems from what the runner
-        # expects).
-        #
         def to_runner_variables
-          self.map do |variable|
-            variable.to_h.reject do |component, value|
-              component == :file && value == false
-            end
-          end
-        end
-
-        private
-
-        def fabricate(resource)
-          case resource
-          when Hash
-            Collection::Variable.new(resource.fetch(:key),
-                                     resource.fetch(:value),
-                                     resource.fetch(:public, false),
-                                     resource.fetch(:file, false))
-          when ::Ci::Variable
-            Variable.new(resource.key, resource.value, false, false)
-          when Collection::Variable
-            resource.dup
-          else
-            raise ArgumentError, 'Unknown CI/CD variable resource!'
-          end
+          self.map(&:to_hash)
         end
       end
     end
diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb
new file mode 100644
index 00000000000..4de96e97417
--- /dev/null
+++ b/lib/gitlab/ci/variables/collection/item.rb
@@ -0,0 +1,46 @@
+module Gitlab
+  module Ci
+    module Variables
+      class Collection
+        class Item
+          def initialize(**options)
+            @variable = {
+              key: options.fetch(:key),
+              value: options.fetch(:value),
+              public: options.fetch(:public, false),
+              file: options.fetch(:files, false)
+            }
+          end
+
+          def ==(other)
+            to_hash == self.class.fabricate(other).to_hash
+          end
+
+          ##
+          # If `file: true` has been provided we expose it, otherwise we
+          # don't expose `file` attribute at all (stems from what the runner
+          # expects).
+          #
+          def to_hash
+            @variable.reject do |hash_key, hash_value|
+              hash_key == :file && hash_value == false
+            end
+          end
+
+          def self.fabricate(resource)
+            case resource
+            when Hash
+              self.new(resource)
+            when ::Ci::Variable
+              self.new(resource.to_hash)
+            when self
+              resource.dup
+            else
+              raise ArgumentError, 'Unknown CI/CD variable resource!'
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/variables/collection/item_spec.rb b/spec/lib/gitlab/ci/variables/collection/item_spec.rb
new file mode 100644
index 00000000000..6f86d658f52
--- /dev/null
+++ b/spec/lib/gitlab/ci/variables/collection/item_spec.rb
@@ -0,0 +1,47 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Variables::Collection::Item do
+  let(:variable) do
+    { key: 'VAR', value: 'something', public: true }
+  end
+
+  describe '.fabricate' do
+    it 'supports using a hash' do
+      resource = described_class.fabricate(variable)
+
+      expect(resource).to be_a(described_class)
+      expect(resource).to eq variable
+    end
+
+    it 'supports using an active record resource' do
+      resource = described_class.fabricate(create(:ci_variable))
+
+      expect(resource).to be_a(described_class)
+      expect(resource).to eq(key: 'VARIABLE_1',
+                             value: 'VARIABLE_VALUE',
+                             public: false)
+    end
+
+    it 'supports using another collection item' do
+      item = described_class.new(**variable)
+
+      resource = described_class.fabricate(item)
+
+      expect(resource).to be_a(described_class)
+      expect(resource).to eq variable
+      expect(resource.object_id).not_to eq item.object_id
+    end
+  end
+
+  describe '#==' do
+    it 'compares a hash representation of a variable' do
+      expect(described_class.new(**variable) == variable).to be true
+    end
+  end
+
+  describe '#to_hash' do
+    it 'returns a hash representation of a collection item' do
+      expect(described_class.new(**variable).to_hash).to eq variable
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb
index 16c23ae450a..c4b2df7dede 100644
--- a/spec/lib/gitlab/ci/variables/collection_spec.rb
+++ b/spec/lib/gitlab/ci/variables/collection_spec.rb
@@ -3,10 +3,11 @@ require 'spec_helper'
 describe Gitlab::Ci::Variables::Collection do
   describe '.new' do
     it 'can be initialized with an array' do
-      variable = { key: 'SOME_VAR', value: 'Some Value' }
+      variable = { key: 'VAR', value: 'value', public: true }
+
       collection = described_class.new([variable])
 
-      expect(collection.first.to_h).to include variable
+      expect(collection.first.to_hash).to eq variable
     end
 
     it 'can be initialized without an argument' do

From b94db067b7d748418cdf27a49e49c1bf175b7dc8 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 12 Mar 2018 15:07:05 +0100
Subject: [PATCH 06/19] Use variables collection as build predefined variables

---
 app/models/ci/build.rb                     | 34 ++++++++++++----------
 app/models/concerns/has_variable.rb        |  6 +++-
 app/models/project.rb                      | 20 +++++++------
 lib/gitlab/ci/variables/collection.rb      |  6 +++-
 lib/gitlab/ci/variables/collection/item.rb |  4 ++-
 5 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index b230b7f47ef..d4b5d964846 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -252,23 +252,25 @@ module Ci
     # All variables, including those dependent on environment, which could
     # contain unexpanded variables.
     def variables(environment: persisted_environment)
-      variables = predefined_variables
-      variables += project.predefined_variables
-      variables += pipeline.predefined_variables
-      variables += runner.predefined_variables if runner
-      variables += project.container_registry_variables
-      variables += project.deployment_variables if has_environment?
-      variables += project.auto_devops_variables
-      variables += yaml_variables
-      variables += user_variables
-      variables += project.group.secret_variables_for(ref, project).map(&:to_runner_variable) if project.group
-      variables += secret_variables(environment: environment)
-      variables += trigger_request.user_variables if trigger_request
-      variables += pipeline.variables.map(&:to_runner_variable)
-      variables += pipeline.pipeline_schedule.job_variables if pipeline.pipeline_schedule
-      variables += persisted_environment_variables if environment
+      collection = Gitlab::Ci::Variables::Collection.new.tap do |variables|
+        variables.concat(predefined_variables)
+        variables.concat(project.predefined_variables)
+        variables.concat(pipeline.predefined_variables)
+        variables.concat(runner.predefined_variables) if runner
+        variables.concat(project.container_registry_variables)
+        variables.concat(project.deployment_variables) if has_environment?
+        variables.concat(project.auto_devops_variables)
+        variables.concat(yaml_variables)
+        variables.concat(user_variables)
+        variables.concat(project.group.secret_variables_for(ref, project).map(&:to_runner_variable)) if project.group
+        variables.concat(secret_variables(environment: environment))
+        variables.concat(trigger_request.user_variables) if trigger_request
+        variables.concat(pipeline.variables)
+        variables.concat(pipeline.pipeline_schedule.job_variables) if pipeline.pipeline_schedule
+        variables.concat(persisted_environment_variables) if environment
+      end
 
-      variables
+      collection.to_runner_variables
     end
 
     def features
diff --git a/app/models/concerns/has_variable.rb b/app/models/concerns/has_variable.rb
index 8a241e4374a..3c29e12ad71 100644
--- a/app/models/concerns/has_variable.rb
+++ b/app/models/concerns/has_variable.rb
@@ -20,8 +20,12 @@ module HasVariable
       super(new_key.to_s.strip)
     end
 
-    def to_runner_variable
+    def to_hash
       { key: key, value: value, public: false }
     end
+
+    def to_runner_variable
+      to_hash
+    end
   end
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index 5cd1da43645..9ca859c5879 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1571,15 +1571,17 @@ class Project < ActiveRecord::Base
   end
 
   def predefined_variables
-    [
-      { key: 'CI_PROJECT_ID', value: id.to_s, public: true },
-      { key: 'CI_PROJECT_NAME', value: path, public: true },
-      { key: 'CI_PROJECT_PATH', value: full_path, public: true },
-      { key: 'CI_PROJECT_PATH_SLUG', value: full_path_slug, public: true },
-      { key: 'CI_PROJECT_NAMESPACE', value: namespace.full_path, public: true },
-      { key: 'CI_PROJECT_URL', value: web_url, public: true },
-      { key: 'CI_PROJECT_VISIBILITY', value: Gitlab::VisibilityLevel.string_level(visibility_level), public: true }
-    ]
+    visibility = Gitlab::VisibilityLevel.string_level(visibility_level)
+
+    Gitlab::Ci::Variables::Collection.new.tap do |variables|
+      variables.append(key: 'CI_PROJECT_ID', value: id.to_s, public: true)
+      variables.append(key: 'CI_PROJECT_NAME', value: path, public: true)
+      variables.append(key: 'CI_PROJECT_PATH', value: full_path, public: true)
+      variables.append(key: 'CI_PROJECT_PATH_SLUG', value: full_path_slug, public: true)
+      variables.append(key: 'CI_PROJECT_NAMESPACE', value: namespace.full_path, public: true)
+      variables.append(key: 'CI_PROJECT_URL', value: web_url, public: true)
+      variables.append(key: 'CI_PROJECT_VISIBILITY', value: visibility, public: true)
+    end
   end
 
   def container_registry_variables
diff --git a/lib/gitlab/ci/variables/collection.rb b/lib/gitlab/ci/variables/collection.rb
index 83db5492d43..ae7415fcdb1 100644
--- a/lib/gitlab/ci/variables/collection.rb
+++ b/lib/gitlab/ci/variables/collection.rb
@@ -7,13 +7,17 @@ module Gitlab
         def initialize(variables = [])
           @variables = []
 
-          variables.each { |variable| append(variable) }
+          variables.each { |variable| self.append(variable) }
         end
 
         def append(resource)
           @variables.append(Collection::Item.fabricate(resource))
         end
 
+        def concat(resources)
+          resources.each { |variable| self.append(variable) }
+        end
+
         def each
           @variables.each { |variable| yield variable }
         end
diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb
index 4de96e97417..c238ac8f18e 100644
--- a/lib/gitlab/ci/variables/collection/item.rb
+++ b/lib/gitlab/ci/variables/collection/item.rb
@@ -33,10 +33,12 @@ module Gitlab
               self.new(resource)
             when ::Ci::Variable
               self.new(resource.to_hash)
+            when ::Ci::PipelineVariable
+              self.new(resource.to_hash)
             when self
               resource.dup
             else
-              raise ArgumentError, 'Unknown CI/CD variable resource!'
+              raise ArgumentError, "Unknown `#{resource.class}` variable resource!"
             end
           end
         end

From a4a29e2ee1970d939a8b9ea9853261120c2eff3d Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 12 Mar 2018 15:09:19 +0100
Subject: [PATCH 07/19] Add TODOs to refactoried variables collections code

---
 lib/gitlab/ci/variables/collection.rb      | 1 +
 lib/gitlab/ci/variables/collection/item.rb | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/lib/gitlab/ci/variables/collection.rb b/lib/gitlab/ci/variables/collection.rb
index ae7415fcdb1..97ca6dc3d3d 100644
--- a/lib/gitlab/ci/variables/collection.rb
+++ b/lib/gitlab/ci/variables/collection.rb
@@ -14,6 +14,7 @@ module Gitlab
           @variables.append(Collection::Item.fabricate(resource))
         end
 
+        # TODO, specs
         def concat(resources)
           resources.each { |variable| self.append(variable) }
         end
diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb
index c238ac8f18e..d6540e86fc5 100644
--- a/lib/gitlab/ci/variables/collection/item.rb
+++ b/lib/gitlab/ci/variables/collection/item.rb
@@ -3,6 +3,8 @@ module Gitlab
     module Variables
       class Collection
         class Item
+          # TODO, public by default?
+          #
           def initialize(**options)
             @variable = {
               key: options.fetch(:key),
@@ -28,6 +30,8 @@ module Gitlab
           end
 
           def self.fabricate(resource)
+            # TODO, to_runner_variable by default for class < ActiveRecord::Base
+            #
             case resource
             when Hash
               self.new(resource)

From 0cf0a7a898f06fc2a154683e76dc9199832c01c8 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 13 Mar 2018 14:00:14 +0100
Subject: [PATCH 08/19] DRY project-level predefined variables

---
 app/models/ci/build.rb                     |  4 +---
 app/models/project.rb                      | 18 +++++++++---------
 app/models/project_auto_devops.rb          | 10 ++++++----
 lib/gitlab/ci/variables/collection/item.rb |  6 ++++++
 spec/models/project_auto_devops_spec.rb    |  8 ++++----
 5 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index d4b5d964846..a3a289c3a12 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -257,12 +257,10 @@ module Ci
         variables.concat(project.predefined_variables)
         variables.concat(pipeline.predefined_variables)
         variables.concat(runner.predefined_variables) if runner
-        variables.concat(project.container_registry_variables)
         variables.concat(project.deployment_variables) if has_environment?
-        variables.concat(project.auto_devops_variables)
         variables.concat(yaml_variables)
         variables.concat(user_variables)
-        variables.concat(project.group.secret_variables_for(ref, project).map(&:to_runner_variable)) if project.group
+        variables.concat(project.group.secret_variables_for(ref, project)) if project.group
         variables.concat(secret_variables(environment: environment))
         variables.concat(trigger_request.user_variables) if trigger_request
         variables.concat(pipeline.variables)
diff --git a/app/models/project.rb b/app/models/project.rb
index 9ca859c5879..85a4d570e9a 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1581,21 +1581,21 @@ class Project < ActiveRecord::Base
       variables.append(key: 'CI_PROJECT_NAMESPACE', value: namespace.full_path, public: true)
       variables.append(key: 'CI_PROJECT_URL', value: web_url, public: true)
       variables.append(key: 'CI_PROJECT_VISIBILITY', value: visibility, public: true)
+      variables.concat(container_registry_variables)
+      variables.concat(auto_devops_variables)
     end
   end
 
   def container_registry_variables
-    return [] unless Gitlab.config.registry.enabled
+    Gitlab::Ci::Variables::Collection.new.tap do |variables|
+      return variables unless Gitlab.config.registry.enabled
 
-    variables = [
-      { key: 'CI_REGISTRY', value: Gitlab.config.registry.host_port, public: true }
-    ]
+      variables.append(key: 'CI_REGISTRY', value: Gitlab.config.registry.host_port, public: true)
 
-    if container_registry_enabled?
-      variables << { key: 'CI_REGISTRY_IMAGE', value: container_registry_url, public: true }
+      if container_registry_enabled?
+        variables.append(key: 'CI_REGISTRY_IMAGE', value: container_registry_url, public: true)
+      end
     end
-
-    variables
   end
 
   def secret_variables_for(ref:, environment: nil)
@@ -1624,7 +1624,7 @@ class Project < ActiveRecord::Base
   def auto_devops_variables
     return [] unless auto_devops_enabled?
 
-    (auto_devops || build_auto_devops)&.variables
+    (auto_devops || build_auto_devops)&.predefined_variables
   end
 
   def append_or_update_attribute(name, value)
diff --git a/app/models/project_auto_devops.rb b/app/models/project_auto_devops.rb
index 112ed7ed434..519bd2fe2ac 100644
--- a/app/models/project_auto_devops.rb
+++ b/app/models/project_auto_devops.rb
@@ -14,9 +14,11 @@ class ProjectAutoDevops < ActiveRecord::Base
     domain.present? || instance_domain.present?
   end
 
-  def variables
-    variables = []
-    variables << { key: 'AUTO_DEVOPS_DOMAIN', value: domain.presence || instance_domain, public: true } if has_domain?
-    variables
+  def predefined_variables
+    Gitlab::Ci::Variables::Collection.new.tap do |variables|
+      variables.append(key: 'AUTO_DEVOPS_DOMAIN',
+                       value: domain.presence || instance_domain,
+                       public: true) if has_domain?
+    end
   end
 end
diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb
index d6540e86fc5..1ed07cedd55 100644
--- a/lib/gitlab/ci/variables/collection/item.rb
+++ b/lib/gitlab/ci/variables/collection/item.rb
@@ -14,6 +14,10 @@ module Gitlab
             }
           end
 
+          def [](key)
+            @variable.fetch(key)
+          end
+
           def ==(other)
             to_hash == self.class.fabricate(other).to_hash
           end
@@ -39,6 +43,8 @@ module Gitlab
               self.new(resource.to_hash)
             when ::Ci::PipelineVariable
               self.new(resource.to_hash)
+            when ::Ci::GroupVariable
+              self.new(resource.to_hash)
             when self
               resource.dup
             else
diff --git a/spec/models/project_auto_devops_spec.rb b/spec/models/project_auto_devops_spec.rb
index 296b91a771c..7545c0797e9 100644
--- a/spec/models/project_auto_devops_spec.rb
+++ b/spec/models/project_auto_devops_spec.rb
@@ -36,14 +36,14 @@ describe ProjectAutoDevops do
     end
   end
 
-  describe '#variables' do
+  describe '#predefined_variables' do
     let(:auto_devops) { build_stubbed(:project_auto_devops, project: project, domain: domain) }
 
     context 'when domain is defined' do
       let(:domain) { 'example.com' }
 
       it 'returns AUTO_DEVOPS_DOMAIN' do
-        expect(auto_devops.variables).to include(domain_variable)
+        expect(auto_devops.predefined_variables).to include(domain_variable)
       end
     end
 
@@ -55,7 +55,7 @@ describe ProjectAutoDevops do
           allow(Gitlab::CurrentSettings).to receive(:auto_devops_domain).and_return('example.com')
         end
 
-        it { expect(auto_devops.variables).to include(domain_variable) }
+        it { expect(auto_devops.predefined_variables).to include(domain_variable) }
       end
 
       context 'when there is no instance domain specified' do
@@ -63,7 +63,7 @@ describe ProjectAutoDevops do
           allow(Gitlab::CurrentSettings).to receive(:auto_devops_domain).and_return(nil)
         end
 
-        it { expect(auto_devops.variables).not_to include(domain_variable) }
+        it { expect(auto_devops.predefined_variables).not_to include(domain_variable) }
       end
     end
 

From 937e7f9e9669d4939cd6c5055a90339ff79d216b Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 13 Mar 2018 14:01:48 +0100
Subject: [PATCH 09/19] Make predefined variables in a collection public by
 default

---
 app/models/project.rb                          | 18 +++++++++---------
 lib/gitlab/ci/variables/collection/item.rb     |  2 +-
 .../lib/gitlab/ci/variables/collection_spec.rb |  2 +-
 spec/models/ci/build_spec.rb                   |  8 ++++----
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index 85a4d570e9a..c1d8d43f380 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1574,13 +1574,13 @@ class Project < ActiveRecord::Base
     visibility = Gitlab::VisibilityLevel.string_level(visibility_level)
 
     Gitlab::Ci::Variables::Collection.new.tap do |variables|
-      variables.append(key: 'CI_PROJECT_ID', value: id.to_s, public: true)
-      variables.append(key: 'CI_PROJECT_NAME', value: path, public: true)
-      variables.append(key: 'CI_PROJECT_PATH', value: full_path, public: true)
-      variables.append(key: 'CI_PROJECT_PATH_SLUG', value: full_path_slug, public: true)
-      variables.append(key: 'CI_PROJECT_NAMESPACE', value: namespace.full_path, public: true)
-      variables.append(key: 'CI_PROJECT_URL', value: web_url, public: true)
-      variables.append(key: 'CI_PROJECT_VISIBILITY', value: visibility, public: true)
+      variables.append(key: 'CI_PROJECT_ID', value: id.to_s)
+      variables.append(key: 'CI_PROJECT_NAME', value: path)
+      variables.append(key: 'CI_PROJECT_PATH', value: full_path)
+      variables.append(key: 'CI_PROJECT_PATH_SLUG', value: full_path_slug)
+      variables.append(key: 'CI_PROJECT_NAMESPACE', value: namespace.full_path)
+      variables.append(key: 'CI_PROJECT_URL', value: web_url)
+      variables.append(key: 'CI_PROJECT_VISIBILITY', value: visibility)
       variables.concat(container_registry_variables)
       variables.concat(auto_devops_variables)
     end
@@ -1590,10 +1590,10 @@ class Project < ActiveRecord::Base
     Gitlab::Ci::Variables::Collection.new.tap do |variables|
       return variables unless Gitlab.config.registry.enabled
 
-      variables.append(key: 'CI_REGISTRY', value: Gitlab.config.registry.host_port, public: true)
+      variables.append(key: 'CI_REGISTRY', value: Gitlab.config.registry.host_port)
 
       if container_registry_enabled?
-        variables.append(key: 'CI_REGISTRY_IMAGE', value: container_registry_url, public: true)
+        variables.append(key: 'CI_REGISTRY_IMAGE', value: container_registry_url)
       end
     end
   end
diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb
index 1ed07cedd55..47e06566607 100644
--- a/lib/gitlab/ci/variables/collection/item.rb
+++ b/lib/gitlab/ci/variables/collection/item.rb
@@ -9,7 +9,7 @@ module Gitlab
             @variable = {
               key: options.fetch(:key),
               value: options.fetch(:value),
-              public: options.fetch(:public, false),
+              public: options.fetch(:public, true),
               file: options.fetch(:files, false)
             }
           end
diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb
index c4b2df7dede..9ee39d40625 100644
--- a/spec/lib/gitlab/ci/variables/collection_spec.rb
+++ b/spec/lib/gitlab/ci/variables/collection_spec.rb
@@ -58,7 +58,7 @@ describe Gitlab::Ci::Variables::Collection do
       collection = described_class.new([{ key: 'TEST', value: 1 }])
 
       expect(collection.to_runner_variables)
-        .to eq [{ key: 'TEST', value: 1, public: false }]
+        .to eq [{ key: 'TEST', value: 1, public: true }]
     end
   end
 end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index c27313ed88b..1365c1d84d6 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1835,10 +1835,10 @@ describe Ci::Build do
     end
 
     context 'returns variables in valid order' do
-      let(:build_pre_var) { { key: 'build', value: 'value' } }
-      let(:project_pre_var) { { key: 'project', value: 'value' } }
-      let(:pipeline_pre_var) { { key: 'pipeline', value: 'value' } }
-      let(:build_yaml_var) { { key: 'yaml', value: 'value' } }
+      let(:build_pre_var) { { key: 'build', value: 'value', public: true } }
+      let(:project_pre_var) { { key: 'project', value: 'value', public: true } }
+      let(:pipeline_pre_var) { { key: 'pipeline', value: 'value', public: true } }
+      let(:build_yaml_var) { { key: 'yaml', value: 'value', public: true } }
 
       before do
         allow(build).to receive(:predefined_variables) { [build_pre_var] }

From e16fba6726adcf7e82862336fab22c6c6baf2010 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 13 Mar 2018 14:12:00 +0100
Subject: [PATCH 10/19] Improve how we handle persistent runner variables

---
 app/models/ci/variable.rb                  |  4 ----
 app/models/concerns/has_variable.rb        |  6 +-----
 lib/gitlab/ci/variables/collection/item.rb | 12 ++----------
 3 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb
index 51389fb82bf..7c71291de84 100644
--- a/app/models/ci/variable.rb
+++ b/app/models/ci/variable.rb
@@ -12,9 +12,5 @@ module Ci
     }
 
     scope :unprotected, -> { where(protected: false) }
-
-    def to_hash
-      { key: key, value: value, public: false }
-    end
   end
 end
diff --git a/app/models/concerns/has_variable.rb b/app/models/concerns/has_variable.rb
index 3c29e12ad71..8a241e4374a 100644
--- a/app/models/concerns/has_variable.rb
+++ b/app/models/concerns/has_variable.rb
@@ -20,12 +20,8 @@ module HasVariable
       super(new_key.to_s.strip)
     end
 
-    def to_hash
-      { key: key, value: value, public: false }
-    end
-
     def to_runner_variable
-      to_hash
+      { key: key, value: value, public: false }
     end
   end
 end
diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb
index 47e06566607..939912981e6 100644
--- a/lib/gitlab/ci/variables/collection/item.rb
+++ b/lib/gitlab/ci/variables/collection/item.rb
@@ -3,8 +3,6 @@ module Gitlab
     module Variables
       class Collection
         class Item
-          # TODO, public by default?
-          #
           def initialize(**options)
             @variable = {
               key: options.fetch(:key),
@@ -34,17 +32,11 @@ module Gitlab
           end
 
           def self.fabricate(resource)
-            # TODO, to_runner_variable by default for class < ActiveRecord::Base
-            #
             case resource
             when Hash
               self.new(resource)
-            when ::Ci::Variable
-              self.new(resource.to_hash)
-            when ::Ci::PipelineVariable
-              self.new(resource.to_hash)
-            when ::Ci::GroupVariable
-              self.new(resource.to_hash)
+            when ::HasVariable
+              self.new(resource.to_runner_variable)
             when self
               resource.dup
             else

From d4319e1700b2eb4070a9cdac71b6f5b78ede08ff Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 13 Mar 2018 14:29:54 +0100
Subject: [PATCH 11/19] Add more unit tests for variables collection class

---
 lib/gitlab/ci/variables/collection.rb         |  1 -
 .../ci/variables/collection/item_spec.rb      |  8 ++++++
 .../gitlab/ci/variables/collection_spec.rb    | 25 +++++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/lib/gitlab/ci/variables/collection.rb b/lib/gitlab/ci/variables/collection.rb
index 97ca6dc3d3d..ae7415fcdb1 100644
--- a/lib/gitlab/ci/variables/collection.rb
+++ b/lib/gitlab/ci/variables/collection.rb
@@ -14,7 +14,6 @@ module Gitlab
           @variables.append(Collection::Item.fabricate(resource))
         end
 
-        # TODO, specs
         def concat(resources)
           resources.each { |variable| self.append(variable) }
         end
diff --git a/spec/lib/gitlab/ci/variables/collection/item_spec.rb b/spec/lib/gitlab/ci/variables/collection/item_spec.rb
index 6f86d658f52..37394c3e80f 100644
--- a/spec/lib/gitlab/ci/variables/collection/item_spec.rb
+++ b/spec/lib/gitlab/ci/variables/collection/item_spec.rb
@@ -39,6 +39,14 @@ describe Gitlab::Ci::Variables::Collection::Item do
     end
   end
 
+  describe '#[]' do
+    it 'behaves like a hash accessor' do
+      item = described_class.new(**variable)
+
+      expect(item[:key]).to eq 'VAR'
+    end
+  end
+
   describe '#to_hash' do
     it 'returns a hash representation of a collection item' do
       expect(described_class.new(**variable).to_hash).to eq variable
diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb
index 9ee39d40625..005e2bb17b4 100644
--- a/spec/lib/gitlab/ci/variables/collection_spec.rb
+++ b/spec/lib/gitlab/ci/variables/collection_spec.rb
@@ -37,6 +37,31 @@ describe Gitlab::Ci::Variables::Collection do
     end
   end
 
+  describe '#concat' do
+    it 'appends all elements from an array' do
+      collection = described_class.new([{ key: 'VAR_1', value: '1' }])
+      variables = [{ key: 'VAR_2', value: '2' }, { key: 'VAR_3', value: '3' }]
+
+      collection.concat(variables)
+
+      expect(collection).to include(key: 'VAR_1', value: '1', public: true)
+      expect(collection).to include(key: 'VAR_2', value: '2', public: true)
+      expect(collection).to include(key: 'VAR_3', value: '3', public: true)
+    end
+
+    it 'appends all elements from other collection' do
+      collection = described_class.new([{ key: 'VAR_1', value: '1' }])
+      additional = described_class.new([{ key: 'VAR_2', value: '2' },
+                                        { key: 'VAR_3', value: '3' }])
+
+      collection.concat(additional)
+
+      expect(collection).to include(key: 'VAR_1', value: '1', public: true)
+      expect(collection).to include(key: 'VAR_2', value: '2', public: true)
+      expect(collection).to include(key: 'VAR_3', value: '3', public: true)
+    end
+  end
+
   describe '#+' do
     it 'makes it possible to combine with an array' do
       collection = described_class.new([{ key: 'TEST', value: 1 }])

From 769f87eebd6a3701770f46dee1e2b09c60306f34 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 13 Mar 2018 15:26:16 +0100
Subject: [PATCH 12/19] Fix rubocop offense in auto devops predefined variables

---
 app/models/project_auto_devops.rb | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/app/models/project_auto_devops.rb b/app/models/project_auto_devops.rb
index 519bd2fe2ac..ed6c1eddbc1 100644
--- a/app/models/project_auto_devops.rb
+++ b/app/models/project_auto_devops.rb
@@ -16,9 +16,10 @@ class ProjectAutoDevops < ActiveRecord::Base
 
   def predefined_variables
     Gitlab::Ci::Variables::Collection.new.tap do |variables|
-      variables.append(key: 'AUTO_DEVOPS_DOMAIN',
-                       value: domain.presence || instance_domain,
-                       public: true) if has_domain?
+      if has_domain?
+        variables.append(key: 'AUTO_DEVOPS_DOMAIN',
+                         value: domain.presence || instance_domain)
+      end
     end
   end
 end

From 1e811eb8ac464df77d831fe8af4580dd9f890b54 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 13 Mar 2018 15:38:49 +0100
Subject: [PATCH 13/19] Use new collections to define build predefined
 variables

---
 app/models/ci/build.rb | 103 ++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 53 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index a3a289c3a12..ef1e1e08fba 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -430,14 +430,14 @@ module Ci
     end
 
     def user_variables
-      return [] if user.blank?
+      Gitlab::Ci::Variables::Collection.new.tap do |variables|
+        return variables if user.blank?
 
-      [
-        { key: 'GITLAB_USER_ID', value: user.id.to_s, public: true },
-        { key: 'GITLAB_USER_EMAIL', value: user.email, public: true },
-        { key: 'GITLAB_USER_LOGIN', value: user.username, public: true },
-        { key: 'GITLAB_USER_NAME', value: user.name, public: true }
-      ]
+        variables.append(key: 'GITLAB_USER_ID', value: user.id.to_s)
+        variables.append(key: 'GITLAB_USER_EMAIL', value: user.email)
+        variables.append(key: 'GITLAB_USER_LOGIN', value: user.username)
+        variables.append(key: 'GITLAB_USER_NAME', value: user.name)
+      end
     end
 
     def secret_variables(environment: persisted_environment)
@@ -540,60 +540,57 @@ module Ci
     CI_REGISTRY_USER = 'gitlab-ci-token'.freeze
 
     def predefined_variables
-      variables = [
-        { key: 'CI', value: 'true', public: true },
-        { key: 'GITLAB_CI', value: 'true', public: true },
-        { key: 'GITLAB_FEATURES', value: project.namespace.features.join(','), public: true },
-        { key: 'CI_SERVER_NAME', value: 'GitLab', public: true },
-        { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true },
-        { key: 'CI_SERVER_REVISION', value: Gitlab::REVISION, public: true },
-        { key: 'CI_JOB_ID', value: id.to_s, public: true },
-        { key: 'CI_JOB_NAME', value: name, public: true },
-        { key: 'CI_JOB_STAGE', value: stage, public: true },
-        { key: 'CI_JOB_TOKEN', value: token, public: false },
-        { key: 'CI_COMMIT_SHA', value: sha, public: true },
-        { key: 'CI_COMMIT_REF_NAME', value: ref, public: true },
-        { key: 'CI_COMMIT_REF_SLUG', value: ref_slug, public: true },
-        { key: 'CI_REGISTRY_USER', value: CI_REGISTRY_USER, public: true },
-        { key: 'CI_REGISTRY_PASSWORD', value: token, public: false },
-        { key: 'CI_REPOSITORY_URL', value: repo_url, public: false }
-      ]
-
-      variables << { key: "CI_COMMIT_TAG", value: ref, public: true } if tag?
-      variables << { key: "CI_PIPELINE_TRIGGERED", value: 'true', public: true } if trigger_request
-      variables << { key: "CI_JOB_MANUAL", value: 'true', public: true } if action?
-      variables.concat(legacy_variables)
+      Gitlab::Ci::Variables::Collection.new.tap do |variables|
+        variables.append(key: 'CI', value: 'true')
+        variables.append(key: 'GITLAB_CI', value: 'true')
+        variables.append(key: 'GITLAB_FEATURES', value: project.namespace.features.join(','))
+        variables.append(key: 'CI_SERVER_NAME', value: 'GitLab')
+        variables.append(key: 'CI_SERVER_VERSION', value: Gitlab::VERSION)
+        variables.append(key: 'CI_SERVER_REVISION', value: Gitlab::REVISION)
+        variables.append(key: 'CI_JOB_ID', value: id.to_s)
+        variables.append(key: 'CI_JOB_NAME', value: name)
+        variables.append(key: 'CI_JOB_STAGE', value: stage)
+        variables.append(key: 'CI_JOB_TOKEN', value: token, public: false)
+        variables.append(key: 'CI_COMMIT_SHA', value: sha)
+        variables.append(key: 'CI_COMMIT_REF_NAME', value: ref)
+        variables.append(key: 'CI_COMMIT_REF_SLUG', value: ref_slug)
+        variables.append(key: 'CI_REGISTRY_USER', value: CI_REGISTRY_USER)
+        variables.append(key: 'CI_REGISTRY_PASSWORD', value: token, public: false)
+        variables.append(key: 'CI_REPOSITORY_URL', value: repo_url, public: false)
+        variables.append(key: "CI_COMMIT_TAG", value: ref) if tag?
+        variables.append(key: "CI_PIPELINE_TRIGGERED", value: 'true') if trigger_request
+        variables.append(key: "CI_JOB_MANUAL", value: 'true') if action?
+        variables.concat(legacy_variables)
+      end
     end
 
     def persisted_environment_variables
-      return [] unless persisted_environment
+      Gitlab::Ci::Variables::Collection.new.tap do |variables|
+        return variables unless persisted_environment
 
-      variables = persisted_environment.predefined_variables
+        variables.concat(persisted_environment.predefined_variables)
 
-      # Here we're passing unexpanded environment_url for runner to expand,
-      # and we need to make sure that CI_ENVIRONMENT_NAME and
-      # CI_ENVIRONMENT_SLUG so on are available for the URL be expanded.
-      variables << { key: 'CI_ENVIRONMENT_URL', value: environment_url, public: true } if environment_url
-
-      variables
+        # Here we're passing unexpanded environment_url for runner to expand,
+        # and we need to make sure that CI_ENVIRONMENT_NAME and
+        # CI_ENVIRONMENT_SLUG so on are available for the URL be expanded.
+        variables.append(key: 'CI_ENVIRONMENT_URL', value: environment_url) if environment_url
+      end
     end
 
     def legacy_variables
-      variables = [
-        { key: 'CI_BUILD_ID', value: id.to_s, public: true },
-        { key: 'CI_BUILD_TOKEN', value: token, public: false },
-        { key: 'CI_BUILD_REF', value: sha, public: true },
-        { key: 'CI_BUILD_BEFORE_SHA', value: before_sha, public: true },
-        { key: 'CI_BUILD_REF_NAME', value: ref, public: true },
-        { key: 'CI_BUILD_REF_SLUG', value: ref_slug, public: true },
-        { key: 'CI_BUILD_NAME', value: name, public: true },
-        { key: 'CI_BUILD_STAGE', value: stage, public: true }
-      ]
-
-      variables << { key: "CI_BUILD_TAG", value: ref, public: true } if tag?
-      variables << { key: "CI_BUILD_TRIGGERED", value: 'true', public: true } if trigger_request
-      variables << { key: "CI_BUILD_MANUAL", value: 'true', public: true } if action?
-      variables
+      Gitlab::Ci::Variables::Collection.new.tap do |variables|
+        variables.append(key: 'CI_BUILD_ID', value: id.to_s)
+        variables.append(key: 'CI_BUILD_TOKEN', value: token, public: false)
+        variables.append(key: 'CI_BUILD_REF', value: sha)
+        variables.append(key: 'CI_BUILD_BEFORE_SHA', value: before_sha)
+        variables.append(key: 'CI_BUILD_REF_NAME', value: ref)
+        variables.append(key: 'CI_BUILD_REF_SLUG', value: ref_slug)
+        variables.append(key: 'CI_BUILD_NAME', value: name)
+        variables.append(key: 'CI_BUILD_STAGE', value: stage)
+        variables.append(key: "CI_BUILD_TAG", value: ref) if tag?
+        variables.append(key: "CI_BUILD_TRIGGERED", value: 'true') if trigger_request
+        variables.append(key: "CI_BUILD_MANUAL", value: 'true') if action?
+      end
     end
 
     def environment_url

From 077298817fe59b5e572b17a6ecd6ffb2c47fd9b5 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Mar 2018 10:02:01 +0100
Subject: [PATCH 14/19] Fix variables collection item sequence specs

---
 spec/lib/gitlab/ci/variables/collection/item_spec.rb | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/spec/lib/gitlab/ci/variables/collection/item_spec.rb b/spec/lib/gitlab/ci/variables/collection/item_spec.rb
index 37394c3e80f..cc1257484d2 100644
--- a/spec/lib/gitlab/ci/variables/collection/item_spec.rb
+++ b/spec/lib/gitlab/ci/variables/collection/item_spec.rb
@@ -14,12 +14,11 @@ describe Gitlab::Ci::Variables::Collection::Item do
     end
 
     it 'supports using an active record resource' do
-      resource = described_class.fabricate(create(:ci_variable))
+      variable = create(:ci_variable, key: 'CI_VAR', value: '123')
+      resource = described_class.fabricate(variable)
 
       expect(resource).to be_a(described_class)
-      expect(resource).to eq(key: 'VARIABLE_1',
-                             value: 'VARIABLE_VALUE',
-                             public: false)
+      expect(resource).to eq(key: 'CI_VAR', value: '123', public: false)
     end
 
     it 'supports using another collection item' do

From e7e06566e150af8ecc54c67bd6f5cd5b86c99628 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Mar 2018 10:05:06 +0100
Subject: [PATCH 15/19] Port deployment variables skeleton code from EE

This commit backports method signatures and related implementation to
the CE, to make CE and EE identical. This does not add any features from
EE, it is only aimed to reduce conflicts between CE and EE in the
future.
---
 app/models/ci/build.rb | 2 +-
 app/models/project.rb  | 6 ++----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index ef1e1e08fba..f8a3600e863 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -257,7 +257,7 @@ module Ci
         variables.concat(project.predefined_variables)
         variables.concat(pipeline.predefined_variables)
         variables.concat(runner.predefined_variables) if runner
-        variables.concat(project.deployment_variables) if has_environment?
+        variables.concat(project.deployment_variables(environment: environment)) if has_environment?
         variables.concat(yaml_variables)
         variables.concat(user_variables)
         variables.concat(project.group.secret_variables_for(ref, project)) if project.group
diff --git a/app/models/project.rb b/app/models/project.rb
index e307d384e74..87d228997be 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1616,10 +1616,8 @@ class Project < ActiveRecord::Base
     end
   end
 
-  def deployment_variables
-    return [] unless deployment_platform
-
-    deployment_platform.predefined_variables
+  def deployment_variables(environment: nil)
+    deployment_platform(environment: environment)&.predefined_variables || []
   end
 
   def auto_devops_variables

From 1d57db0d8d9ed0515fb1839e963550a60d86cb70 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Mar 2018 11:04:42 +0100
Subject: [PATCH 16/19] Improve predefined variables collection methods

---
 app/models/project.rb                         | 21 +++++++++----------
 lib/gitlab/ci/variables/collection.rb         |  4 ++--
 .../gitlab/ci/variables/collection_spec.rb    | 10 +++++++++
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index 87d228997be..73f3794867b 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1574,17 +1574,16 @@ class Project < ActiveRecord::Base
   def predefined_variables
     visibility = Gitlab::VisibilityLevel.string_level(visibility_level)
 
-    Gitlab::Ci::Variables::Collection.new.tap do |variables|
-      variables.append(key: 'CI_PROJECT_ID', value: id.to_s)
-      variables.append(key: 'CI_PROJECT_NAME', value: path)
-      variables.append(key: 'CI_PROJECT_PATH', value: full_path)
-      variables.append(key: 'CI_PROJECT_PATH_SLUG', value: full_path_slug)
-      variables.append(key: 'CI_PROJECT_NAMESPACE', value: namespace.full_path)
-      variables.append(key: 'CI_PROJECT_URL', value: web_url)
-      variables.append(key: 'CI_PROJECT_VISIBILITY', value: visibility)
-      variables.concat(container_registry_variables)
-      variables.concat(auto_devops_variables)
-    end
+    Gitlab::Ci::Variables::Collection.new
+      .append(key: 'CI_PROJECT_ID', value: id.to_s)
+      .append(key: 'CI_PROJECT_NAME', value: path)
+      .append(key: 'CI_PROJECT_PATH', value: full_path)
+      .append(key: 'CI_PROJECT_PATH_SLUG', value: full_path_slug)
+      .append(key: 'CI_PROJECT_NAMESPACE', value: namespace.full_path)
+      .append(key: 'CI_PROJECT_URL', value: web_url)
+      .append(key: 'CI_PROJECT_VISIBILITY', value: visibility)
+      .concat(container_registry_variables)
+      .concat(auto_devops_variables)
   end
 
   def container_registry_variables
diff --git a/lib/gitlab/ci/variables/collection.rb b/lib/gitlab/ci/variables/collection.rb
index ae7415fcdb1..0deca55fe8f 100644
--- a/lib/gitlab/ci/variables/collection.rb
+++ b/lib/gitlab/ci/variables/collection.rb
@@ -11,11 +11,11 @@ module Gitlab
         end
 
         def append(resource)
-          @variables.append(Collection::Item.fabricate(resource))
+          tap { @variables.append(Collection::Item.fabricate(resource)) }
         end
 
         def concat(resources)
-          resources.each { |variable| self.append(variable) }
+          tap { resources.each { |variable| self.append(variable) } }
         end
 
         def each
diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb
index 005e2bb17b4..90b6e178242 100644
--- a/spec/lib/gitlab/ci/variables/collection_spec.rb
+++ b/spec/lib/gitlab/ci/variables/collection_spec.rb
@@ -35,6 +35,11 @@ describe Gitlab::Ci::Variables::Collection do
 
       expect(subject).to be_one
     end
+
+    it 'returns self' do
+      expect(subject.append(key: 'VAR', value: 'test'))
+        .to eq subject
+    end
   end
 
   describe '#concat' do
@@ -60,6 +65,11 @@ describe Gitlab::Ci::Variables::Collection do
       expect(collection).to include(key: 'VAR_2', value: '2', public: true)
       expect(collection).to include(key: 'VAR_3', value: '3', public: true)
     end
+
+    it 'returns self' do
+      expect(subject.concat([key: 'VAR', value: 'test']))
+        .to eq subject
+    end
   end
 
   describe '#+' do

From 33d459aa8da57b701fb3368b40fd10ec98c2e5f4 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Mar 2018 11:15:18 +0100
Subject: [PATCH 17/19] DRY remaining instances of predefined variables

---
 app/models/ci/pipeline.rb                     |  9 ++++----
 app/models/ci/runner.rb                       |  9 ++++----
 app/models/clusters/platforms/kubernetes.rb   | 22 +++++++++----------
 app/models/environment.rb                     |  7 +++---
 .../project_services/kubernetes_service.rb    | 18 +++++++++------
 5 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index a72a815bfe8..4966ea62df9 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -473,11 +473,10 @@ module Ci
     end
 
     def predefined_variables
-      [
-        { key: 'CI_PIPELINE_ID', value: id.to_s, public: true },
-        { key: 'CI_CONFIG_PATH', value: ci_yaml_file_path, public: true },
-        { key: 'CI_PIPELINE_SOURCE', value: source.to_s, public: true }
-      ]
+      Gitlab::Ci::Variables::Collection.new
+        .append(key: 'CI_PIPELINE_ID', value: id.to_s)
+        .append(key: 'CI_CONFIG_PATH', value: ci_yaml_file_path)
+        .append(key: 'CI_PIPELINE_SOURCE', value: source.to_s)
     end
 
     def queued_duration
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 609620a62bb..7173f88f1c7 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -132,11 +132,10 @@ module Ci
     end
 
     def predefined_variables
-      [
-        { key: 'CI_RUNNER_ID', value: id.to_s, public: true },
-        { key: 'CI_RUNNER_DESCRIPTION', value: description, public: true },
-        { key: 'CI_RUNNER_TAGS', value: tag_list.to_s, public: true }
-      ]
+      Gitlab::Ci::Variables::Collection.new
+        .append(key: 'CI_RUNNER_ID', value: id.to_s)
+        .append(key: 'CI_RUNNER_DESCRIPTION', value: description)
+        .append(key: 'CI_RUNNER_TAGS', value: tag_list.to_s)
     end
 
     def tick_runner_queue
diff --git a/app/models/clusters/platforms/kubernetes.rb b/app/models/clusters/platforms/kubernetes.rb
index 7ce8befeeeb..015c8e9bcf8 100644
--- a/app/models/clusters/platforms/kubernetes.rb
+++ b/app/models/clusters/platforms/kubernetes.rb
@@ -56,19 +56,19 @@ module Clusters
       def predefined_variables
         config = YAML.dump(kubeconfig)
 
-        variables = [
-          { key: 'KUBE_URL', value: api_url, public: true },
-          { key: 'KUBE_TOKEN', value: token, public: false },
-          { key: 'KUBE_NAMESPACE', value: actual_namespace, public: true },
-          { key: 'KUBECONFIG', value: config, public: false, file: true }
-        ]
+        Gitlab::Ci::Variables::Collection.new.tap do |variables|
+          variables
+            .append(key: 'KUBE_URL', value: api_url)
+            .append(key: 'KUBE_TOKEN', value: token, public: false)
+            .append(key: 'KUBE_NAMESPACE', value: actual_namespace)
+            .append(key: 'KUBECONFIG', value: config, public: false, file: true)
 
-        if ca_pem.present?
-          variables << { key: 'KUBE_CA_PEM', value: ca_pem, public: true }
-          variables << { key: 'KUBE_CA_PEM_FILE', value: ca_pem, public: true, file: true }
+          if ca_pem.present?
+            variables
+              .append(key: 'KUBE_CA_PEM', value: ca_pem)
+              .append(key: 'KUBE_CA_PEM_FILE', value: ca_pem, file: true)
+          end
         end
-
-        variables
       end
 
       # Constructs a list of terminals from the reactive cache
diff --git a/app/models/environment.rb b/app/models/environment.rb
index 2b0a88ac5b4..9517723d9d9 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -65,10 +65,9 @@ class Environment < ActiveRecord::Base
   end
 
   def predefined_variables
-    [
-      { key: 'CI_ENVIRONMENT_NAME', value: name, public: true },
-      { key: 'CI_ENVIRONMENT_SLUG', value: slug, public: true }
-    ]
+    Gitlab::Ci::Variables::Collection.new
+      .append(key: 'CI_ENVIRONMENT_NAME', value: name)
+      .append(key: 'CI_ENVIRONMENT_SLUG', value: slug)
   end
 
   def recently_updated_on_branch?(ref)
diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb
index 347c9f34033..b4cd1c1a155 100644
--- a/app/models/project_services/kubernetes_service.rb
+++ b/app/models/project_services/kubernetes_service.rb
@@ -105,14 +105,18 @@ class KubernetesService < DeploymentService
   def predefined_variables
     config = YAML.dump(kubeconfig)
 
-    Gitlab::Ci::Variables::Collection.new.tap do |collection|
-      collection.append(key: 'KUBE_URL', value: api_url, public: true)
-      collection.append(key: 'KUBE_TOKEN', value: token, public: false)
-      collection.append(key: 'KUBE_NAMESPACE', value: actual_namespace, public: true)
-      collection.append(key: 'KUBECONFIG', value: config, public: false, file: true)
+    Gitlab::Ci::Variables::Collection.new.tap do |variables|
+      variables
+        .append(key: 'KUBE_URL', value: api_url)
+        .append(key: 'KUBE_TOKEN', value: token, public: false)
+        .append(key: 'KUBE_NAMESPACE', value: actual_namespace)
+        .append(key: 'KUBECONFIG', value: config, public: false, file: true)
 
-      collection.append(key: 'KUBE_CA_PEM', value: ca_pem, public: true)
-      collection.append(key: 'KUBE_CA_PEM_FILE', value: ca_pem, public: true, file: true)
+      if ca_pem.present?
+        variables
+          .append(key: 'KUBE_CA_PEM', value: ca_pem)
+          .append(key: 'KUBE_CA_PEM_FILE', value: ca_pem, file: true)
+      end
     end
   end
 

From 3d5856541b97bf5947437ce26b934522fe52489c Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Mar 2018 12:01:38 +0100
Subject: [PATCH 18/19] Fix pipeline specs for predefined variables

---
 spec/models/ci/pipeline_spec.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 14d234f6aab..7d4726a3cc2 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -170,8 +170,6 @@ describe Ci::Pipeline, :mailer do
   describe '#predefined_variables' do
     subject { pipeline.predefined_variables }
 
-    it { is_expected.to be_an(Array) }
-
     it 'includes the defined keys' do
       keys = subject.map { |v| v[:key] }
 

From a830c49a3fcf684f89ea5068bbca8ccc1b83fc4c Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Mar 2018 12:48:13 +0100
Subject: [PATCH 19/19] Fix pipeline predefined variables specs

---
 spec/models/ci/pipeline_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index e70892b5c76..4635f8cfe9d 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -171,7 +171,7 @@ describe Ci::Pipeline, :mailer do
     subject { pipeline.predefined_variables }
 
     it 'includes all predefined variables in a valid order' do
-      keys = subject.map { |variable| variable.fetch(:key) }
+      keys = subject.map { |variable| variable[:key] }
 
       expect(keys).to eq %w[CI_PIPELINE_ID CI_CONFIG_PATH CI_PIPELINE_SOURCE]
     end