From 6ac38ec0435bd72020d2a5a052d87dae429ae36a Mon Sep 17 00:00:00 2001 From: lma-git Date: Fri, 10 Feb 2023 17:56:41 -0800 Subject: [PATCH 1/2] Update expandset from Set to Array This MR updates the structure we use to count include files so that duplicate files are now counted. FF: ci_includes_count_duplicates --- .../ci_includes_count_duplicates.yml | 8 + lib/gitlab/ci/config/external/context.rb | 5 +- .../ci/config/external/mapper/verifier.rb | 6 +- .../gitlab/ci/config/external/context_spec.rb | 33 ++++- .../config/external/mapper/verifier_spec.rb | 140 +++++++++++++----- .../gitlab/ci/config/external/mapper_spec.rb | 15 +- 6 files changed, 162 insertions(+), 45 deletions(-) create mode 100644 config/feature_flags/development/ci_includes_count_duplicates.yml 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 00000000000000..b50243467d3aad --- /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: +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 138e79db33181d..6eef279d3de843 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 6c161e9515427f..83ef58bb46f8c9 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 45efb16434b8c8..1fd3cf3c99fc58 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 7bdd519cb74de2..ca884b9991b794 100644 --- a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb @@ -97,21 +97,32 @@ Gitlab::Ci::Config::External::File::Project.new( { file: 'myfolder/file3.yml', project: included_project.full_path }, context ) - ] - end + end - around(:all) do |example| - create_and_delete_files(included_project, project_files) do - example.run + it 'adds files to the expandset' do + expect { process }.to change { context.expandset.count }.by(3) end - end - it 'returns an array of file objects' do - expect(process.map(&:location)).to contain_exactly( - 'myfolder/file1.yml', 'myfolder/file2.yml', 'myfolder/file3.yml' - ) + it 'calls Gitaly only once for all files', :request_store do + # 1 for project.commit.id, 1 for the files + expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(2) + end + + context 'when the FF ci_batch_request_for_local_and_project_includes is disabled' do + before do + stub_feature_flags(ci_batch_request_for_local_and_project_includes: false) + end + + it 'calls Gitaly for each file', :request_store do + # 1 for project.commit.id, 3 for the files + expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(4) + end + end end + context 'when files are project files' do + let_it_be(:included_project) { create(:project, :repository, namespace: project.namespace, creator: user) } + it 'adds files to the expandset' do expect { process }.to change { context.expandset.count }.by(3) end @@ -122,35 +133,9 @@ end end - context 'when a file includes other files' do - let(:files) do - [ - Gitlab::Ci::Config::External::File::Local.new({ local: 'nested_configs.yml' }, context) - ] - end - - it 'returns an array of file objects with combined hash' do - expect(process.map(&:to_hash)).to contain_exactly( - { my_build: { script: 'echo Hello World' }, - my_test: { script: 'echo Hello World' }, - remote_test: { script: 'echo Hello World' } } - ) - end - end - - context 'when there is an invalid file' do - let(:files) do - [ - Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/invalid.yml' }, context) - ] - end - - it 'adds an error to the file' do - expect(process.first.errors).to include("Local file `myfolder/invalid.yml` does not exist!") - end - end + it_behaves_like 'when FF ci_includes_count_duplicates is enabled' - 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 [ @@ -183,6 +168,85 @@ expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) end end + + 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 + + before do + allow(context).to receive(:max_includes).and_return(2) + end + + it 'raises error' do + expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + end + end + end + + context 'when FF ci_includes_count_duplicates is disabled' do + before do + stub_feature_flags(ci_includes_count_duplicates: false) + end + + it_behaves_like 'when FF ci_includes_count_duplicates is enabled' + + context 'when total file count exceeds max_includes' do + context 'when files are nested' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'nested_configs.yml' }, context) + ] + end + + before do + allow(context).to receive(:max_includes).and_return(1) + end + + it 'raises Processor::IncludeError' do + expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) + end + end + + context 'when files are not nested' 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/file2.yml' }, context) + ] + end + + before do + allow(context).to receive(:max_includes).and_return(1) + end + + it 'raises Mapper::TooManyIncludesError' do + expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + end + end + + 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 + + before do + allow(context).to receive(:max_includes).and_return(2) + end + + it 'raises error' do + expect { process }.not_to raise_error + end + 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 dc14bf18b982b9..b3115617084b72 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 -- GitLab From e5012bd8fd28d80577a344fce10a60439548bd0b Mon Sep 17 00:00:00 2001 From: lma-git Date: Mon, 13 Feb 2023 10:21:41 -0800 Subject: [PATCH 2/2] Refactor rspec tests --- .../ci_includes_count_duplicates.yml | 2 +- .../config/external/mapper/verifier_spec.rb | 130 ++++++------------ 2 files changed, 44 insertions(+), 88 deletions(-) diff --git a/config/feature_flags/development/ci_includes_count_duplicates.yml b/config/feature_flags/development/ci_includes_count_duplicates.yml index b50243467d3aad..5e33edddc03a9f 100644 --- a/config/feature_flags/development/ci_includes_count_duplicates.yml +++ b/config/feature_flags/development/ci_includes_count_duplicates.yml @@ -1,7 +1,7 @@ --- name: ci_includes_count_duplicates introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111726 -rollout_issue_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/391517 milestone: '15.9' type: development group: group::pipeline authoring 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 ca884b9991b794..a219666f24e7d4 100644 --- a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb @@ -97,31 +97,20 @@ Gitlab::Ci::Config::External::File::Project.new( { file: 'myfolder/file3.yml', project: included_project.full_path }, context ) - end - - it 'adds files to the expandset' do - expect { process }.to change { context.expandset.count }.by(3) - end - - it 'calls Gitaly only once for all files', :request_store do - # 1 for project.commit.id, 1 for the files - expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(2) - end - - context 'when the FF ci_batch_request_for_local_and_project_includes is disabled' do - before do - stub_feature_flags(ci_batch_request_for_local_and_project_includes: false) - end + ] + end - it 'calls Gitaly for each file', :request_store do - # 1 for project.commit.id, 3 for the files - expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(4) - end + around(:all) do |example| + create_and_delete_files(included_project, project_files) do + example.run end end - context 'when files are project files' do - let_it_be(:included_project) { create(:project, :repository, namespace: project.namespace, creator: user) } + it 'returns an array of file objects' do + expect(process.map(&:location)).to contain_exactly( + 'myfolder/file1.yml', 'myfolder/file2.yml', 'myfolder/file3.yml' + ) + end it 'adds files to the expandset' do expect { process }.to change { context.expandset.count }.by(3) @@ -133,7 +122,33 @@ end end - it_behaves_like 'when FF ci_includes_count_duplicates is enabled' + context 'when a file includes other files' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'nested_configs.yml' }, context) + ] + end + + it 'returns an array of file objects with combined hash' do + expect(process.map(&:to_hash)).to contain_exactly( + { my_build: { script: 'echo Hello World' }, + my_test: { script: 'echo Hello World' }, + remote_test: { script: 'echo Hello World' } } + ) + end + end + + context 'when there is an invalid file' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/invalid.yml' }, context) + ] + end + + it 'adds an error to the file' do + expect(process.first.errors).to include("Local file `myfolder/invalid.yml` does not exist!") + end + end context 'when total file count exceeds max_includes' do context 'when files are nested' do @@ -143,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 @@ -160,11 +172,8 @@ ] end - before do - allow(context).to receive(:max_includes).and_return(1) - end - 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 @@ -178,71 +187,18 @@ ] end - before do - allow(context).to receive(:max_includes).and_return(2) - 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 - end - end - - context 'when FF ci_includes_count_duplicates is disabled' do - before do - stub_feature_flags(ci_includes_count_duplicates: false) - end - - it_behaves_like 'when FF ci_includes_count_duplicates is enabled' - - context 'when total file count exceeds max_includes' do - context 'when files are nested' do - let(:files) do - [ - Gitlab::Ci::Config::External::File::Local.new({ local: 'nested_configs.yml' }, context) - ] - end + context 'when FF ci_includes_count_duplicates is disabled' do before do - allow(context).to receive(:max_includes).and_return(1) + stub_feature_flags(ci_includes_count_duplicates: false) end - it 'raises Processor::IncludeError' do - expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) - end - end - - context 'when files are not nested' 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/file2.yml' }, context) - ] - end - - before do - allow(context).to receive(:max_includes).and_return(1) - end - - it 'raises Mapper::TooManyIncludesError' do - expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) - end - end - - 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 - - before do + it 'does not raise error' do allow(context).to receive(:max_includes).and_return(2) - end - - it 'raises error' do expect { process }.not_to raise_error end end -- GitLab