diff options
22 files changed, 288 insertions, 212 deletions
@@ -69,7 +69,6 @@ gem 'omniauth-gitlab', '~> 4.0.0', path: 'vendor/gems/omniauth-gitlab' # See ven gem 'omniauth-google-oauth2', '~> 1.1' gem 'omniauth-oauth2-generic', '~> 0.2.2' gem 'omniauth-saml', '~> 2.0.0' -gem 'omniauth-shibboleth', '~> 1.3.0' gem 'omniauth-twitter', '~> 1.4' gem 'omniauth_crowd', '~> 2.4.0', path: 'vendor/gems/omniauth_crowd' # See vendor/gems/omniauth_crowd/README.md gem 'omniauth-authentiq', '~> 0.3.3' diff --git a/Gemfile.checksum b/Gemfile.checksum index 77c870cf18d..58cca932e04 100644 --- a/Gemfile.checksum +++ b/Gemfile.checksum @@ -398,7 +398,6 @@ {"name":"omniauth-oauth2","version":"1.8.0","platform":"ruby","checksum":"b2f8e9559cc7e2d4efba57607691d6d2b634b879fc5b5b6ccfefa3da85089e78"}, {"name":"omniauth-oauth2-generic","version":"0.2.8","platform":"ruby","checksum":"ce6e8539019d5ebf2f48867072b9f248f148bb4cbe7166dee655865abfae7613"}, {"name":"omniauth-saml","version":"2.0.0","platform":"ruby","checksum":"02594fd6630de26a9e65a2e64223e9ad32324fa97a6c7f1f22a1553ea3dd44c7"}, -{"name":"omniauth-shibboleth","version":"1.3.0","platform":"ruby","checksum":"b0bb725ced5cb76fbfc187ddbb8ad6864d0cd5df714cab36a528df8ee4b1d113"}, {"name":"omniauth-twitter","version":"1.4.0","platform":"ruby","checksum":"c5cc6c77cd767745ffa9ebbd5fbd694a3fa99d1d2d82a4d7def0bf3b6131b264"}, {"name":"omniauth_openid_connect","version":"0.6.0","platform":"ruby","checksum":"b8e48ca67fdea2dff56cc161855b88707a290ae01125149dbe0f8c94e818cfd3"}, {"name":"open4","version":"1.3.4","platform":"ruby","checksum":"a1df037310624ecc1ea1d81264b11c83e96d0c3c1c6043108d37d396dcd0f4b1"}, diff --git a/Gemfile.lock b/Gemfile.lock index 9a6ce249d97..79fbffa7c74 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1016,8 +1016,6 @@ GEM omniauth-saml (2.0.0) omniauth (~> 2.0) ruby-saml (~> 1.9) - omniauth-shibboleth (1.3.0) - omniauth (>= 1.0.0) omniauth-twitter (1.4.0) omniauth-oauth (~> 1.1) rack @@ -1765,7 +1763,6 @@ DEPENDENCIES omniauth-oauth2-generic (~> 0.2.2) omniauth-salesforce (~> 1.0.5)! omniauth-saml (~> 2.0.0) - omniauth-shibboleth (~> 1.3.0) omniauth-twitter (~> 1.4) omniauth_crowd (~> 2.4.0)! omniauth_openid_connect (~> 0.6.0) diff --git a/app/assets/images/auth_buttons/shibboleth_64.png b/app/assets/images/auth_buttons/shibboleth_64.png Binary files differdeleted file mode 100644 index d4c752f9400..00000000000 --- a/app/assets/images/auth_buttons/shibboleth_64.png +++ /dev/null diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index cba0056ccd5..24767804436 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -97,7 +97,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap def target_projects projects = MergeRequestTargetProjectFinder .new(current_user: current_user, source_project: @project, project_feature: :repository) - .execute(include_routes: true).limit(20).search(params[:search]) + .execute(include_routes: false, search: params[:search]).limit(20) render json: ProjectSerializer.new.represent(projects) end diff --git a/app/finders/merge_request_target_project_finder.rb b/app/finders/merge_request_target_project_finder.rb index fdb3bac8935..ea1aa6d2e9e 100644 --- a/app/finders/merge_request_target_project_finder.rb +++ b/app/finders/merge_request_target_project_finder.rb @@ -11,9 +11,10 @@ class MergeRequestTargetProjectFinder @project_feature = project_feature end - def execute(include_routes: false) + def execute(search: nil, include_routes: false) if source_project.fork_network - include_routes ? projects.inc_routes : projects + items = include_routes ? projects.inc_routes : projects + by_search(items, search) else Project.id_in(source_project.id) end @@ -31,4 +32,10 @@ class MergeRequestTargetProjectFinder .non_archived .with_feature_available_for_user(project_feature, current_user) end + + # rubocop: disable CodeReuse/ActiveRecord + def by_search(items, search) + items.joins(:route).fuzzy_search(search, [Route.arel_table[:path], Route.arel_table[:name], :description]) + end + # rubocop: enable CodeReuse/ActiveRecord end diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb index c41b5923d13..f818088a4c6 100644 --- a/app/helpers/auth_helper.rb +++ b/app/helpers/auth_helper.rb @@ -17,7 +17,6 @@ module AuthHelper jwt openid_connect salesforce - shibboleth twitter ).freeze LDAP_PROVIDER = /\Aldap/.freeze diff --git a/db/post_migrate/20230124104310_prepare_web_hook_logs_id_created_at_async_index.rb b/db/post_migrate/20230124104310_prepare_web_hook_logs_id_created_at_async_index.rb new file mode 100644 index 00000000000..ea087265c90 --- /dev/null +++ b/db/post_migrate/20230124104310_prepare_web_hook_logs_id_created_at_async_index.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class PrepareWebHookLogsIdCreatedAtAsyncIndex < Gitlab::Database::Migration[2.1] + include Gitlab::Database::PartitioningMigrationHelpers + + disable_ddl_transaction! + + INDEX_NAME = 'index_web_hook_logs_on_web_hook_id_and_created_at' + + def up + # Since web_hook_logs is a partitioned table, we need to prepare the index + # for each partition individually. We can't use the `prepare_async_index` + # method directly because it will try to prepare the index for the whole + # table, which will fail. + + # In a future migration after this one, we will create the index on the + # parent table itself. + each_partition(:web_hook_logs) do |partition, partition_index_name| + prepare_async_index(partition.identifier, [:web_hook_id, :created_at], + name: partition_index_name) + end + end + + def down + each_partition(:web_hook_logs) do |partition, partition_index_name| + unprepare_async_index_by_name(partition.identifier, partition_index_name) + end + end + + private + + def each_partition(table_name) + partitioned_table = find_partitioned_table(table_name) + partitioned_table.postgres_partitions.order(:name).each do |partition| + partition_index_name = generated_index_name(partition.identifier, INDEX_NAME) + + yield partition, partition_index_name + end + end +end diff --git a/db/schema_migrations/20230124104310 b/db/schema_migrations/20230124104310 new file mode 100644 index 00000000000..92a6e54eff3 --- /dev/null +++ b/db/schema_migrations/20230124104310 @@ -0,0 +1 @@ +00c5c88297137232a7a180452f7ce03dcd56733a0b2e0acc2abfd5a63d36e39e
\ No newline at end of file diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index b9173c96928..d1ab1bbb8cc 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -124,7 +124,9 @@ module Gitlab end def self.has_config?(database_name) - Gitlab::Application.config.database_configuration[Rails.env].include?(database_name.to_s) + ActiveRecord::Base.configurations + .configs_for(env_name: Rails.env, name: database_name.to_s, include_replicas: true) + .present? end class PgUser < ApplicationRecord diff --git a/lib/gitlab/redis/multi_store.rb b/lib/gitlab/redis/multi_store.rb index aa8f390ac10..c0fab462786 100644 --- a/lib/gitlab/redis/multi_store.rb +++ b/lib/gitlab/redis/multi_store.rb @@ -46,13 +46,6 @@ module Gitlab # # Ref: https://www.rubydoc.info/github/redis/redis-rb/Redis/Commands # - ENUMERATOR_CACHE_HIT_VALIDATOR = { - scan_each: ->(val) { val.is_a?(Enumerator) && !val.first.nil? }, - hscan_each: ->(val) { val.is_a?(Enumerator) && !val.first.nil? }, - sscan_each: ->(val) { val.is_a?(Enumerator) && !val.first.nil? }, - zscan_each: ->(val) { val.is_a?(Enumerator) && !val.first.nil? } - }.freeze - READ_CACHE_HIT_VALIDATOR = { exists: ->(val) { val != 0 }, exists?: ->(val) { val }, @@ -62,13 +55,17 @@ module Gitlab hgetall: ->(val) { val.is_a?(Hash) && !val.empty? }, hlen: ->(val) { val != 0 }, hmget: ->(val) { val.is_a?(Array) && !val.compact.empty? }, + hscan_each: ->(val) { val.is_a?(Enumerator) && !val.first.nil? }, mapped_hmget: ->(val) { val.is_a?(Hash) && !val.compact.empty? }, mget: ->(val) { val.is_a?(Array) && !val.compact.empty? }, + scan_each: ->(val) { val.is_a?(Enumerator) && !val.first.nil? }, scard: ->(val) { val != 0 }, sismember: ->(val) { val }, smembers: ->(val) { val.is_a?(Array) && !val.empty? }, sscan: ->(val) { val != ['0', []] }, - ttl: ->(val) { val != 0 && val != -2 } + sscan_each: ->(val) { val.is_a?(Enumerator) && !val.first.nil? }, + ttl: ->(val) { val != 0 && val != -2 }, # ttl returns -2 if the key does not exist. See https://redis.io/commands/ttl/ + zscan_each: ->(val) { val.is_a?(Enumerator) && !val.first.nil? } }.freeze WRITE_COMMANDS = %i[ @@ -134,20 +131,6 @@ module Gitlab end end - ENUMERATOR_CACHE_HIT_VALIDATOR.each_key do |name| - define_method(name) do |*args, **kwargs, &block| - enumerator = if use_primary_and_secondary_stores? - read_command(name, *args, **kwargs) - else - default_store.send(name, *args, **kwargs) - end - - return enumerator if block.nil? - - enumerator.each(&block) - end - end - PIPELINED_COMMANDS.each do |name| define_method(name) do |*args, **kwargs, &block| if use_primary_and_secondary_stores? @@ -281,13 +264,13 @@ module Gitlab multi_store_error_message: FAILED_TO_READ_ERROR_MESSAGE) end - return value if cache_hit?(command_name, value) + return value if block.nil? && cache_hit?(command_name, value) fallback_read(command_name, *args, **kwargs, &block) end def cache_hit?(command, value) - validator = READ_CACHE_HIT_VALIDATOR[command] || ENUMERATOR_CACHE_HIT_VALIDATOR[command] + validator = READ_CACHE_HIT_VALIDATOR[command] return false unless validator !value.nil? && validator.call(value) diff --git a/spec/finders/merge_request_target_project_finder_spec.rb b/spec/finders/merge_request_target_project_finder_spec.rb index bf735152d99..4b6c729dab7 100644 --- a/spec/finders/merge_request_target_project_finder_spec.rb +++ b/spec/finders/merge_request_target_project_finder_spec.rb @@ -85,4 +85,28 @@ RSpec.describe MergeRequestTargetProjectFinder do expect(finder.execute).to contain_exactly(other_fork, base_project) end end + + context 'searching' do + let(:base_project) { create(:project, :private, path: 'base') } + let(:forked_project) { fork_project(base_project, base_project.first_owner) } + let(:other_fork) { fork_project(base_project) } + + before do + base_project.add_developer(user) + forked_project.add_developer(user) + other_fork.add_developer(user) + end + + it 'returns all projects with empty search' do + expect(finder.execute(search: '')).to match_array([base_project, forked_project, other_fork]) + end + + it 'returns forked project with search string' do + expect(finder.execute(search: forked_project.full_path)).to match_array([forked_project]) + end + + it 'returns no projects with search for project that does no exist' do + expect(finder.execute(search: 'root')).to match_array([]) + end + end end diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index 6423c93d1dc..4a67a9da0d7 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -33,20 +33,31 @@ RSpec.describe Gitlab::Database do describe '.has_config?' do context 'three tier database config' do - before do - allow(Gitlab::Application).to receive_message_chain(:config, :database_configuration, :[]).with(Rails.env) - .and_return({ - "primary" => { "adapter" => "postgresql", "database" => "gitlabhq_test" }, - "ci" => { "adapter" => "postgresql", "database" => "gitlabhq_test_ci" } - }) + it 'returns true for main' do + expect(described_class.has_config?(:main)).to eq(true) end - it 'returns true for primary' do - expect(described_class.has_config?(:primary)).to eq(true) - end + context 'ci' do + before do + # CI config might not be configured + allow(ActiveRecord::Base.configurations).to receive(:configs_for) + .with(env_name: 'test', name: 'ci', include_replicas: true) + .and_return(ci_db_config) + end + + let(:ci_db_config) { instance_double('ActiveRecord::DatabaseConfigurations::HashConfig') } - it 'returns true for ci' do - expect(described_class.has_config?(:ci)).to eq(true) + it 'returns true for ci' do + expect(described_class.has_config?(:ci)).to eq(true) + end + + context 'ci database.yml not configured' do + let(:ci_db_config) { nil } + + it 'returns false for ci' do + expect(described_class.has_config?(:ci)).to eq(false) + end + end end it 'returns false for non-existent' do diff --git a/spec/lib/gitlab/omniauth_initializer_spec.rb b/spec/lib/gitlab/omniauth_initializer_spec.rb index 563c97fa2cb..cb1f3473fbd 100644 --- a/spec/lib/gitlab/omniauth_initializer_spec.rb +++ b/spec/lib/gitlab/omniauth_initializer_spec.rb @@ -216,14 +216,6 @@ RSpec.describe Gitlab::OmniauthInitializer do expect { subject.execute([hash_config]) }.to raise_error(NameError) end - it 'configures fail_with_empty_uid for shibboleth' do - shibboleth_config = { 'name' => 'shibboleth', 'args' => {} } - - expect(devise_config).to receive(:omniauth).with(:shibboleth, fail_with_empty_uid: true) - - subject.execute([shibboleth_config]) - end - it 'configures remote_sign_out_handler proc for authentiq' do authentiq_config = { 'name' => 'authentiq', 'args' => {} } diff --git a/spec/lib/gitlab/redis/multi_store_spec.rb b/spec/lib/gitlab/redis/multi_store_spec.rb index f198ba90d0a..74b38cfa464 100644 --- a/spec/lib/gitlab/redis/multi_store_spec.rb +++ b/spec/lib/gitlab/redis/multi_store_spec.rb @@ -283,10 +283,12 @@ RSpec.describe Gitlab::Redis::MultiStore, feature_category: :redis do subject end - it 'does not execute on the secondary store' do - expect(secondary_store).not_to receive(name) + unless params[:block] + it 'does not execute on the secondary store' do + expect(secondary_store).not_to receive(name) - subject + subject + end end include_examples 'reads correct value' @@ -344,8 +346,16 @@ RSpec.describe Gitlab::Redis::MultiStore, feature_category: :redis do end context 'when block is provided' do - it 'yields to the block' do + it 'both stores yields to the block' do expect(primary_store).to receive(name).and_yield(value) + expect(secondary_store).to receive(name).and_yield(value) + + subject + end + + it 'both stores to execute' do + expect(primary_store).to receive(name).with(*expected_args).and_call_original + expect(secondary_store).to receive(name).with(*expected_args).and_call_original subject end @@ -412,7 +422,6 @@ RSpec.describe Gitlab::Redis::MultiStore, feature_category: :redis do multi_store.mget(values) do |v| multi_store.sadd(skey, v) multi_store.scard(skey) - v # mget receiving block returns the last line of the block for cache-hit check end end @@ -422,9 +431,9 @@ RSpec.describe Gitlab::Redis::MultiStore, feature_category: :redis do expect(primary_store).to receive(:send).with(:sadd, skey, %w[1 2 3]).and_call_original expect(primary_store).to receive(:send).with(:scard, skey).and_call_original - expect(secondary_store).not_to receive(:send).with(:mget, values).and_call_original - expect(secondary_store).not_to receive(:send).with(:sadd, skey, %w[1 2 3]).and_call_original - expect(secondary_store).not_to receive(:send).with(:scard, skey).and_call_original + expect(secondary_store).to receive(:send).with(:mget, values).and_call_original + expect(secondary_store).to receive(:send).with(:sadd, skey, %w[10 20 30]).and_call_original + expect(secondary_store).to receive(:send).with(:scard, skey).and_call_original subject end @@ -451,7 +460,15 @@ RSpec.describe Gitlab::Redis::MultiStore, feature_category: :redis do end context 'when primary instance is default store' do - it_behaves_like 'primary instance executes block' + it 'ensures only primary instance is executing the block' do + expect(secondary_store).not_to receive(:send) + + expect(primary_store).to receive(:send).with(:mget, values).and_call_original + expect(primary_store).to receive(:send).with(:sadd, skey, %w[1 2 3]).and_call_original + expect(primary_store).to receive(:send).with(:scard, skey).and_call_original + + subject + end end context 'when secondary instance is default store' do @@ -464,9 +481,7 @@ RSpec.describe Gitlab::Redis::MultiStore, feature_category: :redis do expect(secondary_store).to receive(:send).with(:sadd, skey, %w[10 20 30]).and_call_original expect(secondary_store).to receive(:send).with(:scard, skey).and_call_original - expect(primary_store).not_to receive(:send).with(:mget, values).and_call_original - expect(primary_store).not_to receive(:send).with(:sadd, skey, %w[10 20 30]).and_call_original - expect(primary_store).not_to receive(:send).with(:scard, skey).and_call_original + expect(primary_store).not_to receive(:send) subject end @@ -700,7 +715,7 @@ RSpec.describe Gitlab::Redis::MultiStore, feature_category: :redis do it 'runs block on correct Redis instance' do if both_stores expect(primary_store).to receive(name).with(*expected_args).and_call_original - expect(secondary_store).not_to receive(name) + expect(secondary_store).to receive(name).with(*expected_args).and_call_original expect(primary_store).to receive(:incr).with(rvalue) expect(secondary_store).to receive(:incr).with(rvalue) diff --git a/spec/tooling/danger/config_files_spec.rb b/spec/tooling/danger/config_files_spec.rb index 88b327df63f..65edcabb817 100644 --- a/spec/tooling/danger/config_files_spec.rb +++ b/spec/tooling/danger/config_files_spec.rb @@ -13,7 +13,6 @@ RSpec.describe Tooling::Danger::ConfigFiles do let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) } let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) } - let(:matching_line) { "+ introduced_by_url:" } subject(:config_file) { fake_danger.new(helper: fake_helper) } @@ -22,29 +21,42 @@ RSpec.describe Tooling::Danger::ConfigFiles do end describe '#add_suggestion_for_missing_introduced_by_url' do - let(:file_lines) do + let(:file_diff) do [ - "---", - "name: about_some_new_flow", - "introduced_by_url: #{url}", - "rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/355909", - "milestone: '14.10'" + "+---", + "+name: about_some_new_flow", + "+introduced_by_url: #{url}", + "+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/355909", + "+milestone: '14.10'" ] end + let(:file_lines) do + file_diff.map { |line| line.delete_prefix('+') } + end + let(:filename) { 'config/feature_flags/new_ff.yml' } + let(:mr_url) { 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/1' } before do allow(config_file.project_helper).to receive(:file_lines).and_return(file_lines) allow(config_file.helper).to receive(:added_files).and_return([filename]) - allow(config_file.helper).to receive(:mr_web_url).and_return(url) + allow(config_file.helper).to receive(:changed_lines).with(filename).and_return(file_diff) + allow(config_file.helper).to receive(:mr_web_url).and_return(mr_url) end context 'when config file has an empty introduced_by_url line' do let(:url) { '' } it 'adds suggestions at the correct line' do - expected_format = format(described_class::SUGGEST_INTRODUCED_BY_COMMENT, url: url) + template = <<~SUGGEST_COMMENT + ```suggestion + introduced_by_url: %<mr_url>s + ``` + SUGGEST_COMMENT + + expected_format = format(template, mr_url: mr_url) + expect(config_file).to receive(:markdown).with(expected_format, file: filename, line: 3) config_file.add_suggestion_for_missing_introduced_by_url diff --git a/spec/tooling/danger/feature_flag_spec.rb b/spec/tooling/danger/feature_flag_spec.rb index 0e9eda54510..4575d8ca981 100644 --- a/spec/tooling/danger/feature_flag_spec.rb +++ b/spec/tooling/danger/feature_flag_spec.rb @@ -86,14 +86,20 @@ RSpec.describe Tooling::Danger::FeatureFlag do describe described_class::Found do let(:feature_flag_path) { 'config/feature_flags/development/entry.yml' } let(:group) { 'group::source code' } - let(:raw_yaml) do - YAML.dump( + let(:yaml) do + { 'group' => group, 'default_enabled' => true, - 'rollout_issue_url' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/1' - ) + 'rollout_issue_url' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/1', + 'introduced_by_url' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/2', + 'milestone' => '15.9', + 'type' => 'development', + 'name' => 'entry' + } end + let(:raw_yaml) { YAML.dump(yaml) } + subject(:found) { described_class.new(feature_flag_path) } before do @@ -101,58 +107,34 @@ RSpec.describe Tooling::Danger::FeatureFlag do expect(File).to receive(:read).with(feature_flag_path).and_return(raw_yaml) end - describe '#raw' do - it 'returns the raw YAML' do - expect(found.raw).to eq(raw_yaml) - end - end - - describe '#group' do - it 'returns the group found in the YAML' do - expect(found.group).to eq(group) - end - end - - describe '#default_enabled' do - it 'returns the default_enabled found in the YAML' do - expect(found.default_enabled).to eq(true) + described_class::ATTRIBUTES.each do |attribute| + describe "##{attribute}" do + it 'returns value from the YAML' do + expect(found.public_send(attribute)).to eq(yaml[attribute]) + end end end - describe '#rollout_issue_url' do - it 'returns the rollout_issue_url found in the YAML' do - expect(found.rollout_issue_url).to eq('https://gitlab.com/gitlab-org/gitlab/-/issues/1') + describe '#raw' do + it 'returns the raw YAML' do + expect(found.raw).to eq(raw_yaml) end end describe '#group_match_mr_label?' do - subject(:result) { found.group_match_mr_label?(mr_group_label) } - - context 'when MR labels match FF group' do - let(:mr_group_label) { 'group::source code' } - - specify { expect(result).to eq(true) } - end - - context 'when MR labels does not match FF group' do - let(:mr_group_label) { 'group::authentication and authorization' } - - specify { expect(result).to eq(false) } - end - context 'when group is nil' do let(:group) { nil } - context 'and MR has no group label' do - let(:mr_group_label) { nil } - - specify { expect(result).to eq(true) } + it 'is true only if MR has no group label' do + expect(found.group_match_mr_label?(nil)).to eq true + expect(found.group_match_mr_label?('group::source code')).to eq false end + end - context 'and MR has a group label' do - let(:mr_group_label) { 'group::source code' } - - specify { expect(result).to eq(false) } + context 'when group is not nil' do + it 'is true only if MR has the same group label' do + expect(found.group_match_mr_label?(group)).to eq true + expect(found.group_match_mr_label?('group::authentication and authorization')).to eq false end end end diff --git a/spec/tooling/danger/specs_spec.rb b/spec/tooling/danger/specs_spec.rb index 052e86e7f32..cdac5954f92 100644 --- a/spec/tooling/danger/specs_spec.rb +++ b/spec/tooling/danger/specs_spec.rb @@ -111,7 +111,7 @@ RSpec.describe Tooling::Danger::Specs, feature_category: :tooling do describe '#add_suggestions_for_match_with_array' do let(:template) do - <<~MARKDOWN + <<~MARKDOWN.chomp ```suggestion %<suggested_line>s ``` @@ -141,7 +141,7 @@ RSpec.describe Tooling::Danger::Specs, feature_category: :tooling do describe '#add_suggestions_for_project_factory_usage' do let(:template) do - <<~MARKDOWN + <<~MARKDOWN.chomp ```suggestion %<suggested_line>s ``` @@ -225,7 +225,7 @@ RSpec.describe Tooling::Danger::Specs, feature_category: :tooling do describe '#add_suggestions_for_feature_category' do let(:template) do - <<~SUGGESTION_MARKDOWN + <<~SUGGESTION_MARKDOWN.chomp ```suggestion %<suggested_line>s ``` diff --git a/tooling/danger/config_files.rb b/tooling/danger/config_files.rb index 914605a3783..1b09da6c8c9 100644 --- a/tooling/danger/config_files.rb +++ b/tooling/danger/config_files.rb @@ -1,15 +1,14 @@ # frozen_string_literal: true require 'yaml' +require_relative 'suggestor' module Tooling module Danger module ConfigFiles - SUGGEST_INTRODUCED_BY_COMMENT = <<~SUGGEST_COMMENT - ```suggestion - introduced_by_url: %<url> - ``` - SUGGEST_COMMENT + include ::Tooling::Danger::Suggestor + + MISSING_INTRODUCED_BY_REGEX = /^\+?(?<attr_name>\s*introduced_by_url):\s*$/ CONFIG_DIRS = %w[ config/feature_flags @@ -18,14 +17,12 @@ module Tooling ].freeze def add_suggestion_for_missing_introduced_by_url - new_config_files.each do |file_name| - config_file_lines = project_helper.file_lines(file_name) - - config_file_lines.each_with_index do |added_line, i| - next unless added_line =~ /^introduced_by_url:\s?$/ - - markdown(format(SUGGEST_INTRODUCED_BY_COMMENT, url: helper.mr_web_url), file: file_name, line: i + 1) - end + new_config_files.each do |filename| + add_suggestion( + filename: filename, + regex: MISSING_INTRODUCED_BY_REGEX, + replacement: "\\k<attr_name>: #{helper.mr_web_url}" + ) end end diff --git a/tooling/danger/feature_flag.rb b/tooling/danger/feature_flag.rb index cef64e52af3..da0b7053af1 100644 --- a/tooling/danger/feature_flag.rb +++ b/tooling/danger/feature_flag.rb @@ -16,26 +16,22 @@ module Tooling end class Found + ATTRIBUTES = %w[name introduced_by_url rollout_issue_url milestone type group default_enabled].freeze + attr_reader :path def initialize(path) @path = path end - def raw - @raw ||= File.read(path) - end - - def group - @group ||= yaml['group'] + ATTRIBUTES.each do |attribute| + define_method(attribute) do + yaml[attribute] + end end - def default_enabled - @default_enabled ||= yaml['default_enabled'] - end - - def rollout_issue_url - @rollout_issue_url ||= yaml['rollout_issue_url'] + def raw + @raw ||= File.read(path) end def group_match_mr_label?(mr_group_label) diff --git a/tooling/danger/specs.rb b/tooling/danger/specs.rb index 04f3c9d4c9a..f95a798d53e 100644 --- a/tooling/danger/specs.rb +++ b/tooling/danger/specs.rb @@ -1,12 +1,14 @@ # frozen_string_literal: true +require_relative 'suggestor' + module Tooling module Danger module Specs + include ::Tooling::Danger::Suggestor + SPEC_FILES_REGEX = 'spec/' EE_PREFIX = 'ee/' - MATCH_WITH_ARRAY_REGEX = /(?<to>to\(?\s*)(?<matcher>match|eq)(?<expectation>[( ]?\[(?=.*,)[^\]]+)/.freeze - MATCH_WITH_ARRAY_REPLACEMENT = '\k<to>match_array\k<expectation>' PROJECT_FACTORIES = %w[ :project @@ -29,22 +31,18 @@ module Tooling /x.freeze PROJECT_FACTORY_REPLACEMENT = '\k<head>let_it_be\k<tail>' - SUGGESTION_MARKDOWN = <<~SUGGESTION_MARKDOWN - ```suggestion - %<suggested_line>s - ``` - SUGGESTION_MARKDOWN - - MATCH_WITH_ARRAY_SUGGESTION = <<~SUGGEST_COMMENT - If order of the result is not important, please consider using `match_array` to avoid flakiness. - SUGGEST_COMMENT - PROJECT_FACTORY_SUGGESTION = <<~SUGGEST_COMMENT Project creations are very slow. Use `let_it_be`, `build` or `build_stubbed` if possible. See [testing best practices](https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#optimize-factory-usage) for background information and alternative options. SUGGEST_COMMENT + MATCH_WITH_ARRAY_REGEX = /(?<to>to\(?\s*)(?<matcher>match|eq)(?<expectation>[( ]?\[(?=.*,)[^\]]+)/.freeze + MATCH_WITH_ARRAY_REPLACEMENT = '\k<to>match_array\k<expectation>' + MATCH_WITH_ARRAY_SUGGESTION = <<~SUGGEST_COMMENT + If order of the result is not important, please consider using `match_array` to avoid flakiness. + SUGGEST_COMMENT + RSPEC_TOP_LEVEL_DESCRIBE_REGEX = /^\+.?RSpec\.describe(.+)/.freeze FEATURE_CATEGORY_SUGGESTION = <<~SUGGESTION_MARKDOWN Consider adding `feature_category: <feature_category_name>` for this example if it is not set already. @@ -69,19 +67,19 @@ module Tooling def add_suggestions_for_match_with_array(filename) add_suggestion( - filename, - MATCH_WITH_ARRAY_REGEX, - MATCH_WITH_ARRAY_SUGGESTION, - MATCH_WITH_ARRAY_REPLACEMENT + filename: filename, + regex: MATCH_WITH_ARRAY_REGEX, + replacement: MATCH_WITH_ARRAY_REPLACEMENT, + comment_text: MATCH_WITH_ARRAY_SUGGESTION ) end def add_suggestions_for_project_factory_usage(filename) add_suggestion( - filename, - PROJECT_FACTORY_REGEX, - PROJECT_FACTORY_SUGGESTION, - PROJECT_FACTORY_REPLACEMENT + filename: filename, + regex: PROJECT_FACTORY_REGEX, + replacement: PROJECT_FACTORY_REPLACEMENT, + comment_text: PROJECT_FACTORY_SUGGESTION ) end @@ -103,53 +101,9 @@ module Tooling suggested_line = file_lines[line_number] - text = format(comment(FEATURE_CATEGORY_SUGGESTION), suggested_line: suggested_line) - markdown(text, file: filename, line: line_number + 1) + markdown(comment(FEATURE_CATEGORY_SUGGESTION, suggested_line), file: filename, line: line_number.succ) end end - - private - - def added_lines_matching(filename, regex) - helper.changed_lines(filename).grep(/\A\+( )?/).grep(regex) - end - - def add_suggestion(filename, regex, comment_text, replacement = nil, exclude = nil) - added_lines = added_lines_matching(filename, regex) - - return if added_lines.empty? - - spec_file_lines = project_helper.file_lines(filename) - - added_lines.each_with_object([]) do |added_line, processed_line_numbers| - line_number = find_line_number(spec_file_lines, added_line.delete_prefix('+'), exclude_indexes: processed_line_numbers) - next unless line_number - next if !exclude.nil? && added_line.include?(exclude) - - processed_line_numbers << line_number - - suggested_line = spec_file_lines[line_number] - suggested_line = suggested_line.gsub(regex, replacement) unless replacement.nil? - - text = format(comment(comment_text), suggested_line: suggested_line) - markdown(text, file: filename, line: line_number.succ) - end - end - - def comment(comment_text) - <<~COMMENT_BODY.chomp - #{SUGGESTION_MARKDOWN} - #{comment_text} - COMMENT_BODY - end - - def find_line_number(file_lines, searched_line, exclude_indexes: []) - _, index = file_lines.each_with_index.find do |file_line, index| - file_line == searched_line && !exclude_indexes.include?(index) - end - - index - end end end end diff --git a/tooling/danger/suggestor.rb b/tooling/danger/suggestor.rb new file mode 100644 index 00000000000..ffda98e67d0 --- /dev/null +++ b/tooling/danger/suggestor.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true +module Tooling + module Danger + module Suggestor + # For file lines matching `regex` adds suggestion `replacement` with `comment_text` added. + def add_suggestion(filename:, regex:, replacement: nil, comment_text: nil, exclude: nil) + added_lines = added_lines_matching(filename, regex) + + return if added_lines.empty? + + file_lines = project_helper.file_lines(filename) + + added_lines.each_with_object([]) do |added_line, processed_line_numbers| + line_number = find_line_number(file_lines, added_line.delete_prefix('+'), +exclude_indexes: processed_line_numbers) + + next unless line_number + next if !exclude.nil? && added_line.include?(exclude) + + processed_line_numbers << line_number + + if replacement + suggestion_text = file_lines[line_number] + suggestion_text = suggestion_text.gsub(regex, replacement) + end + + markdown(comment(comment_text, suggestion_text), file: filename, line: line_number.succ) + end + end + + private + + def added_lines_matching(filename, regex) + helper.changed_lines(filename).grep(/\A\+( )?/).grep(regex) + end + + def find_line_number(file_lines, searched_line, exclude_indexes: []) + _, index = file_lines.each_with_index.find do |file_line, index| + file_line == searched_line && !exclude_indexes.include?(index) # rubocop:disable Rails/NegateInclude + end + + index + end + + def comment(comment_text = nil, suggested_line = nil) + if suggested_line + suggestion_text = <<~SUGGESTION + ```suggestion + %<suggested_line>s + ``` + SUGGESTION + end + + comment_body = <<~COMMENT_BODY.chomp + #{suggestion_text} + #{comment_text} + COMMENT_BODY + + format(comment_body.chomp, suggested_line: suggested_line) + end + end + end +end |