Return `Collection` from `Ci::Build` and similar classes

Avoids having separate `#variable_collection` properties

app/models/ci/build.rb

@@ -510,7 +510,6 @@ module Ci
           .concat(scoped_variables)
           .concat(job_variables)
           .concat(persisted_environment_variables)
-          .to_runner_variables
       end
     end
 
@@ -986,7 +985,7 @@ module Ci
       # TODO: Have `debug_mode?` check against data on sent back from runner
       # to capture all the ways that variables can be set.
       # See (https://gitlab.com/gitlab-org/gitlab/-/issues/290955)
-      variables.any? { |variable| variable[:key] == 'CI_DEBUG_TRACE' && variable[:value].casecmp('true') == 0 }
+      variables['CI_DEBUG_TRACE']&.value&.casecmp('true') == 0
     end
 
     def drop_with_exit_code!(failure_reason, exit_code)

app/models/ci/processable.rb

@@ -120,10 +120,6 @@ module Ci
       raise NotImplementedError
     end
 
-    def scoped_variables_hash
-      raise NotImplementedError
-    end
-
     override :all_met_to_become_pending?
     def all_met_to_become_pending?
       super && !with_resource_group?

app/models/concerns/ci/contextable.rb

@@ -28,14 +28,6 @@ module Ci
       end
     end
 
-    ##
-    # Regular Ruby hash of scoped variables, without duplicates that are
-    # possible to be present in an array of hashes returned from `variables`.
-    #
-    def scoped_variables_hash
-      scoped_variables.to_hash
-    end
-
     ##
     # Variables that do not depend on the environment name.
     #

app/presenters/ci/build_runner_presenter.rb

@@ -32,6 +32,10 @@ module Ci
       end.to_i
     end
 
+    def runner_variables
+      variables.to_runner_variables
+    end
+
     def refspecs
       specs = []
       specs << refspec_for_persistent_ref if persistent_ref_exist?

ee/spec/models/ci/build_spec.rb

@@ -142,7 +142,7 @@ RSpec.describe Ci::Build do
 
       context 'when there is a plan for the group' do
         it 'GITLAB_FEATURES should include the features for that plan' do
-          is_expected.to include({ key: 'GITLAB_FEATURES', value: anything, public: true, masked: false })
+          expect(subject.to_runner_variables).to include({ key: 'GITLAB_FEATURES', value: anything, public: true, masked: false })
           features_variable = subject.find { |v| v[:key] == 'GITLAB_FEATURES' }
           expect(features_variable[:value]).to include('multiple_ldap_servers')
         end

ee/spec/services/ci/create_pipeline_service/needs_spec.rb

@@ -114,7 +114,7 @@ RSpec.describe Ci::CreatePipelineService do
 
       expect(job).to be_present
       expect(job.all_dependencies).to include(dependency)
-      expect(job.scoped_variables_hash).to include(dependency_variable.key => dependency_variable.value)
+      expect(job.scoped_variables.to_hash).to include(dependency_variable.key => dependency_variable.value)
     end
   end
 end

lib/api/entities/job_request/response.rb

@@ -20,7 +20,7 @@ module API
           model
         end
 
-        expose :variables
+        expose :runner_variables, as: :variables
         expose :steps, using: Entities::JobRequest::Step
         expose :image, using: Entities::JobRequest::Image
         expose :services, using: Entities::JobRequest::Service

lib/expand_variables.rb

@@ -45,6 +45,9 @@ module ExpandVariables
       # Lazily initialise variables
       variables = variables.call if variables.is_a?(Proc)
 
+      # Convert Collection to variables
+      variables = variables.to_hash if variables.is_a?(Gitlab::Ci::Variables::Collection)
+
       # Convert hash array to variables
       if variables.is_a?(Array)
         variables = variables.reduce({}) do |hash, variable|

lib/gitlab/ci/build/context/build.rb

@@ -21,7 +21,7 @@ module Gitlab
               # to the CI variables to evaluate rules before we persist a Build
               # with the result. We should refactor away the extra Build.new,
               # but be able to get CI Variables directly from the Seed::Build.
-              stub_build.scoped_variables_hash
+              stub_build.scoped_variables
             end
           end
 

lib/gitlab/ci/build/context/global.rb

@@ -19,8 +19,7 @@ module Gitlab
               # to the CI variables to evaluate workflow:rules
               # with the result. We should refactor away the extra Build.new,
               # but be able to get CI Variables directly from the Seed::Build.
-              stub_build.scoped_variables_hash
-                .reject { |key, _value| key =~ /\ACI_(JOB|BUILD)/ }
+              stub_build.scoped_variables.reject { |var| var[:key] =~ /\ACI_(JOB|BUILD)/ }
             end
           end
 

lib/gitlab/ci/pipeline/expression/statement.rb

@@ -7,9 +7,9 @@ module Gitlab
         class Statement
           StatementError = Class.new(Expression::ExpressionError)
 
-          def initialize(statement, variables = {})
+          def initialize(statement, variables = nil)
             @lexer = Expression::Lexer.new(statement)
-            @variables = variables.with_indifferent_access
+            @variables = variables&.to_hash
           end
 
           def parse_tree

lib/gitlab/ci/variables/collection.rb

@@ -55,8 +55,12 @@ module Gitlab
 
         def to_hash
           self.to_runner_variables
-            .map { |env| [env.fetch(:key), env.fetch(:value)] }
-            .to_h.with_indifferent_access
+            .to_h { |env| [env.fetch(:key), env.fetch(:value)] }
+            .with_indifferent_access
+        end
+
+        def reject(&block)
+          Collection.new(@variables.reject(&block))
         end
 
         # Returns a sorted Collection object, and sets errors property in case of an error

spec/lib/expand_variables_spec.rb

@@ -82,6 +82,13 @@ RSpec.describe ExpandVariables do
           value: 'key$variable',
           result: 'keyvalue',
           variables: -> { [{ key: 'variable', value: 'value' }] }
+        },
+        "simple expansion using Collection": {
+          value: 'key$variable',
+          result: 'keyvalue',
+          variables: Gitlab::Ci::Variables::Collection.new([
+            { key: 'variable', value: 'value' }
+          ])
         }
       }
     end

spec/lib/gitlab/ci/build/context/build_spec.rb

@@ -9,7 +9,9 @@ RSpec.describe Gitlab::Ci::Build::Context::Build do
   let(:context) { described_class.new(pipeline, seed_attributes) }
 
   describe '#variables' do
-    subject { context.variables }
+    subject { context.variables.to_hash }
+
+    it { expect(context.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) }
 
     it { is_expected.to include('CI_COMMIT_REF_NAME' => 'master') }
     it { is_expected.to include('CI_PIPELINE_IID'    => pipeline.iid.to_s) }

spec/lib/gitlab/ci/build/context/global_spec.rb

@@ -9,7 +9,9 @@ RSpec.describe Gitlab::Ci::Build::Context::Global do
   let(:context) { described_class.new(pipeline, yaml_variables: yaml_variables) }
 
   describe '#variables' do
-    subject { context.variables }
+    subject { context.variables.to_hash }
+
+    it { expect(context.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) }
 
     it { is_expected.to include('CI_COMMIT_REF_NAME' => 'master') }
     it { is_expected.to include('CI_PIPELINE_IID'    => pipeline.iid.to_s) }

spec/lib/gitlab/ci/build/policy/variables_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Gitlab::Ci::Build::Policy::Variables do
   let(:seed) do
     double('build seed',
       to_resource: ci_build,
-      variables: ci_build.scoped_variables_hash
+      variables: ci_build.scoped_variables
     )
   end
 
@@ -91,7 +91,7 @@ RSpec.describe Gitlab::Ci::Build::Policy::Variables do
       let(:seed) do
         double('bridge seed',
           to_resource: bridge,
-          variables: ci_build.scoped_variables_hash
+          variables: ci_build.scoped_variables
         )
       end
 

spec/lib/gitlab/ci/build/rules/rule_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Gitlab::Ci::Build::Rules::Rule do
   let(:seed) do
     double('build seed',
       to_resource: ci_build,
-      variables: ci_build.scoped_variables_hash
+      variables: ci_build.scoped_variables
     )
   end
 

spec/lib/gitlab/ci/build/rules_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Gitlab::Ci::Build::Rules do
   let(:seed) do
     double('build seed',
       to_resource: ci_build,
-      variables: ci_build.scoped_variables_hash
+      variables: ci_build.scoped_variables
     )
   end
 

spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb

@@ -3,17 +3,16 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Pipeline::Expression::Statement do
-  subject do
-    described_class.new(text, variables)
+  let(:variables) do
+    Gitlab::Ci::Variables::Collection.new
+      .append(key: 'PRESENT_VARIABLE', value: 'my variable')
+      .append(key: 'PATH_VARIABLE', value: 'a/path/variable/value')
+      .append(key: 'FULL_PATH_VARIABLE', value: '/a/full/path/variable/value')
+      .append(key: 'EMPTY_VARIABLE', value: '')
   end
 
-  let(:variables) do
-    {
-      'PRESENT_VARIABLE'   => 'my variable',
-      'PATH_VARIABLE'      => 'a/path/variable/value',
-      'FULL_PATH_VARIABLE' => '/a/full/path/variable/value',
-      'EMPTY_VARIABLE'     =>  ''
-    }
+  subject do
+    described_class.new(text, variables)
   end
 
   describe '.new' do

spec/lib/gitlab/ci/variables/collection_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
     end
 
     it 'can be initialized without an argument' do
-      expect(subject).to be_none
+      is_expected.to be_none
     end
   end
 
@@ -21,13 +21,13 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
     it 'appends a hash' do
       subject.append(key: 'VARIABLE', value: 'something')
 
-      expect(subject).to be_one
+      is_expected.to be_one
     end
 
     it 'appends a Ci::Variable' do
       subject.append(build(:ci_variable))
 
-      expect(subject).to be_one
+      is_expected.to be_one
     end
 
     it 'appends an internal resource' do
@@ -35,7 +35,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
 
       subject.append(collection.first)
 
-      expect(subject).to be_one
+      is_expected.to be_one
     end
 
     it 'returns self' do
@@ -210,4 +210,24 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
       end
     end
   end
+
+  describe '#reject' do
+    let(:collection) do
+      described_class.new
+        .append(key: 'CI_JOB_NAME', value: 'test-1')
+        .append(key: 'CI_BUILD_ID', value: '1')
+        .append(key: 'TEST1', value: 'test-3')
+    end
+
+    subject { collection.reject { |var| var[:key] =~ /\ACI_(JOB|BUILD)/ } }
+
+    it 'returns a Collection instance' do
+      is_expected.to be_an_instance_of(described_class)
+    end
+
+    it 'returns correctly filtered Collection' do
+      comp = collection.to_runner_variables.reject { |var| var[:key] =~ /\ACI_(JOB|BUILD)/ }
+      expect(subject.to_runner_variables).to eq(comp)
+    end
+  end
 end

spec/models/ci/bridge_spec.rb

@@ -41,7 +41,7 @@ RSpec.describe Ci::Bridge do
     end
   end
 
-  describe '#scoped_variables_hash' do
+  describe '#scoped_variables' do
     it 'returns a hash representing variables' do
       variables = %w[
         CI_JOB_NAME CI_JOB_STAGE CI_COMMIT_SHA CI_COMMIT_SHORT_SHA
@@ -53,7 +53,7 @@ RSpec.describe Ci::Bridge do
         CI_COMMIT_TIMESTAMP
       ]
 
-      expect(bridge.scoped_variables_hash.keys).to include(*variables)
+      expect(bridge.scoped_variables.map { |v| v[:key] }).to include(*variables)
     end
 
     context 'when bridge has dependency which has dotenv variable' do
@@ -63,7 +63,7 @@ RSpec.describe Ci::Bridge do
       let!(:job_variable) { create(:ci_job_variable, :dotenv_source, job: test) }
 
       it 'includes inherited variable' do
-        expect(bridge.scoped_variables_hash).to include(job_variable.key => job_variable.value)
+        expect(bridge.scoped_variables.to_hash).to include(job_variable.key => job_variable.value)
       end
     end
   end

spec/models/ci/build_spec.rb

@@ -2440,7 +2440,8 @@ RSpec.describe Ci::Build do
         build.yaml_variables = []
       end
 
-      it { is_expected.to eq(predefined_variables) }
+      it { is_expected.to be_instance_of(Gitlab::Ci::Variables::Collection) }
+      it { expect(subject.to_runner_variables).to eq(predefined_variables) }
 
       context 'when ci_job_jwt feature flag is disabled' do
         before do
@@ -2495,7 +2496,7 @@ RSpec.describe Ci::Build do
           end
 
           it 'returns variables in order depending on resource hierarchy' do
-            is_expected.to eq(
+            expect(subject.to_runner_variables).to eq(
               [dependency_proxy_var,
                job_jwt_var,
                build_pre_var,
@@ -2525,7 +2526,7 @@ RSpec.describe Ci::Build do
           end
 
           it 'matches explicit variables ordering' do
-            received_variables = subject.map { |variable| variable.fetch(:key) }
+            received_variables = subject.map { |variable| variable[:key] }
 
             expect(received_variables).to eq expected_variables
           end
@@ -2618,7 +2619,7 @@ RSpec.describe Ci::Build do
             it_behaves_like 'containing environment variables'
 
             it 'puts $CI_ENVIRONMENT_URL in the last so all other variables are available to be used when runners are trying to expand it' do
-              expect(subject.last).to eq(environment_variables.last)
+              expect(subject.to_runner_variables.last).to eq(environment_variables.last)
             end
           end
         end
@@ -2951,7 +2952,7 @@ RSpec.describe Ci::Build do
       end
 
       it 'overrides YAML variable using a pipeline variable' do
-        variables = subject.reverse.uniq { |variable| variable[:key] }.reverse
+        variables = subject.to_runner_variables.reverse.uniq { |variable| variable[:key] }.reverse
 
         expect(variables)
           .not_to include(key: 'MYVAR', value: 'myvar', public: true, masked: false)
@@ -3248,47 +3249,6 @@ RSpec.describe Ci::Build do
     end
   end
 
-  describe '#scoped_variables_hash' do
-    context 'when overriding CI variables' do
-      before do
-        project.variables.create!(key: 'MY_VAR', value: 'my value 1')
-        pipeline.variables.create!(key: 'MY_VAR', value: 'my value 2')
-      end
-
-      it 'returns a regular hash created using valid ordering' do
-        expect(build.scoped_variables_hash).to include('MY_VAR': 'my value 2')
-        expect(build.scoped_variables_hash).not_to include('MY_VAR': 'my value 1')
-      end
-    end
-
-    context 'when overriding user-provided variables' do
-      let(:build) do
-        create(:ci_build, pipeline: pipeline, yaml_variables: [{ key: 'MY_VAR', value: 'myvar', public: true }])
-      end
-
-      before do
-        pipeline.variables.build(key: 'MY_VAR', value: 'pipeline value')
-      end
-
-      it 'returns a hash including variable with higher precedence' do
-        expect(build.scoped_variables_hash).to include('MY_VAR': 'pipeline value')
-        expect(build.scoped_variables_hash).not_to include('MY_VAR': 'myvar')
-      end
-    end
-
-    context 'when overriding CI instance variables' do
-      before do
-        create(:ci_instance_variable, key: 'MY_VAR', value: 'my value 1')
-        group.variables.create!(key: 'MY_VAR', value: 'my value 2')
-      end
-
-      it 'returns a regular hash created using valid ordering' do
-        expect(build.scoped_variables_hash).to include('MY_VAR': 'my value 2')
-        expect(build.scoped_variables_hash).not_to include('MY_VAR': 'my value 1')
-      end
-    end
-  end
-
   describe '#any_unmet_prerequisites?' do
     let(:build) { create(:ci_build, :created) }
 

spec/presenters/ci/build_runner_presenter_spec.rb

@@ -271,4 +271,28 @@ RSpec.describe Ci::BuildRunnerPresenter do
       end
     end
   end
+
+  describe '#variables' do
+    subject { presenter.variables }
+
+    let(:build) { create(:ci_build) }
+
+    it 'returns a Collection' do
+      is_expected.to be_an_instance_of(Gitlab::Ci::Variables::Collection)
+    end
+  end
+
+  describe '#runner_variables' do
+    subject { presenter.runner_variables }
+
+    let(:build) { create(:ci_build) }
+
+    it 'returns an array' do
+      is_expected.to be_an_instance_of(Array)
+    end
+
+    it 'returns the expected variables' do
+      is_expected.to eq(presenter.variables.to_runner_variables)
+    end
+  end
 end

spec/services/ci/create_pipeline_service/rules_spec.rb

@@ -174,7 +174,7 @@ RSpec.describe Ci::CreatePipelineService do
           let(:ref) { 'refs/heads/master' }
 
           it 'overrides VAR1' do
-            variables = job.scoped_variables_hash
+            variables = job.scoped_variables.to_hash
 
             expect(variables['VAR1']).to eq('overridden var 1')
             expect(variables['VAR2']).to eq('my var 2')
@@ -186,7 +186,7 @@ RSpec.describe Ci::CreatePipelineService do
           let(:ref) { 'refs/heads/feature' }
 
           it 'overrides VAR2 and adds VAR3' do
-            variables = job.scoped_variables_hash
+            variables = job.scoped_variables.to_hash
 
             expect(variables['VAR1']).to eq('my var 1')
             expect(variables['VAR2']).to eq('overridden var 2')
@@ -198,7 +198,7 @@ RSpec.describe Ci::CreatePipelineService do
           let(:ref) { 'refs/heads/wip' }
 
           it 'does not affect vars' do
-            variables = job.scoped_variables_hash
+            variables = job.scoped_variables.to_hash
 
             expect(variables['VAR1']).to eq('my var 1')
             expect(variables['VAR2']).to eq('my var 2')