diff --git a/config/feature_flags/development/ci_fix_max_includes.yml b/config/feature_flags/development/ci_fix_max_includes.yml new file mode 100644 index 0000000000000000000000000000000000000000..8abcf2640cd76a310d8a4b72b0ba40bbcd54e147 --- /dev/null +++ b/config/feature_flags/development/ci_fix_max_includes.yml @@ -0,0 +1,8 @@ +--- +name: ci_fix_max_includes +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111391 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/390909 +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..71e21f06f19dd968e7b1b5a28b13335ca1ed6f4b 100644 --- a/lib/gitlab/ci/config/external/context.rb +++ b/lib/gitlab/ci/config/external/context.rb @@ -27,7 +27,7 @@ def initialize( @user = user @parent_pipeline = parent_pipeline @variables = variables || Ci::Variables::Collection.new - @expandset = Set.new + @expandset = Feature.enabled?(:ci_fix_max_includes, project) ? [] : Set.new @execution_deadline = 0 @logger = logger || Gitlab::Ci::Pipeline::Logger.new(project: project) @max_includes = MAX_INCLUDES diff --git a/lib/gitlab/ci/config/external/mapper/verifier.rb b/lib/gitlab/ci/config/external/mapper/verifier.rb index beb4dd1add7f8fbc5e8b3db9e5aa3a6726ca0d78..896bd18a3aa8f9f7452634be33cba47c5c653c46 100644 --- a/lib/gitlab/ci/config/external/mapper/verifier.rb +++ b/lib/gitlab/ci/config/external/mapper/verifier.rb @@ -15,6 +15,11 @@ def process_without_instrumentation(files) end files.each do |file| + if ::Feature.enabled?(:ci_fix_max_includes, context.project) + context.expandset << file + verify_max_includes! + end + verify_execution_time! file.validate_location! @@ -25,31 +30,44 @@ def process_without_instrumentation(files) # We do not combine the loops because we need to load the content of all files before continuing # to call `BatchLoader` for all locations. files.each do |file| # rubocop:disable Style/CombinableLoops - # Checking the max includes will be changed with https://gitlab.com/gitlab-org/gitlab/-/issues/367150 - verify_max_includes! + verify_max_includes! if ::Feature.disabled?(:ci_fix_max_includes, context.project) + verify_execution_time! file.validate_content! if file.valid? file.load_and_validate_expanded_hash! if file.valid? - context.expandset.add(file) + context.expandset.add(file) if ::Feature.disabled?(:ci_fix_max_includes, context.project) end end # Will be removed with the FF ci_batch_request_for_local_and_project_includes def legacy_process_without_instrumentation(files) files.select do |file| - verify_max_includes! - verify_execution_time! + if ::Feature.enabled?(:ci_fix_max_includes, context.project) + context.expandset << file + verify_max_includes! - file.validate! + verify_execution_time! - context.expandset.add(file) + file.validate! + else + verify_max_includes! + verify_execution_time! + + file.validate! + + context.expandset.add(file) + end end end def verify_max_includes! - return if context.expandset.count < context.max_includes + if ::Feature.enabled?(:ci_fix_max_includes, context.project) + return if context.expandset.count <= context.max_includes + else + return if context.expandset.count < context.max_includes # rubocop:disable Style/IfInsideElse + end raise Mapper::TooManyIncludesError, "Maximum of #{context.max_includes} nested includes are allowed!" end diff --git a/spec/lib/gitlab/ci/config/external/context_spec.rb b/spec/lib/gitlab/ci/config/external/context_spec.rb index 40702e7540442bfdafe5c2754aebce12be1a6c4d..73167be40d7803c9b066f9005bd538013f963d99 100644 --- a/spec/lib/gitlab/ci/config/external/context_spec.rb +++ b/spec/lib/gitlab/ci/config/external/context_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Ci::Config::External::Context do +RSpec.describe Gitlab::Ci::Config::External::Context, feature_category: :pipeline_authoring do let(:project) { build(:project) } let(:user) { double('User') } let(:sha) { '12345' } @@ -14,7 +14,7 @@ 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.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 +25,36 @@ 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.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_fix_max_includes is disabled' do + before do + stub_feature_flags(ci_fix_max_includes: false) + end + + context 'with values' do + it { is_expected.to have_attributes(**attributes) } + it { expect(subject.expandset).to eq(Set.new) } + 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.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 4303f279cacb0c4486225e5cc0113af732a6cf2e..e39a1c6786b767450dd5bbe651ee0f448ca4919e 100644 --- a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb @@ -58,121 +58,184 @@ describe '#process' do subject(:process) { verifier.process(files) } - context 'when files are local' 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), - Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file3.yml' }, context) - ] - end + shared_examples 'when FF ci_fix_max_includes is enabled' do + context 'when files are local' 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), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file3.yml' }, context) + ] + 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' - ) - 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' + ) + end - it 'adds files to the expandset' do - expect { process }.to change { context.expandset.count }.by(3) - 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 - 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) + 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 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) + context 'when files are project files' do + let_it_be(:included_project) { create(:project, :repository, namespace: project.namespace, creator: user) } + + let(:files) do + [ + Gitlab::Ci::Config::External::File::Project.new( + { file: 'myfolder/file1.yml', project: included_project.full_path }, context + ), + Gitlab::Ci::Config::External::File::Project.new( + { file: 'myfolder/file2.yml', project: included_project.full_path }, context + ), + Gitlab::Ci::Config::External::File::Project.new( + { file: 'myfolder/file3.yml', project: included_project.full_path }, context + ) + ] 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) + around(:all) do |example| + create_and_delete_files(included_project, project_files) do + example.run + end end - end - end - context 'when files are project files' do - let_it_be(:included_project) { create(:project, :repository, namespace: project.namespace, creator: user) } - - let(:files) do - [ - Gitlab::Ci::Config::External::File::Project.new( - { file: 'myfolder/file1.yml', project: included_project.full_path }, context - ), - Gitlab::Ci::Config::External::File::Project.new( - { file: 'myfolder/file2.yml', project: included_project.full_path }, context - ), - Gitlab::Ci::Config::External::File::Project.new( - { file: 'myfolder/file3.yml', project: included_project.full_path }, context + 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 + 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' - ) - end + it 'calls Gitaly only once for all files', :request_store do + # 1 for project.commit.id, 3 for the sha check, 1 for the files + expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(5) + 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 'adds files to the expandset' do - expect { process }.to change { context.expandset.count }.by(3) + it 'calls Gitaly for each file', :request_store do + # 1 for project.commit.id, 3 for the sha check, 3 for the files + expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(7) + end + end end - it 'calls Gitaly only once for all files', :request_store do - # 1 for project.commit.id, 3 for the sha check, 1 for the files - expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(5) + 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 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) + 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 'calls Gitaly for each file', :request_store do - # 1 for project.commit.id, 3 for the sha check, 3 for the files - expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(7) + it 'adds an error to the file' do + expect(process.first.errors).to include("Local file `myfolder/invalid.yml` does not exist!") end 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) - ] + it_behaves_like 'when FF ci_fix_max_includes is enabled' + + context 'when FF ci_fix_max_includes is disabled' do + before do + stub_feature_flags(ci_fix_max_includes: false) 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' } } - ) + it_behaves_like 'when FF ci_fix_max_includes is enabled' + + context 'when max_includes is exceeded' 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 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 + describe 'max includes detection' do + shared_examples 'when total file count is equal to max_includes' do + before do + allow(context).to receive(:max_includes).and_return(expected_total_file_count) + end + + it 'adds the expected number of files to expandset' do + process - it 'adds an error to the file' do - expect(process.first.errors).to include("Local file `myfolder/invalid.yml` does not exist!") + expect(context.expandset.count).to eq(expected_total_file_count) + end + + it 'does not raise an error' do + expect { process }.not_to raise_error + end end - end - context 'when max_includes is exceeded' do context 'when files are nested' do let(:files) do [ @@ -180,13 +243,19 @@ ] end - before do - allow(context).to receive(:max_includes).and_return(1) - end + let(:expected_total_file_count) { 4 } - it 'raises Processor::IncludeError' do - expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) + context 'when total file count exceeds max_includes' do + before do + allow(context).to receive(:max_includes).and_return(expected_total_file_count - 1) + end + + it 'raises Processor::IncludeError' do + expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) + end end + + it_behaves_like 'when total file count is equal to max_includes' end context 'when files are not nested' do @@ -197,13 +266,19 @@ ] end - before do - allow(context).to receive(:max_includes).and_return(1) - end + let(:expected_total_file_count) { files.count } - it 'raises Mapper::TooManyIncludesError' do - expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + context 'when total file count exceeds max_includes' do + before do + allow(context).to receive(:max_includes).and_return(expected_total_file_count - 1) + end + + it 'raises Mapper::TooManyIncludesError' do + expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + end end + + it_behaves_like 'when total file count is equal to max_includes' 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 a053c3047de2f1405955a19f794bcf94e236447d..bc05ac380deab16fb7a397738f2f1ed53a332b1d 100644 --- a/spec/lib/gitlab/ci/config/external/mapper_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper_spec.rb @@ -236,9 +236,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_fix_max_includes is disabled' do + before do + stub_feature_flags(ci_fix_max_includes: false) + end + + it 'has expanset with one' do + process + expect(context.expandset.size).to eq(1) + end end end