From b66dfc8e646188e3f27a1a76acb046eff427a7f5 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Mon, 22 Mar 2021 18:52:12 +0300 Subject: [PATCH] Refactor workflow rules config passing It will help us to implement workflow:rules:variables --- .../ci/create_web_ide_terminal_service.rb | 3 +- lib/gitlab/ci/pipeline/chain/command.rb | 2 +- .../pipeline/chain/evaluate_workflow_rules.rb | 22 ++++++++++----- lib/gitlab/ci/pipeline/chain/seed.rb | 10 ++++++- lib/gitlab/ci/pipeline/seed/build.rb | 7 +++-- lib/gitlab/ci/pipeline/seed/context.rb | 18 ++++++++++++ lib/gitlab/ci/pipeline/seed/pipeline.rb | 6 ++-- lib/gitlab/ci/pipeline/seed/stage.rb | 7 +++-- lib/gitlab/ci/yaml_processor/result.rb | 11 ++++---- .../chain/evaluate_workflow_rules_spec.rb | 20 ++++++++++--- .../gitlab/ci/pipeline/chain/populate_spec.rb | 1 + .../lib/gitlab/ci/pipeline/chain/seed_spec.rb | 24 ++++++++++++++-- .../lib/gitlab/ci/pipeline/seed/build_spec.rb | 28 +++++++++++++++++-- .../gitlab/ci/pipeline/seed/pipeline_spec.rb | 4 ++- .../lib/gitlab/ci/pipeline/seed/stage_spec.rb | 3 +- spec/lib/gitlab/ci/yaml_processor_spec.rb | 26 ++++++++--------- .../create_web_ide_terminal_service_spec.rb | 19 +++++++++++++ 17 files changed, 163 insertions(+), 48 deletions(-) create mode 100644 lib/gitlab/ci/pipeline/seed/context.rb diff --git a/app/services/ci/create_web_ide_terminal_service.rb b/app/services/ci/create_web_ide_terminal_service.rb index 785d82094b9d51..3b89a599180edc 100644 --- a/app/services/ci/create_web_ide_terminal_service.rb +++ b/app/services/ci/create_web_ide_terminal_service.rb @@ -58,7 +58,8 @@ def terminal_stage_seed(pipeline) 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 diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index 815fe6bac6de95..46ecb10ea2b01b 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -12,7 +12,7 @@ module Chain :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 diff --git a/lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules.rb b/lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules.rb index 3c910963a2ada5..cceaa52de16d43 100644 --- a/lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules.rb +++ b/lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules.rb @@ -9,6 +9,8 @@ class EvaluateWorkflowRules < Chain::Base 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 @@ def break? 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 diff --git a/lib/gitlab/ci/pipeline/chain/seed.rb b/lib/gitlab/ci/pipeline/chain/seed.rb index 7b537125b9bd94..46524e8999f648 100644 --- a/lib/gitlab/ci/pipeline/chain/seed.rb +++ b/lib/gitlab/ci/pipeline/chain/seed.rb @@ -38,9 +38,17 @@ def break? 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 diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index 11b01822e4b685..b95a23d226330f 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -11,8 +11,9 @@ class Build < Seed::Base 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 @@ def initialize(pipeline, attributes, previous_stages) @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 diff --git a/lib/gitlab/ci/pipeline/seed/context.rb b/lib/gitlab/ci/pipeline/seed/context.rb new file mode 100644 index 00000000000000..6194a78f682dde --- /dev/null +++ b/lib/gitlab/ci/pipeline/seed/context.rb @@ -0,0 +1,18 @@ +# 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 diff --git a/lib/gitlab/ci/pipeline/seed/pipeline.rb b/lib/gitlab/ci/pipeline/seed/pipeline.rb index da9d853cf68c01..e1a15fb8d5bc1a 100644 --- a/lib/gitlab/ci/pipeline/seed/pipeline.rb +++ b/lib/gitlab/ci/pipeline/seed/pipeline.rb @@ -7,8 +7,8 @@ module Seed 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 @@ def deployments_count 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 diff --git a/lib/gitlab/ci/pipeline/seed/stage.rb b/lib/gitlab/ci/pipeline/seed/stage.rb index b600df2f656834..c988ea10e416f2 100644 --- a/lib/gitlab/ci/pipeline/seed/stage.rb +++ b/lib/gitlab/ci/pipeline/seed/stage.rb @@ -10,13 +10,14 @@ class Stage < Seed::Base 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 diff --git a/lib/gitlab/ci/yaml_processor/result.rb b/lib/gitlab/ci/yaml_processor/result.rb index f2c75227002144..c266dfd75d3785 100644 --- a/lib/gitlab/ci/yaml_processor/result.rb +++ b/lib/gitlab/ci/yaml_processor/result.rb @@ -38,11 +38,12 @@ def stage_builds_attributes(stage) .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 diff --git a/spec/lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules_spec.rb index 4ae51ac8bf9c12..e30a78546afea5 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/evaluate_workflow_rules_spec.rb @@ -16,8 +16,10 @@ 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 @@ 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 @@ 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 diff --git a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb index 5506b079d0f780..53d8ca740b506a 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb @@ -22,6 +22,7 @@ [ 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) ] diff --git a/spec/lib/gitlab/ci/pipeline/chain/seed_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/seed_spec.rb index 80013cab6ee3c4..1326b3b0fb5359 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/seed_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/seed_spec.rb @@ -20,7 +20,6 @@ describe '#perform!' do before do stub_ci_pipeline_yaml_file(YAML.dump(config)) - run_chain end let(:config) do @@ -30,21 +29,28 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ end it 'does not execute the block' do + run_chain + expect(pipeline.variables.size).to eq(0) end end diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index 7ec6949f852e4f..10f2f42e268a8a 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -6,10 +6,12 @@ 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 @@ 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 @@ 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 @@ 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] } diff --git a/spec/lib/gitlab/ci/pipeline/seed/pipeline_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/pipeline_spec.rb index 860b07647bdbdd..21be8660def00a 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/pipeline_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/pipeline_spec.rb @@ -6,6 +6,8 @@ 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 @@ end subject(:seed) do - described_class.new(pipeline, stages_attributes) + described_class.new(seed_context, stages_attributes) end describe '#stages' do diff --git a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb index 4b9db9fa6c6793..5b04d2abd88d4a 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb @@ -6,6 +6,7 @@ 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 @@ end subject do - described_class.new(pipeline, attributes, previous_stages) + described_class.new(seed_context, attributes, previous_stages) end describe '#size' do diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index 5462a587d16908..4f45eb2c985905 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -372,7 +372,7 @@ module Ci 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 Ci 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 Ci 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 Ci 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 Ci 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 diff --git a/spec/services/ci/create_web_ide_terminal_service_spec.rb b/spec/services/ci/create_web_ide_terminal_service_spec.rb index c1c94e30018361..c1acf8fd60cef1 100644 --- a/spec/services/ci/create_web_ide_terminal_service_spec.rb +++ b/spec/services/ci/create_web_ide_terminal_service_spec.rb @@ -66,6 +66,25 @@ 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 -- GitLab