diff options
21 files changed, 84 insertions, 222 deletions
@@ -420,6 +420,7 @@ group :test do gem 'fuubar', '~> 2.2.0' gem 'rspec-retry', '~> 0.6.1' gem 'rspec_profiling', '~> 0.0.6' + gem 'rspec-benchmark', '~> 0.6.0' gem 'rspec-parameterized', require: false gem 'capybara', '~> 3.35.3' diff --git a/Gemfile.lock b/Gemfile.lock index f0063f0dee7..69092758a69 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -139,8 +139,11 @@ GEM bcrypt (3.1.16) benchmark (0.1.1) benchmark-ips (2.3.0) + benchmark-malloc (0.2.0) benchmark-memory (0.1.2) memory_profiler (~> 0.9) + benchmark-perf (0.6.0) + benchmark-trend (0.4.0) better_errors (2.9.1) coderay (>= 1.0.0) erubi (>= 1.0.0) @@ -1062,6 +1065,11 @@ GEM rspec-core (~> 3.10.0) rspec-expectations (~> 3.10.0) rspec-mocks (~> 3.10.0) + rspec-benchmark (0.6.0) + benchmark-malloc (~> 0.2) + benchmark-perf (~> 0.6) + benchmark-trend (~> 0.4) + rspec (>= 3.0) rspec-core (3.10.1) rspec-support (~> 3.10.0) rspec-expectations (3.10.1) @@ -1600,6 +1608,7 @@ DEPENDENCIES rexml (~> 3.2.5) rouge (~> 3.27.0) rqrcode-rails3 (~> 0.1.7) + rspec-benchmark (~> 0.6.0) rspec-parameterized rspec-rails (~> 5.0.1) rspec-retry (~> 0.6.1) diff --git a/app/helpers/invite_members_helper.rb b/app/helpers/invite_members_helper.rb index 1f225e9c0e5..fa06bac292a 100644 --- a/app/helpers/invite_members_helper.rb +++ b/app/helpers/invite_members_helper.rb @@ -73,7 +73,7 @@ module InviteMembersHelper def show_invite_members_for_task?(source) return unless current_user - invite_for_help_continuous_onboarding = source.is_a?(Project) && experiment(:invite_for_help_continuous_onboarding, namespace: source.namespace).variant.name == 'candidate' + invite_for_help_continuous_onboarding = source.is_a?(Project) && experiment(:invite_for_help_continuous_onboarding, namespace: source.namespace).assigned.name == 'candidate' params[:open_modal] == 'invite_members_for_task' || invite_for_help_continuous_onboarding end diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb index 84518fd6b0e..bb6a52eb2f4 100644 --- a/app/services/auth/container_registry_authentication_service.rb +++ b/app/services/auth/container_registry_authentication_service.rb @@ -59,12 +59,7 @@ module Auth token.expire_time = token_expire_at token[:access] = names.map do |name| - { - type: type, - name: name, - actions: actions, - migration_eligible: type == 'repository' ? migration_eligible(repository_path: name) : nil - }.compact + { type: type, name: name, actions: actions } end token.encoded @@ -136,12 +131,7 @@ module Auth # ensure_container_repository!(path, authorized_actions) - { - type: type, - name: path.to_s, - actions: authorized_actions, - migration_eligible: self.class.migration_eligible(project: requested_project) - }.compact + { type: type, name: path.to_s, actions: authorized_actions } end def actively_importing?(actions, path) @@ -153,28 +143,6 @@ module Auth container_repository.migration_importing? end - def self.migration_eligible(project: nil, repository_path: nil) - return unless Feature.enabled?(:container_registry_migration_phase1) - - # project has precedence over repository_path. If only the latter is provided, we find the corresponding Project. - unless project - return unless repository_path - - project = ContainerRegistry::Path.new(repository_path).repository_project - end - - # The migration process will start by allowing only specific test and gitlab-org projects using the - # `container_registry_migration_phase1_allow` FF. We'll then move on to a percentage rollout using this same FF. - # To remove the risk of impacting enterprise customers that rely heavily on the registry during the percentage - # rollout, we'll add their top-level group/namespace to the `container_registry_migration_phase1_deny` FF. Later, - # we'll remove them manually from this deny list, and their new repositories will become eligible. - Feature.disabled?(:container_registry_migration_phase1_deny, project.root_ancestor) && - Feature.enabled?(:container_registry_migration_phase1_allow, project) - rescue ContainerRegistry::Path::InvalidRegistryPathError => ex - Gitlab::ErrorTracking.track_and_raise_for_dev_exception(ex, **Gitlab::ApplicationContext.current) - false - end - ## # Because we do not have two way communication with registry yet, # we create a container repository image resource when push to the diff --git a/app/services/loose_foreign_keys/batch_cleaner_service.rb b/app/services/loose_foreign_keys/batch_cleaner_service.rb index f3db2037911..b89de15a568 100644 --- a/app/services/loose_foreign_keys/batch_cleaner_service.rb +++ b/app/services/loose_foreign_keys/batch_cleaner_service.rb @@ -54,7 +54,7 @@ module LooseForeignKeys attr_reader :parent_table, :loose_foreign_key_definitions, :deleted_parent_records, :modification_tracker, :deleted_records_counter, :deleted_records_rescheduled_count, :deleted_records_incremented_count def handle_over_limit - return if Feature.disabled?(:lfk_fair_queueing) + return if Feature.disabled?(:lfk_fair_queueing, default_enabled: :yaml) records_to_reschedule = [] records_to_increment = [] diff --git a/app/views/projects/pipelines/show.html.haml b/app/views/projects/pipelines/show.html.haml index 70815dbe7a7..ba498352278 100644 --- a/app/views/projects/pipelines/show.html.haml +++ b/app/views/projects/pipelines/show.html.haml @@ -20,7 +20,7 @@ .bs-callout.bs-callout-danger %h4= _('Found errors in your %{gitlab_ci_yml}:') % { gitlab_ci_yml: '.gitlab-ci.yml' } %ul - - @pipeline.yaml_errors.split(",").each do |error| + - @pipeline.yaml_errors.split("\n").each do |error| %li= error - lint_link_url = project_ci_pipeline_editor_path(@project, tab: "LINT_TAB") - lint_link_start = '<a href="%{url}" class="gl-text-blue-500!">'.html_safe % { url: lint_link_url } diff --git a/app/views/sandbox/mermaid.html.erb b/app/views/sandbox/mermaid.html.erb index 2d2391c8866..48c7baeaeed 100644 --- a/app/views/sandbox/mermaid.html.erb +++ b/app/views/sandbox/mermaid.html.erb @@ -2,6 +2,9 @@ <html> <head> <%= webpack_bundle_tag("sandboxed_mermaid") %> + <% if params[:darkMode] == 'true' %> + <meta name="color-scheme" content="dark light"> + <% end %> </head> <body> <div id="app"></div> diff --git a/config/feature_flags/development/container_registry_migration_phase1.yml b/config/feature_flags/development/container_registry_migration_phase1.yml deleted file mode 100644 index 85fbdcfab01..00000000000 --- a/config/feature_flags/development/container_registry_migration_phase1.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: container_registry_migration_phase1 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63907 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/335703 -milestone: '14.1' -type: development -group: group::package -default_enabled: false diff --git a/config/feature_flags/development/container_registry_migration_phase1_allow.yml b/config/feature_flags/development/container_registry_migration_phase1_allow.yml deleted file mode 100644 index 1e8d260c93b..00000000000 --- a/config/feature_flags/development/container_registry_migration_phase1_allow.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: container_registry_migration_phase1_allow -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63907 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/335705 -milestone: '14.1' -type: development -group: group::package -default_enabled: false diff --git a/config/feature_flags/development/container_registry_migration_phase1_deny.yml b/config/feature_flags/development/container_registry_migration_phase1_deny.yml deleted file mode 100644 index 1aa66059045..00000000000 --- a/config/feature_flags/development/container_registry_migration_phase1_deny.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: container_registry_migration_phase1_deny -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63907 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/335706 -milestone: '14.1' -type: development -group: group::package -default_enabled: false diff --git a/config/feature_flags/development/lfk_fair_queueing.yml b/config/feature_flags/development/lfk_fair_queueing.yml index ac67ffa14f0..6dc6cb72e56 100644 --- a/config/feature_flags/development/lfk_fair_queueing.yml +++ b/config/feature_flags/development/lfk_fair_queueing.yml @@ -5,4 +5,4 @@ rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/351082 milestone: '14.8' type: development group: group::sharding -default_enabled: false +default_enabled: true diff --git a/lib/gitlab/ci/config/entry/rules/rule.rb b/lib/gitlab/ci/config/entry/rules/rule.rb index 840f2d6f31a..4722f2e9a61 100644 --- a/lib/gitlab/ci/config/entry/rules/rule.rb +++ b/lib/gitlab/ci/config/entry/rules/rule.rb @@ -24,7 +24,7 @@ module Gitlab validates :config, allowed_keys: ALLOWED_KEYS validates :config, disallowed_keys: %i[start_in], unless: :specifies_delay? validates :start_in, presence: true, if: :specifies_delay? - validates :start_in, duration: { limit: '1 day' }, if: :specifies_delay? + validates :start_in, duration: { limit: '1 week' }, if: :specifies_delay? with_options allow_nil: true do validates :if, expression: true diff --git a/lib/gitlab/utils/strong_memoize.rb b/lib/gitlab/utils/strong_memoize.rb index 255fa0169bf..3c954f817a7 100644 --- a/lib/gitlab/utils/strong_memoize.rb +++ b/lib/gitlab/utils/strong_memoize.rb @@ -22,10 +22,12 @@ module Gitlab # end # def strong_memoize(name) - if strong_memoized?(name) - instance_variable_get(ivar(name)) + key = ivar(name) + + if instance_variable_defined?(key) + instance_variable_get(key) else - instance_variable_set(ivar(name), yield) + instance_variable_set(key, yield) end end @@ -34,13 +36,23 @@ module Gitlab end def clear_memoization(name) - remove_instance_variable(ivar(name)) if instance_variable_defined?(ivar(name)) + key = ivar(name) + remove_instance_variable(key) if instance_variable_defined?(key) end private + # Convert `"name"`/`:name` into `:@name` + # + # Depending on a type ensure that there's a single memory allocation def ivar(name) - "@#{name}" + if name.is_a?(Symbol) + name.to_s.prepend("@").to_sym + elsif name.is_a?(String) + :"@#{name}" + else + raise ArgumentError, "Invalid type of '#{name}'" + end end end end diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb index 6d3d86d0663..f27e2892807 100644 --- a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb @@ -37,7 +37,7 @@ module QA target_project.visit! Page::Project::Show.perform do |project| - expect(project).to have_content('README.md') + expect { project.has_file?('README.md') }.to eventually_be_truthy.within(max_duration: 60), "Expected a file named README.md but it did not appear." expect(project).to have_content('This is a test project') end end diff --git a/spec/experiments/application_experiment_spec.rb b/spec/experiments/application_experiment_spec.rb index f0a993bd03b..b99c28627f2 100644 --- a/spec/experiments/application_experiment_spec.rb +++ b/spec/experiments/application_experiment_spec.rb @@ -305,7 +305,7 @@ RSpec.describe ApplicationExperiment, :experiment do end it "caches the variant determined by the variant resolver" do - expect(application_experiment.variant.name).to eq('candidate') # we should be in the experiment + expect(application_experiment.assigned.name).to eq('candidate') # we should be in the experiment application_experiment.run @@ -320,7 +320,7 @@ RSpec.describe ApplicationExperiment, :experiment do # the control. stub_feature_flags(namespaced_stub: false) # simulate being not rolled out - expect(application_experiment.variant.name).to eq('control') # if we ask, it should be control + expect(application_experiment.assigned.name).to eq('control') # if we ask, it should be control application_experiment.run diff --git a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb index d1bd22e5573..9fe0be29fa8 100644 --- a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb @@ -174,13 +174,13 @@ RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule do end context 'specifying a delayed job' do - let(:config) { { if: '$THIS || $THAT', when: 'delayed', start_in: '15 minutes' } } + let(:config) { { if: '$THIS || $THAT', when: 'delayed', start_in: '2 days' } } it { is_expected.to be_valid } it 'sets attributes for the job delay' do expect(entry.when).to eq('delayed') - expect(entry.start_in).to eq('15 minutes') + expect(entry.start_in).to eq('2 days') end context 'without a when: key' do @@ -198,10 +198,20 @@ RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule do it { is_expected.not_to be_valid } - it 'returns an error about tstart_in being blank' do + it 'returns an error about start_in being blank' do expect(entry.errors).to include(/start in can't be blank/) end end + + context 'when start_in value is longer than a week' do + let(:config) { { if: '$THIS || $THAT', when: 'delayed', start_in: '2 weeks' } } + + it { is_expected.not_to be_valid } + + it 'returns an error about start_in exceeding the limit' do + expect(entry.errors).to include(/start in should not exceed the limit/) + end + end end context 'when specifying unknown policy' do diff --git a/spec/lib/gitlab/utils/strong_memoize_spec.rb b/spec/lib/gitlab/utils/strong_memoize_spec.rb index d9fa2e516e1..5350e090e2b 100644 --- a/spec/lib/gitlab/utils/strong_memoize_spec.rb +++ b/spec/lib/gitlab/utils/strong_memoize_spec.rb @@ -48,6 +48,36 @@ RSpec.describe Gitlab::Utils::StrongMemoize do let(:value) { value } it_behaves_like 'caching the value' + + it 'raises exception for invalid key' do + expect { object.strong_memoize(10) { 20 } }.to raise_error /Invalid type of '10'/ + end + end + end + + context "memory allocation", type: :benchmark do + let(:value) { 'aaa' } + + before do + object.method_name # warmup + end + + [:method_name, "method_name"].each do |argument| + context "for #{argument.class}" do + it 'does allocate exactly one string when fetching value' do + expect do + object.strong_memoize(argument) { 10 } + end.to perform_allocation(1) + end + + it 'does allocate exactly one string when storing value' do + object.clear_memoization(:method_name) # clear to force set + + expect do + object.strong_memoize(argument) { 10 } + end.to perform_allocation(1) + end + end end end end diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 00841de9ff4..ba7acd3d3df 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -6,143 +6,4 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do include AdminModeHelper it_behaves_like 'a container registry auth service' - - context 'when in migration mode' do - include_context 'container registry auth service context' - - let_it_be(:current_user) { create(:user) } - let_it_be(:project) { create(:project) } - - before do - project.add_developer(current_user) - end - - shared_examples 'a modified token with migration eligibility' do |eligible| - it_behaves_like 'a valid token' - it { expect(payload['access']).to include(include('migration_eligible' => eligible)) } - end - - shared_examples 'a modified token' do - context 'with a non eligible root ancestor and project' do - before do - stub_feature_flags(container_registry_migration_phase1_deny: project.root_ancestor) - stub_feature_flags(container_registry_migration_phase1_allow: false) - end - - it_behaves_like 'a modified token with migration eligibility', false - end - - context 'with a non eligible root ancestor and eligible project' do - before do - stub_feature_flags(container_registry_migration_phase1_deny: false) - stub_feature_flags(container_registry_migration_phase1_deny: project.root_ancestor) - stub_feature_flags(container_registry_migration_phase1_allow: project) - end - - it_behaves_like 'a modified token with migration eligibility', false - end - - context 'with an eligible root ancestor and non eligible project' do - before do - stub_feature_flags(container_registry_migration_phase1_deny: false) - stub_feature_flags(container_registry_migration_phase1_allow: false) - end - - it_behaves_like 'a modified token with migration eligibility', false - end - - context 'with an eligible root ancestor and project' do - before do - stub_feature_flags(container_registry_migration_phase1_deny: false) - stub_feature_flags(container_registry_migration_phase1_allow: project) - end - - it_behaves_like 'a modified token with migration eligibility', true - end - end - - context 'with pull action' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull"] } - end - - it_behaves_like 'a modified token' - end - - context 'with push action' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - it_behaves_like 'a modified token' - end - - context 'with multiple actions' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull,push,delete"] } - end - - it_behaves_like 'a modified token' - end - - describe '#access_token' do - let(:token) { described_class.access_token(%w[push], [project.full_path]) } - - subject { { token: token } } - - it_behaves_like 'a modified token' - end - - context 'with a project with a path with trailing underscore' do - let(:bad_project) { create(:project) } - - before do - bad_project.update!(path: bad_project.path + '_') - bad_project.add_developer(current_user) - end - - describe '#full_access_token' do - let(:token) { described_class.full_access_token(bad_project.full_path) } - let(:access) do - [{ 'type' => 'repository', - 'name' => bad_project.full_path, - 'actions' => ['*'], - 'migration_eligible' => false }] - end - - subject { { token: token } } - - it 'logs an exception and returns a valid access token' do - expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) - - expect(token).to be_present - expect(payload).to be_a(Hash) - expect(payload).to include('access' => access) - end - end - end - end - - context 'when not in migration mode' do - include_context 'container registry auth service context' - - let_it_be(:project) { create(:project) } - - before do - stub_feature_flags(container_registry_migration_phase1: false) - end - - shared_examples 'an unmodified token' do - it_behaves_like 'a valid token' - it { expect(payload['access']).not_to include(have_key('migration_eligible')) } - end - - describe '#access_token' do - let(:token) { described_class.access_token(%w[push], [project.full_path]) } - - subject { { token: token } } - - it_behaves_like 'an unmodified token' - end - end end diff --git a/spec/simplecov_env.rb b/spec/simplecov_env.rb index be2ff396bc4..da4a0e8da80 100644 --- a/spec/simplecov_env.rb +++ b/spec/simplecov_env.rb @@ -49,7 +49,6 @@ module SimpleCovEnv def configure_profile SimpleCov.configure do - enable_coverage :branch load_profile 'test_frameworks' track_files '{app,config/initializers,config/initializers_before_autoloader,db/post_migrate,haml_lint,lib,rubocop,tooling}/**/*.rb' diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 55f28d7cf98..724b8be36ad 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -199,6 +199,7 @@ RSpec.configure do |config| config.include SidekiqMiddleware config.include StubActionCableConnection, type: :channel config.include StubSpamServices + config.include RSpec::Benchmark::Matchers, type: :benchmark include StubFeatureFlags diff --git a/spec/support/shared_examples/services/container_registry_auth_service_shared_examples.rb b/spec/support/shared_examples/services/container_registry_auth_service_shared_examples.rb index c808b9a5318..a780952d51b 100644 --- a/spec/support/shared_examples/services/container_registry_auth_service_shared_examples.rb +++ b/spec/support/shared_examples/services/container_registry_auth_service_shared_examples.rb @@ -69,10 +69,6 @@ RSpec.shared_examples 'a browsable' do end RSpec.shared_examples 'an accessible' do - before do - stub_feature_flags(container_registry_migration_phase1: false) - end - let(:access) do [{ 'type' => 'repository', 'name' => project.full_path, @@ -161,10 +157,6 @@ end RSpec.shared_examples 'a container registry auth service' do include_context 'container registry auth service context' - before do - stub_feature_flags(container_registry_migration_phase1: false) - end - describe '.full_access_token' do let_it_be(:project) { create(:project) } |