From da6bfd9b801c4d1c91593c6b4a755b097b644ab0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 27 Mar 2018 13:05:29 +0200 Subject: [PATCH] Decouple build variables from persisted environment --- app/models/ci/build.rb | 70 +++++++++++++++---------- lib/gitlab/ci/build/policy/variables.rb | 3 +- spec/models/ci/build_spec.rb | 8 +-- 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 + # Variables in the environment name scope. # - 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. - # - 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