From 44669f11730bbdbf196202e8f729b75d29299969 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Wed, 15 Jun 2022 13:19:41 +0300 Subject: [PATCH 1/3] Refactor CI rules entry to use composed value When we return a value from an entry, we always need to return the composed version. In Rules, we return "config", which means the hash version of what the YAML file has. I changed it to "super" because it needs to have composed value of each sub entry. With this, we also needed to change the validate_against_warnings method. Since we haven't composed the nested entries yet, we cannot use *_value attributes there. These changes are behind a feature flag ci_value_change_for_processable_and_rules_entry --- lib/gitlab/ci/config/entry/processable.rb | 6 ++++-- lib/gitlab/ci/config/entry/rules.rb | 3 ++- spec/lib/gitlab/ci/config/entry/rules_spec.rb | 17 ++++++++++------- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/lib/gitlab/ci/config/entry/processable.rb b/lib/gitlab/ci/config/entry/processable.rb index 46afedbcc3ad72..e6844f8cb98f7b 100644 --- a/lib/gitlab/ci/config/entry/processable.rb +++ b/lib/gitlab/ci/config/entry/processable.rb @@ -96,9 +96,11 @@ def compose!(deps = nil) def validate_against_warnings # If rules are valid format and workflow rules are not specified - return unless rules_value + return unless rules - last_rule = rules_value.last + # Since we haven't composed the nested entries yet, we cannot use *_value attributes. + # However, we need to duplicate the [].flatten logic in lib/gitlab/ci/config/entry/rules.rb + last_rule = [rules].flatten.last if last_rule&.keys == [:when] && last_rule[:when] != 'never' docs_url = 'read more: https://docs.gitlab.com/ee/ci/troubleshooting.html#pipeline-warnings' diff --git a/lib/gitlab/ci/config/entry/rules.rb b/lib/gitlab/ci/config/entry/rules.rb index 53e52981471718..64308dc7060c98 100644 --- a/lib/gitlab/ci/config/entry/rules.rb +++ b/lib/gitlab/ci/config/entry/rules.rb @@ -13,7 +13,8 @@ class Rules < ::Gitlab::Config::Entry::ComposableArray end def value - [@config].flatten + # `flatten` is needed to make it work with nested `!reference` + [super].flatten end def composable_class diff --git a/spec/lib/gitlab/ci/config/entry/rules_spec.rb b/spec/lib/gitlab/ci/config/entry/rules_spec.rb index cfec33003e4e6f..ab6590e88cefd1 100644 --- a/spec/lib/gitlab/ci/config/entry/rules_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules_spec.rb @@ -12,13 +12,12 @@ end let(:metadata) { { allowed_when: %w[always never] } } - let(:entry) { factory.create! } - describe '.new' do - subject { entry } + subject(:entry) { factory.create! } + describe '.new' do before do - subject.compose! + entry.compose! end context 'with a list of rule rule' do @@ -73,7 +72,11 @@ end describe '#value' do - subject { entry.value } + subject(:value) { entry.value } + + before do + entry.compose! + end context 'with a list of rule rule' do let(:config) do @@ -96,10 +99,10 @@ context 'with a single rule object' do let(:config) do - { if: '$SKIP', when: 'never' } + [{ if: '$SKIP', when: 'never' }] end - it { is_expected.to eq([config]) } + it { is_expected.to eq(config) } end context 'with nested rules' do -- GitLab From 7eff9948eb5472e8ec6090f95be0d0490cfb92e9 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Wed, 22 Jun 2022 00:27:13 +0300 Subject: [PATCH 2/3] Add feature flag ci_value_change_for_processable_and_rules_entry --- ...ue_change_for_processable_and_rules_entry.yml | 8 ++++++++ lib/gitlab/ci/config/entry/processable.rb | 16 +++++++++++----- lib/gitlab/ci/config/entry/rules.rb | 10 ++++++++-- .../gitlab/ci/config/entry/processable_spec.rb | 13 +++++++++++++ spec/lib/gitlab/ci/config/entry/rules_spec.rb | 15 +++++++++++++-- 5 files changed, 53 insertions(+), 9 deletions(-) create mode 100644 config/feature_flags/development/ci_value_change_for_processable_and_rules_entry.yml diff --git a/config/feature_flags/development/ci_value_change_for_processable_and_rules_entry.yml b/config/feature_flags/development/ci_value_change_for_processable_and_rules_entry.yml new file mode 100644 index 00000000000000..f7c2542cb22f8b --- /dev/null +++ b/config/feature_flags/development/ci_value_change_for_processable_and_rules_entry.yml @@ -0,0 +1,8 @@ +--- +name: ci_value_change_for_processable_and_rules_entry +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/90238 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/365876 +milestone: '15.2' +type: development +group: group::pipeline authoring +default_enabled: false diff --git a/lib/gitlab/ci/config/entry/processable.rb b/lib/gitlab/ci/config/entry/processable.rb index e6844f8cb98f7b..4d55c359171c7e 100644 --- a/lib/gitlab/ci/config/entry/processable.rb +++ b/lib/gitlab/ci/config/entry/processable.rb @@ -95,12 +95,18 @@ def compose!(deps = nil) end def validate_against_warnings - # If rules are valid format and workflow rules are not specified - return unless rules + if ::Feature.enabled?(:ci_value_change_for_processable_and_rules_entry) + # If rules are valid format and workflow rules are not specified + return unless rules - # Since we haven't composed the nested entries yet, we cannot use *_value attributes. - # However, we need to duplicate the [].flatten logic in lib/gitlab/ci/config/entry/rules.rb - last_rule = [rules].flatten.last + # Since we haven't composed the nested entries yet, we cannot use *_value attributes. + # However, we need to duplicate the [].flatten logic in lib/gitlab/ci/config/entry/rules.rb + last_rule = [rules].flatten.last + else + return unless rules_value + + last_rule = rules_value.last + end if last_rule&.keys == [:when] && last_rule[:when] != 'never' docs_url = 'read more: https://docs.gitlab.com/ee/ci/troubleshooting.html#pipeline-warnings' diff --git a/lib/gitlab/ci/config/entry/rules.rb b/lib/gitlab/ci/config/entry/rules.rb index 64308dc7060c98..1e88ac033181ca 100644 --- a/lib/gitlab/ci/config/entry/rules.rb +++ b/lib/gitlab/ci/config/entry/rules.rb @@ -13,8 +13,14 @@ class Rules < ::Gitlab::Config::Entry::ComposableArray end def value - # `flatten` is needed to make it work with nested `!reference` - [super].flatten + if ::Feature.enabled?(:ci_value_change_for_processable_and_rules_entry) + # `flatten` is needed to make it work with nested `!reference` + # Notice that this logic is also duplicated in + # the `validate_against_warnings` method of `lib/gitlab/ci/config/entry/processable.rb` + [super].flatten + else + [@config].flatten + end end def composable_class diff --git a/spec/lib/gitlab/ci/config/entry/processable_spec.rb b/spec/lib/gitlab/ci/config/entry/processable_spec.rb index 5b9337ede3437a..cc32c57e873f43 100644 --- a/spec/lib/gitlab/ci/config/entry/processable_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/processable_spec.rb @@ -278,8 +278,13 @@ def self.name context 'when workflow rules is not used' do let(:workflow) { double('workflow', 'has_rules?' => false) } + let(:ci_value_change_for_processable_and_rules_entry) { true } before do + stub_feature_flags( + ci_value_change_for_processable_and_rules_entry: ci_value_change_for_processable_and_rules_entry + ) + entry.compose!(deps) end @@ -303,6 +308,14 @@ def self.name it 'raises a warning' do expect(entry.warnings).to contain_exactly(/may allow multiple pipelines/) end + + context 'when the FF ci_value_change_for_processable_and_rules_entry is disabled' do + let(:ci_value_change_for_processable_and_rules_entry) { false } + + it 'raises a warning' do + expect(entry.warnings).to contain_exactly(/may allow multiple pipelines/) + end + end end context 'and its value is `never`' do diff --git a/spec/lib/gitlab/ci/config/entry/rules_spec.rb b/spec/lib/gitlab/ci/config/entry/rules_spec.rb index ab6590e88cefd1..faae5aa121f122 100644 --- a/spec/lib/gitlab/ci/config/entry/rules_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules_spec.rb @@ -1,10 +1,13 @@ # frozen_string_literal: true require 'fast_spec_helper' +require 'support/helpers/stubbed_feature' require 'support/helpers/stub_feature_flags' require_dependency 'active_model' RSpec.describe Gitlab::Ci::Config::Entry::Rules do + include StubFeatureFlags + let(:factory) do Gitlab::Config::Entry::Factory.new(described_class) .metadata(metadata) @@ -99,10 +102,18 @@ context 'with a single rule object' do let(:config) do - [{ if: '$SKIP', when: 'never' }] + { if: '$SKIP', when: 'never' } end - it { is_expected.to eq(config) } + it { is_expected.to eq([]) } + + context 'when the FF ci_value_change_for_processable_and_rules_entry is disabled' do + before do + stub_feature_flags(ci_value_change_for_processable_and_rules_entry: false) + end + + it { is_expected.to eq([config]) } + end end context 'with nested rules' do -- GitLab From 57154c487b21e147df98c7df2c96b8768e26f09f Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Thu, 23 Jun 2022 13:42:16 +0300 Subject: [PATCH 3/3] Move validate_against_warnings to outside of the block --- lib/gitlab/ci/config/entry/processable.rb | 26 ++++++++++------------- lib/gitlab/ci/config/entry/rules.rb | 2 -- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/lib/gitlab/ci/config/entry/processable.rb b/lib/gitlab/ci/config/entry/processable.rb index 4d55c359171c7e..d17c52a3ace6ea 100644 --- a/lib/gitlab/ci/config/entry/processable.rb +++ b/lib/gitlab/ci/config/entry/processable.rb @@ -71,9 +71,9 @@ module Processable end def compose!(deps = nil) - super do - has_workflow_rules = deps&.workflow_entry&.has_rules? + has_workflow_rules = deps&.workflow_entry&.has_rules? + super do # If workflow:rules: or rules: are used # they are considered not compatible # with `only/except` defaults @@ -86,27 +86,23 @@ def compose!(deps = nil) @entries.delete(:except) unless except_defined? # rubocop:disable Gitlab/ModuleWithInstanceVariables end - unless has_workflow_rules - validate_against_warnings + unless ::Feature.enabled?(:ci_value_change_for_processable_and_rules_entry) + validate_against_warnings unless has_workflow_rules end yield if block_given? end - end - def validate_against_warnings if ::Feature.enabled?(:ci_value_change_for_processable_and_rules_entry) - # If rules are valid format and workflow rules are not specified - return unless rules + validate_against_warnings unless has_workflow_rules + end + end - # Since we haven't composed the nested entries yet, we cannot use *_value attributes. - # However, we need to duplicate the [].flatten logic in lib/gitlab/ci/config/entry/rules.rb - last_rule = [rules].flatten.last - else - return unless rules_value + def validate_against_warnings + # If rules are valid format and workflow rules are not specified + return unless rules_value - last_rule = rules_value.last - end + last_rule = rules_value.last if last_rule&.keys == [:when] && last_rule[:when] != 'never' docs_url = 'read more: https://docs.gitlab.com/ee/ci/troubleshooting.html#pipeline-warnings' diff --git a/lib/gitlab/ci/config/entry/rules.rb b/lib/gitlab/ci/config/entry/rules.rb index 1e88ac033181ca..4f2f08889a16ae 100644 --- a/lib/gitlab/ci/config/entry/rules.rb +++ b/lib/gitlab/ci/config/entry/rules.rb @@ -15,8 +15,6 @@ class Rules < ::Gitlab::Config::Entry::ComposableArray def value if ::Feature.enabled?(:ci_value_change_for_processable_and_rules_entry) # `flatten` is needed to make it work with nested `!reference` - # Notice that this logic is also duplicated in - # the `validate_against_warnings` method of `lib/gitlab/ci/config/entry/processable.rb` [super].flatten else [@config].flatten -- GitLab