Merge branch '206929-fix-workflow-rules-to-access-variables' into 'master'

Fix workflow:rules not accessing passed-upstream and trigger variables

See merge request gitlab-org/gitlab!45674

app/services/ci/create_pipeline_service.rb

@@ -14,6 +14,7 @@ module Ci
                 Gitlab::Ci::Pipeline::Chain::Config::Process,
                 Gitlab::Ci::Pipeline::Chain::RemoveUnwantedChatJobs,
                 Gitlab::Ci::Pipeline::Chain::Skip,
+                Gitlab::Ci::Pipeline::Chain::SeedBlock,
                 Gitlab::Ci::Pipeline::Chain::EvaluateWorkflowRules,
                 Gitlab::Ci::Pipeline::Chain::Seed,
                 Gitlab::Ci::Pipeline::Chain::Limit::Size,

config/feature_flags/development/ci_seed_block_run_before_workflow_rules.yml

+---
+name: ci_seed_block_run_before_workflow_rules
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45674
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/270439
+type: development
+group: group::pipeline authoring
+default_enabled: false

lib/gitlab/ci/features.rb

@@ -62,6 +62,10 @@ module Gitlab
       def self.manual_bridges_enabled?(project)
         ::Feature.enabled?(:ci_manual_bridges, project, default_enabled: true)
       end
+
+      def self.seed_block_run_before_workflow_rules_enabled?(project)
+        ::Feature.enabled?(:ci_seed_block_run_before_workflow_rules, project, default_enabled: false)
+      end
     end
   end
 end

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

@@ -19,10 +19,12 @@ module Gitlab
             # Build to prevent erroring out on ambiguous refs.
             pipeline.protected = @command.protected_ref?
 
+            unless ::Gitlab::Ci::Features.seed_block_run_before_workflow_rules_enabled?(project)
               ##
               # Populate pipeline with block argument of CreatePipelineService#execute.
               #
               @command.seeds_block&.call(pipeline)
+            end
 
             ##
             # Gather all runtime build/stage errors

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class SeedBlock < Chain::Base
+          include Chain::Helpers
+          include Gitlab::Utils::StrongMemoize
+
+          def perform!
+            return unless ::Gitlab::Ci::Features.seed_block_run_before_workflow_rules_enabled?(project)
+
+            ##
+            # Populate pipeline with block argument of CreatePipelineService#execute.
+            #
+            @command.seeds_block&.call(pipeline)
+
+            raise "Pipeline cannot be persisted by `seeds_block`" if pipeline.persisted?
+          end
+
+          def break?
+            return false unless ::Gitlab::Ci::Features.seed_block_run_before_workflow_rules_enabled?(project)
+
+            pipeline.errors.any?
+          end
+        end
+      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::SeedBlock.new(pipeline, command),
       Gitlab::Ci::Pipeline::Chain::Seed.new(pipeline, command)
     ]
   end
@@ -180,23 +181,21 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Populate do
         ->(pipeline) { pipeline.variables.create!(key: 'VAR', value: '123') }
       end
 
-      it 'wastes pipeline iid' do
-        expect { run_chain }.to raise_error(ActiveRecord::RecordNotSaved)
-
-        last_iid = InternalId.ci_pipelines
-          .where(project_id: project.id)
-          .last.last_value
-
-        expect(last_iid).to be > 0
+      it 'raises error' do
+        expect { run_chain }.to raise_error(ActiveRecord::RecordNotSaved,
+                                            'You cannot call create unless the parent is saved')
       end
     end
   end
 
   context 'when pipeline gets persisted during the process' do
-    let(:pipeline) { create(:ci_pipeline, project: project) }
+    before do
+      dependencies.each(&:perform!)
+      pipeline.save!
+    end
 
     it 'raises error' do
-      expect { run_chain }.to raise_error(described_class::PopulateError)
+      expect { step.perform! }.to raise_error(described_class::PopulateError)
     end
   end
 

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Pipeline::Chain::SeedBlock do
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user, developer_projects: [project]) }
+  let(:seeds_block) { }
+
+  let(:command) do
+    Gitlab::Ci::Pipeline::Chain::Command.new(
+      project: project,
+      current_user: user,
+      origin_ref: 'master',
+      seeds_block: seeds_block)
+  end
+
+  let(:pipeline) { build(:ci_pipeline, project: project) }
+
+  describe '#perform!' do
+    before do
+      stub_ci_pipeline_yaml_file(YAML.dump(config))
+    end
+
+    subject(:run_chain) do
+      [
+        Gitlab::Ci::Pipeline::Chain::Config::Content.new(pipeline, command),
+        Gitlab::Ci::Pipeline::Chain::Config::Process.new(pipeline, command)
+      ].map(&:perform!)
+
+      described_class.new(pipeline, command).perform!
+    end
+
+    let(:config) do
+      { rspec: { script: 'rake' } }
+    end
+
+    context 'when there is not seeds_block' do
+      it 'does nothing' do
+        expect { run_chain }.not_to raise_error
+      end
+    end
+
+    context 'when there is seeds_block' do
+      let(:seeds_block) do
+        ->(pipeline) { pipeline.variables.build(key: 'VAR', value: '123') }
+      end
+
+      it 'executes the block' do
+        run_chain
+
+        expect(pipeline.variables.size).to eq(1)
+      end
+
+      context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+        before do
+          stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+        end
+
+        it 'does not execute the block' do
+          run_chain
+
+          expect(pipeline.variables.size).to eq(0)
+        end
+      end
+    end
+
+    context 'when the seeds_block tries to save the pipelie' do
+      let(:seeds_block) do
+        ->(pipeline) { pipeline.save! }
+      end
+
+      it 'raises error' do
+        expect { run_chain }.to raise_error('Pipeline cannot be persisted by `seeds_block`')
+      end
+    end
+  end
+end

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

@@ -5,22 +5,14 @@ require 'spec_helper'
 RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user, developer_projects: [project]) }
+  let(:seeds_block) { }
 
   let(:command) do
     Gitlab::Ci::Pipeline::Chain::Command.new(
       project: project,
       current_user: user,
       origin_ref: 'master',
-      seeds_block: nil)
-  end
-
-  def run_chain(pipeline, command)
-    [
-      Gitlab::Ci::Pipeline::Chain::Config::Content.new(pipeline, command),
-      Gitlab::Ci::Pipeline::Chain::Config::Process.new(pipeline, command)
-    ].map(&:perform!)
-
-    described_class.new(pipeline, command).perform!
+      seeds_block: seeds_block)
   end
 
   let(:pipeline) { build(:ci_pipeline, project: project) }
@@ -28,22 +20,36 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
   describe '#perform!' do
     before do
       stub_ci_pipeline_yaml_file(YAML.dump(config))
-      run_chain(pipeline, command)
     end
 
     let(:config) do
       { rspec: { script: 'rake' } }
     end
 
+    subject(:run_chain) do
+      [
+        Gitlab::Ci::Pipeline::Chain::Config::Content.new(pipeline, command),
+        Gitlab::Ci::Pipeline::Chain::Config::Process.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.stage_seeds).to all(be_a Gitlab::Ci::Pipeline::Seed::Base)
       expect(command.stage_seeds.count).to eq 1
     end
@@ -58,6 +64,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'correctly fabricates a stage seeds object' do
+        run_chain
+
         seeds = command.stage_seeds
         expect(seeds.size).to eq 2
         expect(seeds.first.attributes[:name]).to eq 'test'
@@ -81,6 +89,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'returns stage seeds only assigned to master' do
+        run_chain
+
         seeds = command.stage_seeds
 
         expect(seeds.size).to eq 1
@@ -100,6 +110,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'returns stage seeds only assigned to schedules' do
+        run_chain
+
         seeds = command.stage_seeds
 
         expect(seeds.size).to eq 1
@@ -127,6 +139,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
+
             seeds = command.stage_seeds
 
             expect(seeds.size).to eq 2
@@ -138,6 +152,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
+
           seeds = command.stage_seeds
 
           expect(seeds.size).to eq 1
@@ -155,11 +171,39 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'returns stage seeds only when variables expression is truthy' do
+        run_chain
+
         seeds = command.stage_seeds
 
         expect(seeds.size).to eq 1
         expect(seeds.dig(0, 0, :name)).to eq 'unit'
       end
     end
+
+    context 'when there is seeds_block' do
+      let(:seeds_block) do
+        ->(pipeline) { pipeline.variables.build(key: 'VAR', value: '123') }
+      end
+
+      context 'when FF ci_seed_block_run_before_workflow_rules is enabled' do
+        it 'does not execute the block' do
+          run_chain
+
+          expect(pipeline.variables.size).to eq(0)
+        end
+      end
+
+      context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+        before do
+          stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+        end
+
+        it 'executes the block' do
+          run_chain
+
+          expect(pipeline.variables.size).to eq(1)
+        end
+      end
+    end
   end
 end

spec/services/ci/create_downstream_pipeline_service_spec.rb

@@ -581,5 +581,40 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
         )
       end
     end
+
+    context 'when downstream pipeline has workflow rule' do
+      before do
+        stub_ci_pipeline_yaml_file(config)
+      end
+
+      let(:config) do
+        <<-EOY
+          workflow:
+            rules:
+              - if: $my_var
+
+          regular-job:
+            script: 'echo Hello, World!'
+        EOY
+      end
+
+      context 'when passing the required variable' do
+        before do
+          bridge.yaml_variables = [{ key: 'my_var', value: 'var', public: true }]
+        end
+
+        it 'creates the pipeline' do
+          expect { service.execute(bridge) }.to change(downstream_project.ci_pipelines, :count).by(1)
+
+          expect(bridge.reload).to be_success
+        end
+      end
+
+      context 'when not passing the required variable' do
+        it 'does not create the pipeline' do
+          expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count)
+        end
+      end
+    end
   end
 end

spec/services/ci/create_pipeline_service_spec.rb

@@ -41,7 +41,9 @@ RSpec.describe Ci::CreatePipelineService do
         save_on_errors: save_on_errors,
         trigger_request: trigger_request,
         merge_request: merge_request,
-        external_pull_request: external_pull_request)
+        external_pull_request: external_pull_request) do |pipeline|
+        yield(pipeline) if block_given?
+      end
     end
     # rubocop:enable Metrics/ParameterLists
 
@@ -2274,6 +2276,207 @@ RSpec.describe Ci::CreatePipelineService do
           end
         end
       end
+
+      context 'with workflow rules with persisted variables' do
+        let(:config) do
+          <<-EOY
+            workflow:
+              rules:
+                - if: $CI_COMMIT_REF_NAME == "master"
+
+            regular-job:
+              script: 'echo Hello, World!'
+          EOY
+        end
+
+        context 'with matches' do
+          it 'creates a pipeline' do
+            expect(pipeline).to be_persisted
+            expect(build_names).to contain_exactly('regular-job')
+          end
+        end
+
+        context 'with no matches' do
+          let(:ref_name) { 'refs/heads/feature' }
+
+          it 'does not create a pipeline' do
+            expect(pipeline).not_to be_persisted
+          end
+        end
+      end
+
+      context 'with workflow rules with pipeline variables' do
+        let(:pipeline) do
+          execute_service(variables_attributes: variables_attributes)
+        end
+
+        let(:config) do
+          <<-EOY
+            workflow:
+              rules:
+                - if: $SOME_VARIABLE
+
+            regular-job:
+              script: 'echo Hello, World!'
+          EOY
+        end
+
+        context 'with matches' do
+          let(:variables_attributes) do
+            [{ key: 'SOME_VARIABLE', secret_value: 'SOME_VAR' }]
+          end
+
+          it 'creates a pipeline' do
+            expect(pipeline).to be_persisted
+            expect(build_names).to contain_exactly('regular-job')
+          end
+        end
+
+        context 'with no matches' do
+          let(:variables_attributes) { {} }
+
+          it 'does not create a pipeline' do
+            expect(pipeline).not_to be_persisted
+          end
+        end
+      end
+
+      context 'with workflow rules with trigger variables' do
+        let(:pipeline) do
+          execute_service do |pipeline|
+            pipeline.variables.build(variables)
+          end
+        end
+
+        let(:config) do
+          <<-EOY
+            workflow:
+              rules:
+                - if: $SOME_VARIABLE
+
+            regular-job:
+              script: 'echo Hello, World!'
+          EOY
+        end
+
+        context 'with matches' do
+          let(:variables) do
+            [{ key: 'SOME_VARIABLE', secret_value: 'SOME_VAR' }]
+          end
+
+          it 'creates a pipeline' do
+            expect(pipeline).to be_persisted
+            expect(build_names).to contain_exactly('regular-job')
+          end
+
+          context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+            before do
+              stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+            end
+
+            it 'does not a pipeline' do
+              expect(pipeline).not_to be_persisted
+            end
+          end
+
+          context 'when a job requires the same variable' do
+            let(:config) do
+              <<-EOY
+                workflow:
+                  rules:
+                    - if: $SOME_VARIABLE
+
+                build:
+                  stage: build
+                  script: 'echo build'
+                  rules:
+                    - if: $SOME_VARIABLE
+
+                test1:
+                  stage: test
+                  script: 'echo test1'
+                  needs: [build]
+
+                test2:
+                  stage: test
+                  script: 'echo test2'
+              EOY
+            end
+
+            it 'creates a pipeline' do
+              expect(pipeline).to be_persisted
+              expect(build_names).to contain_exactly('build', 'test1', 'test2')
+            end
+
+            context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+              before do
+                stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+              end
+
+              it 'does not a pipeline' do
+                expect(pipeline).not_to be_persisted
+              end
+            end
+          end
+        end
+
+        context 'with no matches' do
+          let(:variables) { {} }
+
+          it 'does not create a pipeline' do
+            expect(pipeline).not_to be_persisted
+          end
+
+          context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+            before do
+              stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+            end
+
+            it 'does not create a pipeline' do
+              expect(pipeline).not_to be_persisted
+            end
+          end
+
+          context 'when a job requires the same variable' do
+            let(:config) do
+              <<-EOY
+                workflow:
+                  rules:
+                    - if: $SOME_VARIABLE
+
+                build:
+                  stage: build
+                  script: 'echo build'
+                  rules:
+                    - if: $SOME_VARIABLE
+
+                test1:
+                  stage: test
+                  script: 'echo test1'
+                  needs: [build]
+
+                test2:
+                  stage: test
+                  script: 'echo test2'
+              EOY
+            end
+
+            it 'does not create a pipeline' do
+              expect(pipeline).not_to be_persisted
+            end
+
+            context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+              before do
+                stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+              end
+
+              it 'does not create a pipeline' do
+                expect(pipeline).not_to be_persisted
+              end
+            end
+          end
+        end
+      end
     end
   end