From 769ddb40acc8ff14feb18d377febc6b28dbdbc5e Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Thu, 30 Mar 2023 17:52:07 +0200 Subject: [PATCH 01/19] Add needs to rules on jobs --- app/assets/javascripts/editor/schema/ci.json | 29 ++ .../introduce_rules_with_needs.yml | 8 + doc/ci/yaml/index.md | 69 +++ lib/gitlab/ci/build/rules.rb | 34 +- lib/gitlab/ci/config/entry/rules/rule.rb | 7 +- lib/gitlab/ci/pipeline/seed/build.rb | 8 +- lib/gitlab/ci/yaml_processor.rb | 27 + spec/lib/gitlab/ci/build/rules_spec.rb | 43 ++ spec/lib/gitlab/ci/config/entry/rules_spec.rb | 29 ++ .../lib/gitlab/ci/pipeline/seed/build_spec.rb | 157 ++++++ spec/lib/gitlab/ci/yaml_processor_spec.rb | 188 +++++++ .../ci/create_pipeline_service/rules_spec.rb | 486 ++++++++++++++++++ 12 files changed, 1079 insertions(+), 6 deletions(-) create mode 100644 config/feature_flags/development/introduce_rules_with_needs.yml diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index 70f1dddcac9225..d617e22cf77f0a 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -774,6 +774,9 @@ }, "allow_failure": { "$ref": "#/definitions/allow_failure" + }, + "needs": { + "$ref": "#/definitions/needs" } } }, @@ -936,6 +939,32 @@ "markdownDescription": "Used in conjunction with 'when: delayed' to set how long to delay before starting a job. e.g. '5', 5 seconds, 30 minutes, 1 week, etc. [Learn More](https://docs.gitlab.com/ee/ci/jobs/job_control.html#run-a-job-after-a-delay).", "minLength": 1 }, + "needs": { + "description": "Use needs on rules to update jobs needs for specific conditions. Once specific conditions are met, jobs needs are reset with the needs and its attributes defined by the rule.", + "type": "array", + "items": { + "oneOf": [ + { + "type": "string" + }, + { + "type": "object", + "additionalProperties": false, + "properties": { + "job": { + "type": "string" + }, + "artifacts": { + "type": "boolean" + }, + "optional": { + "type": "boolean" + } + } + } + ] + } + }, "allow_failure": { "markdownDescription": "Allow job to fail. A failed job does not cause the pipeline to fail. [Learn More](https://docs.gitlab.com/ee/ci/yaml/#allow_failure).", "oneOf": [ diff --git a/config/feature_flags/development/introduce_rules_with_needs.yml b/config/feature_flags/development/introduce_rules_with_needs.yml new file mode 100644 index 00000000000000..943d5aa5f8ced4 --- /dev/null +++ b/config/feature_flags/development/introduce_rules_with_needs.yml @@ -0,0 +1,8 @@ +--- +name: introduce_rules_with_needs +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112725 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/394769 +milestone: '15.10' +type: development +group: group::pipeline authoring +default_enabled: false diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 8eb7e5a13dfd87..cc1e91f090b8b3 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -3666,6 +3666,75 @@ If the rule matches, then the job is a manual job with `allow_failure: true`. - The rule-level `rules:allow_failure` overrides the job-level [`allow_failure`](#allow_failure), and only applies when the specific rule triggers the job. +#### `rules:needs` + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/31581) in GitLab 15.10. + +Use `needs` on rules to update jobs needs for specific conditions. Once specific conditions are met, jobs needs are reset with the needs and its attributes defined by the rule. + +**Keyword type**: Job-specific. You can use it only as part of a job. + +**Possible inputs**: + +- An array of jobs names as strings or in a hash. +- An empty array (`[]`), to set the job needs to none when the specific condition is met. + +**Example of `rules:needs`**: + +```yaml +lint: + stage: test + needs: ['mac:rspec'] + rules: + - if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH + needs: ['mac:build'] # Override job needs defined + +mac:rspec: + stage: test + script: echo "Running rspec on mac..." + +mac:build: + stage: test + script: echo "Building mac..." +``` + +Paths of execution: + +- If the specific condition of the rule on lint job is met, the sequance of jobs is: `mac:build`, `lint`, `mac:rspec` +- If the specific condition of the rule on lint job is not met, the sequance of jobs is: `mac:rspec`, `lint`, `mac:build` + +```yaml +lint: + stage: test + needs: ['mac:rspec'] + rules: + - if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH + needs: + - job: 'mac:build' + optional: false + - mac:rspec + +mac:rspec: + stage: test + script: echo "Running rspec on mac..." + +mac:build: + stage: build + script: echo "Building mac..." +``` + +Paths of execution: + +- If the specific condition of the rule on lint job is met, the sequance of jobs is: `mac:build`, `lint`, `mac:rspec` +- If the specific condition of the rule on lint job is not met, the sequance of jobs is: `mac:rspec`, `lint`, `mac:build` + +See job `lint`. It has job need `mac:rspec` defined as a plain string in an array. Under rules, the job has two needs: `mac:build` defined as a hash and `mac:rspec` defined aslo as a plain string. All 3 methods of defining needs on jobs and job rules are valid. + +**Additional details**: + +- Once the specific condition is met, the rules needs override the jobs needs and the behavior is same as per [job needs](#needs). +- The needs on rules takes keywords: [artifacts](#needsartifacts) and [optional](#needsoptional). + #### `rules:variables` > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/209864) in GitLab 13.7. diff --git a/lib/gitlab/ci/build/rules.rb b/lib/gitlab/ci/build/rules.rb index dee95534b07554..cf46c3204484ea 100644 --- a/lib/gitlab/ci/build/rules.rb +++ b/lib/gitlab/ci/build/rules.rb @@ -6,18 +6,43 @@ module Build class Rules include ::Gitlab::Utils::StrongMemoize - Result = Struct.new(:when, :start_in, :allow_failure, :variables, :errors) do + Result = Struct.new(:when, :start_in, :allow_failure, :variables, :needs, :errors) do def build_attributes { when: self.when, options: { start_in: start_in }.compact, - allow_failure: allow_failure + allow_failure: allow_failure, + needs_attributes: format_needs(needs) }.compact end def pass? self.when != 'never' end + + def format_needs(needs) + return unless needs + + formatted_needs = [] + needs.map do |need| + if need.instance_of? Hash + formatted_needs << format_hash_need(need) + + elsif need.instance_of? String + formatted_needs << { name: need } + end + end + formatted_needs + end + + def format_hash_need(need) + hash_need = {} + hash_need[:name] = need[:job] + hash_need[:artifacts] = need[:artifacts] if need.key?(:artifacts) + hash_need[:optional] = need[:optional] if need.key?(:optional) + + hash_need + end end def initialize(rule_hashes, default_when:) @@ -33,13 +58,14 @@ def evaluate(pipeline, context) matched_rule.attributes[:when] || @default_when, matched_rule.attributes[:start_in], matched_rule.attributes[:allow_failure], - matched_rule.attributes[:variables] + matched_rule.attributes[:variables], + (matched_rule.attributes[:needs] if Feature.enabled?(:introduce_rules_with_needs, pipeline.project)) ) else Result.new('never') end rescue Rule::Clause::ParseError => e - Result.new('never', nil, nil, nil, [e.message]) + Result.new('never', nil, nil, nil, nil, [e.message]) end private diff --git a/lib/gitlab/ci/config/entry/rules/rule.rb b/lib/gitlab/ci/config/entry/rules/rule.rb index 63bf1b38ac68c0..18764a5354626c 100644 --- a/lib/gitlab/ci/config/entry/rules/rule.rb +++ b/lib/gitlab/ci/config/entry/rules/rule.rb @@ -9,7 +9,7 @@ class Rules::Rule < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Configurable include ::Gitlab::Config::Entry::Attributable - ALLOWED_KEYS = %i[if changes exists when start_in allow_failure variables].freeze + ALLOWED_KEYS = %i[if changes exists when start_in allow_failure variables needs].freeze ALLOWED_WHEN = %w[on_success on_failure always never manual delayed].freeze attributes :if, :exists, :when, :start_in, :allow_failure @@ -20,6 +20,11 @@ class Rules::Rule < ::Gitlab::Config::Entry::Node entry :variables, Entry::Variables, description: 'Environment variables to define for rule conditions.' + entry :needs, Entry::Needs, + description: 'Needs configuration to define for rule conditions.', + metadata: { allowed_needs: %i[job] }, + inherit: false + validations do validates :config, presence: true validates :config, type: { with: Hash } diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index 98f488d0f387b3..b0e0a070a1b90b 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -64,12 +64,18 @@ def errors strong_memoize_attr :errors def attributes - @seed_attributes + chain = @seed_attributes .deep_merge(pipeline_attributes) .deep_merge(rules_attributes) .deep_merge(allow_failure_criteria_attributes) .deep_merge(@cache.cache_attributes) .deep_merge(runner_tags) + + if rules_attributes[:needs_attributes].present? + chain = chain.deep_merge(scheduling_type: :dag) + end + + chain end def bridge? diff --git a/lib/gitlab/ci/yaml_processor.rb b/lib/gitlab/ci/yaml_processor.rb index 63242d60c85d43..04847f26012f40 100644 --- a/lib/gitlab/ci/yaml_processor.rb +++ b/lib/gitlab/ci/yaml_processor.rb @@ -62,6 +62,7 @@ def validate_job!(name, job) validate_job_stage!(name, job) validate_job_dependencies!(name, job) validate_job_needs!(name, job) + validate_job_rule_needs!(job) if job[:rules].present? validate_dynamic_child_pipeline_dependencies!(name, job) validate_job_environment!(name, job) end @@ -133,6 +134,32 @@ def validate_job_dependency!(name, dependency, dependency_type = 'dependency', o end end + def validate_job_rule_needs!(job) + job[:rules].each do |rule| + next unless rule[:needs].present? + + rule_needs = needs_from_strings_and_hashes(rule[:needs]) + + validate_duplicate_needs!(job[:name], rule_needs) + + rule_needs.each do |need| + validate_job_dependency!(job[:name], need, 'need') + end + end + end + + def needs_from_strings_and_hashes(rule_needs) + compiled_needs = [] + rule_needs.each do |need| + if need.instance_of? String + compiled_needs << need + elsif need.instance_of? Hash + compiled_needs << need[:job] + end + end + compiled_needs + end + def stage_index(name) stage = @jobs.dig(name.to_sym, :stage) @stages.index(stage) diff --git a/spec/lib/gitlab/ci/build/rules_spec.rb b/spec/lib/gitlab/ci/build/rules_spec.rb index e82dcd0254d24e..884026044a156a 100644 --- a/spec/lib/gitlab/ci/build/rules_spec.rb +++ b/spec/lib/gitlab/ci/build/rules_spec.rb @@ -181,6 +181,49 @@ end end + context 'with needs' do + context 'when single needs is specified' do + let(:rule_list) { [{ if: '$VAR == null', needs: ['test'] }] } + + it { is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, ['test'], nil)) } + end + + context 'when multiple needs is specified' do + let(:rule_list) { [{ if: '$VAR == null', needs: %w[test lint] }] } + + it { is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, %w[test lint], nil)) } + end + + context 'when empty needs is specified' do + let(:rule_list) { [{ if: '$VAR == null', needs: [] }] } + + it { is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, [], nil)) } + end + + context 'when needs is specified with additional attibutes' do + let(:rule_list) do + [{ if: '$VAR == null', needs: { + job: { + artifacts: true, + name: 'test', + optional: false, + when: 'never' + } + } }] + end + + it { + is_expected.to eq( + described_class::Result.new('on_success', nil, nil, nil, { job: { + artifacts: true, + name: 'test', + optional: false, + when: 'never' + } }, nil)) + } + end + end + context 'with variables' do context 'with matching rule' do let(:rule_list) { [{ if: '$VAR == null', variables: { MY_VAR: 'my var' } }] } diff --git a/spec/lib/gitlab/ci/config/entry/rules_spec.rb b/spec/lib/gitlab/ci/config/entry/rules_spec.rb index b0871f2345e0d3..629e97e7a3d4b9 100644 --- a/spec/lib/gitlab/ci/config/entry/rules_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules_spec.rb @@ -132,6 +132,35 @@ it { is_expected.to contain_exactly(first_rule, second_rule, third_rule) } end + + context 'with rule with needs' do + context 'with needs in array' do + let(:config) do + [{ if: '$THIS == "that"', when: 'never', needs: ['test'] }] + end + + it { is_expected.to eq(config) } + end + + context 'with a rule with needs in object' do + let(:config) do + [{ + if: '$THIS == "that"', + when: 'never', + needs: { + job: { + artifacts: true, + name: 'test', + optional: false, + when: 'never' + } + } + }] + end + + it { is_expected.to eq(config) } + end + end end describe '.default' do diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index 86a11111283242..cc9a43c96ecc78 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -109,6 +109,163 @@ end end + context 'with job:rules:[needs:]' do + context 'with a single rule in array' do + let(:job_needs_attributes) { [{ name: 'rspec' }] } + + context 'when job has needs set' do + context 'when rule evaluates to true' do + let(:attributes) do + { name: 'rspec', + ref: 'master', + needs_attributes: job_needs_attributes, + rules: [{ if: '$VAR == null', needs: ['build-job'] }] } + end + + it 'overrides the job needs' do + expect(subject).to include(needs_attributes: [{ name: 'build-job' }]) + end + end + + context 'when rule evaluates to false' do + let(:job_needs_attributes) { [{ name: 'rspec' }] } + let(:attributes) do + { name: 'rspec', + ref: 'master', + needs_attributes: job_needs_attributes, + rules: [{ if: '$VAR == true', needs: ['build-job'] }] } + end + + it 'keeps the job needs' do + expect(subject).to include(needs_attributes: job_needs_attributes) + end + end + end + + context 'with multiple rules' do + context 'when a rule evaluates to true' do + let(:attributes) do + { name: 'rspec', + ref: 'master', + needs_attributes: job_needs_attributes, + rules: [ + { if: '$VAR == true', needs: 'build-job' }, + { if: '$VAR2 == true', needs: 'lint' }, + { if: '$VAR3 == null', needs: %w[rspec lint] } + ] } + end + + it 'overrides the job needs' do + expect(subject).to include(needs_attributes: [{ name: 'rspec' }, { name: 'lint' }]) + end + end + + context 'when all rules evaluates to false' do + let(:attributes) do + { name: 'rspec', + ref: 'master', + needs_attributes: job_needs_attributes, + rules: [ + { if: '$VAR == true', needs: ['build-job'] }, + { if: '$VAR2 == true', needs: ['rspec'] }, + { if: '$VAR3 == true', needs: ['lint'] } + ] } + end + + it 'keeps the job needs' do + expect(subject).to include(needs_attributes: job_needs_attributes) + end + end + end + + context 'when job doesnt have needs set' do + context 'when rule evaluates to true' do + let(:attributes) do + { name: 'rspec', + ref: 'master', + rules: [{ if: '$VAR == null', needs: ['build-job'] }] } + end + + it 'sets the job needs' do + expect(subject).to include(needs_attributes: [{ name: 'build-job' }]) + end + end + + context 'when rule evaluates to false' do + let(:attributes) do + { name: 'rspec', + ref: 'master', + rules: [{ if: '$VAR == true', needs: ['build-job'] }] } + end + + it 'doesnt set the job needs' do + expect(subject).not_to include(:needs_attributes) + end + end + end + + context 'with multiple rules' do + context 'when a rule evaluates to true' do + let(:attributes) do + { name: 'rspec', + ref: 'master', + rules: [ + { if: '$VAR == true', needs: 'build-job' }, + { if: '$VAR2 == true', needs: 'lint' }, + { if: '$VAR3 == null', needs: %w[rspec lint] } + ] } + end + + it 'sets the job needs' do + expect(subject).to include(needs_attributes: [{ name: 'rspec' }, { name: 'lint' }]) + end + end + + context 'when all rules evaluates to false' do + let(:attributes) do + { name: 'rspec', + ref: 'master', + rules: [ + { if: '$VAR == true', needs: ['build-job'] }, + { if: '$VAR2 == true', needs: ['rspec'] }, + { if: '$VAR3 == true', needs: ['lint'] } + ] } + end + + it 'doesnt set the job needs' do + expect(subject).not_to include(:needs_attributes) + end + end + end + end + + context 'with needs with additional attributes' do + let(:job_needs_attributes) { [{ name: 'rspec' }] } + let(:attributes) do + { name: 'rspec', + ref: 'master', + needs_attributes: job_needs_attributes, + rules: + [ + { if: '$VAR == null', + needs: [{ + job: 'build-job', + optional: false, + artifacts: true + }] } + ] } + end + + it 'overrides the job needs with attributes' do + expect(subject[:needs_attributes]).to match_array([{ name: 'build-job', optional: false, artifacts: true }]) + end + + it 'overrides the scheduling type to dag' do + expect(subject[:scheduling_type]).to eq(:dag) + end + end + end + context 'with job:tags' do let(:attributes) do { diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index a35dd968cd6648..52377065809227 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -659,6 +659,194 @@ module Ci it_behaves_like 'has warnings and expected error', /build job: need test is not defined in current or prior stages/ end + + describe '#validate_job_rule_needs!' do + context 'needs as array' do + context 'single need in following state' do + let(:config) do + <<-EOYML + stages: + - lint + - test + lint_job: + stage: lint + script: 'echo lint_job' + rules: + - if: $var == null + needs: [test_job] + test_job: + stage: test + script: 'echo job' + rules: + - if: $var == null + EOYML + end + + it 'raises an error' do + expect(subject.errors).to eq(["lint_job job: need test_job is not defined in current or prior stages"]) + end + end + + context 'multiple needs in the following stage' do + let(:config) do + <<-EOYML + stages: + - lint + - test + lint_job: + stage: lint + script: 'echo lint_job' + rules: + - if: $var == null + needs: [test_job, test_job_2] + test_job: + stage: test + script: 'echo job' + rules: + - if: $var == null + test_job_2: + stage: test + script: 'echo job' + rules: + - if: $var == null + EOYML + end + + it 'raises an error' do + expect(subject.errors).to eq(["lint_job job: need test_job is not defined in current or prior stages"]) + end + end + + context 'single need in following state - hyphen need' do + let(:config) do + <<-EOYML + stages: + - lint + - test + lint_job: + stage: lint + script: 'echo lint_job' + rules: + - if: $var == null + needs:#{' '} + - test_job + test_job: + stage: test + script: 'echo job' + rules: + - if: $var == null + EOYML + end + + it 'raises an error' do + expect(subject.errors).to eq(["lint_job job: need test_job is not defined in current or prior stages"]) + end + end + + context 'when there are duplicate needs (string and hash)' do + let(:config) do + <<-EOYML + stages: + - test + test_job_1: + stage: test + script: 'echo lint_job' + rules: + - if: $var == null + needs: + - test_job_2 + - job: test_job_2 + artifacts: false + test_job_2: + stage: test + script: 'echo job' + rules: + - if: $var == null + EOYML + end + + it 'raises an error' do + expect(subject.errors).to eq(["test_job_1 has duplicate entries in the needs section."]) + end + end + end + + context 'rule needs as hash' do + context 'single hash need in following state' do + let(:config) do + <<-EOYML + stages: + - lint + - test + lint_job: + stage: lint + script: 'echo lint_job' + rules: + - if: $var == null + needs:#{' '} + - job: test_job + artifacts: false + optional: false + test_job: + stage: test + script: 'echo job' + rules: + - if: $var == null + EOYML + end + + it 'raises an error' do + expect(subject.errors).to eq(["lint_job job: need test_job is not defined in current or prior stages"]) + end + end + + context 'when needs have a duplicate' do + let(:config) do + <<-EOYML + stages: + - test + test_job_1: + stage: test + script: 'echo lint_job' + needs: + - job: test_job_2 + - job: test_job_2 + rules: + - if: $var == null + test_job_2: + stage: test + script: 'echo job' + rules: + - if: $var == null + EOYML + end + + it 'raises an error' do + expect(subject.errors).to eq(["test_job_1 has duplicate entries in the needs section."]) + end + end + end + + context 'job rule need does not exist' do + let(:config) do + <<-EOYML + build: + stage: build + script: echo + rules: + - when: always + test: + stage: test + script: echo + rules:#{' '} + - if: $var == null + needs: [unknown_job] + EOYML + end + + it_behaves_like 'has warnings and expected error', /test job: undefined need: unknown_job/ + end + end end end diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index 19f9e7e3e4a68f..2724107ed4759e 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -386,6 +386,492 @@ def find_job(name) expect(regular_job.allow_failure).to eq(true) end end + + context 'needs:' do + context 'when needs are in an array' do + context 'with introduce_rules_with_needs enabled' do + before do + stub_feature_flags(introduce_rules_with_needs: true) + end + + let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } + let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } + let(:ref) { 'refs/heads/master' } + + context 'when job has needs: set' do + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + + context 'when rule is returning array of needs' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + needs: [lint_job] + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: [lint_job, rspec_job] + - if: $CI_COMMIT_REF_NAME =~ /feature/ + EOY + end + + it 'overriddes the needs of the job as outcome of the rule' do + expect(pipeline).to be_persisted + + need_1 = job_with_needs.needs.first + need_2 = job_with_needs.needs.last + expect(job_with_needs.needs.count).to eq(2) + expect(need_1.class).to eq(Ci::BuildNeed) + expect(need_1.name).to eq("lint_job") + + expect(need_2.class).to eq(Ci::BuildNeed) + expect(need_2.name).to eq("rspec_job") + end + + it 'sets the scheduling type to dag' do + expect(job_with_needs.scheduling_type).to eq('dag') + end + end + + context 'when the second rule is true' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + needs: [rspec_job] + rules: + - if: $CI_COMMIT_REF_NAME =~ /feature/ + needs: [lint_job] + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: [lint_job, rspec_job] + EOY + end + + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + + it 'resets the needs of the job as outcome of the rule' do + expect(pipeline).to be_persisted + + need_1 = job_with_needs.needs.first + need_2 = job_with_needs.needs.last + expect(job_with_needs.needs.count).to eq(2) + expect(need_1.class).to eq(Ci::BuildNeed) + expect(need_1.name).to eq("lint_job") + + expect(need_2.class).to eq(Ci::BuildNeed) + expect(need_2.name).to eq("rspec_job") + end + end + end + + context 'when job doesnt have needs: set' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: [lint_job] + - if: $CI_COMMIT_REF_NAME =~ /feature/ + EOY + end + + let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } + + it 'sets the needs of the job as outcome of the rule' do + expect(pipeline).to be_persisted + + expect(job_without_needs.needs.count).to eq(1) + expect(job_without_needs.needs.first.class).to be(Ci::BuildNeed) + expect(job_without_needs.needs.first.name).to eq("lint_job") + end + end + end + + context 'with introduce_rules_with_needs disabled' do + before do + stub_feature_flags(introduce_rules_with_needs: false) + end + + context 'when rule is returning multiple needs' do + let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } + let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + + let(:ref) { 'refs/heads/master' } + + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + needs: [rspec_job] + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: [lint_job, rspec_job] + - if: $CI_COMMIT_REF_NAME =~ /feature/ + EOY + end + + it 'keeps the needs of the job' do + expect(pipeline).to be_persisted + + expect(job_with_needs.needs.count).to eq(1) + + need = job_with_needs.needs.first + expect(need.class).to eq(Ci::BuildNeed) + expect(need.name).to eq("rspec_job") + end + end + + context 'when job doesnt have needs: set' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: [lint_job] + - if: $CI_COMMIT_REF_NAME =~ /feature/ + EOY + end + + let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } + + it 'return an empty array' do + expect(pipeline).to be_persisted + + expect(job_without_needs.needs).to eq([]) + end + end + end + end + + context 'when needs are in a hash' do + context 'with introduce_rules_with_needs enabled' do + before do + stub_feature_flags(introduce_rules_with_needs: true) + end + + let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } + let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } + let(:ref) { 'refs/heads/master' } + + context 'when job has needs: set' do + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + + context 'when rule is returning array of needs' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + needs: + - job: lint_job + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: + - job: lint_job + - job: rspec_job + - if: $CI_COMMIT_REF_NAME =~ /feature/ + EOY + end + + it 'overriddes the needs of the job as outcome of the rule' do + expect(pipeline).to be_persisted + + need_1 = job_with_needs.needs.first + need_2 = job_with_needs.needs.last + expect(job_with_needs.needs.count).to eq(2) + expect(need_1.class).to eq(Ci::BuildNeed) + expect(need_1.name).to eq("lint_job") + + expect(need_2.class).to eq(Ci::BuildNeed) + expect(need_2.name).to eq("rspec_job") + end + + it 'sets the scheduling type to dag' do + expect(job_with_needs.scheduling_type).to eq('dag') + end + end + + context 'when the second rule is true' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + needs: + - job: rspec_job + rules: + - if: $CI_COMMIT_REF_NAME =~ /feature/ + needs: + - job: lint_job + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: + - job: lint_job + - job: rspec_job + EOY + end + + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + + it 'resets the needs of the job as outcome of the rule' do + expect(pipeline).to be_persisted + + need_1 = job_with_needs.needs.first + need_2 = job_with_needs.needs.last + expect(job_with_needs.needs.count).to eq(2) + expect(need_1.class).to eq(Ci::BuildNeed) + expect(need_1.name).to eq("lint_job") + + expect(need_2.class).to eq(Ci::BuildNeed) + expect(need_2.name).to eq("rspec_job") + end + end + end + + context 'when job doesnt have needs: set' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: + - job: lint_job + - if: $CI_COMMIT_REF_NAME =~ /feature/ + EOY + end + + let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } + + it 'sets the needs of the job as outcome of the rule' do + expect(pipeline).to be_persisted + + expect(job_without_needs.needs.count).to eq(1) + expect(job_without_needs.needs.first.class).to be(Ci::BuildNeed) + expect(job_without_needs.needs.first.name).to eq("lint_job") + end + end + + context 'when needs array and hash are present' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + lint_job_2: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: + - lint_job + - job: lint_job_2 + artifacts: false + optional: true + - job: rspec_job + artifacts: false + optional: true + EOY + end + + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + + it 'overiddes the needs of the job as outcome of the rule' do + expect(pipeline).to be_persisted + + lint_job = job_with_needs.needs.find_by(name: 'lint_job') + lint_job_2 = job_with_needs.needs.find_by(name: 'lint_job_2') + rspec_job = job_with_needs.needs.find_by(name: 'rspec_job') + + expect(job_with_needs.needs.count).to eq(3) + expect(lint_job.class).to eq(Ci::BuildNeed) + expect(lint_job.name).to eq("lint_job") + expect(lint_job.artifacts).to eq(true) # default + expect(lint_job.optional).to eq(false) # default + + expect(lint_job_2.class).to eq(Ci::BuildNeed) + expect(lint_job_2.name).to eq("lint_job_2") + expect(lint_job_2.artifacts).to eq(false) + expect(lint_job_2.optional).to eq(true) + + expect(rspec_job.class).to eq(Ci::BuildNeed) + expect(rspec_job.name).to eq("rspec_job") + expect(rspec_job.artifacts).to eq(false) + expect(rspec_job.optional).to eq(true) + end + + it 'sets the scheduling type to dag' do + expect(job_with_needs.scheduling_type).to eq('dag') + end + end + end + + context 'with introduce_rules_with_needs disabled' do + before do + stub_feature_flags(introduce_rules_with_needs: false) + end + + context 'when rule is returning multiple needs' do + let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } + let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + + let(:ref) { 'refs/heads/master' } + + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + needs:#{' '} + - job: rspec_job + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs:#{' '} + - job: lint_job + - if: $CI_COMMIT_REF_NAME =~ /feature/ + EOY + end + + it 'keeps the needs of the job' do + expect(pipeline).to be_persisted + + expect(job_with_needs.needs.count).to eq(1) + + need = job_with_needs.needs.first + expect(need.class).to eq(Ci::BuildNeed) + expect(need.name).to eq("rspec_job") + end + end + + context 'when job doesnt have needs: set' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs:#{' '} + - job: lint_job + - if: $CI_COMMIT_REF_NAME =~ /feature/ + EOY + end + + let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } + + it 'return an empty array' do + expect(pipeline).to be_persisted + + expect(job_without_needs.needs).to eq([]) + end + end + end + end + end end context 'changes:' do -- GitLab From 567373ca6fa29b6e1ec38773fddf3de9c2f4bf47 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Mon, 3 Apr 2023 22:14:39 +0200 Subject: [PATCH 02/19] Refactor to utilize the built in needs formatting --- lib/gitlab/ci/build/rules.rb | 38 +++------- lib/gitlab/ci/config/entry/rules/rule.rb | 3 +- lib/gitlab/ci/pipeline/seed/build.rb | 4 +- lib/gitlab/ci/yaml_processor.rb | 31 ++------ spec/lib/gitlab/ci/build/rules_spec.rb | 41 +++++++---- spec/lib/gitlab/ci/config/entry/rules_spec.rb | 29 -------- .../lib/gitlab/ci/pipeline/seed/build_spec.rb | 44 ++++++------ spec/lib/gitlab/ci/yaml_processor_spec.rb | 70 ++++++++++--------- .../ci/create_pipeline_service/rules_spec.rb | 10 +-- 9 files changed, 115 insertions(+), 155 deletions(-) diff --git a/lib/gitlab/ci/build/rules.rb b/lib/gitlab/ci/build/rules.rb index cf46c3204484ea..9c900896c77601 100644 --- a/lib/gitlab/ci/build/rules.rb +++ b/lib/gitlab/ci/build/rules.rb @@ -11,38 +11,13 @@ def build_attributes { when: self.when, options: { start_in: start_in }.compact, - allow_failure: allow_failure, - needs_attributes: format_needs(needs) + allow_failure: allow_failure }.compact end def pass? self.when != 'never' end - - def format_needs(needs) - return unless needs - - formatted_needs = [] - needs.map do |need| - if need.instance_of? Hash - formatted_needs << format_hash_need(need) - - elsif need.instance_of? String - formatted_needs << { name: need } - end - end - formatted_needs - end - - def format_hash_need(need) - hash_need = {} - hash_need[:name] = need[:job] - hash_need[:artifacts] = need[:artifacts] if need.key?(:artifacts) - hash_need[:optional] = need[:optional] if need.key?(:optional) - - hash_need - end end def initialize(rule_hashes, default_when:) @@ -54,12 +29,13 @@ def evaluate(pipeline, context) if @rule_list.nil? Result.new(@default_when) elsif matched_rule = match_rule(pipeline, context) + needs = return_needs(matched_rule) Result.new( matched_rule.attributes[:when] || @default_when, matched_rule.attributes[:start_in], matched_rule.attributes[:allow_failure], matched_rule.attributes[:variables], - (matched_rule.attributes[:needs] if Feature.enabled?(:introduce_rules_with_needs, pipeline.project)) + (needs if Feature.enabled?(:introduce_rules_with_needs, pipeline.project)) ) else Result.new('never') @@ -70,6 +46,14 @@ def evaluate(pipeline, context) private + def return_needs(matched_rule) + if matched_rule.attributes[:needs] == [] + [] + else + matched_rule.attributes.dig(:needs, :job) + end + end + def match_rule(pipeline, context) @rule_list.find { |rule| rule.matches?(pipeline, context) } end diff --git a/lib/gitlab/ci/config/entry/rules/rule.rb b/lib/gitlab/ci/config/entry/rules/rule.rb index 18764a5354626c..1e7f6056a651e9 100644 --- a/lib/gitlab/ci/config/entry/rules/rule.rb +++ b/lib/gitlab/ci/config/entry/rules/rule.rb @@ -51,7 +51,8 @@ class Rules::Rule < ::Gitlab::Config::Entry::Node def value config.merge( changes: (changes_value if changes_defined?), - variables: (variables_value if variables_defined?) + variables: (variables_value if variables_defined?), + needs: (needs_value if needs_defined?) ).compact end diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index b0e0a070a1b90b..52df1dcfdeb055 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -174,8 +174,10 @@ def rules_attributes rules_variables_result = ::Gitlab::Ci::Variables::Helpers.merge_variables( @seed_attributes[:yaml_variables], rules_result.variables ) + chain = rules_result.build_attributes.merge(yaml_variables: rules_variables_result) + chain = chain.merge(needs_attributes: rules_result[:needs]) if rules_result[:needs].present? && Feature.enabled?(:introduce_rules_with_needs, @pipeline.project) - rules_result.build_attributes.merge(yaml_variables: rules_variables_result) + chain end strong_memoize_attr :rules_attributes diff --git a/lib/gitlab/ci/yaml_processor.rb b/lib/gitlab/ci/yaml_processor.rb index 04847f26012f40..e80aaac0ec0845 100644 --- a/lib/gitlab/ci/yaml_processor.rb +++ b/lib/gitlab/ci/yaml_processor.rb @@ -62,7 +62,7 @@ def validate_job!(name, job) validate_job_stage!(name, job) validate_job_dependencies!(name, job) validate_job_needs!(name, job) - validate_job_rule_needs!(job) if job[:rules].present? + validate_job_rule_needs!(name, job) if job[:rules].present? validate_dynamic_child_pipeline_dependencies!(name, job) validate_job_environment!(name, job) end @@ -97,6 +97,8 @@ def validate_dynamic_child_pipeline_dependencies!(name, job) def validate_job_needs!(name, job) return unless needs = job.dig(:needs, :job) + # binding.pry + validate_duplicate_needs!(name, needs) needs.each do |need| @@ -134,30 +136,11 @@ def validate_job_dependency!(name, dependency, dependency_type = 'dependency', o end end - def validate_job_rule_needs!(job) - job[:rules].each do |rule| - next unless rule[:needs].present? - - rule_needs = needs_from_strings_and_hashes(rule[:needs]) + def validate_job_rule_needs!(name, job) + return unless job[:rules] - validate_duplicate_needs!(job[:name], rule_needs) - - rule_needs.each do |need| - validate_job_dependency!(job[:name], need, 'need') - end - end - end - - def needs_from_strings_and_hashes(rule_needs) - compiled_needs = [] - rule_needs.each do |need| - if need.instance_of? String - compiled_needs << need - elsif need.instance_of? Hash - compiled_needs << need[:job] - end - end - compiled_needs + # below validation is for job rules needs + job[:rules].each { |rule| validate_job_needs!(name, rule) } end def stage_index(name) diff --git a/spec/lib/gitlab/ci/build/rules_spec.rb b/spec/lib/gitlab/ci/build/rules_spec.rb index 884026044a156a..d4602a0ba2cdf3 100644 --- a/spec/lib/gitlab/ci/build/rules_spec.rb +++ b/spec/lib/gitlab/ci/build/rules_spec.rb @@ -183,43 +183,54 @@ context 'with needs' do context 'when single needs is specified' do - let(:rule_list) { [{ if: '$VAR == null', needs: ['test'] }] } + let(:rule_list) do + [{ if: '$VAR == null', needs: { job: [{ name: 'test', artifacts: true, optional: false }] } }] + end - it { is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, ['test'], nil)) } + it { + is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, + [{ name: 'test', artifacts: true, optional: false }], nil)) + } end - context 'when multiple needs is specified' do - let(:rule_list) { [{ if: '$VAR == null', needs: %w[test lint] }] } + context 'when multiple needs are specified' do + let(:rule_list) do + [{ if: '$VAR == null', + needs: { job: [{ name: 'test', artifacts: true, optional: false }, + { name: 'rspec', artifacts: true, optional: false }] } }] + end - it { is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, %w[test lint], nil)) } + it { + is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, + [{ name: 'test', artifacts: true, optional: false }, + { name: 'rspec', artifacts: true, optional: false }], nil)) + } end - context 'when empty needs is specified' do + context 'when there are no needs specified' do let(:rule_list) { [{ if: '$VAR == null', needs: [] }] } it { is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, [], nil)) } end - context 'when needs is specified with additional attibutes' do + context 'when need is specified with additional attibutes' do let(:rule_list) do [{ if: '$VAR == null', needs: { - job: { + job: [{ artifacts: true, name: 'test', optional: false, when: 'never' - } + }] } }] end it { is_expected.to eq( - described_class::Result.new('on_success', nil, nil, nil, { job: { - artifacts: true, - name: 'test', - optional: false, - when: 'never' - } }, nil)) + described_class::Result.new('on_success', nil, nil, nil, [{ artifacts: true, + name: 'test', + optional: false, + when: 'never' }], nil)) } end end diff --git a/spec/lib/gitlab/ci/config/entry/rules_spec.rb b/spec/lib/gitlab/ci/config/entry/rules_spec.rb index 629e97e7a3d4b9..b0871f2345e0d3 100644 --- a/spec/lib/gitlab/ci/config/entry/rules_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules_spec.rb @@ -132,35 +132,6 @@ it { is_expected.to contain_exactly(first_rule, second_rule, third_rule) } end - - context 'with rule with needs' do - context 'with needs in array' do - let(:config) do - [{ if: '$THIS == "that"', when: 'never', needs: ['test'] }] - end - - it { is_expected.to eq(config) } - end - - context 'with a rule with needs in object' do - let(:config) do - [{ - if: '$THIS == "that"', - when: 'never', - needs: { - job: { - artifacts: true, - name: 'test', - optional: false, - when: 'never' - } - } - }] - end - - it { is_expected.to eq(config) } - end - end end describe '.default' do diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index cc9a43c96ecc78..789d7e032ed542 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -119,7 +119,7 @@ { name: 'rspec', ref: 'master', needs_attributes: job_needs_attributes, - rules: [{ if: '$VAR == null', needs: ['build-job'] }] } + rules: [{ if: '$VAR == null', needs: { job: [{ name: 'build-job' }] } }] } end it 'overrides the job needs' do @@ -133,7 +133,7 @@ { name: 'rspec', ref: 'master', needs_attributes: job_needs_attributes, - rules: [{ if: '$VAR == true', needs: ['build-job'] }] } + rules: [{ if: '$VAR == true', needs: { job: [{ name: 'build-job' }] } }] } end it 'keeps the job needs' do @@ -149,9 +149,9 @@ ref: 'master', needs_attributes: job_needs_attributes, rules: [ - { if: '$VAR == true', needs: 'build-job' }, - { if: '$VAR2 == true', needs: 'lint' }, - { if: '$VAR3 == null', needs: %w[rspec lint] } + { if: '$VAR == true', needs: { job: [{ name: 'rspec-1' }] } }, + { if: '$VAR2 == true', needs: { job: [{ name: 'rspec-2' }] } }, + { if: '$VAR3 == null', needs: { job: [{ name: 'rspec' }, { name: 'lint' }] } } ] } end @@ -166,9 +166,9 @@ ref: 'master', needs_attributes: job_needs_attributes, rules: [ - { if: '$VAR == true', needs: ['build-job'] }, - { if: '$VAR2 == true', needs: ['rspec'] }, - { if: '$VAR3 == true', needs: ['lint'] } + { if: '$VAR == true', needs: { job: [{ name: 'rspec-1' }] } }, + { if: '$VAR2 == true', needs: { job: [{ name: 'rspec-2' }] } }, + { if: '$VAR3 == true', needs: { job: [{ name: 'rspec-3' }] } } ] } end @@ -183,7 +183,7 @@ let(:attributes) do { name: 'rspec', ref: 'master', - rules: [{ if: '$VAR == null', needs: ['build-job'] }] } + rules: [{ if: '$VAR == null', needs: { job: [{ name: 'build-job' }] } }] } end it 'sets the job needs' do @@ -195,7 +195,7 @@ let(:attributes) do { name: 'rspec', ref: 'master', - rules: [{ if: '$VAR == true', needs: ['build-job'] }] } + rules: [{ if: '$VAR == true', needs: { job: [{ name: 'build-job' }] } }] } end it 'doesnt set the job needs' do @@ -210,9 +210,9 @@ { name: 'rspec', ref: 'master', rules: [ - { if: '$VAR == true', needs: 'build-job' }, - { if: '$VAR2 == true', needs: 'lint' }, - { if: '$VAR3 == null', needs: %w[rspec lint] } + { if: '$VAR == true', needs: { job: [{ name: 'build-job' }] } }, + { if: '$VAR2 == true', needs: { job: [{ name: 'lint' }] } }, + { if: '$VAR3 == null', needs: { job: [{ name: 'rspec' }, { name: 'lint' }] } } ] } end @@ -226,9 +226,9 @@ { name: 'rspec', ref: 'master', rules: [ - { if: '$VAR == true', needs: ['build-job'] }, - { if: '$VAR2 == true', needs: ['rspec'] }, - { if: '$VAR3 == true', needs: ['lint'] } + { if: '$VAR == true', needs: { job: [{ name: 'build-job' }] } }, + { if: '$VAR2 == true', needs: { job: [{ name: 'lint' }] } }, + { if: '$VAR3 == true', needs: { job: [{ name: 'rspec' }, { name: 'lint' }] } } ] } end @@ -248,11 +248,13 @@ rules: [ { if: '$VAR == null', - needs: [{ - job: 'build-job', - optional: false, - artifacts: true - }] } + needs: { + job: [{ + name: 'build-job', + optional: false, + artifacts: true + }] + } } ] } end diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index 52377065809227..e74592d5ad9c99 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -661,8 +661,41 @@ module Ci end describe '#validate_job_rule_needs!' do + context "when all validations pass" do + let(:config) do + <<-EOYML + stages: + - lint + lint_job: + stage: lint + script: 'echo lint_job' + rules: + - if: $var == null + needs: + - lint_job_2 + - job: lint_job_3 + optional: true + lint_job_2: + stage: lint + script: 'echo job' + rules: + - if: $var == null + lint_job_3: + stage: lint + script: 'echo job' + rules: + - if: $var == null + EOYML + end + + it 'returns a valid response' do + expect(subject).to be_valid + expect(subject).to be_instance_of(Gitlab::Ci::YamlProcessor::Result) + end + end + context 'needs as array' do - context 'single need in following state' do + context 'single need in following stage' do let(:config) do <<-EOYML stages: @@ -728,7 +761,7 @@ module Ci script: 'echo lint_job' rules: - if: $var == null - needs:#{' '} + needs: - test_job test_job: stage: test @@ -756,7 +789,6 @@ module Ci needs: - test_job_2 - job: test_job_2 - artifacts: false test_job_2: stage: test script: 'echo job' @@ -772,7 +804,7 @@ module Ci end context 'rule needs as hash' do - context 'single hash need in following state' do + context 'single hash need in following stage' do let(:config) do <<-EOYML stages: @@ -783,7 +815,7 @@ module Ci script: 'echo lint_job' rules: - if: $var == null - needs:#{' '} + needs: - job: test_job artifacts: false optional: false @@ -799,32 +831,6 @@ module Ci expect(subject.errors).to eq(["lint_job job: need test_job is not defined in current or prior stages"]) end end - - context 'when needs have a duplicate' do - let(:config) do - <<-EOYML - stages: - - test - test_job_1: - stage: test - script: 'echo lint_job' - needs: - - job: test_job_2 - - job: test_job_2 - rules: - - if: $var == null - test_job_2: - stage: test - script: 'echo job' - rules: - - if: $var == null - EOYML - end - - it 'raises an error' do - expect(subject.errors).to eq(["test_job_1 has duplicate entries in the needs section."]) - end - end end context 'job rule need does not exist' do @@ -838,7 +844,7 @@ module Ci test: stage: test script: echo - rules:#{' '} + rules: - if: $var == null needs: [unknown_job] EOYML diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index 2724107ed4759e..5611589796af97 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -418,9 +418,8 @@ def find_job(name) needs: [lint_job] script: 'echo job' rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ + - if: $var == null needs: [lint_job, rspec_job] - - if: $CI_COMMIT_REF_NAME =~ /feature/ EOY end @@ -429,6 +428,7 @@ def find_job(name) need_1 = job_with_needs.needs.first need_2 = job_with_needs.needs.last + expect(job_with_needs.needs.count).to eq(2) expect(need_1.class).to eq(Ci::BuildNeed) expect(need_1.name).to eq("lint_job") @@ -817,11 +817,11 @@ def find_job(name) - when: always job: script: 'echo job' - needs:#{' '} + needs: - job: rspec_job rules: - if: $CI_COMMIT_REF_NAME =~ /master/ - needs:#{' '} + needs: - job: lint_job - if: $CI_COMMIT_REF_NAME =~ /feature/ EOY @@ -855,7 +855,7 @@ def find_job(name) script: 'echo job' rules: - if: $CI_COMMIT_REF_NAME =~ /master/ - needs:#{' '} + needs: - job: lint_job - if: $CI_COMMIT_REF_NAME =~ /feature/ EOY -- GitLab From 06414f25b2d287e154795638bd909112d800151d Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Wed, 5 Apr 2023 18:34:22 +0200 Subject: [PATCH 03/19] Fix typos etc --- .../introduce_rules_with_needs.yml | 2 +- lib/gitlab/ci/yaml_processor.rb | 2 - .../lib/gitlab/ci/pipeline/seed/build_spec.rb | 114 +++++------------- .../ci/create_pipeline_service/rules_spec.rb | 20 ++- 4 files changed, 36 insertions(+), 102 deletions(-) diff --git a/config/feature_flags/development/introduce_rules_with_needs.yml b/config/feature_flags/development/introduce_rules_with_needs.yml index 943d5aa5f8ced4..489a2867e47ed1 100644 --- a/config/feature_flags/development/introduce_rules_with_needs.yml +++ b/config/feature_flags/development/introduce_rules_with_needs.yml @@ -2,7 +2,7 @@ name: introduce_rules_with_needs introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112725 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/394769 -milestone: '15.10' +milestone: '15.11' type: development group: group::pipeline authoring default_enabled: false diff --git a/lib/gitlab/ci/yaml_processor.rb b/lib/gitlab/ci/yaml_processor.rb index e80aaac0ec0845..4ec83cbb9fbd9f 100644 --- a/lib/gitlab/ci/yaml_processor.rb +++ b/lib/gitlab/ci/yaml_processor.rb @@ -97,8 +97,6 @@ def validate_dynamic_child_pipeline_dependencies!(name, job) def validate_job_needs!(name, job) return unless needs = job.dig(:needs, :job) - # binding.pry - validate_duplicate_needs!(name, needs) needs.each do |need| diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index 789d7e032ed542..3fb87516bbca53 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -110,7 +110,7 @@ end context 'with job:rules:[needs:]' do - context 'with a single rule in array' do + context 'with a single rule' do let(:job_needs_attributes) { [{ name: 'rspec' }] } context 'when job has needs set' do @@ -140,66 +140,32 @@ expect(subject).to include(needs_attributes: job_needs_attributes) end end - end - - context 'with multiple rules' do - context 'when a rule evaluates to true' do - let(:attributes) do - { name: 'rspec', - ref: 'master', - needs_attributes: job_needs_attributes, - rules: [ - { if: '$VAR == true', needs: { job: [{ name: 'rspec-1' }] } }, - { if: '$VAR2 == true', needs: { job: [{ name: 'rspec-2' }] } }, - { if: '$VAR3 == null', needs: { job: [{ name: 'rspec' }, { name: 'lint' }] } } - ] } - end - - it 'overrides the job needs' do - expect(subject).to include(needs_attributes: [{ name: 'rspec' }, { name: 'lint' }]) - end - end - context 'when all rules evaluates to false' do + context 'with needs with additional attributes' do + let(:job_needs_attributes) { [{ name: 'rspec' }] } let(:attributes) do { name: 'rspec', ref: 'master', needs_attributes: job_needs_attributes, - rules: [ - { if: '$VAR == true', needs: { job: [{ name: 'rspec-1' }] } }, - { if: '$VAR2 == true', needs: { job: [{ name: 'rspec-2' }] } }, - { if: '$VAR3 == true', needs: { job: [{ name: 'rspec-3' }] } } + rules: + [ + { if: '$VAR == null', + needs: { + job: [{ + name: 'build-job', + optional: false, + artifacts: true + }] + } } ] } end - it 'keeps the job needs' do - expect(subject).to include(needs_attributes: job_needs_attributes) - end - end - end - - context 'when job doesnt have needs set' do - context 'when rule evaluates to true' do - let(:attributes) do - { name: 'rspec', - ref: 'master', - rules: [{ if: '$VAR == null', needs: { job: [{ name: 'build-job' }] } }] } - end - - it 'sets the job needs' do - expect(subject).to include(needs_attributes: [{ name: 'build-job' }]) + it 'overrides the job needs with attributes' do + expect(subject[:needs_attributes]).to match_array([{ name: 'build-job', optional: false, artifacts: true }]) end - end - context 'when rule evaluates to false' do - let(:attributes) do - { name: 'rspec', - ref: 'master', - rules: [{ if: '$VAR == true', needs: { job: [{ name: 'build-job' }] } }] } - end - - it 'doesnt set the job needs' do - expect(subject).not_to include(:needs_attributes) + it 'overrides the scheduling type to dag' do + expect(subject[:scheduling_type]).to eq(:dag) end end end @@ -209,14 +175,15 @@ let(:attributes) do { name: 'rspec', ref: 'master', + needs_attributes: job_needs_attributes, rules: [ - { if: '$VAR == true', needs: { job: [{ name: 'build-job' }] } }, - { if: '$VAR2 == true', needs: { job: [{ name: 'lint' }] } }, + { if: '$VAR == true', needs: { job: [{ name: 'rspec-1' }] } }, + { if: '$VAR2 == true', needs: { job: [{ name: 'rspec-2' }] } }, { if: '$VAR3 == null', needs: { job: [{ name: 'rspec' }, { name: 'lint' }] } } ] } end - it 'sets the job needs' do + it 'overrides the job needs' do expect(subject).to include(needs_attributes: [{ name: 'rspec' }, { name: 'lint' }]) end end @@ -225,47 +192,20 @@ let(:attributes) do { name: 'rspec', ref: 'master', + needs_attributes: job_needs_attributes, rules: [ - { if: '$VAR == true', needs: { job: [{ name: 'build-job' }] } }, - { if: '$VAR2 == true', needs: { job: [{ name: 'lint' }] } }, - { if: '$VAR3 == true', needs: { job: [{ name: 'rspec' }, { name: 'lint' }] } } + { if: '$VAR == true', needs: { job: [{ name: 'rspec-1' }] } }, + { if: '$VAR2 == true', needs: { job: [{ name: 'rspec-2' }] } }, + { if: '$VAR3 == true', needs: { job: [{ name: 'rspec-3' }] } } ] } end - it 'doesnt set the job needs' do - expect(subject).not_to include(:needs_attributes) + it 'keeps the job needs' do + expect(subject).to include(needs_attributes: job_needs_attributes) end end end end - - context 'with needs with additional attributes' do - let(:job_needs_attributes) { [{ name: 'rspec' }] } - let(:attributes) do - { name: 'rspec', - ref: 'master', - needs_attributes: job_needs_attributes, - rules: - [ - { if: '$VAR == null', - needs: { - job: [{ - name: 'build-job', - optional: false, - artifacts: true - }] - } } - ] } - end - - it 'overrides the job needs with attributes' do - expect(subject[:needs_attributes]).to match_array([{ name: 'build-job', optional: false, artifacts: true }]) - end - - it 'overrides the scheduling type to dag' do - expect(subject[:scheduling_type]).to eq(:dag) - end - end end context 'with job:tags' do diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index 5611589796af97..ede264b9bfdd36 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -423,7 +423,7 @@ def find_job(name) EOY end - it 'overriddes the needs of the job as outcome of the rule' do + it 'overrides the needs of the job' do expect(pipeline).to be_persisted need_1 = job_with_needs.needs.first @@ -468,7 +468,7 @@ def find_job(name) let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - it 'resets the needs of the job as outcome of the rule' do + it 'overrides the needs of the job' do expect(pipeline).to be_persisted need_1 = job_with_needs.needs.first @@ -501,13 +501,12 @@ def find_job(name) rules: - if: $CI_COMMIT_REF_NAME =~ /master/ needs: [lint_job] - - if: $CI_COMMIT_REF_NAME =~ /feature/ EOY end let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } - it 'sets the needs of the job as outcome of the rule' do + it 'sets the needs of the job' do expect(pipeline).to be_persisted expect(job_without_needs.needs.count).to eq(1) @@ -630,11 +629,10 @@ def find_job(name) needs: - job: lint_job - job: rspec_job - - if: $CI_COMMIT_REF_NAME =~ /feature/ EOY end - it 'overriddes the needs of the job as outcome of the rule' do + it 'overrides the needs of the job' do expect(pipeline).to be_persisted need_1 = job_with_needs.needs.first @@ -682,7 +680,7 @@ def find_job(name) let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - it 'resets the needs of the job as outcome of the rule' do + it 'overrides the needs of the job' do expect(pipeline).to be_persisted need_1 = job_with_needs.needs.first @@ -711,13 +709,12 @@ def find_job(name) - if: $CI_COMMIT_REF_NAME =~ /master/ needs: - job: lint_job - - if: $CI_COMMIT_REF_NAME =~ /feature/ EOY end let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } - it 'sets the needs of the job as outcome of the rule' do + it 'sets the needs of the job' do expect(pipeline).to be_persisted expect(job_without_needs.needs.count).to eq(1) @@ -761,7 +758,7 @@ def find_job(name) let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - it 'overiddes the needs of the job as outcome of the rule' do + it 'overrides the needs of the job' do expect(pipeline).to be_persisted lint_job = job_with_needs.needs.find_by(name: 'lint_job') @@ -823,7 +820,7 @@ def find_job(name) - if: $CI_COMMIT_REF_NAME =~ /master/ needs: - job: lint_job - - if: $CI_COMMIT_REF_NAME =~ /feature/ + - rspec_job EOY end @@ -857,7 +854,6 @@ def find_job(name) - if: $CI_COMMIT_REF_NAME =~ /master/ needs: - job: lint_job - - if: $CI_COMMIT_REF_NAME =~ /feature/ EOY end -- GitLab From bae62a810a2daa5b37ad7c93e8a1cb97c26d0046 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Wed, 5 Apr 2023 19:02:57 +0200 Subject: [PATCH 04/19] Remove rules check --- app/assets/javascripts/editor/schema/ci.json | 7 +++-- lib/gitlab/ci/build/rules.rb | 9 ++---- lib/gitlab/ci/pipeline/seed/build.rb | 8 +---- lib/gitlab/ci/yaml_processor.rb | 3 +- spec/lib/gitlab/ci/build/rules_spec.rb | 31 ++++++++++++++----- .../lib/gitlab/ci/pipeline/seed/build_spec.rb | 17 +++++----- spec/lib/gitlab/ci/yaml_processor_spec.rb | 20 +++--------- .../ci/create_pipeline_service/rules_spec.rb | 2 +- 8 files changed, 48 insertions(+), 49 deletions(-) diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index d617e22cf77f0a..7f3cdbc6196afb 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -940,7 +940,7 @@ "minLength": 1 }, "needs": { - "description": "Use needs on rules to update jobs needs for specific conditions. Once specific conditions are met, jobs needs are reset with the needs and its attributes defined by the rule.", + "markdownDescription": "Use needs on rules to update jobs needs for specific conditions. Once specific conditions are met, jobs needs are reset with the needs and its attributes defined by the rule.", "type": "array", "items": { "oneOf": [ @@ -960,7 +960,10 @@ "optional": { "type": "boolean" } - } + }, + "required": [ + "job" + ] } ] } diff --git a/lib/gitlab/ci/build/rules.rb b/lib/gitlab/ci/build/rules.rb index 9c900896c77601..f80b81b0e80841 100644 --- a/lib/gitlab/ci/build/rules.rb +++ b/lib/gitlab/ci/build/rules.rb @@ -11,7 +11,8 @@ def build_attributes { when: self.when, options: { start_in: start_in }.compact, - allow_failure: allow_failure + allow_failure: allow_failure, + scheduling_type: (:dag if needs) }.compact end @@ -47,11 +48,7 @@ def evaluate(pipeline, context) private def return_needs(matched_rule) - if matched_rule.attributes[:needs] == [] - [] - else - matched_rule.attributes.dig(:needs, :job) - end + matched_rule.attributes.dig(:needs, :job) unless matched_rule.attributes[:needs].nil? end def match_rule(pipeline, context) diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index 52df1dcfdeb055..1672f65bacdb21 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -64,18 +64,12 @@ def errors strong_memoize_attr :errors def attributes - chain = @seed_attributes + @seed_attributes .deep_merge(pipeline_attributes) .deep_merge(rules_attributes) .deep_merge(allow_failure_criteria_attributes) .deep_merge(@cache.cache_attributes) .deep_merge(runner_tags) - - if rules_attributes[:needs_attributes].present? - chain = chain.deep_merge(scheduling_type: :dag) - end - - chain end def bridge? diff --git a/lib/gitlab/ci/yaml_processor.rb b/lib/gitlab/ci/yaml_processor.rb index 4ec83cbb9fbd9f..3b5ad73838da0d 100644 --- a/lib/gitlab/ci/yaml_processor.rb +++ b/lib/gitlab/ci/yaml_processor.rb @@ -62,7 +62,7 @@ def validate_job!(name, job) validate_job_stage!(name, job) validate_job_dependencies!(name, job) validate_job_needs!(name, job) - validate_job_rule_needs!(name, job) if job[:rules].present? + validate_job_rule_needs!(name, job) validate_dynamic_child_pipeline_dependencies!(name, job) validate_job_environment!(name, job) end @@ -137,7 +137,6 @@ def validate_job_dependency!(name, dependency, dependency_type = 'dependency', o def validate_job_rule_needs!(name, job) return unless job[:rules] - # below validation is for job rules needs job[:rules].each { |rule| validate_job_needs!(name, rule) } end diff --git a/spec/lib/gitlab/ci/build/rules_spec.rb b/spec/lib/gitlab/ci/build/rules_spec.rb index d4602a0ba2cdf3..677796277f31ba 100644 --- a/spec/lib/gitlab/ci/build/rules_spec.rb +++ b/spec/lib/gitlab/ci/build/rules_spec.rb @@ -208,9 +208,9 @@ end context 'when there are no needs specified' do - let(:rule_list) { [{ if: '$VAR == null', needs: [] }] } + let(:rule_list) { [{ if: '$VAR == null' }] } - it { is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, [], nil)) } + it { is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, nil, nil)) } end context 'when need is specified with additional attibutes' do @@ -227,10 +227,8 @@ it { is_expected.to eq( - described_class::Result.new('on_success', nil, nil, nil, [{ artifacts: true, - name: 'test', - optional: false, - when: 'never' }], nil)) + described_class::Result.new('on_success', nil, nil, nil, + [{ artifacts: true, name: 'test', optional: false, when: 'never' }], nil)) } end end @@ -262,9 +260,10 @@ let(:start_in) { nil } let(:allow_failure) { nil } let(:variables) { nil } + let(:needs) { nil } subject(:result) do - Gitlab::Ci::Build::Rules::Result.new(when_value, start_in, allow_failure, variables) + Gitlab::Ci::Build::Rules::Result.new(when_value, start_in, allow_failure, variables, needs) end describe '#build_attributes' do @@ -275,6 +274,24 @@ it 'compacts nil values' do is_expected.to eq(options: {}, when: 'on_success') end + + context 'scheduling_type' do + context 'when rules have needs' do + let(:needs) do + [{ name: 'build-job' }] + end + + it 'adds schedule type to the build_attributes' do + expect(subject[:scheduling_type]).to eq(:dag) + end + end + + context 'when rules dont have needs' do + it 'adds schedule type to the build_attributes' do + expect(subject.key?(:scheduling_type)).to be_falsy + end + end + end end describe '#pass?' do diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index 3fb87516bbca53..8b7afc6d44a45c 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -128,7 +128,6 @@ end context 'when rule evaluates to false' do - let(:job_needs_attributes) { [{ name: 'rspec' }] } let(:attributes) do { name: 'rspec', ref: 'master', @@ -141,12 +140,10 @@ end end - context 'with needs with additional attributes' do - let(:job_needs_attributes) { [{ name: 'rspec' }] } + context 'with subkeys: artifacts, optional' do let(:attributes) do { name: 'rspec', ref: 'master', - needs_attributes: job_needs_attributes, rules: [ { if: '$VAR == null', @@ -160,12 +157,14 @@ ] } end - it 'overrides the job needs with attributes' do - expect(subject[:needs_attributes]).to match_array([{ name: 'build-job', optional: false, artifacts: true }]) - end + context 'given that the job has needs as a result of the rule' do + it 'sets the job needs as well as the job subkeys' do + expect(subject[:needs_attributes]).to match_array([{ name: 'build-job', optional: false, artifacts: true }]) + end - it 'overrides the scheduling type to dag' do - expect(subject[:scheduling_type]).to eq(:dag) + it 'sets the scheduling type to dag' do + expect(subject[:scheduling_type]).to eq(:dag) + end end end end diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index e74592d5ad9c99..962b4b95f4d944 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -715,9 +715,7 @@ module Ci EOYML end - it 'raises an error' do - expect(subject.errors).to eq(["lint_job job: need test_job is not defined in current or prior stages"]) - end + it_behaves_like 'returns errors', 'lint_job job: need test_job is not defined in current or prior stages' end context 'multiple needs in the following stage' do @@ -745,9 +743,7 @@ module Ci EOYML end - it 'raises an error' do - expect(subject.errors).to eq(["lint_job job: need test_job is not defined in current or prior stages"]) - end + it_behaves_like 'returns errors', 'lint_job job: need test_job is not defined in current or prior stages' end context 'single need in following state - hyphen need' do @@ -771,9 +767,7 @@ module Ci EOYML end - it 'raises an error' do - expect(subject.errors).to eq(["lint_job job: need test_job is not defined in current or prior stages"]) - end + it_behaves_like 'returns errors', 'lint_job job: need test_job is not defined in current or prior stages' end context 'when there are duplicate needs (string and hash)' do @@ -797,9 +791,7 @@ module Ci EOYML end - it 'raises an error' do - expect(subject.errors).to eq(["test_job_1 has duplicate entries in the needs section."]) - end + it_behaves_like 'returns errors', 'test_job_1 has duplicate entries in the needs section.' end end @@ -827,9 +819,7 @@ module Ci EOYML end - it 'raises an error' do - expect(subject.errors).to eq(["lint_job job: need test_job is not defined in current or prior stages"]) - end + it_behaves_like 'returns errors', 'lint_job job: need test_job is not defined in current or prior stages' end end diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index ede264b9bfdd36..cad683d17395a1 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -483,7 +483,7 @@ def find_job(name) end end - context 'when job doesnt have needs: set' do + context 'when job does not have needs: set' do let(:config) do <<-EOY lint_job: -- GitLab From c26bc615668f60459f7295da5f99e345d035f081 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Tue, 18 Apr 2023 16:18:20 +0200 Subject: [PATCH 05/19] Move needs to rules class --- lib/gitlab/ci/build/rules.rb | 3 +- lib/gitlab/ci/pipeline/seed/build.rb | 5 +-- spec/lib/gitlab/ci/build/rules_spec.rb | 54 ++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/ci/build/rules.rb b/lib/gitlab/ci/build/rules.rb index f80b81b0e80841..0eab1de766a33a 100644 --- a/lib/gitlab/ci/build/rules.rb +++ b/lib/gitlab/ci/build/rules.rb @@ -12,7 +12,8 @@ def build_attributes when: self.when, options: { start_in: start_in }.compact, allow_failure: allow_failure, - scheduling_type: (:dag if needs) + scheduling_type: (:dag if needs), + needs_attributes: needs }.compact end diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index 1672f65bacdb21..1d6d109039db7b 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -168,10 +168,7 @@ def rules_attributes rules_variables_result = ::Gitlab::Ci::Variables::Helpers.merge_variables( @seed_attributes[:yaml_variables], rules_result.variables ) - chain = rules_result.build_attributes.merge(yaml_variables: rules_variables_result) - chain = chain.merge(needs_attributes: rules_result[:needs]) if rules_result[:needs].present? && Feature.enabled?(:introduce_rules_with_needs, @pipeline.project) - - chain + rules_result.build_attributes.merge(yaml_variables: rules_variables_result) end strong_memoize_attr :rules_attributes diff --git a/spec/lib/gitlab/ci/build/rules_spec.rb b/spec/lib/gitlab/ci/build/rules_spec.rb index 677796277f31ba..f1e1f3179b40f1 100644 --- a/spec/lib/gitlab/ci/build/rules_spec.rb +++ b/spec/lib/gitlab/ci/build/rules_spec.rb @@ -231,6 +231,60 @@ [{ artifacts: true, name: 'test', optional: false, when: 'never' }], nil)) } end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(introduce_rules_with_needs: false) + end + + context 'with needs' do + context 'when single needs is specified' do + let(:rule_list) do + [{ if: '$VAR == null', needs: { job: [{ name: 'test', artifacts: true, optional: false }] } }] + end + + it { + is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, nil, nil)) + } + end + + context 'when multiple needs are specified' do + let(:rule_list) do + [{ if: '$VAR == null', + needs: { job: [{ name: 'test', artifacts: true, optional: false }, + { name: 'rspec', artifacts: true, optional: false }] } }] + end + + it { + is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, nil, nil)) + } + end + + context 'when there are no needs specified' do + let(:rule_list) { [{ if: '$VAR == null' }] } + + it { is_expected.to eq(described_class::Result.new('on_success', nil, nil, nil, nil, nil)) } + end + + context 'when need is specified with additional attibutes' do + let(:rule_list) do + [{ if: '$VAR == null', needs: { + job: [{ + artifacts: true, + name: 'test', + optional: false, + when: 'never' + }] + } }] + end + + it { + is_expected.to eq( + described_class::Result.new('on_success', nil, nil, nil, nil, nil)) + } + end + end + end end context 'with variables' do -- GitLab From deb2bf52f9a2e9d3ba27815268bfa91677469636 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Tue, 18 Apr 2023 16:29:00 +0200 Subject: [PATCH 06/19] Remove redundant code --- .../ci/create_pipeline_service/rules_spec.rb | 274 +++++++++--------- 1 file changed, 131 insertions(+), 143 deletions(-) diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index cad683d17395a1..52ccef2972a5e7 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -389,21 +389,16 @@ def find_job(name) context 'needs:' do context 'when needs are in an array' do - context 'with introduce_rules_with_needs enabled' do - before do - stub_feature_flags(introduce_rules_with_needs: true) - end - - let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } - let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } - let(:ref) { 'refs/heads/master' } + let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } + let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } + let(:ref) { 'refs/heads/master' } - context 'when job has needs: set' do - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + context 'when job has needs: set' do + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - context 'when rule is returning array of needs' do - let(:config) do - <<-EOY + context 'when rule is returning array of needs' do + let(:config) do + <<-EOY lint_job: script: 'echo lint_job' rules: @@ -420,31 +415,31 @@ def find_job(name) rules: - if: $var == null needs: [lint_job, rspec_job] - EOY - end + EOY + end - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted + it 'overrides the needs of the job' do + expect(pipeline).to be_persisted - need_1 = job_with_needs.needs.first - need_2 = job_with_needs.needs.last + need_1 = job_with_needs.needs.first + need_2 = job_with_needs.needs.last - expect(job_with_needs.needs.count).to eq(2) - expect(need_1.class).to eq(Ci::BuildNeed) - expect(need_1.name).to eq("lint_job") + expect(job_with_needs.needs.count).to eq(2) + expect(need_1.class).to eq(Ci::BuildNeed) + expect(need_1.name).to eq("lint_job") - expect(need_2.class).to eq(Ci::BuildNeed) - expect(need_2.name).to eq("rspec_job") - end + expect(need_2.class).to eq(Ci::BuildNeed) + expect(need_2.name).to eq("rspec_job") + end - it 'sets the scheduling type to dag' do - expect(job_with_needs.scheduling_type).to eq('dag') - end + it 'sets the scheduling type to dag' do + expect(job_with_needs.scheduling_type).to eq('dag') end + end - context 'when the second rule is true' do - let(:config) do - <<-EOY + context 'when the second rule is true' do + let(:config) do + <<-EOY lint_job: script: 'echo lint_job' rules: @@ -463,29 +458,29 @@ def find_job(name) needs: [lint_job] - if: $CI_COMMIT_REF_NAME =~ /master/ needs: [lint_job, rspec_job] - EOY - end + EOY + end - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted + it 'overrides the needs of the job' do + expect(pipeline).to be_persisted - need_1 = job_with_needs.needs.first - need_2 = job_with_needs.needs.last - expect(job_with_needs.needs.count).to eq(2) - expect(need_1.class).to eq(Ci::BuildNeed) - expect(need_1.name).to eq("lint_job") + need_1 = job_with_needs.needs.first + need_2 = job_with_needs.needs.last + expect(job_with_needs.needs.count).to eq(2) + expect(need_1.class).to eq(Ci::BuildNeed) + expect(need_1.name).to eq("lint_job") - expect(need_2.class).to eq(Ci::BuildNeed) - expect(need_2.name).to eq("rspec_job") - end + expect(need_2.class).to eq(Ci::BuildNeed) + expect(need_2.name).to eq("rspec_job") end end + end - context 'when job does not have needs: set' do - let(:config) do - <<-EOY + context 'when job does not have needs: set' do + let(:config) do + <<-EOY lint_job: script: 'echo lint_job' rules: @@ -501,18 +496,17 @@ def find_job(name) rules: - if: $CI_COMMIT_REF_NAME =~ /master/ needs: [lint_job] - EOY - end + EOY + end - let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } + let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } - it 'sets the needs of the job' do - expect(pipeline).to be_persisted + it 'sets the needs of the job' do + expect(pipeline).to be_persisted - expect(job_without_needs.needs.count).to eq(1) - expect(job_without_needs.needs.first.class).to be(Ci::BuildNeed) - expect(job_without_needs.needs.first.name).to eq("lint_job") - end + expect(job_without_needs.needs.count).to eq(1) + expect(job_without_needs.needs.first.class).to be(Ci::BuildNeed) + expect(job_without_needs.needs.first.name).to eq("lint_job") end end @@ -595,21 +589,16 @@ def find_job(name) end context 'when needs are in a hash' do - context 'with introduce_rules_with_needs enabled' do - before do - stub_feature_flags(introduce_rules_with_needs: true) - end - - let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } - let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } - let(:ref) { 'refs/heads/master' } + let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } + let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } + let(:ref) { 'refs/heads/master' } - context 'when job has needs: set' do - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + context 'when job has needs: set' do + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - context 'when rule is returning array of needs' do - let(:config) do - <<-EOY + context 'when rule is returning array of needs' do + let(:config) do + <<-EOY lint_job: script: 'echo lint_job' rules: @@ -629,30 +618,30 @@ def find_job(name) needs: - job: lint_job - job: rspec_job - EOY - end + EOY + end - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted + it 'overrides the needs of the job' do + expect(pipeline).to be_persisted - need_1 = job_with_needs.needs.first - need_2 = job_with_needs.needs.last - expect(job_with_needs.needs.count).to eq(2) - expect(need_1.class).to eq(Ci::BuildNeed) - expect(need_1.name).to eq("lint_job") + need_1 = job_with_needs.needs.first + need_2 = job_with_needs.needs.last + expect(job_with_needs.needs.count).to eq(2) + expect(need_1.class).to eq(Ci::BuildNeed) + expect(need_1.name).to eq("lint_job") - expect(need_2.class).to eq(Ci::BuildNeed) - expect(need_2.name).to eq("rspec_job") - end + expect(need_2.class).to eq(Ci::BuildNeed) + expect(need_2.name).to eq("rspec_job") + end - it 'sets the scheduling type to dag' do - expect(job_with_needs.scheduling_type).to eq('dag') - end + it 'sets the scheduling type to dag' do + expect(job_with_needs.scheduling_type).to eq('dag') end + end - context 'when the second rule is true' do - let(:config) do - <<-EOY + context 'when the second rule is true' do + let(:config) do + <<-EOY lint_job: script: 'echo lint_job' rules: @@ -675,29 +664,29 @@ def find_job(name) needs: - job: lint_job - job: rspec_job - EOY - end + EOY + end - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted + it 'overrides the needs of the job' do + expect(pipeline).to be_persisted - need_1 = job_with_needs.needs.first - need_2 = job_with_needs.needs.last - expect(job_with_needs.needs.count).to eq(2) - expect(need_1.class).to eq(Ci::BuildNeed) - expect(need_1.name).to eq("lint_job") + need_1 = job_with_needs.needs.first + need_2 = job_with_needs.needs.last + expect(job_with_needs.needs.count).to eq(2) + expect(need_1.class).to eq(Ci::BuildNeed) + expect(need_1.name).to eq("lint_job") - expect(need_2.class).to eq(Ci::BuildNeed) - expect(need_2.name).to eq("rspec_job") - end + expect(need_2.class).to eq(Ci::BuildNeed) + expect(need_2.name).to eq("rspec_job") end end + end - context 'when job doesnt have needs: set' do - let(:config) do - <<-EOY + context 'when job doesnt have needs: set' do + let(:config) do + <<-EOY lint_job: script: 'echo lint_job' rules: @@ -709,23 +698,23 @@ def find_job(name) - if: $CI_COMMIT_REF_NAME =~ /master/ needs: - job: lint_job - EOY - end + EOY + end - let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } + let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } - it 'sets the needs of the job' do - expect(pipeline).to be_persisted + it 'sets the needs of the job' do + expect(pipeline).to be_persisted - expect(job_without_needs.needs.count).to eq(1) - expect(job_without_needs.needs.first.class).to be(Ci::BuildNeed) - expect(job_without_needs.needs.first.name).to eq("lint_job") - end + expect(job_without_needs.needs.count).to eq(1) + expect(job_without_needs.needs.first.class).to be(Ci::BuildNeed) + expect(job_without_needs.needs.first.name).to eq("lint_job") end + end - context 'when needs array and hash are present' do - let(:config) do - <<-EOY + context 'when needs array and hash are present' do + let(:config) do + <<-EOY lint_job: script: 'echo lint_job' rules: @@ -753,38 +742,37 @@ def find_job(name) - job: rspec_job artifacts: false optional: true - EOY - end + EOY + end - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted + it 'overrides the needs of the job' do + expect(pipeline).to be_persisted - lint_job = job_with_needs.needs.find_by(name: 'lint_job') - lint_job_2 = job_with_needs.needs.find_by(name: 'lint_job_2') - rspec_job = job_with_needs.needs.find_by(name: 'rspec_job') - - expect(job_with_needs.needs.count).to eq(3) - expect(lint_job.class).to eq(Ci::BuildNeed) - expect(lint_job.name).to eq("lint_job") - expect(lint_job.artifacts).to eq(true) # default - expect(lint_job.optional).to eq(false) # default - - expect(lint_job_2.class).to eq(Ci::BuildNeed) - expect(lint_job_2.name).to eq("lint_job_2") - expect(lint_job_2.artifacts).to eq(false) - expect(lint_job_2.optional).to eq(true) - - expect(rspec_job.class).to eq(Ci::BuildNeed) - expect(rspec_job.name).to eq("rspec_job") - expect(rspec_job.artifacts).to eq(false) - expect(rspec_job.optional).to eq(true) - end + lint_job = job_with_needs.needs.find_by(name: 'lint_job') + lint_job_2 = job_with_needs.needs.find_by(name: 'lint_job_2') + rspec_job = job_with_needs.needs.find_by(name: 'rspec_job') + + expect(job_with_needs.needs.count).to eq(3) + expect(lint_job.class).to eq(Ci::BuildNeed) + expect(lint_job.name).to eq("lint_job") + expect(lint_job.artifacts).to eq(true) # default + expect(lint_job.optional).to eq(false) # default + + expect(lint_job_2.class).to eq(Ci::BuildNeed) + expect(lint_job_2.name).to eq("lint_job_2") + expect(lint_job_2.artifacts).to eq(false) + expect(lint_job_2.optional).to eq(true) + + expect(rspec_job.class).to eq(Ci::BuildNeed) + expect(rspec_job.name).to eq("rspec_job") + expect(rspec_job.artifacts).to eq(false) + expect(rspec_job.optional).to eq(true) + end - it 'sets the scheduling type to dag' do - expect(job_with_needs.scheduling_type).to eq('dag') - end + it 'sets the scheduling type to dag' do + expect(job_with_needs.scheduling_type).to eq('dag') end end -- GitLab From 377f0a59e391e7c7e1d853f7e4ddd6ed500b3c17 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Tue, 18 Apr 2023 16:38:52 +0200 Subject: [PATCH 07/19] Update ff info --- config/feature_flags/development/introduce_rules_with_needs.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/feature_flags/development/introduce_rules_with_needs.yml b/config/feature_flags/development/introduce_rules_with_needs.yml index 489a2867e47ed1..8b2940438ee199 100644 --- a/config/feature_flags/development/introduce_rules_with_needs.yml +++ b/config/feature_flags/development/introduce_rules_with_needs.yml @@ -2,7 +2,7 @@ name: introduce_rules_with_needs introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112725 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/394769 -milestone: '15.11' +milestone: '16.0' type: development group: group::pipeline authoring default_enabled: false -- GitLab From 700901797503fcbc6ad629cd6a35dd5c82fefaa1 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Wed, 19 Apr 2023 16:04:04 +0200 Subject: [PATCH 08/19] Refactor rules class --- lib/gitlab/ci/build/rules.rb | 9 +--- spec/lib/gitlab/ci/build/rules_spec.rb | 69 ++++++++++++++++---------- 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/lib/gitlab/ci/build/rules.rb b/lib/gitlab/ci/build/rules.rb index 0eab1de766a33a..bc7aad1b186042 100644 --- a/lib/gitlab/ci/build/rules.rb +++ b/lib/gitlab/ci/build/rules.rb @@ -13,7 +13,7 @@ def build_attributes options: { start_in: start_in }.compact, allow_failure: allow_failure, scheduling_type: (:dag if needs), - needs_attributes: needs + needs_attributes: needs&.[](:job) }.compact end @@ -31,13 +31,12 @@ def evaluate(pipeline, context) if @rule_list.nil? Result.new(@default_when) elsif matched_rule = match_rule(pipeline, context) - needs = return_needs(matched_rule) Result.new( matched_rule.attributes[:when] || @default_when, matched_rule.attributes[:start_in], matched_rule.attributes[:allow_failure], matched_rule.attributes[:variables], - (needs if Feature.enabled?(:introduce_rules_with_needs, pipeline.project)) + (matched_rule.attributes[:needs] if Feature.enabled?(:introduce_rules_with_needs, pipeline.project)) ) else Result.new('never') @@ -48,10 +47,6 @@ def evaluate(pipeline, context) private - def return_needs(matched_rule) - matched_rule.attributes.dig(:needs, :job) unless matched_rule.attributes[:needs].nil? - end - def match_rule(pipeline, context) @rule_list.find { |rule| rule.matches?(pipeline, context) } end diff --git a/spec/lib/gitlab/ci/build/rules_spec.rb b/spec/lib/gitlab/ci/build/rules_spec.rb index f1e1f3179b40f1..f14daadd0b50aa 100644 --- a/spec/lib/gitlab/ci/build/rules_spec.rb +++ b/spec/lib/gitlab/ci/build/rules_spec.rb @@ -184,7 +184,7 @@ context 'with needs' do context 'when single needs is specified' do let(:rule_list) do - [{ if: '$VAR == null', needs: { job: [{ name: 'test', artifacts: true, optional: false }] } }] + [{ if: '$VAR == null', needs: [{ name: 'test', artifacts: true, optional: false }] }] end it { @@ -196,8 +196,8 @@ context 'when multiple needs are specified' do let(:rule_list) do [{ if: '$VAR == null', - needs: { job: [{ name: 'test', artifacts: true, optional: false }, - { name: 'rspec', artifacts: true, optional: false }] } }] + needs: [{ name: 'test', artifacts: true, optional: false }, + { name: 'rspec', artifacts: true, optional: false }] }] end it { @@ -215,14 +215,12 @@ context 'when need is specified with additional attibutes' do let(:rule_list) do - [{ if: '$VAR == null', needs: { - job: [{ - artifacts: true, - name: 'test', - optional: false, - when: 'never' - }] - } }] + [{ if: '$VAR == null', needs: [{ + artifacts: true, + name: 'test', + optional: false, + when: 'never' + }] }] end it { @@ -240,7 +238,7 @@ context 'with needs' do context 'when single needs is specified' do let(:rule_list) do - [{ if: '$VAR == null', needs: { job: [{ name: 'test', artifacts: true, optional: false }] } }] + [{ if: '$VAR == null', needs: [{ name: 'test', artifacts: true, optional: false }] }] end it { @@ -251,8 +249,8 @@ context 'when multiple needs are specified' do let(:rule_list) do [{ if: '$VAR == null', - needs: { job: [{ name: 'test', artifacts: true, optional: false }, - { name: 'rspec', artifacts: true, optional: false }] } }] + needs: [{ name: 'test', artifacts: true, optional: false }, + { name: 'rspec', artifacts: true, optional: false }] }] end it { @@ -268,14 +266,12 @@ context 'when need is specified with additional attibutes' do let(:rule_list) do - [{ if: '$VAR == null', needs: { - job: [{ - artifacts: true, - name: 'test', - optional: false, - when: 'never' - }] - } }] + [{ if: '$VAR == null', needs: [{ + artifacts: true, + name: 'test', + optional: false, + when: 'never' + }] }] end it { @@ -331,12 +327,33 @@ context 'scheduling_type' do context 'when rules have needs' do - let(:needs) do - [{ name: 'build-job' }] + context 'single need' do + let(:needs) do + { job: [{ name: 'test' }] } + end + + it 'saves needs' do + expect(subject[:needs_attributes]).to eq([{ name: "test" }]) + end + + it 'adds schedule type to the build_attributes' do + expect(subject[:scheduling_type]).to eq(:dag) + end end - it 'adds schedule type to the build_attributes' do - expect(subject[:scheduling_type]).to eq(:dag) + context 'multiple needs' do + let(:needs) do + { job: [{ name: 'test' }, { name: 'test_2', artifacts: true, optional: false }] } + end + + it 'saves needs' do + expect(subject[:needs_attributes]).to eq([{ name: "test" }, + { name: 'test_2', artifacts: true, optional: false }]) + end + + it 'adds schedule type to the build_attributes' do + expect(subject[:scheduling_type]).to eq(:dag) + end end end -- GitLab From 5b2df0c96c41407f964bb5aa1a3dfb519e6075e1 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Wed, 19 Apr 2023 16:31:03 +0200 Subject: [PATCH 09/19] Refactor specs --- lib/gitlab/ci/pipeline/seed/build.rb | 1 + spec/lib/gitlab/ci/build/rules_spec.rb | 2 +- .../ci/create_pipeline_service/rules_spec.rb | 41 ++++--------------- 3 files changed, 10 insertions(+), 34 deletions(-) diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index 1d6d109039db7b..5ae452708b917c 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -170,6 +170,7 @@ def rules_attributes ) rules_result.build_attributes.merge(yaml_variables: rules_variables_result) end + strong_memoize_attr :rules_attributes def rules_result diff --git a/spec/lib/gitlab/ci/build/rules_spec.rb b/spec/lib/gitlab/ci/build/rules_spec.rb index f14daadd0b50aa..f3c1029a8e9e05 100644 --- a/spec/lib/gitlab/ci/build/rules_spec.rb +++ b/spec/lib/gitlab/ci/build/rules_spec.rb @@ -347,7 +347,7 @@ end it 'saves needs' do - expect(subject[:needs_attributes]).to eq([{ name: "test" }, + expect(subject[:needs_attributes]).to match_array([{ name: "test" }, { name: 'test_2', artifacts: true, optional: false }]) end diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index 52ccef2972a5e7..cddfdd5a3b3edf 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -421,15 +421,8 @@ def find_job(name) it 'overrides the needs of the job' do expect(pipeline).to be_persisted - need_1 = job_with_needs.needs.first - need_2 = job_with_needs.needs.last - - expect(job_with_needs.needs.count).to eq(2) - expect(need_1.class).to eq(Ci::BuildNeed) - expect(need_1.name).to eq("lint_job") - - expect(need_2.class).to eq(Ci::BuildNeed) - expect(need_2.name).to eq("rspec_job") + needs = job_with_needs.needs.map(&:name) + expect(needs).to contain_exactly('lint_job', 'rspec_job') end it 'sets the scheduling type to dag' do @@ -466,14 +459,8 @@ def find_job(name) it 'overrides the needs of the job' do expect(pipeline).to be_persisted - need_1 = job_with_needs.needs.first - need_2 = job_with_needs.needs.last - expect(job_with_needs.needs.count).to eq(2) - expect(need_1.class).to eq(Ci::BuildNeed) - expect(need_1.name).to eq("lint_job") - - expect(need_2.class).to eq(Ci::BuildNeed) - expect(need_2.name).to eq("rspec_job") + needs = job_with_needs.needs.map(&:name) + expect(needs).to contain_exactly('lint_job', 'rspec_job') end end end @@ -624,14 +611,8 @@ def find_job(name) it 'overrides the needs of the job' do expect(pipeline).to be_persisted - need_1 = job_with_needs.needs.first - need_2 = job_with_needs.needs.last - expect(job_with_needs.needs.count).to eq(2) - expect(need_1.class).to eq(Ci::BuildNeed) - expect(need_1.name).to eq("lint_job") - - expect(need_2.class).to eq(Ci::BuildNeed) - expect(need_2.name).to eq("rspec_job") + needs = job_with_needs.needs.map(&:name) + expect(needs).to contain_exactly('lint_job', 'rspec_job') end it 'sets the scheduling type to dag' do @@ -672,14 +653,8 @@ def find_job(name) it 'overrides the needs of the job' do expect(pipeline).to be_persisted - need_1 = job_with_needs.needs.first - need_2 = job_with_needs.needs.last - expect(job_with_needs.needs.count).to eq(2) - expect(need_1.class).to eq(Ci::BuildNeed) - expect(need_1.name).to eq("lint_job") - - expect(need_2.class).to eq(Ci::BuildNeed) - expect(need_2.name).to eq("rspec_job") + needs = job_with_needs.needs.map(&:name) + expect(needs).to contain_exactly('lint_job', 'rspec_job') end end end -- GitLab From 3f230372f431f3a55cc450999a2e7d7f0c6b5c66 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Wed, 19 Apr 2023 19:27:27 +0200 Subject: [PATCH 10/19] Address frontend comments --- app/assets/javascripts/editor/schema/ci.json | 6 ++++-- lib/gitlab/ci/pipeline/seed/build.rb | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index 7f3cdbc6196afb..89b836b9b0052b 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -940,7 +940,7 @@ "minLength": 1 }, "needs": { - "markdownDescription": "Use needs on rules to update jobs needs for specific conditions. Once specific conditions are met, jobs needs are reset with the needs and its attributes defined by the rule.", + "markdownDescription": "Use needs on rules to update jobs needs for specific conditions. Once specific conditions are met, jobs needs are reset with the needs and its attributes defined by the rule. [Learn More](https://docs.gitlab.com/ee/ci/yaml/index.html#rulesneeds).", "type": "array", "items": { "oneOf": [ @@ -952,7 +952,9 @@ "additionalProperties": false, "properties": { "job": { - "type": "string" + "type": "string", + "minLength": 1, + "description": "Name of a job that is defined in the pipeline." }, "artifacts": { "type": "boolean" diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index 5ae452708b917c..98f488d0f387b3 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -168,9 +168,9 @@ def rules_attributes rules_variables_result = ::Gitlab::Ci::Variables::Helpers.merge_variables( @seed_attributes[:yaml_variables], rules_result.variables ) + rules_result.build_attributes.merge(yaml_variables: rules_variables_result) end - strong_memoize_attr :rules_attributes def rules_result -- GitLab From 4f2bb1907b4730eca25076f22718e53757c5b764 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Thu, 20 Apr 2023 16:00:03 +0200 Subject: [PATCH 11/19] Add yaml spec --- .../editor/schema/ci/ci_schema_spec.js | 4 +++ .../yaml_tests/negative_tests/rules_needs.yml | 21 ++++++++++++ .../yaml_tests/positive_tests/rules_needs.yml | 32 +++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 spec/frontend/editor/schema/ci/yaml_tests/negative_tests/rules_needs.yml create mode 100644 spec/frontend/editor/schema/ci/yaml_tests/positive_tests/rules_needs.yml diff --git a/spec/frontend/editor/schema/ci/ci_schema_spec.js b/spec/frontend/editor/schema/ci/ci_schema_spec.js index 87208ec7aa8b43..51fcf26c39aa46 100644 --- a/spec/frontend/editor/schema/ci/ci_schema_spec.js +++ b/spec/frontend/editor/schema/ci/ci_schema_spec.js @@ -27,6 +27,7 @@ import CacheYaml from './yaml_tests/positive_tests/cache.yml'; import FilterYaml from './yaml_tests/positive_tests/filter.yml'; import IncludeYaml from './yaml_tests/positive_tests/include.yml'; import RulesYaml from './yaml_tests/positive_tests/rules.yml'; +import RulesNeedsYaml from './yaml_tests/positive_tests/rules_needs.yml'; import ProjectPathYaml from './yaml_tests/positive_tests/project_path.yml'; import VariablesYaml from './yaml_tests/positive_tests/variables.yml'; import JobWhenYaml from './yaml_tests/positive_tests/job_when.yml'; @@ -46,6 +47,7 @@ import ProjectPathIncludeLeadSlashYaml from './yaml_tests/negative_tests/project import ProjectPathIncludeNoSlashYaml from './yaml_tests/negative_tests/project_path/include/no_slash.yml'; import ProjectPathIncludeTailSlashYaml from './yaml_tests/negative_tests/project_path/include/tailing_slash.yml'; import RulesNegativeYaml from './yaml_tests/negative_tests/rules.yml'; +import RulesNeedsNegativeYaml from './yaml_tests/negative_tests/rules_needs.yml'; import TriggerNegative from './yaml_tests/negative_tests/trigger.yml'; import VariablesInvalidOptionsYaml from './yaml_tests/negative_tests/variables/invalid_options.yml'; import VariablesInvalidSyntaxDescYaml from './yaml_tests/negative_tests/variables/invalid_syntax_desc.yml'; @@ -88,6 +90,7 @@ describe('positive tests', () => { JobWhenYaml, HooksYaml, RulesYaml, + RulesNeedsYaml, VariablesYaml, ProjectPathYaml, IdTokensYaml, @@ -121,6 +124,7 @@ describe('negative tests', () => { IncludeNegativeYaml, JobWhenNegativeYaml, RulesNegativeYaml, + RulesNeedsNegativeYaml, TriggerNegative, VariablesInvalidOptionsYaml, VariablesInvalidSyntaxDescYaml, diff --git a/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/rules_needs.yml b/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/rules_needs.yml new file mode 100644 index 00000000000000..95a2282420116d --- /dev/null +++ b/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/rules_needs.yml @@ -0,0 +1,21 @@ +# invalid rules:needs +lint_job: + script: exit 0 + rules: + - if: $var == null + needs: + +# invalid rules:needs +lint_job_2: + script: exit 0 + rules: + - if: $var == null + needs: [20] + +# invalid rules:needs +lint_job_3: + script: exit 0 + rules: + - if: $var == null + needs: + - job: \ No newline at end of file diff --git a/spec/frontend/editor/schema/ci/yaml_tests/positive_tests/rules_needs.yml b/spec/frontend/editor/schema/ci/yaml_tests/positive_tests/rules_needs.yml new file mode 100644 index 00000000000000..a4a5183dcf4ec9 --- /dev/null +++ b/spec/frontend/editor/schema/ci/yaml_tests/positive_tests/rules_needs.yml @@ -0,0 +1,32 @@ +# valid workflow:rules:needs +pre_lint_job: + script: exit 0 + rules: + - if: $var == null + +lint_job: + script: exit 0 + rules: + - if: $var == null + +rspec_job: + script: exit 0 + rules: + - if: $var == null + needs: [lint_job] + +job: + needs: [rspec_job] + script: exit 0 + rules: + - if: $var == null + needs: + - job: lint_job + artifacts: false + optional: true + - job: pre_lint_job + artifacts: true + optional: false + - rspec_job + - if: $var == true + needs: [lint_job, pre_lint_job] \ No newline at end of file -- GitLab From 9ec366706846200913650d5173c32e70f1f0db94 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Thu, 20 Apr 2023 18:01:34 +0200 Subject: [PATCH 12/19] Fix specs --- spec/lib/gitlab/ci/build/rules_spec.rb | 4 ++-- spec/lib/gitlab/ci/pipeline/seed/build_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/lib/gitlab/ci/build/rules_spec.rb b/spec/lib/gitlab/ci/build/rules_spec.rb index f3c1029a8e9e05..1ece0f6b7b9d0e 100644 --- a/spec/lib/gitlab/ci/build/rules_spec.rb +++ b/spec/lib/gitlab/ci/build/rules_spec.rb @@ -357,8 +357,8 @@ end end - context 'when rules dont have needs' do - it 'adds schedule type to the build_attributes' do + context 'when rules do not have needs' do + it 'does not add schedule type to the build_attributes' do expect(subject.key?(:scheduling_type)).to be_falsy 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 8b7afc6d44a45c..9d5a9bc8058b5d 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -157,7 +157,7 @@ ] } end - context 'given that the job has needs as a result of the rule' do + context 'when rule evaluates to true' do it 'sets the job needs as well as the job subkeys' do expect(subject[:needs_attributes]).to match_array([{ name: 'build-job', optional: false, artifacts: true }]) end -- GitLab From faedc14c1ccb6c7db116a3ef290e5b0b4e76fd90 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Thu, 20 Apr 2023 18:39:40 +0200 Subject: [PATCH 13/19] Update docs --- app/assets/javascripts/editor/schema/ci.json | 8 ++-- doc/ci/yaml/index.md | 47 +++++++------------- 2 files changed, 22 insertions(+), 33 deletions(-) diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index 89b836b9b0052b..047297c2359dbc 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -940,7 +940,7 @@ "minLength": 1 }, "needs": { - "markdownDescription": "Use needs on rules to update jobs needs for specific conditions. Once specific conditions are met, jobs needs are reset with the needs and its attributes defined by the rule. [Learn More](https://docs.gitlab.com/ee/ci/yaml/index.html#rulesneeds).", + "markdownDescription": "Use needs in rules to update job needs for specific conditions. When a condition matches a rule, jobs needs and any specified attributes are replaced with the needs in the rule. [Learn More](https://docs.gitlab.com/ee/ci/yaml/index.html#rulesneeds).", "type": "array", "items": { "oneOf": [ @@ -957,10 +957,12 @@ "description": "Name of a job that is defined in the pipeline." }, "artifacts": { - "type": "boolean" + "type": "boolean", + "description": "Download artifacts of the job in needs." }, "optional": { - "type": "boolean" + "type": "boolean", + "description": "Whether the job needs to be present in the pipeline to run ahead of the current job." } }, "required": [ diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index cc1e91f090b8b3..80819cd508c8d7 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -3670,7 +3670,7 @@ If the rule matches, then the job is a manual job with `allow_failure: true`. > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/31581) in GitLab 15.10. -Use `needs` on rules to update jobs needs for specific conditions. Once specific conditions are met, jobs needs are reset with the needs and its attributes defined by the rule. +Use `needs` in rules to update a job's [`needs`](#needs) for specific conditions. When a rule matches a condition, the job's `needs` and any specified attributes are replaced with the `needs` in the rule. **Keyword type**: Job-specific. You can use it only as part of a job. @@ -3686,29 +3686,7 @@ lint: stage: test needs: ['mac:rspec'] rules: - - if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH - needs: ['mac:build'] # Override job needs defined - -mac:rspec: - stage: test - script: echo "Running rspec on mac..." - -mac:build: - stage: test - script: echo "Building mac..." -``` - -Paths of execution: - -- If the specific condition of the rule on lint job is met, the sequance of jobs is: `mac:build`, `lint`, `mac:rspec` -- If the specific condition of the rule on lint job is not met, the sequance of jobs is: `mac:rspec`, `lint`, `mac:build` - -```yaml -lint: - stage: test - needs: ['mac:rspec'] - rules: - - if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH + - if: $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH needs: - job: 'mac:build' optional: false @@ -3717,23 +3695,32 @@ lint: mac:rspec: stage: test script: echo "Running rspec on mac..." + rules: + - if: $CI_COMMIT_BRANCH != $CI_DEFAULT_BRANCH + needs: ['mac:build'] mac:build: - stage: build + stage: test script: echo "Building mac..." + rules: + - if: $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH + ``` -Paths of execution: +In this example: + +- If the pipeline runs on a branch that is not the default branch, the `lint` job needs the `mac:rspec` job (default behavior). +- If the pipeline runs on the default branch, and therefore the rule matches the condition, the `lint` job needs the `mac:build` and `mac:rspec` job instead. -- If the specific condition of the rule on lint job is met, the sequance of jobs is: `mac:build`, `lint`, `mac:rspec` -- If the specific condition of the rule on lint job is not met, the sequance of jobs is: `mac:rspec`, `lint`, `mac:build` +- If the pipeline runs on a branch that is not the default branch, the `mac:rspec` job has no needs. +- If the pipeline runs on the default branch, and therefore the rule matches the condition, the `mac:rspec` job needs the `mac:build` job instead. See job `lint`. It has job need `mac:rspec` defined as a plain string in an array. Under rules, the job has two needs: `mac:build` defined as a hash and `mac:rspec` defined aslo as a plain string. All 3 methods of defining needs on jobs and job rules are valid. **Additional details**: -- Once the specific condition is met, the rules needs override the jobs needs and the behavior is same as per [job needs](#needs). -- The needs on rules takes keywords: [artifacts](#needsartifacts) and [optional](#needsoptional). +- `needs` in rules override any `needs` defined at the job-level, once overridden, the behavior is same as [job-level `needs`](#needs). +- `needs` in rules can accept [`artifacts`](#needsartifacts) and [`optional`](#needsoptional). #### `rules:variables` -- GitLab From 4cfa8c36bf4721f0d7fad1e6d84d13d7782ce4b5 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Mon, 24 Apr 2023 16:20:12 +0200 Subject: [PATCH 14/19] Add negative cases --- doc/ci/yaml/index.md | 25 ++++++++--------- .../yaml_tests/negative_tests/rules_needs.yml | 27 ++++++++++++++++++- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 80819cd508c8d7..77a1054cf2b272 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -3668,7 +3668,8 @@ If the rule matches, then the job is a manual job with `allow_failure: true`. #### `rules:needs` -> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/31581) in GitLab 15.10. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/31581) in GitLab 16.0. +> - [Deployed behind a feature flag](../../user/feature_flags.md), disabled by default. Use `needs` in rules to update a job's [`needs`](#needs) for specific conditions. When a rule matches a condition, the job's `needs` and any specified attributes are replaced with the `needs` in the rule. @@ -3684,26 +3685,26 @@ Use `needs` in rules to update a job's [`needs`](#needs) for specific conditions ```yaml lint: stage: test - needs: ['mac:rspec'] + script: echo "Running lint job..." rules: - - if: $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH + - if: $VAR == null needs: - - job: 'mac:build' + - job: build optional: false - - mac:rspec + - rspec -mac:rspec: +rspec: stage: test - script: echo "Running rspec on mac..." + script: echo "Running rspec job..." rules: - - if: $CI_COMMIT_BRANCH != $CI_DEFAULT_BRANCH - needs: ['mac:build'] + - if: $VAR == null + needs: [build] -mac:build: +build: stage: test - script: echo "Building mac..." + script: echo "Running build job..." rules: - - if: $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH + - if: $VAR == null ``` diff --git a/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/rules_needs.yml b/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/rules_needs.yml index 95a2282420116d..f2f1eb118f879f 100644 --- a/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/rules_needs.yml +++ b/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/rules_needs.yml @@ -18,4 +18,29 @@ lint_job_3: rules: - if: $var == null needs: - - job: \ No newline at end of file + - job: + +# invalid rules:needs +lint_job_5: + script: exit 0 + rules: + - if: $var == null + needs: + - pipeline: 5 + +# invalid rules:needs +lint_job_6: + script: exit 0 + rules: + - if: $var == null + needs: + - project: namespace/group/project-name + +# invalid rules:needs +lint_job_7: + script: exit 0 + rules: + - if: $var == null + needs: + - pipeline: 5 + job: lint_job_6 -- GitLab From 840cb4d802d8899c83c186f8c07975ab3fbc7d7b Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Mon, 24 Apr 2023 17:23:29 +0200 Subject: [PATCH 15/19] Refactor spec file --- app/assets/javascripts/editor/schema/ci.json | 2 +- .../ci/create_pipeline_service/rules_spec.rb | 502 +++++++----------- 2 files changed, 180 insertions(+), 324 deletions(-) diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index 047297c2359dbc..71238b77ce8afe 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -939,7 +939,7 @@ "markdownDescription": "Used in conjunction with 'when: delayed' to set how long to delay before starting a job. e.g. '5', 5 seconds, 30 minutes, 1 week, etc. [Learn More](https://docs.gitlab.com/ee/ci/jobs/job_control.html#run-a-job-after-a-delay).", "minLength": 1 }, - "needs": { + "rulesNeeds": { "markdownDescription": "Use needs in rules to update job needs for specific conditions. When a condition matches a rule, jobs needs and any specified attributes are replaced with the needs in the rule. [Learn More](https://docs.gitlab.com/ee/ci/yaml/index.html#rulesneeds).", "type": "array", "items": { diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index cddfdd5a3b3edf..cd70c6aada6345 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -388,278 +388,180 @@ def find_job(name) end context 'needs:' do - context 'when needs are in an array' do - let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } - let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } - let(:ref) { 'refs/heads/master' } - - context 'when job has needs: set' do - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - - context 'when rule is returning array of needs' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - needs: [lint_job] - script: 'echo job' - rules: - - if: $var == null - needs: [lint_job, rspec_job] - EOY - end - - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted - - needs = job_with_needs.needs.map(&:name) - expect(needs).to contain_exactly('lint_job', 'rspec_job') - end + let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } + let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } + let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + let(:ref) { 'refs/heads/master' } - it 'sets the scheduling type to dag' do - expect(job_with_needs.scheduling_type).to eq('dag') - end + context 'when job has needs' do + context 'when rules needs are in array' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + needs: [lint_job] + script: 'echo job' + rules: + - if: $var == null + needs: [lint_job, rspec_job] + EOY end - context 'when the second rule is true' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - needs: [rspec_job] - rules: - - if: $CI_COMMIT_REF_NAME =~ /feature/ - needs: [lint_job] - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: [lint_job, rspec_job] - EOY - end - - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } + it 'overrides the needs of the job' do + expect(pipeline).to be_persisted - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted + needs = job_with_needs.needs.map(&:name) + expect(needs).to contain_exactly('lint_job', 'rspec_job') + end - needs = job_with_needs.needs.map(&:name) - expect(needs).to contain_exactly('lint_job', 'rspec_job') - end + it 'sets the scheduling type to dag' do + expect(job_with_needs.scheduling_type).to eq('dag') end end - context 'when job does not have needs: set' do + context 'when rules needs are a hash' do let(:config) do <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: [lint_job] + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + needs: + - job: lint_job + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: + - job: lint_job + - job: rspec_job EOY end - let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } - - it 'sets the needs of the job' do + it 'overrides the needs of the job' do expect(pipeline).to be_persisted - expect(job_without_needs.needs.count).to eq(1) - expect(job_without_needs.needs.first.class).to be(Ci::BuildNeed) - expect(job_without_needs.needs.first.name).to eq("lint_job") + needs = job_with_needs.needs.map(&:name) + expect(needs).to contain_exactly('lint_job', 'rspec_job') end - end - context 'with introduce_rules_with_needs disabled' do - before do - stub_feature_flags(introduce_rules_with_needs: false) + it 'sets the scheduling type to dag' do + expect(job_with_needs.scheduling_type).to eq('dag') end + end - context 'when rule is returning multiple needs' do - let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } - let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - - let(:ref) { 'refs/heads/master' } - - let(:config) do - <<-EOY - lint_job: + context 'when there are both array and hash needs on a rule' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + lint_job_2: script: 'echo lint_job' rules: - if: $var == null - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - needs: [rspec_job] - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: [lint_job, rspec_job] - - if: $CI_COMMIT_REF_NAME =~ /feature/ - EOY - end - - it 'keeps the needs of the job' do - expect(pipeline).to be_persisted + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: + - lint_job + - job: lint_job_2 + artifacts: false + optional: true + - job: rspec_job + artifacts: false + optional: true + EOY + end - expect(job_with_needs.needs.count).to eq(1) + it 'overrides the needs of the job' do + expect(pipeline).to be_persisted - need = job_with_needs.needs.first - expect(need.class).to eq(Ci::BuildNeed) - expect(need.name).to eq("rspec_job") - end - end + lint_job = job_with_needs.needs.find_by(name: 'lint_job') + lint_job_2 = job_with_needs.needs.find_by(name: 'lint_job_2') + rspec_job = job_with_needs.needs.find_by(name: 'rspec_job') - context 'when job doesnt have needs: set' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: [lint_job] - - if: $CI_COMMIT_REF_NAME =~ /feature/ - EOY - end + expect(job_with_needs.needs.count).to eq(3) + expect(lint_job.class).to eq(Ci::BuildNeed) + expect(lint_job.name).to eq("lint_job") + expect(lint_job.artifacts).to eq(true) # default + expect(lint_job.optional).to eq(false) # default - let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } + expect(lint_job_2.class).to eq(Ci::BuildNeed) + expect(lint_job_2.name).to eq("lint_job_2") + expect(lint_job_2.artifacts).to eq(false) + expect(lint_job_2.optional).to eq(true) - it 'return an empty array' do - expect(pipeline).to be_persisted + expect(rspec_job.class).to eq(Ci::BuildNeed) + expect(rspec_job.name).to eq("rspec_job") + expect(rspec_job.artifacts).to eq(false) + expect(rspec_job.optional).to eq(true) + end - expect(job_without_needs.needs).to eq([]) - end + it 'sets the scheduling type to dag' do + expect(job_with_needs.scheduling_type).to eq('dag') end end end - context 'when needs are in a hash' do - let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } - let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } - let(:ref) { 'refs/heads/master' } - - context 'when job has needs: set' do - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - - context 'when rule is returning array of needs' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - needs: - - job: lint_job - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: - - job: lint_job - - job: rspec_job - EOY - end - - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted - - needs = job_with_needs.needs.map(&:name) - expect(needs).to contain_exactly('lint_job', 'rspec_job') - end + context 'when job has no needs' do + let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } - it 'sets the scheduling type to dag' do - expect(job_with_needs.scheduling_type).to eq('dag') - end + context 'when rules needs are in array' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: [lint_job] + EOY end - context 'when the second rule is true' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - needs: - - job: rspec_job - rules: - - if: $CI_COMMIT_REF_NAME =~ /feature/ - needs: - - job: lint_job - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: - - job: lint_job - - job: rspec_job - EOY - end - - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted + it 'sets the needs of the job' do + expect(pipeline).to be_persisted - needs = job_with_needs.needs.map(&:name) - expect(needs).to contain_exactly('lint_job', 'rspec_job') - end + expect(job_without_needs.needs.count).to eq(1) + expect(job_without_needs.needs.first.class).to be(Ci::BuildNeed) + expect(job_without_needs.needs.first.name).to eq("lint_job") end end - context 'when job doesnt have needs: set' do + context 'when rules needs are a hash' do let(:config) do <<-EOY lint_job: @@ -676,8 +578,6 @@ def find_job(name) EOY end - let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } - it 'sets the needs of the job' do expect(pipeline).to be_persisted @@ -687,82 +587,34 @@ def find_job(name) end end - context 'when needs array and hash are present' do + context 'when rules have no needs' do let(:config) do <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - lint_job_2: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: - - lint_job - - job: lint_job_2 - artifacts: false - optional: true - - job: rspec_job - artifacts: false - optional: true + job: + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ EOY end - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - - it 'overrides the needs of the job' do + it 'sets the needs of the job' do expect(pipeline).to be_persisted - - lint_job = job_with_needs.needs.find_by(name: 'lint_job') - lint_job_2 = job_with_needs.needs.find_by(name: 'lint_job_2') - rspec_job = job_with_needs.needs.find_by(name: 'rspec_job') - - expect(job_with_needs.needs.count).to eq(3) - expect(lint_job.class).to eq(Ci::BuildNeed) - expect(lint_job.name).to eq("lint_job") - expect(lint_job.artifacts).to eq(true) # default - expect(lint_job.optional).to eq(false) # default - - expect(lint_job_2.class).to eq(Ci::BuildNeed) - expect(lint_job_2.name).to eq("lint_job_2") - expect(lint_job_2.artifacts).to eq(false) - expect(lint_job_2.optional).to eq(true) - - expect(rspec_job.class).to eq(Ci::BuildNeed) - expect(rspec_job.name).to eq("rspec_job") - expect(rspec_job.artifacts).to eq(false) - expect(rspec_job.optional).to eq(true) + expect(job_without_needs.needs).to be_empty end - it 'sets the scheduling type to dag' do - expect(job_with_needs.scheduling_type).to eq('dag') + it 'does not change the scheduling type' do + expect(job_with_needs.scheduling_type).to eq('stage') end end + end - context 'with introduce_rules_with_needs disabled' do - before do - stub_feature_flags(introduce_rules_with_needs: false) - end + context 'with introduce_rules_with_needs disabled' do + before do + stub_feature_flags(introduce_rules_with_needs: false) + end + context 'when job has needs' do context 'when rule is returning multiple needs' do - let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } - let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - - let(:ref) { 'refs/heads/master' } - let(:config) do <<-EOY lint_job: @@ -797,36 +649,40 @@ def find_job(name) expect(need.name).to eq("rspec_job") end end + end - context 'when job doesnt have needs: set' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: - - job: lint_job - EOY - end + context 'when job has no needs' do + let(:config) do + <<-EOY + lint_job: + script: 'echo lint_job' + rules: + - if: $var == null + - when: always + rspec_job: + script: 'echo rspec_job' + rules: + - if: $var == null + - when: always + job: + script: 'echo job' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + needs: + - job: lint_job + EOY + end - let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } + let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } - it 'return an empty array' do - expect(pipeline).to be_persisted + it 'return an empty array' do + expect(pipeline).to be_persisted - expect(job_without_needs.needs).to eq([]) - end + expect(job_without_needs.needs).to eq([]) + end + + it 'does not change the scheduling type' do + expect(job_with_needs.scheduling_type).to eq('stage') end end end -- GitLab From 09c6bd3c2a5f343ad9717f5c4545c63435f6062f Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Mon, 24 Apr 2023 18:05:07 +0200 Subject: [PATCH 16/19] Edit docs --- app/assets/javascripts/editor/schema/ci.json | 4 +- doc/ci/jobs/job_control.md | 38 +++++++++++++++ doc/ci/yaml/index.md | 49 +++++++++----------- 3 files changed, 63 insertions(+), 28 deletions(-) diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index 71238b77ce8afe..1fb6f606b6b865 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -776,7 +776,7 @@ "$ref": "#/definitions/allow_failure" }, "needs": { - "$ref": "#/definitions/needs" + "$ref": "#/definitions/rulesNeeds" } } }, @@ -940,7 +940,7 @@ "minLength": 1 }, "rulesNeeds": { - "markdownDescription": "Use needs in rules to update job needs for specific conditions. When a condition matches a rule, jobs needs and any specified attributes are replaced with the needs in the rule. [Learn More](https://docs.gitlab.com/ee/ci/yaml/index.html#rulesneeds).", + "markdownDescription": "Use needs in rules to update job needs for specific conditions. When a condition matches a rule, the job's needs configuration is completely replaced with the needs in the rule. [Learn More](https://docs.gitlab.com/ee/ci/yaml/index.html#rulesneeds).", "type": "array", "items": { "oneOf": [ diff --git a/doc/ci/jobs/job_control.md b/doc/ci/jobs/job_control.md index 3cd57ff6a6ad45..449b53c934734b 100644 --- a/doc/ci/jobs/job_control.md +++ b/doc/ci/jobs/job_control.md @@ -559,6 +559,44 @@ test: - "README.md" ``` +## Specify when jobs run with `rules` and `needs` + +- `needs` in rules can accept [`artifacts`](../yaml/index.md#needsartifacts) and [`optional`](../yaml/index.md#needsoptional) + +**Example of `rules:needs`**: + +```yaml +build-dev: + stage: build + rules: + - if: $CI_COMMIT_BRANCH != $CI_DEFAULT_BRANCH + script: echo "Feature branch, so building dev version..." + +build-prod: + stage: build + rules: + - if: $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH + script: echo "Default branch, so building prod version..." + +specs: + stage: test + needs: ['build-dev'] + rules: + - if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH + needs: + - job: 'build-prod' + artifacts: false + optional: true + - 'build-dev' + - when: on_success # Run the job in other cases + script: echo "Running dev specs by default, or prod specs when default branch..." +``` + +- If the pipeline runs on a branch that is not the default branch, the `specs` job needs the `build-dev` job (default behavior) with defalt attributes. +- If the pipeline runs on the default branch, and therefore the rule matches the condition, the `specs` job needs the `build-dev` job with default attributes as well as the `build-prod` job with customised attributes as per above YAML. + +- See job `specs`. It has job need `build-dev` defined as a plain string in an array. Under rules, the job has two needs: `build-prod` defined as a hash and `build-dev` defined aslo as a plain string. All 3 methods of defining needs on rules are valid. + ## Create a job that must be run manually You can require that a job doesn't run unless a user starts it. This is called a **manual job**. diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 77a1054cf2b272..0fc93cff41e141 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -3671,58 +3671,55 @@ If the rule matches, then the job is a manual job with `allow_failure: true`. > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/31581) in GitLab 16.0. > - [Deployed behind a feature flag](../../user/feature_flags.md), disabled by default. -Use `needs` in rules to update a job's [`needs`](#needs) for specific conditions. When a rule matches a condition, the job's `needs` and any specified attributes are replaced with the `needs` in the rule. +Use `needs` in rules to update a job's [`needs`](#needs) for specific conditions. When a condition matches a rule, the job's `needs` configuration is completely replaced with the `needs in the rule. **Keyword type**: Job-specific. You can use it only as part of a job. **Possible inputs**: -- An array of jobs names as strings or in a hash. +- An array of job names as strings. +- A hash with job name, optionally with additional attributes. - An empty array (`[]`), to set the job needs to none when the specific condition is met. **Example of `rules:needs`**: ```yaml -lint: - stage: test - script: echo "Running lint job..." +build-dev: + stage: build rules: - - if: $VAR == null - needs: - - job: build - optional: false - - rspec + - if: $CI_COMMIT_BRANCH != $CI_DEFAULT_BRANCH + script: echo "Feature branch, so building dev version..." -rspec: - stage: test - script: echo "Running rspec job..." +build-prod: + stage: build rules: - - if: $VAR == null - needs: [build] + - if: $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH + script: echo "Default branch, so building prod version..." -build: +specs: stage: test - script: echo "Running build job..." + needs: ['build-dev'] rules: - - if: $VAR == null - + - if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH + needs: ['build-prod'] + - when: on_success # Run the job in other cases + script: echo "Running dev specs by default, or prod specs when default branch..." ``` In this example: -- If the pipeline runs on a branch that is not the default branch, the `lint` job needs the `mac:rspec` job (default behavior). -- If the pipeline runs on the default branch, and therefore the rule matches the condition, the `lint` job needs the `mac:build` and `mac:rspec` job instead. - -- If the pipeline runs on a branch that is not the default branch, the `mac:rspec` job has no needs. -- If the pipeline runs on the default branch, and therefore the rule matches the condition, the `mac:rspec` job needs the `mac:build` job instead. - -See job `lint`. It has job need `mac:rspec` defined as a plain string in an array. Under rules, the job has two needs: `mac:build` defined as a hash and `mac:rspec` defined aslo as a plain string. All 3 methods of defining needs on jobs and job rules are valid. +- If the pipeline runs on a branch that is not the default branch, the `specs` job needs the `build-dev` job (default behavior). +- If the pipeline runs on the default branch, and therefore the rule matches the condition, the `specs` job needs the `build-prod` job instead. **Additional details**: - `needs` in rules override any `needs` defined at the job-level, once overridden, the behavior is same as [job-level `needs`](#needs). - `needs` in rules can accept [`artifacts`](#needsartifacts) and [`optional`](#needsoptional). +**Related topics**: + +- Using [rules with needs and with additional attributes.](../jobs/job_control.md#specify-when-jobs-run-with-rules-and-needs) + #### `rules:variables` > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/209864) in GitLab 13.7. -- GitLab From 30d67f3e4c177aee955385acc86d60c84f8b0826 Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Wed, 26 Apr 2023 15:40:07 +0200 Subject: [PATCH 17/19] Refactor spec and fix typos --- doc/ci/jobs/job_control.md | 2 +- doc/ci/yaml/index.md | 9 +- .../ci/create_pipeline_service/rules_spec.rb | 338 ++++-------------- 3 files changed, 75 insertions(+), 274 deletions(-) diff --git a/doc/ci/jobs/job_control.md b/doc/ci/jobs/job_control.md index 449b53c934734b..e98b09db544864 100644 --- a/doc/ci/jobs/job_control.md +++ b/doc/ci/jobs/job_control.md @@ -592,7 +592,7 @@ specs: script: echo "Running dev specs by default, or prod specs when default branch..." ``` -- If the pipeline runs on a branch that is not the default branch, the `specs` job needs the `build-dev` job (default behavior) with defalt attributes. +- If the pipeline runs on a branch that is not the default branch, the `specs` job needs the `build-dev` job (default behavior) with default attributes. - If the pipeline runs on the default branch, and therefore the rule matches the condition, the `specs` job needs the `build-dev` job with default attributes as well as the `build-prod` job with customised attributes as per above YAML. - See job `specs`. It has job need `build-dev` defined as a plain string in an array. Under rules, the job has two needs: `build-prod` defined as a hash and `build-dev` defined aslo as a plain string. All 3 methods of defining needs on rules are valid. diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 0fc93cff41e141..1db298220b5e18 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -3668,17 +3668,16 @@ If the rule matches, then the job is a manual job with `allow_failure: true`. #### `rules:needs` -> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/31581) in GitLab 16.0. -> - [Deployed behind a feature flag](../../user/feature_flags.md), disabled by default. +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/31581) in GitLab 16.0 [with a flag](../../user/feature_flags.md) named `introduce_rules_with_needs`. Disabled by default. -Use `needs` in rules to update a job's [`needs`](#needs) for specific conditions. When a condition matches a rule, the job's `needs` configuration is completely replaced with the `needs in the rule. +Use `needs` in rules to update a job's [`needs`](#needs) for specific conditions. When a condition matches a rule, the job's `needs` configuration is completely replaced with the `needs` in the rule. **Keyword type**: Job-specific. You can use it only as part of a job. **Possible inputs**: - An array of job names as strings. -- A hash with job name, optionally with additional attributes. +- A hash with a job name, optionally with additional attributes. - An empty array (`[]`), to set the job needs to none when the specific condition is met. **Example of `rules:needs`**: @@ -3713,7 +3712,7 @@ In this example: **Additional details**: -- `needs` in rules override any `needs` defined at the job-level, once overridden, the behavior is same as [job-level `needs`](#needs). +- `needs` in rules override any `needs` defined at the job-level. When overridden, the behavior is same as [job-level `needs`](#needs). - `needs` in rules can accept [`artifacts`](#needsartifacts) and [`optional`](#needsoptional). **Related topics**: diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index cd70c6aada6345..87112137675de2 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -387,302 +387,104 @@ def find_job(name) end end - context 'needs:' do - let(:lint_job) { pipeline.builds.find_by(name: 'lint_job') } - let(:rspec_job) { pipeline.builds.find_by(name: 'rspec_job') } - let(:job_with_needs) { pipeline.builds.find_by(name: 'job') } - let(:ref) { 'refs/heads/master' } - - context 'when job has needs' do - context 'when rules needs are in array' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - needs: [lint_job] - script: 'echo job' - rules: - - if: $var == null - needs: [lint_job, rspec_job] - EOY - end - - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted - - needs = job_with_needs.needs.map(&:name) - expect(needs).to contain_exactly('lint_job', 'rspec_job') - end - - it 'sets the scheduling type to dag' do - expect(job_with_needs.scheduling_type).to eq('dag') - end - end - - context 'when rules needs are a hash' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - needs: - - job: lint_job - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: - - job: lint_job - - job: rspec_job - EOY - end - - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted - - needs = job_with_needs.needs.map(&:name) - expect(needs).to contain_exactly('lint_job', 'rspec_job') - end - - it 'sets the scheduling type to dag' do - expect(job_with_needs.scheduling_type).to eq('dag') - end - end - - context 'when there are both array and hash needs on a rule' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - lint_job_2: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: - - lint_job - - job: lint_job_2 - artifacts: false - optional: true - - job: rspec_job - artifacts: false - optional: true - EOY - end + context 'with needs:' do + let(:config) do + <<-EOY + job1: + script: ls - it 'overrides the needs of the job' do - expect(pipeline).to be_persisted + job2: + script: ls + rules: + - if: $var == null + needs: [job1] + - when: on_success - lint_job = job_with_needs.needs.find_by(name: 'lint_job') - lint_job_2 = job_with_needs.needs.find_by(name: 'lint_job_2') - rspec_job = job_with_needs.needs.find_by(name: 'rspec_job') - - expect(job_with_needs.needs.count).to eq(3) - expect(lint_job.class).to eq(Ci::BuildNeed) - expect(lint_job.name).to eq("lint_job") - expect(lint_job.artifacts).to eq(true) # default - expect(lint_job.optional).to eq(false) # default - - expect(lint_job_2.class).to eq(Ci::BuildNeed) - expect(lint_job_2.name).to eq("lint_job_2") - expect(lint_job_2.artifacts).to eq(false) - expect(lint_job_2.optional).to eq(true) - - expect(rspec_job.class).to eq(Ci::BuildNeed) - expect(rspec_job.name).to eq("rspec_job") - expect(rspec_job.artifacts).to eq(false) - expect(rspec_job.optional).to eq(true) - end + job3: + script: ls + rules: + - if: $var == null + needs: [job1] + - needs: [job2] - it 'sets the scheduling type to dag' do - expect(job_with_needs.scheduling_type).to eq('dag') - end - end + job4: + script: ls + needs: [job1] + rules: + - if: $var == null + needs: [job2] + - when: on_success + needs: [job3] + EOY end - context 'when job has no needs' do - let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } - - context 'when rules needs are in array' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: [lint_job] - EOY - end + let(:job1) { pipeline.builds.find_by(name: 'job1') } + let(:job2) { pipeline.builds.find_by(name: 'job2') } + let(:job3) { pipeline.builds.find_by(name: 'job3') } + let(:job4) { pipeline.builds.find_by(name: 'job4') } - it 'sets the needs of the job' do - expect(pipeline).to be_persisted + context 'when the `$var` rule matches' do + it 'creates a pipeline with overridden needs' do + expect(pipeline).to be_persisted + expect(build_names).to contain_exactly('job1', 'job2', 'job3', 'job4') - expect(job_without_needs.needs.count).to eq(1) - expect(job_without_needs.needs.first.class).to be(Ci::BuildNeed) - expect(job_without_needs.needs.first.name).to eq("lint_job") - end + expect(job1.needs).to be_empty + expect(job2.needs).to contain_exactly(an_object_having_attributes(name: 'job1')) + expect(job3.needs).to contain_exactly(an_object_having_attributes(name: 'job1')) + expect(job4.needs).to contain_exactly(an_object_having_attributes(name: 'job2')) end + end - context 'when rules needs are a hash' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: - - job: lint_job - EOY - end - - it 'sets the needs of the job' do - expect(pipeline).to be_persisted + context 'when the `$var` rule does not match' do + let(:initialization_params) { base_initialization_params.merge(variables_attributes: variables_attributes) } - expect(job_without_needs.needs.count).to eq(1) - expect(job_without_needs.needs.first.class).to be(Ci::BuildNeed) - expect(job_without_needs.needs.first.name).to eq("lint_job") - end + let(:variables_attributes) do + [{ key: 'var', secret_value: 'SOME_VAR' }] end - context 'when rules have no needs' do - let(:config) do - <<-EOY - job: - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - EOY - end - - it 'sets the needs of the job' do - expect(pipeline).to be_persisted - expect(job_without_needs.needs).to be_empty - end + it 'creates a pipeline with overridden needs' do + expect(pipeline).to be_persisted + expect(build_names).to contain_exactly('job1', 'job2', 'job3', 'job4') - it 'does not change the scheduling type' do - expect(job_with_needs.scheduling_type).to eq('stage') - end + expect(job1.needs).to be_empty + expect(job2.needs).to be_empty + expect(job3.needs).to contain_exactly(an_object_having_attributes(name: 'job2')) + expect(job4.needs).to contain_exactly(an_object_having_attributes(name: 'job3')) end end - context 'with introduce_rules_with_needs disabled' do + context 'when the FF introduce_rules_with_needs is disabled' do before do stub_feature_flags(introduce_rules_with_needs: false) end - context 'when job has needs' do - context 'when rule is returning multiple needs' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - needs: - - job: rspec_job - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: - - job: lint_job - - rspec_job - EOY - end - - it 'keeps the needs of the job' do - expect(pipeline).to be_persisted - - expect(job_with_needs.needs.count).to eq(1) + context 'when the `$var` rule matches' do + it 'creates a pipeline without overridden needs' do + expect(pipeline).to be_persisted + expect(build_names).to contain_exactly('job1', 'job2', 'job3', 'job4') - need = job_with_needs.needs.first - expect(need.class).to eq(Ci::BuildNeed) - expect(need.name).to eq("rspec_job") - end + expect(job1.needs).to be_empty + expect(job2.needs).to be_empty + expect(job3.needs).to be_empty + expect(job4.needs).to contain_exactly(an_object_having_attributes(name: 'job1')) end end - context 'when job has no needs' do - let(:config) do - <<-EOY - lint_job: - script: 'echo lint_job' - rules: - - if: $var == null - - when: always - rspec_job: - script: 'echo rspec_job' - rules: - - if: $var == null - - when: always - job: - script: 'echo job' - rules: - - if: $CI_COMMIT_REF_NAME =~ /master/ - needs: - - job: lint_job - EOY - end + context 'when the `$var` rule does not match' do + let(:initialization_params) { base_initialization_params.merge(variables_attributes: variables_attributes) } - let(:job_without_needs) { pipeline.builds.find_by(name: 'job') } + let(:variables_attributes) do + [{ key: 'var', secret_value: 'SOME_VAR' }] + end - it 'return an empty array' do + it 'creates a pipeline without overridden needs' do expect(pipeline).to be_persisted + expect(build_names).to contain_exactly('job1', 'job2', 'job3', 'job4') - expect(job_without_needs.needs).to eq([]) - end - - it 'does not change the scheduling type' do - expect(job_with_needs.scheduling_type).to eq('stage') + expect(job1.needs).to be_empty + expect(job2.needs).to be_empty + expect(job3.needs).to be_empty + expect(job4.needs).to contain_exactly(an_object_having_attributes(name: 'job1')) end end end -- GitLab From 1b54129add1dc64e3bb72f3d7e76c58ba002f8eb Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Wed, 26 Apr 2023 18:21:22 +0200 Subject: [PATCH 18/19] Remove additional doc - to be added at later stage --- doc/ci/jobs/job_control.md | 38 -------------------------------------- doc/ci/yaml/index.md | 4 ---- 2 files changed, 42 deletions(-) diff --git a/doc/ci/jobs/job_control.md b/doc/ci/jobs/job_control.md index e98b09db544864..3cd57ff6a6ad45 100644 --- a/doc/ci/jobs/job_control.md +++ b/doc/ci/jobs/job_control.md @@ -559,44 +559,6 @@ test: - "README.md" ``` -## Specify when jobs run with `rules` and `needs` - -- `needs` in rules can accept [`artifacts`](../yaml/index.md#needsartifacts) and [`optional`](../yaml/index.md#needsoptional) - -**Example of `rules:needs`**: - -```yaml -build-dev: - stage: build - rules: - - if: $CI_COMMIT_BRANCH != $CI_DEFAULT_BRANCH - script: echo "Feature branch, so building dev version..." - -build-prod: - stage: build - rules: - - if: $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH - script: echo "Default branch, so building prod version..." - -specs: - stage: test - needs: ['build-dev'] - rules: - - if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH - needs: - - job: 'build-prod' - artifacts: false - optional: true - - 'build-dev' - - when: on_success # Run the job in other cases - script: echo "Running dev specs by default, or prod specs when default branch..." -``` - -- If the pipeline runs on a branch that is not the default branch, the `specs` job needs the `build-dev` job (default behavior) with default attributes. -- If the pipeline runs on the default branch, and therefore the rule matches the condition, the `specs` job needs the `build-dev` job with default attributes as well as the `build-prod` job with customised attributes as per above YAML. - -- See job `specs`. It has job need `build-dev` defined as a plain string in an array. Under rules, the job has two needs: `build-prod` defined as a hash and `build-dev` defined aslo as a plain string. All 3 methods of defining needs on rules are valid. - ## Create a job that must be run manually You can require that a job doesn't run unless a user starts it. This is called a **manual job**. diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 1db298220b5e18..49dbd08c90b3ac 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -3715,10 +3715,6 @@ In this example: - `needs` in rules override any `needs` defined at the job-level. When overridden, the behavior is same as [job-level `needs`](#needs). - `needs` in rules can accept [`artifacts`](#needsartifacts) and [`optional`](#needsoptional). -**Related topics**: - -- Using [rules with needs and with additional attributes.](../jobs/job_control.md#specify-when-jobs-run-with-rules-and-needs) - #### `rules:variables` > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/209864) in GitLab 13.7. -- GitLab From 86850a48e5105242fed9723a13735ddd63150a7e Mon Sep 17 00:00:00 2001 From: Kasia Misirli Date: Wed, 10 May 2023 14:09:39 +0200 Subject: [PATCH 19/19] Refactor implementation --- lib/gitlab/ci/yaml_processor.rb | 19 ++++++++----------- spec/lib/gitlab/ci/yaml_processor_spec.rb | 5 +++-- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/lib/gitlab/ci/yaml_processor.rb b/lib/gitlab/ci/yaml_processor.rb index 3b5ad73838da0d..c69d9218a664b9 100644 --- a/lib/gitlab/ci/yaml_processor.rb +++ b/lib/gitlab/ci/yaml_processor.rb @@ -62,7 +62,6 @@ def validate_job!(name, job) validate_job_stage!(name, job) validate_job_dependencies!(name, job) validate_job_needs!(name, job) - validate_job_rule_needs!(name, job) validate_dynamic_child_pipeline_dependencies!(name, job) validate_job_environment!(name, job) end @@ -95,16 +94,20 @@ def validate_dynamic_child_pipeline_dependencies!(name, job) end def validate_job_needs!(name, job) - return unless needs = job.dig(:needs, :job) + validate_needs_specification!(name, job.dig(:needs, :job)) - validate_duplicate_needs!(name, needs) + job[:rules]&.each do |rule| + validate_needs_specification!(name, rule.dig(:needs, :job)) + end + end + + def validate_needs_specification!(name, needs) + return unless needs needs.each do |need| validate_job_dependency!(name, need[:name], 'need', optional: need[:optional]) end - end - def validate_duplicate_needs!(name, needs) duplicated_needs = needs .group_by { |need| need[:name] } @@ -134,12 +137,6 @@ def validate_job_dependency!(name, dependency, dependency_type = 'dependency', o end end - def validate_job_rule_needs!(name, job) - return unless job[:rules] - - job[:rules].each { |rule| validate_job_needs!(name, rule) } - end - def stage_index(name) stage = @jobs.dig(name.to_sym, :stage) @stages.index(stage) diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index 962b4b95f4d944..f8c2889798fde6 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -660,13 +660,14 @@ module Ci it_behaves_like 'has warnings and expected error', /build job: need test is not defined in current or prior stages/ end - describe '#validate_job_rule_needs!' do + describe '#validate_job_needs!' do context "when all validations pass" do let(:config) do <<-EOYML stages: - lint lint_job: + needs: [lint_job_2] stage: lint script: 'echo lint_job' rules: @@ -791,7 +792,7 @@ module Ci EOYML end - it_behaves_like 'returns errors', 'test_job_1 has duplicate entries in the needs section.' + it_behaves_like 'returns errors', 'test_job_1 has the following needs duplicated: test_job_2.' end end -- GitLab