Decouple build variables from persisted environment

3 files changed, 47 insertions, 34 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 627064d3dec..5f149ad3057 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -23,7 +23,9 @@ module Ci
     has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
     has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
 
-    # The "environment" field for builds is a String, and is the unexpanded name
+    ##
+    # The "environment" field for builds is a String, and is the unexpanded name!
+    #
     def persisted_environment
       @persisted_environment ||= Environment.find_by(
         name: expanded_environment_name,
@@ -198,7 +200,9 @@ module Ci
     end
 
     def expanded_environment_name
-      ExpandVariables.expand(environment, simple_variables) if environment
+      if has_environment?
+        ExpandVariables.expand(environment, simple_variables)
+      end
     end
 
     def has_environment?
@@ -249,41 +253,46 @@ module Ci
     end
 
     ##
-    # Variables whose value does not depend on environment
-    #
-    def simple_variables
-      variables(environment: nil)
-    end
-
-    ##
-    # Variables that are available for evaluation using variables policy.
-    #
-    def evaluable_variables
-      Gitlab::Ci::Variables::Collection.new
-        .concat(simple_variables)
-    end
-
-    ## All variables, including those dependent on environment, which could
-    #  contain unexpanded variables.
+    # Variables in the environment name scope.
     #
-    def variables(environment: persisted_environment)
-      collection = Gitlab::Ci::Variables::Collection.new.tap do |variables|
+    def scoped_variables(environment: expanded_environment_name)
+      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.deployment_variables(environment: environment)) if has_environment?
+        variables.concat(project.deployment_variables(environment: environment)) if environment
         variables.concat(yaml_variables)
         variables.concat(user_variables)
-        variables.concat(project.group.secret_variables_for(ref, project)) if project.group
-        variables.concat(secret_variables(environment: environment))
+        variables.concat(secret_group_variables)
+        variables.concat(secret_project_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
+    end
 
-      collection.to_runner_variables
+    ##
+    # Variables that do not depend on the environment name.
+    #
+    def simple_variables
+      scoped_variables(environment: nil).to_runner_variables
+    end
+
+    ##
+    # All variables, including persisted environment variables.
+    #
+    def variables
+      scoped_variables
+        .concat(persisted_environment_variables)
+        .to_runner_variables
+    end
+
+    ##
+    # TODO, add specs
+    #
+    def variables_hash
+      scoped_variables.to_hash
     end
 
     def features
@@ -454,9 +463,14 @@ module Ci
       end
     end
 
-    def secret_variables(environment: persisted_environment)
+    def secret_group_variables
+      return [] unless project.group
+
+      project.group.secret_variables_for(ref, project)
+    end
+
+    def secret_project_variables(environment: persisted_environment)
       project.secret_variables_for(ref: ref, environment: environment)
-        .map(&:to_runner_variable)
     end
 
     def steps
@@ -580,7 +594,7 @@ module Ci
 
     def persisted_environment_variables
       Gitlab::Ci::Variables::Collection.new.tap do |variables|
-        return variables unless persisted_environment
+        return variables unless persisted? && persisted_environment.present?
 
         variables.concat(persisted_environment.predefined_variables)
 
diff --git a/lib/gitlab/ci/build/policy/variables.rb b/lib/gitlab/ci/build/policy/variables.rb
index a22e9a1a80e..8f1a7225da1 100644
--- a/lib/gitlab/ci/build/policy/variables.rb
+++ b/lib/gitlab/ci/build/policy/variables.rb
@@ -8,8 +8,7 @@ module Gitlab
           end
 
           def satisfied_by?(pipeline, seed)
-            variables = seed.to_resource
-              .evaluable_variables.to_hash
+            variables = seed.to_resource.variables_hash
 
             statements = @expressions.map do |statement|
               ::Gitlab::Ci::Pipeline::Expression::Statement
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 6a45169c6a8..f4d3e57b225 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1998,7 +1998,7 @@ describe Ci::Build do
     end
   end
 
-  describe '#evaluable_variables' do
+  describe '#scoped_variables' do
     context 'when build has not been persisted yet' do
       let(:build) do
         described_class.new(
@@ -2014,7 +2014,7 @@ describe Ci::Build do
         expect(build).to be_valid
         expect(build).not_to be_persisted
 
-        variables = build.evaluable_variables
+        build.scoped_variables
 
         expect(build).not_to be_persisted
       end
@@ -2026,7 +2026,7 @@ describe Ci::Build do
                   CI_COMMIT_REF_SLUG
                   CI_JOB_STAGE]
 
-        variables = build.evaluable_variables
+        variables = build.scoped_variables
 
         variables.map { |env| env[:key] }.tap do |names|
           expect(names).to include(*keys)
@@ -2046,7 +2046,7 @@ describe Ci::Build do
                   CI_REPOSITORY_URL
                   CI_ENVIRONMENT_URL]
 
-        build.evaluable_variables.map { |env| env[:key] }.tap do |names|
+        build.scoped_variables.map { |env| env[:key] }.tap do |names|
           expect(names).not_to include(*keys)
         end
       end