diff --git a/config/feature_flags/development/ci_includes_count_duplicates.yml b/config/feature_flags/development/ci_includes_count_duplicates.yml new file mode 100644 index 0000000000000000000000000000000000000000..5e33edddc03a9f79c0e2cbbbb4a3155d29d43008 --- /dev/null +++ b/config/feature_flags/development/ci_includes_count_duplicates.yml @@ -0,0 +1,8 @@ +--- +name: ci_includes_count_duplicates +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111726 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/391517 +milestone: '15.9' +type: development +group: group::pipeline authoring +default_enabled: false diff --git a/lib/gitlab/ci/config/external/context.rb b/lib/gitlab/ci/config/external/context.rb index 138e79db33181d05a03d015ac46dc346070be3f4..6eef279d3de843396efe9d3e6399babb1bea6f5f 100644 --- a/lib/gitlab/ci/config/external/context.rb +++ b/lib/gitlab/ci/config/external/context.rb @@ -10,6 +10,7 @@ class Context TimeoutError = Class.new(StandardError) MAX_INCLUDES = 100 + NEW_MAX_INCLUDES = 150 # Update to MAX_INCLUDES when FF ci_includes_count_duplicates is removed include ::Gitlab::Utils::StrongMemoize @@ -27,10 +28,10 @@ def initialize( @user = user @parent_pipeline = parent_pipeline @variables = variables || Ci::Variables::Collection.new - @expandset = Set.new + @expandset = Feature.enabled?(:ci_includes_count_duplicates, project) ? [] : Set.new @execution_deadline = 0 @logger = logger || Gitlab::Ci::Pipeline::Logger.new(project: project) - @max_includes = MAX_INCLUDES + @max_includes = Feature.enabled?(:ci_includes_count_duplicates, project) ? NEW_MAX_INCLUDES : MAX_INCLUDES yield self if block_given? end diff --git a/lib/gitlab/ci/config/external/mapper/verifier.rb b/lib/gitlab/ci/config/external/mapper/verifier.rb index 6c161e9515427f8f49a3dc163133e0475040ed61..83ef58bb46f8c9dd3f8d4e3df9c3816233c4a6ff 100644 --- a/lib/gitlab/ci/config/external/mapper/verifier.rb +++ b/lib/gitlab/ci/config/external/mapper/verifier.rb @@ -28,7 +28,11 @@ def process_without_instrumentation(files) file.validate_content! if file.valid? file.load_and_validate_expanded_hash! if file.valid? - context.expandset.add(file) + if ::Feature.enabled?(:ci_includes_count_duplicates, context.project) + context.expandset << file + else + context.expandset.add(file) + end end end diff --git a/spec/lib/gitlab/ci/config/external/context_spec.rb b/spec/lib/gitlab/ci/config/external/context_spec.rb index 45efb16434b8c8616c85fef0d329e92c7144b4b9..1fd3cf3c99fc58e6ba4c90795d414c09ca20a08f 100644 --- a/spec/lib/gitlab/ci/config/external/context_spec.rb +++ b/spec/lib/gitlab/ci/config/external/context_spec.rb @@ -14,7 +14,8 @@ describe 'attributes' do context 'with values' do it { is_expected.to have_attributes(**attributes) } - it { expect(subject.expandset).to eq(Set.new) } + it { expect(subject.expandset).to eq([]) } + it { expect(subject.max_includes).to eq(Gitlab::Ci::Config::External::Context::NEW_MAX_INCLUDES) } it { expect(subject.execution_deadline).to eq(0) } it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) } it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) } @@ -25,11 +26,39 @@ let(:attributes) { { project: nil, user: nil, sha: nil } } it { is_expected.to have_attributes(**attributes) } - it { expect(subject.expandset).to eq(Set.new) } + it { expect(subject.expandset).to eq([]) } + it { expect(subject.max_includes).to eq(Gitlab::Ci::Config::External::Context::NEW_MAX_INCLUDES) } it { expect(subject.execution_deadline).to eq(0) } it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) } it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) } end + + context 'when FF ci_includes_count_duplicates is disabled' do + before do + stub_feature_flags(ci_includes_count_duplicates: false) + end + + context 'with values' do + it { is_expected.to have_attributes(**attributes) } + it { expect(subject.expandset).to eq(Set.new) } + it { expect(subject.max_includes).to eq(Gitlab::Ci::Config::External::Context::MAX_INCLUDES) } + it { expect(subject.execution_deadline).to eq(0) } + it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) } + it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) } + it { expect(subject.variables_hash).to include('a' => 'b') } + end + + context 'without values' do + let(:attributes) { { project: nil, user: nil, sha: nil } } + + it { is_expected.to have_attributes(**attributes) } + it { expect(subject.expandset).to eq(Set.new) } + it { expect(subject.max_includes).to eq(Gitlab::Ci::Config::External::Context::MAX_INCLUDES) } + it { expect(subject.execution_deadline).to eq(0) } + it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) } + it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) } + end + end end describe '#set_deadline' do diff --git a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb index 7bdd519cb74de27ac57fc4eb4b9cd229a3579153..a219666f24e7d4484f3c0ca1dcb9a331547e58b0 100644 --- a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb @@ -150,7 +150,7 @@ end end - context 'when max_includes is exceeded' do + context 'when total file count exceeds max_includes' do context 'when files are nested' do let(:files) do [ @@ -158,11 +158,8 @@ ] end - before do - allow(context).to receive(:max_includes).and_return(1) - end - it 'raises Processor::IncludeError' do + allow(context).to receive(:max_includes).and_return(1) expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) end end @@ -175,13 +172,36 @@ ] end - before do + it 'raises Mapper::TooManyIncludesError' do allow(context).to receive(:max_includes).and_return(1) + expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) end + end - it 'raises Mapper::TooManyIncludesError' do + context 'when files are duplicates' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context) + ] + end + + it 'raises error' do + allow(context).to receive(:max_includes).and_return(2) expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) end + + context 'when FF ci_includes_count_duplicates is disabled' do + before do + stub_feature_flags(ci_includes_count_duplicates: false) + end + + it 'does not raise error' do + allow(context).to receive(:max_includes).and_return(2) + expect { process }.not_to raise_error + end + end end end end diff --git a/spec/lib/gitlab/ci/config/external/mapper_spec.rb b/spec/lib/gitlab/ci/config/external/mapper_spec.rb index dc14bf18b982b9385a80f832c3d55fe6e4292f54..b3115617084b728c3a08ec7b3a38c8159d0f9f5b 100644 --- a/spec/lib/gitlab/ci/config/external/mapper_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper_spec.rb @@ -230,9 +230,20 @@ expect { process }.not_to raise_error end - it 'has expanset with one' do + it 'has expanset with two' do process - expect(context.expandset.size).to eq(1) + expect(context.expandset.size).to eq(2) + end + + context 'when FF ci_includes_count_duplicates is disabled' do + before do + stub_feature_flags(ci_includes_count_duplicates: false) + end + + it 'has expanset with one' do + process + expect(context.expandset.size).to eq(1) + end end end