Merge branch '294232-refactoring' into 'master'

Refactor workflow rules config passing

See merge request gitlab-org/gitlab!57136

app/services/ci/create_web_ide_terminal_service.rb

@@ -58,7 +58,8 @@ module Ci
         builds: [terminal_build_seed]
       }
 
-      Gitlab::Ci::Pipeline::Seed::Stage.new(pipeline, attributes, [])
+      seed_context = Gitlab::Ci::Pipeline::Seed::Context.new(pipeline)
+      Gitlab::Ci::Pipeline::Seed::Stage.new(seed_context, attributes, [])
     end
 
     def terminal_build_seed

lib/gitlab/ci/pipeline/chain/command.rb

@@ -12,7 +12,7 @@ module Gitlab
           :seeds_block, :variables_attributes, :push_options,
           :chat_data, :allow_mirror_update, :bridge, :content, :dry_run,
           # These attributes are set by Chains during processing:
-          :config_content, :yaml_processor_result, :pipeline_seed
+          :config_content, :yaml_processor_result, :workflow_rules_result, :pipeline_seed
         ) do
           include Gitlab::Utils::StrongMemoize
 

lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules.rb

@@ -9,6 +9,8 @@ module Gitlab
           include Chain::Helpers
 
           def perform!
+            @command.workflow_rules_result = workflow_rules_result
+
             error('Pipeline filtered out by workflow rules.') unless workflow_passed?
           end
 
@@ -19,27 +21,33 @@ module Gitlab
           private
 
           def workflow_passed?
-            strong_memoize(:workflow_passed) do
-              workflow_rules.evaluate(@pipeline, global_context).pass?
+            workflow_rules_result.pass?
+          end
+
+          def workflow_rules_result
+            strong_memoize(:workflow_rules_result) do
+              workflow_rules.evaluate(@pipeline, global_context)
             end
           end
 
           def workflow_rules
             Gitlab::Ci::Build::Rules.new(
-              workflow_config[:rules], default_when: 'always')
+              workflow_rules_config, default_when: 'always')
           end
 
           def global_context
             Gitlab::Ci::Build::Context::Global.new(
-              @pipeline, yaml_variables: workflow_config[:yaml_variables])
+              @pipeline, yaml_variables: @command.yaml_processor_result.root_variables)
           end
 
           def has_workflow_rules?
-            workflow_config[:rules].present?
+            workflow_rules_config.present?
           end
 
-          def workflow_config
-            @command.yaml_processor_result.workflow_attributes || {}
+          def workflow_rules_config
+            strong_memoize(:workflow_rules_config) do
+              @command.yaml_processor_result.workflow_rules
+            end
           end
         end
       end

lib/gitlab/ci/pipeline/chain/seed.rb

@@ -38,9 +38,17 @@ module Gitlab
           def pipeline_seed
             strong_memoize(:pipeline_seed) do
               stages_attributes = @command.yaml_processor_result.stages_attributes
-              Gitlab::Ci::Pipeline::Seed::Pipeline.new(pipeline, stages_attributes)
+              Gitlab::Ci::Pipeline::Seed::Pipeline.new(context, stages_attributes)
             end
           end
+
+          def context
+            Gitlab::Ci::Pipeline::Seed::Context.new(pipeline, root_variables: root_variables)
+          end
+
+          def root_variables
+            @command.yaml_processor_result.root_variables
+          end
         end
       end
     end

lib/gitlab/ci/pipeline/seed/build.rb

@@ -11,8 +11,9 @@ module Gitlab
 
           delegate :dig, to: :@seed_attributes
 
-          def initialize(pipeline, attributes, previous_stages)
-            @pipeline = pipeline
+          def initialize(context, attributes, previous_stages)
+            @context = context
+            @pipeline = context.pipeline
             @seed_attributes = attributes
             @previous_stages = previous_stages
             @needs_attributes = dig(:needs_attributes)
@@ -29,7 +30,7 @@ module Gitlab
             @rules = Gitlab::Ci::Build::Rules
               .new(attributes.delete(:rules), default_when: 'on_success')
             @cache = Gitlab::Ci::Build::Cache
-              .new(attributes.delete(:cache), pipeline)
+              .new(attributes.delete(:cache), @pipeline)
           end
 
           def name

lib/gitlab/ci/pipeline/seed/context.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Pipeline
+      module Seed
+        class Context
+          attr_reader :pipeline, :root_variables
+
+          def initialize(pipeline, root_variables: [])
+            @pipeline = pipeline
+            @root_variables = root_variables
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/pipeline/seed/pipeline.rb

@@ -7,8 +7,8 @@ module Gitlab
         class Pipeline
           include Gitlab::Utils::StrongMemoize
 
-          def initialize(pipeline, stages_attributes)
-            @pipeline = pipeline
+          def initialize(context, stages_attributes)
+            @context = context
             @stages_attributes = stages_attributes
           end
 
@@ -37,7 +37,7 @@ module Gitlab
           def stage_seeds
             strong_memoize(:stage_seeds) do
               seeds = @stages_attributes.inject([]) do |previous_stages, attributes|
-                seed = Gitlab::Ci::Pipeline::Seed::Stage.new(@pipeline, attributes, previous_stages)
+                seed = Gitlab::Ci::Pipeline::Seed::Stage.new(@context, attributes, previous_stages)
                 previous_stages + [seed]
               end
 

lib/gitlab/ci/pipeline/seed/stage.rb

@@ -10,13 +10,14 @@ module Gitlab
           delegate :size, to: :seeds
           delegate :dig, to: :seeds
 
-          def initialize(pipeline, attributes, previous_stages)
-            @pipeline = pipeline
+          def initialize(context, attributes, previous_stages)
+            @context = context
+            @pipeline = context.pipeline
             @attributes = attributes
             @previous_stages = previous_stages
 
             @builds = attributes.fetch(:builds).map do |attributes|
-              Seed::Build.new(@pipeline, attributes, previous_stages)
+              Seed::Build.new(context, attributes, previous_stages)
             end
           end
 

lib/gitlab/ci/yaml_processor/result.rb

@@ -38,11 +38,12 @@ module Gitlab
             .map { |job| build_attributes(job[:name]) }
         end
 
-        def workflow_attributes
-          {
-            rules: hash_config.dig(:workflow, :rules),
-            yaml_variables: transform_to_yaml_variables(variables)
-          }
+        def workflow_rules
+          @workflow_rules ||= hash_config.dig(:workflow, :rules)
+        end
+
+        def root_variables
+          @root_variables ||= transform_to_yaml_variables(variables)
         end
 
         def jobs

spec/lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules_spec.rb

@@ -16,8 +16,10 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::EvaluateWorkflowRules do
   describe '#perform!' do
     context 'when pipeline has been skipped by workflow configuration' do
       before do
-        allow(step).to receive(:workflow_passed?)
-          .and_return(false)
+        allow(step).to receive(:workflow_rules_result)
+          .and_return(
+            double(pass?: false, variables: {})
+          )
 
         step.perform!
       end
@@ -33,12 +35,18 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::EvaluateWorkflowRules do
       it 'attaches an error to the pipeline' do
         expect(pipeline.errors[:base]).to include('Pipeline filtered out by workflow rules.')
       end
+
+      it 'saves workflow_rules_result' do
+        expect(command.workflow_rules_result.variables).to eq({})
+      end
     end
 
     context 'when pipeline has not been skipped by workflow configuration' do
       before do
-        allow(step).to receive(:workflow_passed?)
-          .and_return(true)
+        allow(step).to receive(:workflow_rules_result)
+          .and_return(
+            double(pass?: true, variables: { 'VAR1' => 'val2' })
+          )
 
         step.perform!
       end
@@ -55,6 +63,10 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::EvaluateWorkflowRules do
       it 'attaches no errors' do
         expect(pipeline.errors).to be_empty
       end
+
+      it 'saves workflow_rules_result' do
+        expect(command.workflow_rules_result.variables).to eq({ 'VAR1' => 'val2' })
+      end
     end
   end
 end

spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb

@@ -22,6 +22,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Populate do
     [
       Gitlab::Ci::Pipeline::Chain::Config::Content.new(pipeline, command),
       Gitlab::Ci::Pipeline::Chain::Config::Process.new(pipeline, command),
+      Gitlab::Ci::Pipeline::Chain::EvaluateWorkflowRules.new(pipeline, command),
       Gitlab::Ci::Pipeline::Chain::SeedBlock.new(pipeline, command),
       Gitlab::Ci::Pipeline::Chain::Seed.new(pipeline, command)
     ]

spec/lib/gitlab/ci/pipeline/chain/seed_spec.rb

@@ -20,7 +20,6 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
   describe '#perform!' do
     before do
       stub_ci_pipeline_yaml_file(YAML.dump(config))
-      run_chain
     end
 
     let(:config) do
@@ -30,21 +29,28 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
     subject(:run_chain) do
       [
         Gitlab::Ci::Pipeline::Chain::Config::Content.new(pipeline, command),
-        Gitlab::Ci::Pipeline::Chain::Config::Process.new(pipeline, command)
+        Gitlab::Ci::Pipeline::Chain::Config::Process.new(pipeline, command),
+        Gitlab::Ci::Pipeline::Chain::EvaluateWorkflowRules.new(pipeline, command)
       ].map(&:perform!)
 
       described_class.new(pipeline, command).perform!
     end
 
     it 'allocates next IID' do
+      run_chain
+
       expect(pipeline.iid).to be_present
     end
 
     it 'ensures ci_ref' do
+      run_chain
+
       expect(pipeline.ci_ref).to be_present
     end
 
     it 'sets the seeds in the command object' do
+      run_chain
+
       expect(command.pipeline_seed).to be_a(Gitlab::Ci::Pipeline::Seed::Pipeline)
       expect(command.pipeline_seed.size).to eq 1
     end
@@ -59,6 +65,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'correctly fabricates stages and builds' do
+        run_chain
+
         seed = command.pipeline_seed
 
         expect(seed.stages.size).to eq 2
@@ -84,6 +92,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'returns pipeline seed with jobs only assigned to master' do
+        run_chain
+
         seed = command.pipeline_seed
 
         expect(seed.size).to eq 1
@@ -103,6 +113,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'returns pipeline seed with jobs only assigned to schedules' do
+        run_chain
+
         seed = command.pipeline_seed
 
         expect(seed.size).to eq 1
@@ -130,6 +142,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
           let(:pipeline) { build(:ci_pipeline, project: project) }
 
           it 'returns seeds for kubernetes dependent job' do
+            run_chain
+
             seed = command.pipeline_seed
 
             expect(seed.size).to eq 2
@@ -141,6 +155,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
 
       context 'when kubernetes is not active' do
         it 'does not return seeds for kubernetes dependent job' do
+          run_chain
+
           seed = command.pipeline_seed
 
           expect(seed.size).to eq 1
@@ -158,6 +174,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'returns stage seeds only when variables expression is truthy' do
+        run_chain
+
         seed = command.pipeline_seed
 
         expect(seed.size).to eq 1
@@ -171,6 +189,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'does not execute the block' do
+        run_chain
+
         expect(pipeline.variables.size).to eq(0)
       end
     end

spec/lib/gitlab/ci/pipeline/seed/build_spec.rb

@@ -6,10 +6,12 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
   let_it_be(:project) { create(:project, :repository) }
   let_it_be(:head_sha) { project.repository.head_commit.id }
   let(:pipeline) { build(:ci_empty_pipeline, project: project, sha: head_sha) }
+  let(:root_variables) { [] }
+  let(:seed_context) { double(pipeline: pipeline, root_variables: root_variables) }
   let(:attributes) { { name: 'rspec', ref: 'master', scheduling_type: :stage } }
   let(:previous_stages) { [] }
 
-  let(:seed_build) { described_class.new(pipeline, attributes, previous_stages) }
+  let(:seed_build) { described_class.new(seed_context, attributes, previous_stages) }
 
   describe '#attributes' do
     subject { seed_build.attributes }
@@ -301,6 +303,26 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
         it { is_expected.to match a_hash_including(options: { allow_failure_criteria: nil }) }
       end
     end
+
+    context 'when the job rule depends on variables' do
+      let(:attributes) do
+        { name: 'rspec',
+          ref: 'master',
+          yaml_variables: [{ key: 'VAR1', value: 'var 1', public: true }],
+          rules: rules }
+      end
+
+      context 'when the rules use job variables' do
+        let(:rules) do
+          [{ if: '$VAR1 == "var 1"', variables: { VAR1: 'overridden var 1', VAR2: 'new var 2' } }]
+        end
+
+        it 'recalculates the variables' do
+          expect(subject[:yaml_variables]).to contain_exactly({ key: 'VAR1', value: 'overridden var 1', public: true },
+                                                              { key: 'VAR2', value: 'new var 2', public: true })
+        end
+      end
+    end
   end
 
   describe '#bridge?' do
@@ -377,7 +399,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
         it 'does not have environment' do
           expect(subject).not_to be_has_environment
           expect(subject.environment).to be_nil
-          expect(subject.metadata).to be_nil
+          expect(subject.metadata&.expanded_environment_name).to be_nil
           expect(Environment.exists?(name: expected_environment_name)).to eq(false)
         end
       end
@@ -1080,7 +1102,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
       end
 
       let(:stage_seed) do
-        Gitlab::Ci::Pipeline::Seed::Stage.new(pipeline, stage_attributes, [])
+        Gitlab::Ci::Pipeline::Seed::Stage.new(seed_context, stage_attributes, [])
       end
 
       let(:previous_stages) { [stage_seed] }

spec/lib/gitlab/ci/pipeline/seed/pipeline_spec.rb

@@ -6,6 +6,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Pipeline do
   let_it_be(:project) { create(:project, :repository) }
   let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
 
+  let(:seed_context) { double(pipeline: pipeline, root_variables: []) }
+
   let(:stages_attributes) do
     [
       {
@@ -29,7 +31,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Pipeline do
   end
 
   subject(:seed) do
-    described_class.new(pipeline, stages_attributes)
+    described_class.new(seed_context, stages_attributes)
   end
 
   describe '#stages' do

spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Stage do
   let(:project) { create(:project, :repository) }
   let(:pipeline) { create(:ci_empty_pipeline, project: project) }
   let(:previous_stages) { [] }
+  let(:seed_context) { double(pipeline: pipeline, root_variables: []) }
 
   let(:attributes) do
     { name: 'test',
@@ -16,7 +17,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Stage do
   end
 
   subject do
-    described_class.new(pipeline, attributes, previous_stages)
+    described_class.new(seed_context, attributes, previous_stages)
   end
 
   describe '#size' do

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -372,7 +372,7 @@ module Gitlab
         end
       end
 
-      describe '#workflow_attributes' do
+      describe 'workflow attributes' do
         context 'with disallowed workflow:variables' do
           let(:config) do
             <<-EOYML
@@ -403,11 +403,11 @@ module Gitlab
           end
 
           it 'parses the workflow:rules configuration' do
-            expect(subject.workflow_attributes[:rules]).to contain_exactly({ if: '$VAR == "value"' })
+            expect(subject.workflow_rules).to contain_exactly({ if: '$VAR == "value"' })
           end
 
-          it 'parses the root:variables as yaml_variables:' do
-            expect(subject.workflow_attributes[:yaml_variables])
+          it 'parses the root:variables as #root_variables' do
+            expect(subject.root_variables)
               .to contain_exactly({ key: 'SUPPORTED', value: 'parsed', public: true })
           end
         end
@@ -425,11 +425,11 @@ module Gitlab
           end
 
           it 'parses the workflow:rules configuration' do
-            expect(subject.workflow_attributes[:rules]).to contain_exactly({ if: '$VAR == "value"' })
+            expect(subject.workflow_rules).to contain_exactly({ if: '$VAR == "value"' })
           end
 
-          it 'parses the root:variables as yaml_variables:' do
-            expect(subject.workflow_attributes[:yaml_variables]).to eq([])
+          it 'parses the root:variables as #root_variables' do
+            expect(subject.root_variables).to eq([])
           end
         end
 
@@ -445,11 +445,11 @@ module Gitlab
           end
 
           it 'parses the workflow:rules configuration' do
-            expect(subject.workflow_attributes[:rules]).to be_nil
+            expect(subject.workflow_rules).to be_nil
           end
 
-          it 'parses the root:variables as yaml_variables:' do
-            expect(subject.workflow_attributes[:yaml_variables])
+          it 'parses the root:variables as #root_variables' do
+            expect(subject.root_variables)
               .to contain_exactly({ key: 'SUPPORTED', value: 'parsed', public: true })
           end
         end
@@ -463,11 +463,11 @@ module Gitlab
           end
 
           it 'parses the workflow:rules configuration' do
-            expect(subject.workflow_attributes[:rules]).to be_nil
+            expect(subject.workflow_rules).to be_nil
           end
 
-          it 'parses the root:variables as yaml_variables:' do
-            expect(subject.workflow_attributes[:yaml_variables]).to eq([])
+          it 'parses the root:variables as #root_variables' do
+            expect(subject.root_variables).to eq([])
           end
         end
       end

spec/services/ci/create_web_ide_terminal_service_spec.rb

@@ -66,6 +66,25 @@ RSpec.describe Ci::CreateWebIdeTerminalService do
 
           it_behaves_like 'be successful'
         end
+
+        context 'for configuration with variables' do
+          let(:config_content) do
+            <<-EOS
+              terminal:
+                script: rspec
+                variables:
+                  KEY1: VAL1
+            EOS
+          end
+
+          it_behaves_like 'be successful'
+
+          it 'saves the variables' do
+            expect(subject[:pipeline].builds[0].variables).to include(
+              key: 'KEY1', value: 'VAL1', public: true, masked: false
+            )
+          end
+        end
       end
     end