diff options
27 files changed, 134 insertions, 411 deletions
diff --git a/app/assets/stylesheets/page_bundles/merge_requests.scss b/app/assets/stylesheets/page_bundles/merge_requests.scss index 03b28f28c8d..b4ede515a7c 100644 --- a/app/assets/stylesheets/page_bundles/merge_requests.scss +++ b/app/assets/stylesheets/page_bundles/merge_requests.scss @@ -1080,6 +1080,10 @@ $tabs-holder-z-index: 250; } .merge-request-notification-toggle { + .gl-toggle { + @include gl-ml-auto; + } + .gl-toggle-label { @include gl-font-weight-normal; } diff --git a/app/finders/environments/environments_finder.rb b/app/finders/environments/environments_finder.rb index 011a317a349..85cd37c267e 100644 --- a/app/finders/environments/environments_finder.rb +++ b/app/finders/environments/environments_finder.rb @@ -63,7 +63,7 @@ module Environments def by_ids(environments) if params[:environment_ids].present? - environments.for_id(params[:environment_ids]) + environments.id_in(params[:environment_ids]) else environments end diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 020f5cf9d8e..1f147266fee 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -350,7 +350,6 @@ module Ci scope :for_sha_or_source_sha, -> (sha) { for_sha(sha).or(for_source_sha(sha)) } scope :for_ref, -> (ref) { where(ref: ref) } scope :for_branch, -> (branch) { for_ref(branch).where(tag: false) } - scope :for_id, -> (id) { where(id: id) } scope :for_iid, -> (iid) { where(iid: iid) } scope :for_project, -> (project_id) { where(project_id: project_id) } scope :created_after, -> (time) { where(arel_table[:created_at].gt(time)) } diff --git a/app/models/environment.rb b/app/models/environment.rb index b4586bfbca8..10e0bc91566 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -117,7 +117,6 @@ class Environment < ApplicationRecord scope :with_rank, -> do select('environments.*, rank() OVER (PARTITION BY project_id ORDER BY id DESC)') end - scope :for_id, -> (id) { where(id: id) } scope :with_deployment, -> (sha, status: nil) do deployments = Deployment.select(1).where('deployments.environment_id = environments.id').where(sha: sha) diff --git a/app/services/environments/schedule_to_delete_review_apps_service.rb b/app/services/environments/schedule_to_delete_review_apps_service.rb index 041b834f11b..8e9fe3300c4 100644 --- a/app/services/environments/schedule_to_delete_review_apps_service.rb +++ b/app/services/environments/schedule_to_delete_review_apps_service.rb @@ -68,7 +68,7 @@ module Environments end def mark_for_deletion(deletable_environments) - Environment.for_id(deletable_environments).schedule_to_delete + Environment.id_in(deletable_environments).schedule_to_delete end class Result diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index f18269454e3..5afc13701e0 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -15,15 +15,15 @@ module Members @skip_auth = skip_authorization last_owner = true - in_lock("delete_members:#{member.source.class}:#{member.source.id}") do + in_lock("delete_members:#{member.source.class}:#{member.source.id}", sleep_sec: 0.1.seconds) do break if member.is_a?(GroupMember) && member.source.last_owner?(member.user) last_owner = false member.destroy - member.user&.invalidate_cache_counts end unless last_owner + member.user&.invalidate_cache_counts delete_member_associations(member, skip_subresources, unassign_issuables) end diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 063aca7937c..ac764b219b1 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -68,13 +68,7 @@ module ObjectStorage end def schedule_background_upload(*args) - return unless schedule_background_upload? - return unless upload - - ObjectStorage::BackgroundMoveWorker.perform_async(self.class.name, - upload.class.to_s, - mounted_as, - upload.id) + # TODO remove this method https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1690 end def exclusive_lease_key @@ -312,12 +306,7 @@ module ObjectStorage end def schedule_background_upload(*args) - return unless schedule_background_upload? - - ObjectStorage::BackgroundMoveWorker.perform_async(self.class.name, - model.class.name, - mounted_as, - model.id) + # TODO remove this method https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1690 end def fog_directory diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index b9168a65764..1b1b68320bc 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -1560,15 +1560,6 @@ :weight: 1 :idempotent: false :tags: [] -- :name: object_storage:object_storage_background_move - :worker_name: ObjectStorage::BackgroundMoveWorker - :feature_category: :not_owned - :has_external_dependencies: false - :urgency: :low - :resource_boundary: :unknown - :weight: 1 - :idempotent: false - :tags: [] - :name: object_storage:object_storage_migrate_uploads :worker_name: ObjectStorage::MigrateUploadsWorker :feature_category: :not_owned diff --git a/app/workers/object_storage/background_move_worker.rb b/app/workers/object_storage/background_move_worker.rb deleted file mode 100644 index 8b4c97e9a06..00000000000 --- a/app/workers/object_storage/background_move_worker.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -module ObjectStorage - class BackgroundMoveWorker # rubocop:disable Scalability/IdempotentWorker - include ApplicationWorker - - data_consistency :always - include ObjectStorageQueue - - sidekiq_options retry: 5 - feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned - loggable_arguments 0, 1, 2, 3 - - def perform(uploader_class_name, subject_class_name, file_field, subject_id) - uploader_class = uploader_class_name.constantize - subject_class = subject_class_name.constantize - mount_point = file_field&.to_sym - - return unless uploader_class < ObjectStorage::Concern - return unless uploader_class.object_store_enabled? - return unless uploader_class.background_upload_enabled? - - unless valid_mount_point?(subject_class, uploader_class, mount_point) - raise(ArgumentError, "#{mount_point} not allowed for #{subject_class} in #{self.class.name}") - end - - subject = subject_class.find(subject_id) - uploader = build_uploader(subject, mount_point) - uploader.migrate!(ObjectStorage::Store::REMOTE) - end - - def build_uploader(subject, mount_point) - case subject - when Upload then subject.retrieve_uploader(mount_point) - else - # This is safe because: - # 1. We don't pass any arguments to the method. - # 2. valid_mount_point? checks that this is in fact an uploader of the correct class. - # - subject.public_send(mount_point) # rubocop:disable GitlabSecurity/PublicSend - end - end - - def valid_mount_point?(subject_class, uploader_class, mount_point) - subject_class == Upload || subject_class.uploaders[mount_point] == uploader_class - end - end -end diff --git a/danger/specs/Dangerfile b/danger/specs/Dangerfile index 1eb11843732..d17c17bc545 100644 --- a/danger/specs/Dangerfile +++ b/danger/specs/Dangerfile @@ -57,4 +57,5 @@ end specs.changed_specs_files.each do |filename| specs.add_suggestions_for_match_with_array(filename) specs.add_suggestions_for_project_factory_usage(filename) + specs.add_suggestions_for_feature_category(filename) end diff --git a/db/post_migrate/20221117153015_add_index_merge_request_id_created_at_on_scan_finding_approval_merge_request_rules.rb b/db/post_migrate/20221117153015_add_index_merge_request_id_created_at_on_scan_finding_approval_merge_request_rules.rb new file mode 100644 index 00000000000..ecd3a8be02e --- /dev/null +++ b/db/post_migrate/20221117153015_add_index_merge_request_id_created_at_on_scan_finding_approval_merge_request_rules.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddIndexMergeRequestIdCreatedAtOnScanFindingApprovalMergeRequestRules < Gitlab::Database::Migration[2.0] + INDEX_NAME = 'scan_finding_approval_mr_rule_index_mr_id_and_created_at' + SCAN_FINDING_REPORT_TYPE = 4 + + disable_ddl_transaction! + + def up + add_concurrent_index :approval_merge_request_rules, %i[merge_request_id created_at], + where: "report_type = #{SCAN_FINDING_REPORT_TYPE}", name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :approval_merge_request_rules, INDEX_NAME + end +end diff --git a/db/schema_migrations/20221117153015 b/db/schema_migrations/20221117153015 new file mode 100644 index 00000000000..438ddfdcfbf --- /dev/null +++ b/db/schema_migrations/20221117153015 @@ -0,0 +1 @@ +ce905f8497f63b909fee18cb20f2bfc95c33f09d01df09798ca30cdcd72280dc
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 82ad720f1c9..de560c954ec 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -31410,6 +31410,8 @@ CREATE INDEX scan_finding_approval_mr_rule_index_id ON approval_merge_request_ru CREATE INDEX scan_finding_approval_mr_rule_index_merge_request_id ON approval_merge_request_rules USING btree (merge_request_id) WHERE (report_type = 4); +CREATE INDEX scan_finding_approval_mr_rule_index_mr_id_and_created_at ON approval_merge_request_rules USING btree (merge_request_id, created_at) WHERE (report_type = 4); + CREATE INDEX scan_finding_approval_project_rule_index_created_at_project_id ON approval_project_rules USING btree (created_at, project_id) WHERE (report_type = 4); CREATE INDEX scan_finding_approval_project_rule_index_project_id ON approval_project_rules USING btree (project_id) WHERE (report_type = 4); diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index e785d10f7e7..7a198f30e38 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -161,7 +161,7 @@ module API def all_matching_pipelines pipelines = user_project.ci_pipelines.newest_first(sha: commit.sha) pipelines = pipelines.for_ref(params[:ref]) if params[:ref] - pipelines = pipelines.for_id(params[:pipeline_id]) if params[:pipeline_id] + pipelines = pipelines.id_in(params[:pipeline_id]) if params[:pipeline_id] pipelines end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 476bed83c0c..fce0bf197b5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -19148,7 +19148,7 @@ msgstr "" msgid "GroupActivityMetrics|Members added" msgstr "" -msgid "GroupActivityMetrics|Merge Requests created" +msgid "GroupActivityMetrics|Merge requests created" msgstr "" msgid "GroupActivityMetrics|Recent activity" @@ -34944,6 +34944,9 @@ msgstr "" msgid "Restricted shift times are not available for hourly shifts" msgstr "" +msgid "Results limit reached" +msgstr "" + msgid "Resume" msgstr "" diff --git a/spec/frontend/content_editor/test_utils.js b/spec/frontend/content_editor/test_utils.js index 0768fa6e8df..46240c4ac94 100644 --- a/spec/frontend/content_editor/test_utils.js +++ b/spec/frontend/content_editor/test_utils.js @@ -15,6 +15,7 @@ import DescriptionItem from '~/content_editor/extensions/description_item'; import DescriptionList from '~/content_editor/extensions/description_list'; import Details from '~/content_editor/extensions/details'; import DetailsContent from '~/content_editor/extensions/details_content'; +import Diagram from '~/content_editor/extensions/diagram'; import Emoji from '~/content_editor/extensions/emoji'; import FootnoteDefinition from '~/content_editor/extensions/footnote_definition'; import FootnoteReference from '~/content_editor/extensions/footnote_reference'; @@ -215,6 +216,7 @@ export const createTiptapEditor = (extensions = []) => DescriptionList, Details, DetailsContent, + Diagram, Emoji, FootnoteDefinition, FootnoteReference, diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index 098f8bd4514..18aaab1d1f3 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -356,50 +356,6 @@ RSpec.describe Ci::JobArtifact do end end - describe 'callbacks' do - describe '#schedule_background_upload' do - subject { create(:ci_job_artifact, :archive) } - - context 'when object storage is disabled' do - before do - stub_artifacts_object_storage(enabled: false) - end - - it 'does not schedule the migration' do - expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) - - subject - end - end - - context 'when object storage is enabled' do - context 'when background upload is enabled' do - before do - stub_artifacts_object_storage(background_upload: true) - end - - it 'schedules the model for migration' do - expect(ObjectStorage::BackgroundMoveWorker).to receive(:perform_async).with('JobArtifactUploader', described_class.name, :file, kind_of(Numeric)) - - subject - end - end - - context 'when background upload is disabled' do - before do - stub_artifacts_object_storage(background_upload: false) - end - - it 'schedules the model for migration' do - expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) - - subject - end - end - end - end - end - context 'creating the artifact' do let(:project) { create(:project) } let(:artifact) { create(:ci_job_artifact, :archive, project: project) } diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 8fef895901d..cc48c627049 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -212,7 +212,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do end describe ".schedule_to_delete" do - subject { described_class.for_id(deletable_environment).schedule_to_delete } + subject { described_class.id_in(deletable_environment).schedule_to_delete } it "schedules the record for deletion" do freeze_time do diff --git a/spec/models/lfs_object_spec.rb b/spec/models/lfs_object_spec.rb index c25d0451f18..aedbea3cd26 100644 --- a/spec/models/lfs_object_spec.rb +++ b/spec/models/lfs_object_spec.rb @@ -99,58 +99,6 @@ RSpec.describe LfsObject do subject { create(:lfs_object, :with_file) } - context 'when object storage is disabled' do - before do - stub_lfs_object_storage(enabled: false) - end - - it 'does not schedule the migration' do - expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) - - subject - end - end - - context 'when object storage is enabled' do - context 'when background upload is enabled' do - context 'when is licensed' do - before do - stub_lfs_object_storage(background_upload: true) - end - - it 'schedules the model for migration' do - expect(ObjectStorage::BackgroundMoveWorker) - .to receive(:perform_async) - .with('LfsObjectUploader', described_class.name, :file, kind_of(Numeric)) - .once - - subject - end - - it 'schedules the model for migration once' do - expect(ObjectStorage::BackgroundMoveWorker) - .to receive(:perform_async) - .with('LfsObjectUploader', described_class.name, :file, kind_of(Numeric)) - .once - - create(:lfs_object, :with_file) - end - end - end - - context 'when background upload is disabled' do - before do - stub_lfs_object_storage(background_upload: false) - end - - it 'schedules the model for migration' do - expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) - - subject - end - end - end - describe 'file is being stored' do subject { create(:lfs_object, :with_file) } diff --git a/spec/requests/api/group_import_spec.rb b/spec/requests/api/group_import_spec.rb index efad6334518..893e60d80ce 100644 --- a/spec/requests/api/group_import_spec.rb +++ b/spec/requests/api/group_import_spec.rb @@ -198,12 +198,6 @@ RSpec.describe API::GroupImport do include_examples 'when all params are correct' include_examples 'when some params are missing' end - - it "doesn't attempt to migrate file to object storage" do - expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) - - subject - end end context 'with object storage enabled' do diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 3529239a4d9..310ee24c2a8 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -842,14 +842,6 @@ RSpec.describe 'Git LFS API and storage' do lfs_object.destroy! end - context 'with object storage disabled' do - it "doesn't attempt to migrate file to object storage" do - expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) - - put_finalize(with_tempfile: true) - end - end - context 'with object storage enabled' do context 'and direct upload enabled' do let!(:fog_connection) do @@ -911,18 +903,6 @@ RSpec.describe 'Git LFS API and storage' do end end end - - context 'and background upload enabled' do - before do - stub_lfs_object_storage(background_upload: true) - end - - it 'schedules migration of file to object storage' do - expect(ObjectStorage::BackgroundMoveWorker).to receive(:perform_async).with('LfsObjectUploader', 'LfsObject', :file, kind_of(Numeric)) - - put_finalize(with_tempfile: true) - end - end end end diff --git a/spec/tooling/danger/specs_spec.rb b/spec/tooling/danger/specs_spec.rb index d6aed86e7dc..a0d0b03a0da 100644 --- a/spec/tooling/danger/specs_spec.rb +++ b/spec/tooling/danger/specs_spec.rb @@ -9,7 +9,7 @@ require 'gitlab/dangerfiles/spec_helper' require_relative '../../../tooling/danger/specs' require_relative '../../../tooling/danger/project_helper' -RSpec.describe Tooling::Danger::Specs do +RSpec.describe Tooling::Danger::Specs, feature_category: :tooling do include_context "with dangerfile" let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) } @@ -217,4 +217,68 @@ RSpec.describe Tooling::Danger::Specs do specs.add_suggestions_for_project_factory_usage(filename) end end + + describe '#add_suggestions_for_feature_category' do + let(:template) do + <<~SUGGESTION_MARKDOWN + ```suggestion + %<suggested_line>s + ``` + + Consider addding `feature_category: <feature_category_name>` for this example if it is not set already. + See [testing best practices](https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#feature-category-metadata). + SUGGESTION_MARKDOWN + end + + let(:file_lines) do + [ + " require 'spec_helper'", + " \n", + " RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController, feature_category: :planning_analytics do", + " end", + "RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController do", + " let_it_be(:user) { create(:user) }", + " end", + " describe 'GET \"time_summary\"' do", + " end", + " RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController do", + " let_it_be(:user) { create(:user) }", + " end", + " describe 'GET \"time_summary\"' do", + " end" + ] + end + + let(:matching_lines) do + [ + "+ RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController, feature_category: :planning_analytics do", + "+RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController do", + "+ RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController do" + ] + end + + let(:changed_lines) do + [ + "+ RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController, feature_category: :planning_analytics do", + "+RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController do", + "+ let_it_be(:user) { create(:user) }", + "- end", + "+ describe 'GET \"time_summary\"' do", + "+ RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController do" + ] + end + + it 'adds suggestions at the correct lines', :aggregate_failures do + [ + { suggested_line: "RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController do", number: 5 }, + { suggested_line: " RSpec.describe Projects::Analytics::CycleAnalytics::SummaryController do", number: 10 } + + ].each do |test_case| + comment = format(template, suggested_line: test_case[:suggested_line]) + expect(specs).to receive(:markdown).with(comment, file: filename, line: test_case[:number]) + end + + specs.add_suggestions_for_feature_category(filename) + end + end end diff --git a/spec/uploaders/external_diff_uploader_spec.rb b/spec/uploaders/external_diff_uploader_spec.rb index ee23c1e36b7..a889181b72c 100644 --- a/spec/uploaders/external_diff_uploader_spec.rb +++ b/spec/uploaders/external_diff_uploader_spec.rb @@ -24,29 +24,6 @@ RSpec.describe ExternalDiffUploader do store_dir: %r[merge_request_diffs/mr-\d+] end - describe 'migration to object storage' do - context 'with object storage disabled' do - it "is skipped" do - expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) - - diff - end - end - - context 'with object storage enabled' do - before do - stub_external_diffs_setting(enabled: true) - stub_external_diffs_object_storage(background_upload: true) - end - - it 'is scheduled to run after creation' do - expect(ObjectStorage::BackgroundMoveWorker).to receive(:perform_async).with(described_class.name, 'MergeRequestDiff', :external_diff, kind_of(Numeric)) - - diff - end - end - end - describe 'remote file' do context 'with object storage enabled' do before do @@ -57,8 +34,6 @@ RSpec.describe ExternalDiffUploader do end it 'can store file remotely' do - allow(ObjectStorage::BackgroundMoveWorker).to receive(:perform_async) - diff expect(diff.external_diff_store).to eq(described_class::Store::REMOTE) diff --git a/spec/uploaders/lfs_object_uploader_spec.rb b/spec/uploaders/lfs_object_uploader_spec.rb index d1a3fb243ac..b85892a42b5 100644 --- a/spec/uploaders/lfs_object_uploader_spec.rb +++ b/spec/uploaders/lfs_object_uploader_spec.rb @@ -25,28 +25,6 @@ RSpec.describe LfsObjectUploader do store_dir: %r[\h{2}/\h{2}] end - describe 'migration to object storage' do - context 'with object storage disabled' do - it "is skipped" do - expect(ObjectStorage::BackgroundMoveWorker).not_to receive(:perform_async) - - lfs_object - end - end - - context 'with object storage enabled' do - before do - stub_lfs_object_storage(background_upload: true) - end - - it 'is scheduled to run after creation' do - expect(ObjectStorage::BackgroundMoveWorker).to receive(:perform_async).with(described_class.name, 'LfsObject', :file, kind_of(Numeric)) - - lfs_object - end - end - end - describe 'remote file' do let(:lfs_object) { create(:lfs_object, :object_storage, :with_file) } @@ -56,8 +34,6 @@ RSpec.describe LfsObjectUploader do end it 'can store file remotely' do - allow(ObjectStorage::BackgroundMoveWorker).to receive(:perform_async) - lfs_object expect(lfs_object.file_store).to eq(described_class::Store::REMOTE) diff --git a/spec/uploaders/workers/object_storage/background_move_worker_spec.rb b/spec/uploaders/workers/object_storage/background_move_worker_spec.rb deleted file mode 100644 index e549d42e28f..00000000000 --- a/spec/uploaders/workers/object_storage/background_move_worker_spec.rb +++ /dev/null @@ -1,152 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ObjectStorage::BackgroundMoveWorker do - let(:local) { ObjectStorage::Store::LOCAL } - let(:remote) { ObjectStorage::Store::REMOTE } - - def perform - described_class.perform_async(uploader_class.name, subject_class, file_field, subject_id) - end - - context 'for LFS' do - let!(:lfs_object) { create(:lfs_object, :with_file, file_store: local) } - let(:uploader_class) { LfsObjectUploader } - let(:subject_class) { LfsObject } - let(:file_field) { :file } - let(:subject_id) { lfs_object.id } - - context 'when object storage is enabled' do - before do - stub_lfs_object_storage(background_upload: true) - end - - it 'uploads object to storage', :sidekiq_might_not_need_inline do - expect { perform }.to change { lfs_object.reload.file_store }.from(local).to(remote) - end - - context 'when background upload is disabled' do - before do - allow(Gitlab.config.lfs.object_store).to receive(:background_upload) { false } - end - - it 'is skipped' do - expect { perform }.not_to change { lfs_object.reload.file_store } - end - end - end - - context 'when object storage is disabled' do - before do - stub_lfs_object_storage(enabled: false) - end - - it "doesn't migrate files" do - perform - - expect(lfs_object.reload.file_store).to eq(local) - end - end - end - - context 'for job artifacts' do - let(:artifact) { create(:ci_job_artifact, :archive) } - let(:uploader_class) { JobArtifactUploader } - let(:subject_class) { Ci::JobArtifact } - let(:file_field) { :file } - let(:subject_id) { artifact.id } - - context 'when local storage is used' do - let(:store) { local } - - context 'and remote storage is defined' do - before do - stub_artifacts_object_storage(background_upload: true) - end - - it "migrates file to remote storage", :sidekiq_might_not_need_inline do - perform - - expect(artifact.reload.file_store).to eq(remote) - end - end - end - end - - context 'for uploads' do - let!(:project) { create(:project, :with_avatar) } - let(:uploader_class) { AvatarUploader } - let(:file_field) { :avatar } - - context 'when local storage is used' do - let(:store) { local } - - context 'and remote storage is defined' do - before do - stub_uploads_object_storage(uploader_class, background_upload: true) - end - - describe 'supports using the model' do - let(:subject_class) { project.class } - let(:subject_id) { project.id } - - it "migrates file to remote storage", :sidekiq_might_not_need_inline do - perform - project.reload - BatchLoader::Executor.clear_current - - expect(project.avatar).not_to be_file_storage - end - end - - describe 'supports using the Upload' do - let(:subject_class) { Upload } - let(:subject_id) { project.avatar.upload.id } - - it "migrates file to remote storage", :sidekiq_might_not_need_inline do - perform - - expect(project.reload.avatar).not_to be_file_storage - end - end - end - end - end - - context 'with invalid input' do - before do - stub_lfs_object_storage(background_upload: true) - stub_artifacts_object_storage(background_upload: true) - stub_uploads_object_storage(AvatarUploader, background_upload: true) - end - - context 'with a file_field argument that is not an upload mount' do - it 'does nothing' do - expect(subject).not_to receive(:build_uploader) - expect(LfsObject).not_to receive(:find) - expect(Ci::JobArtifact).not_to receive(:find) - expect(AvatarUploader).not_to receive(:find) - - expect { subject.perform('LfsObjectUploader', 'LfsObject', 'avatar', 1) } - .to raise_error(ArgumentError, 'avatar not allowed for LfsObject in ObjectStorage::BackgroundMoveWorker') - - expect { subject.perform('JobArtifactUploader', 'Ci::JobArtifact', 'id', 2) } - .to raise_error(ArgumentError, 'id not allowed for Ci::JobArtifact in ObjectStorage::BackgroundMoveWorker') - - expect { subject.perform('AvatarUploader', 'User', 'file', 3) } - .to raise_error(ArgumentError, 'file not allowed for User in ObjectStorage::BackgroundMoveWorker') - end - end - - context 'with an uploader that does not match the given subject' do - it 'raises ArgumentError' do - expect(subject).not_to receive(:build_uploader) - expect(LfsObject).not_to receive(:find) - - expect { subject.perform('AvatarUploader', 'LfsObject', 'file', 1) } - .to raise_error(ArgumentError, 'file not allowed for LfsObject in ObjectStorage::BackgroundMoveWorker') - end - end - end -end diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index 105519fe7c9..146d134667c 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -366,7 +366,6 @@ RSpec.describe 'Every Sidekiq worker' do 'ObjectPool::DestroyWorker' => 3, 'ObjectPool::JoinWorker' => 3, 'ObjectPool::ScheduleJoinWorker' => 3, - 'ObjectStorage::BackgroundMoveWorker' => 5, 'ObjectStorage::MigrateUploadsWorker' => 3, 'Onboarding::CreateLearnGitlabWorker' => 3, 'Packages::CleanupPackageFileWorker' => 0, diff --git a/tooling/danger/specs.rb b/tooling/danger/specs.rb index 6832f7d10d1..c2427f56f33 100644 --- a/tooling/danger/specs.rb +++ b/tooling/danger/specs.rb @@ -45,6 +45,13 @@ module Tooling for background information and alternative options. SUGGEST_COMMENT + FEATURE_CATEGORY_REGEX = /^\+.?RSpec\.describe(.+)(?!feature_category)/.freeze + FEATURE_CATEGORY_SUGGESTION = <<~SUGGESTION_MARKDOWN + Consider addding `feature_category: <feature_category_name>` for this example if it is not set already. + See [testing best practices](https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#feature-category-metadata). + SUGGESTION_MARKDOWN + FEATURE_CATEGORY_EXCLUDE = 'feature_category' + def changed_specs_files(ee: :include) changed_files = helper.all_changed_files folder_prefix = @@ -64,8 +71,8 @@ module Tooling add_suggestion( filename, MATCH_WITH_ARRAY_REGEX, - MATCH_WITH_ARRAY_REPLACEMENT, - MATCH_WITH_ARRAY_SUGGESTION + MATCH_WITH_ARRAY_SUGGESTION, + MATCH_WITH_ARRAY_REPLACEMENT ) end @@ -73,19 +80,30 @@ module Tooling add_suggestion( filename, PROJECT_FACTORY_REGEX, - PROJECT_FACTORY_REPLACEMENT, - PROJECT_FACTORY_SUGGESTION + PROJECT_FACTORY_SUGGESTION, + PROJECT_FACTORY_REPLACEMENT + ) + end + + def add_suggestions_for_feature_category(filename) + add_suggestion( + filename, + FEATURE_CATEGORY_REGEX, + FEATURE_CATEGORY_SUGGESTION, + nil, + FEATURE_CATEGORY_EXCLUDE ) end private def added_lines_matching(filename, regex) - helper.changed_lines(filename).grep(/\A\+ /).grep(regex) + helper.changed_lines(filename).grep(/\A\+( )?/).grep(regex) end - def add_suggestion(filename, regex, replacement, comment_text) + 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) @@ -93,9 +111,14 @@ module Tooling 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 - text = format(comment(comment_text), suggested_line: spec_file_lines[line_number].gsub(regex, replacement)) + + 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 |