diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-02-13 12:09:48 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-02-13 12:09:48 +0300 |
commit | 6419f4883904680223a460291f9c538719283058 (patch) | |
tree | 9e1bfd94f9a0ff5f5e11cb6016fe968ea3865de8 | |
parent | 00d1f41541a21e13e3c6bd94d897dc5e4da8278a (diff) |
Add latest changes from gitlab-org/gitlab@master
16 files changed, 222 insertions, 245 deletions
diff --git a/.rubocop_todo/layout/argument_alignment.yml b/.rubocop_todo/layout/argument_alignment.yml index e658ac751e7..c813b997f3f 100644 --- a/.rubocop_todo/layout/argument_alignment.yml +++ b/.rubocop_todo/layout/argument_alignment.yml @@ -1818,8 +1818,6 @@ Layout/ArgumentAlignment: - 'ee/spec/services/protected_environments/create_service_spec.rb' - 'ee/spec/services/protected_environments/update_service_spec.rb' - 'ee/spec/services/quick_actions/interpret_service_spec.rb' - - 'ee/spec/services/registrations/import_namespace_create_service_spec.rb' - - 'ee/spec/services/registrations/standard_namespace_create_service_spec.rb' - 'ee/spec/services/security/auto_fix_service_spec.rb' - 'ee/spec/services/security/findings/dismiss_service_spec.rb' - 'ee/spec/services/security/ingestion/finding_map_spec.rb' diff --git a/.rubocop_todo/rspec/expect_change.yml b/.rubocop_todo/rspec/expect_change.yml index 24db05a5004..81531d19982 100644 --- a/.rubocop_todo/rspec/expect_change.yml +++ b/.rubocop_todo/rspec/expect_change.yml @@ -161,8 +161,6 @@ RSpec/ExpectChange: - 'ee/spec/services/projects/transfer_service_spec.rb' - 'ee/spec/services/protected_environments/create_service_spec.rb' - 'ee/spec/services/quality_management/test_cases/create_service_spec.rb' - - 'ee/spec/services/registrations/import_namespace_create_service_spec.rb' - - 'ee/spec/services/registrations/standard_namespace_create_service_spec.rb' - 'ee/spec/services/requirements_management/export_csv_service_spec.rb' - 'ee/spec/services/sbom/ingestion/tasks/ingest_component_versions_spec.rb' - 'ee/spec/services/sbom/ingestion/tasks/ingest_components_spec.rb' diff --git a/.rubocop_todo/rspec/missing_feature_category.yml b/.rubocop_todo/rspec/missing_feature_category.yml index 77004d41167..7d604d682ca 100644 --- a/.rubocop_todo/rspec/missing_feature_category.yml +++ b/.rubocop_todo/rspec/missing_feature_category.yml @@ -2222,8 +2222,6 @@ RSpec/MissingFeatureCategory: - 'ee/spec/services/protected_environments/update_service_spec.rb' - 'ee/spec/services/push_rules/create_or_update_service_spec.rb' - 'ee/spec/services/quality_management/test_cases/create_service_spec.rb' - - 'ee/spec/services/registrations/import_namespace_create_service_spec.rb' - - 'ee/spec/services/registrations/standard_namespace_create_service_spec.rb' - 'ee/spec/services/releases/create_service_spec.rb' - 'ee/spec/services/releases/update_service_spec.rb' - 'ee/spec/services/requirements_management/export_csv_service_spec.rb' diff --git a/app/assets/javascripts/packages_and_registries/package_registry/components/details/version_row.vue b/app/assets/javascripts/packages_and_registries/package_registry/components/details/version_row.vue index 9b5c8f63814..d6732cad6c1 100644 --- a/app/assets/javascripts/packages_and_registries/package_registry/components/details/version_row.vue +++ b/app/assets/javascripts/packages_and_registries/package_registry/components/details/version_row.vue @@ -1,15 +1,21 @@ <script> -import { GlLink, GlSprintf, GlTruncate } from '@gitlab/ui'; +import { GlIcon, GlLink, GlSprintf, GlTooltipDirective, GlTruncate } from '@gitlab/ui'; import { getIdFromGraphQLId } from '~/graphql_shared/utils'; import PackageTags from '~/packages_and_registries/shared/components/package_tags.vue'; import PublishMethod from '~/packages_and_registries/shared/components/publish_method.vue'; import ListItem from '~/vue_shared/components/registry/list_item.vue'; import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue'; -import { PACKAGE_DEFAULT_STATUS } from '../../constants'; +import { + ERRORED_PACKAGE_TEXT, + ERROR_PUBLISHING, + PACKAGE_ERROR_STATUS, + WARNING_TEXT, +} from '../../constants'; export default { name: 'PackageVersionRow', components: { + GlIcon, GlLink, GlSprintf, GlTruncate, @@ -18,6 +24,9 @@ export default { ListItem, TimeAgoTooltip, }, + directives: { + GlTooltip: GlTooltipDirective, + }, props: { packageEntity: { type: Object, @@ -31,23 +40,23 @@ export default { packageLink() { return `${getIdFromGraphQLId(this.packageEntity.id)}`; }, - disabledRow() { - return this.packageEntity.status && this.packageEntity.status !== PACKAGE_DEFAULT_STATUS; + errorStatusRow() { + return this.packageEntity?.status === PACKAGE_ERROR_STATUS; }, }, + i18n: { + erroredPackageText: ERRORED_PACKAGE_TEXT, + errorPublishing: ERROR_PUBLISHING, + warningText: WARNING_TEXT, + }, }; </script> <template> - <list-item :disabled="disabledRow"> + <list-item> <template #left-primary> <div class="gl-display-flex gl-align-items-center gl-mr-3 gl-min-w-0"> - <gl-link - v-if="containsWebPathLink" - class="gl-text-body gl-min-w-0" - :disabled="disabledRow" - :href="packageLink" - > + <gl-link v-if="containsWebPathLink" class="gl-text-body gl-min-w-0" :href="packageLink"> <gl-truncate :text="packageEntity.name" /> </gl-link> <gl-truncate v-else :text="packageEntity.name" /> @@ -62,7 +71,20 @@ export default { </div> </template> <template #left-secondary> - {{ packageEntity.version }} + <div v-if="errorStatusRow" class="gl-text-red-500"> + <gl-icon + v-gl-tooltip="{ title: $options.i18n.erroredPackageText }" + name="warning" + :aria-label="$options.i18n.warningText" + /> + <span>{{ $options.i18n.errorPublishing }}</span> + </div> + <gl-truncate + v-else + class="gl-max-w-15 gl-md-max-w-26" + :text="packageEntity.version" + :with-tooltip="true" + /> </template> <template #right-primary> diff --git a/app/assets/javascripts/packages_and_registries/package_registry/components/list/package_list_row.vue b/app/assets/javascripts/packages_and_registries/package_registry/components/list/package_list_row.vue index 332ebf4892d..a8a96b89256 100644 --- a/app/assets/javascripts/packages_and_registries/package_registry/components/list/package_list_row.vue +++ b/app/assets/javascripts/packages_and_registries/package_registry/components/list/package_list_row.vue @@ -11,8 +11,11 @@ import { import { s__, __ } from '~/locale'; import ListItem from '~/vue_shared/components/registry/list_item.vue'; import { + ERRORED_PACKAGE_TEXT, + ERROR_PUBLISHING, PACKAGE_ERROR_STATUS, PACKAGE_DEFAULT_STATUS, + WARNING_TEXT, } from '~/packages_and_registries/package_registry/constants'; import { getPackageTypeLabel } from '~/packages_and_registries/package_registry/utils'; import PackagePath from '~/packages_and_registries/shared/components/package_path.vue'; @@ -86,11 +89,11 @@ export default { }, }, i18n: { - erroredPackageText: s__('PackageRegistry|Invalid Package: failed metadata extraction'), + erroredPackageText: ERRORED_PACKAGE_TEXT, createdAt: __('Created %{timestamp}'), deletePackage: s__('PackageRegistry|Delete package'), - errorPublishing: s__('PackageRegistry|Error publishing'), - warning: __('Warning'), + errorPublishing: ERROR_PUBLISHING, + warning: WARNING_TEXT, moreActions: __('More actions'), }, }; diff --git a/app/assets/javascripts/packages_and_registries/package_registry/constants.js b/app/assets/javascripts/packages_and_registries/package_registry/constants.js index beb8d11a944..ddd2fda7733 100644 --- a/app/assets/javascripts/packages_and_registries/package_registry/constants.js +++ b/app/assets/javascripts/packages_and_registries/package_registry/constants.js @@ -128,6 +128,12 @@ export const DELETE_PACKAGE_ERROR_MESSAGE = s__( 'PackageRegistry|Something went wrong while deleting the package.', ); +export const ERRORED_PACKAGE_TEXT = s__( + 'PackageRegistry|Invalid Package: failed metadata extraction', +); +export const ERROR_PUBLISHING = s__('PackageRegistry|Error publishing'); +export const WARNING_TEXT = __('Warning'); + export const PACKAGE_REGISTRY_TITLE = __('Package Registry'); export const PACKAGE_ERROR_STATUS = 'ERROR'; diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index 68bb6427350..25a1e9a9873 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -39,7 +39,6 @@ module Groups if @group.save @group.add_owner(current_user) Integration.create_from_active_default_integrations(@group, :group_id) - Onboarding::Progress.onboard(@group) end end diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 52c3b3e449f..a2798bfda4d 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -828,6 +828,9 @@ Gitlab.ee do Settings.cron_jobs['abandoned_trial_emails'] ||= Settingslogic.new({}) Settings.cron_jobs['abandoned_trial_emails']['cron'] ||= "0 1 * * *" Settings.cron_jobs['abandoned_trial_emails']['job_class'] = 'Emails::AbandonedTrialEmailsCronWorker' + Settings.cron_jobs['package_metadata_sync_worker'] ||= Settingslogic.new({}) + Settings.cron_jobs['package_metadata_sync_worker']['cron'] ||= "0 1 * * *" + Settings.cron_jobs['package_metadata_sync_worker']['job_class'] = 'PackageMetadata::SyncWorker' Gitlab.com do Settings.cron_jobs['disable_legacy_open_source_license_for_inactive_projects'] ||= Settingslogic.new({}) Settings.cron_jobs['disable_legacy_open_source_license_for_inactive_projects']['cron'] ||= "30 5 * * 0" diff --git a/db/migrate/20230203145514_allow_null_pipeline_id_to_dast_pre_scan_verification.rb b/db/migrate/20230203145514_allow_null_pipeline_id_to_dast_pre_scan_verification.rb new file mode 100644 index 00000000000..11e8a856c11 --- /dev/null +++ b/db/migrate/20230203145514_allow_null_pipeline_id_to_dast_pre_scan_verification.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AllowNullPipelineIdToDastPreScanVerification < Gitlab::Database::Migration[2.1] + def up + change_column_null :dast_pre_scan_verifications, :ci_pipeline_id, true + end + + def down + # There may now be nulls in the table, so we cannot re-add the constraint here. + end +end diff --git a/db/schema_migrations/20230203145514 b/db/schema_migrations/20230203145514 new file mode 100644 index 00000000000..f929711f279 --- /dev/null +++ b/db/schema_migrations/20230203145514 @@ -0,0 +1 @@ +42f7cf3cb5d8b9b3f1c8a30b1f48fb6a5bf650e368c927b2b3c6c74c2c339088
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 9f4100a0d20..6628492e8e8 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -14614,7 +14614,7 @@ ALTER SEQUENCE dast_pre_scan_verification_steps_id_seq OWNED BY dast_pre_scan_ve CREATE TABLE dast_pre_scan_verifications ( id bigint NOT NULL, dast_profile_id bigint NOT NULL, - ci_pipeline_id bigint NOT NULL, + ci_pipeline_id bigint, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, status smallint DEFAULT 0 NOT NULL diff --git a/doc/user/project/integrations/gitlab_slack_application.md b/doc/user/project/integrations/gitlab_slack_application.md index 3db7d2cc97a..ac29a639436 100644 --- a/doc/user/project/integrations/gitlab_slack_application.md +++ b/doc/user/project/integrations/gitlab_slack_application.md @@ -7,9 +7,9 @@ info: To determine the technical writer assigned to the Stage/Group associated w # GitLab for Slack app **(FREE SAAS)** NOTE: -The GitLab for Slack app is only configurable for GitLab.com. It does **not** -work for on-premises installations where you can configure -[Slack slash commands](slack_slash_commands.md) instead. See +The GitLab for Slack app is only configurable for GitLab SaaS customers. +Self-managed GitLab customers should configure +[Slack slash commands](slack_slash_commands.md) and [Slack notifications](slack.md) instead. See [Slack application integration for self-managed instances](https://gitlab.com/groups/gitlab-org/-/epics/1211) for our plans to make the app configurable for all GitLab installations. @@ -94,7 +94,7 @@ instead: ## Slack notifications -> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/381012) in GitLab 15.7. +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/381012) in GitLab 15.9. With Slack notifications, GitLab can send messages to Slack workspace channels for certain GitLab [events](#events-for-slack-notifications) (for example, when an issue is created). diff --git a/spec/frontend/packages_and_registries/package_registry/components/details/__snapshots__/version_row_spec.js.snap b/spec/frontend/packages_and_registries/package_registry/components/details/__snapshots__/version_row_spec.js.snap deleted file mode 100644 index f00d707417a..00000000000 --- a/spec/frontend/packages_and_registries/package_registry/components/details/__snapshots__/version_row_spec.js.snap +++ /dev/null @@ -1,92 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`VersionRow renders 1`] = ` -<div - class="gl-display-flex gl-flex-direction-column gl-border-b-solid gl-border-t-solid gl-border-t-1 gl-border-b-1 gl-border-t-transparent gl-border-b-gray-100" -> - <div - class="gl-display-flex gl-align-items-center gl-py-3" - > - <!----> - - <div - class="gl-display-flex gl-xs-flex-direction-column gl-justify-content-space-between gl-align-items-stretch gl-flex-grow-1" - > - <div - class="gl-display-flex gl-flex-direction-column gl-xs-mb-3 gl-min-w-0 gl-flex-grow-1" - > - <div - class="gl-display-flex gl-align-items-center gl-text-body gl-font-weight-bold gl-min-h-6 gl-min-w-0" - > - <div - class="gl-display-flex gl-align-items-center gl-mr-3 gl-min-w-0" - > - <gl-link-stub - class="gl-text-body gl-min-w-0" - href="243" - > - <span - class="gl-truncate" - data-testid="truncate-end-container" - title="@gitlab-org/package-15" - > - <span - class="gl-truncate-end" - > - @gitlab-org/package-15 - </span> - </span> - </gl-link-stub> - - <package-tags-stub - class="gl-ml-3" - hidelabel="true" - tagdisplaylimit="1" - tags="[object Object],[object Object],[object Object]" - /> - </div> - - <!----> - </div> - - <div - class="gl-display-flex gl-align-items-center gl-text-gray-500 gl-min-h-6 gl-min-w-0 gl-flex-grow-1" - > - - 1.0.1 - - </div> - </div> - - <div - class="gl-display-flex gl-flex-direction-column gl-sm-align-items-flex-end gl-justify-content-space-between gl-text-gray-500 gl-flex-shrink-0" - > - <div - class="gl-display-flex gl-align-items-center gl-sm-text-body gl-sm-font-weight-bold gl-min-h-6" - > - <publish-method-stub - packageentity="[object Object]" - /> - </div> - - <div - class="gl-display-flex gl-align-items-center gl-min-h-6" - > - <span> - Created - <time-ago-tooltip-stub - cssclass="" - time="2021-08-10T09:33:54Z" - tooltipplacement="top" - /> - </span> - </div> - </div> - </div> - - <!----> - </div> - - <!----> -</div> -`; diff --git a/spec/frontend/packages_and_registries/package_registry/components/details/version_row_spec.js b/spec/frontend/packages_and_registries/package_registry/components/details/version_row_spec.js index 31e4c68a3f4..0d74b59ae5b 100644 --- a/spec/frontend/packages_and_registries/package_registry/components/details/version_row_spec.js +++ b/spec/frontend/packages_and_registries/package_registry/components/details/version_row_spec.js @@ -1,11 +1,12 @@ -import { GlLink, GlSprintf, GlTruncate } from '@gitlab/ui'; +import { GlIcon, GlLink, GlSprintf, GlTruncate } from '@gitlab/ui'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import { getIdFromGraphQLId } from '~/graphql_shared/utils'; import PackageTags from '~/packages_and_registries/shared/components/package_tags.vue'; import PublishMethod from '~/packages_and_registries/shared/components/publish_method.vue'; import VersionRow from '~/packages_and_registries/package_registry/components/details/version_row.vue'; -import ListItem from '~/vue_shared/components/registry/list_item.vue'; import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue'; +import { PACKAGE_ERROR_STATUS } from '~/packages_and_registries/package_registry/constants'; +import { createMockDirective, getBinding } from 'helpers/vue_mock_directive'; import { packageVersions } from '../../mock_data'; @@ -14,12 +15,12 @@ const packageVersion = packageVersions()[0]; describe('VersionRow', () => { let wrapper; - const findListItem = () => wrapper.findComponent(ListItem); const findLink = () => wrapper.findComponent(GlLink); const findPackageTags = () => wrapper.findComponent(PackageTags); const findPublishMethod = () => wrapper.findComponent(PublishMethod); const findTimeAgoTooltip = () => wrapper.findComponent(TimeAgoTooltip); const findPackageName = () => wrapper.findComponent(GlTruncate); + const findWarningIcon = () => wrapper.findComponent(GlIcon); function createComponent(packageEntity = packageVersion) { wrapper = shallowMountExtended(VersionRow, { @@ -27,10 +28,12 @@ describe('VersionRow', () => { packageEntity, }, stubs: { - ListItem, GlSprintf, GlTruncate, }, + directives: { + GlTooltip: createMockDirective(), + }, }); } @@ -38,16 +41,15 @@ describe('VersionRow', () => { wrapper.destroy(); }); - it('renders', () => { + it('has a link to the version detail', () => { createComponent(); - expect(wrapper.element).toMatchSnapshot(); + expect(findLink().attributes('href')).toBe(`${getIdFromGraphQLId(packageVersion.id)}`); }); - it('has a link to the version detail', () => { + it('lists the package name', () => { createComponent(); - expect(findLink().attributes('href')).toBe(`${getIdFromGraphQLId(packageVersion.id)}`); expect(findLink().text()).toBe(packageVersion.name); }); @@ -74,27 +76,51 @@ describe('VersionRow', () => { expect(findTimeAgoTooltip().props('time')).toBe(packageVersion.createdAt); }); - describe('disabled status', () => { + describe(`when the package is in ${PACKAGE_ERROR_STATUS} status`, () => { beforeEach(() => { createComponent({ ...packageVersion, - status: 'something', + status: PACKAGE_ERROR_STATUS, _links: { webPath: null, }, }); }); - it('disables the list item', () => { - expect(findListItem().props('disabled')).toBe(true); + it('lists the package name', () => { + expect(findPackageName().props('text')).toBe('@gitlab-org/package-15'); }); - it('lists the package name', () => { - expect(findPackageName().props()).toMatchObject({ - text: '@gitlab-org/package-15', + it('does not have a link to navigate to the details page', () => { + expect(findLink().exists()).toBe(false); + }); + + it('has a warning icon', () => { + const icon = findWarningIcon(); + const tooltip = getBinding(icon.element, 'gl-tooltip'); + expect(icon.props('name')).toBe('warning'); + expect(icon.props('ariaLabel')).toBe('Warning'); + expect(tooltip.value).toMatchObject({ + title: 'Invalid Package: failed metadata extraction', + }); + }); + }); + + describe('disabled status', () => { + beforeEach(() => { + createComponent({ + ...packageVersion, + status: 'something', + _links: { + webPath: null, + }, }); }); + it('lists the package name', () => { + expect(findPackageName().props('text')).toBe('@gitlab-org/package-15'); + }); + it('does not have a link to navigate to the details page', () => { expect(findLink().exists()).toBe(false); }); diff --git a/spec/lib/gitlab/database/tables_locker_spec.rb b/spec/lib/gitlab/database/tables_locker_spec.rb index 471a039588b..d74f455eaad 100644 --- a/spec/lib/gitlab/database/tables_locker_spec.rb +++ b/spec/lib/gitlab/database/tables_locker_spec.rb @@ -4,15 +4,13 @@ require 'spec_helper' RSpec.describe Gitlab::Database::TablesLocker, :reestablished_active_record_base, :delete, :silence_stdout, :suppress_gitlab_schemas_validate_connection, feature_category: :pods do - let(:main_connection) { ApplicationRecord.connection } - let(:ci_connection) { Ci::ApplicationRecord.connection } - let!(:user) { create(:user) } - let!(:ci_build) { create(:ci_build) } - let(:detached_partition_table) { '_test_gitlab_main_part_20220101' } + let(:lock_writes_manager) do + instance_double(Gitlab::Database::LockWritesManager, lock_writes: nil, unlock_writes: nil) + end before do - described_class.new.unlock_writes + allow(Gitlab::Database::LockWritesManager).to receive(:new).with(any_args).and_return(lock_writes_manager) end before(:all) do @@ -34,8 +32,6 @@ RSpec.describe Gitlab::Database::TablesLocker, :reestablished_active_record_base end after(:all) do - described_class.new.unlock_writes - drop_detached_partition_sql = <<~SQL DROP TABLE IF EXISTS gitlab_partitions_dynamic._test_gitlab_main_part_20220101 SQL @@ -48,6 +44,54 @@ RSpec.describe Gitlab::Database::TablesLocker, :reestablished_active_record_base end end + shared_examples "lock tables" do |table_schema, database_name| + let(:table_name) do + Gitlab::Database::GitlabSchema + .tables_to_schema.filter_map { |table_name, schema| table_name if schema == table_schema } + .first + end + + let(:database) { database_name } + + it "locks table in schema #{table_schema} and database #{database_name}" do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with( + table_name: table_name, + connection: anything, + database_name: database, + with_retries: true, + logger: anything, + dry_run: anything + ).once.and_return(lock_writes_manager) + expect(lock_writes_manager).to receive(:lock_writes) + + subject + end + end + + shared_examples "unlock tables" do |table_schema, database_name| + let(:table_name) do + Gitlab::Database::GitlabSchema + .tables_to_schema.filter_map { |table_name, schema| table_name if schema == table_schema } + .first + end + + let(:database) { database_name } + + it "unlocks table in schema #{table_schema} and database #{database_name}" do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with( + table_name: table_name, + connection: anything, + database_name: database, + with_retries: true, + logger: anything, + dry_run: anything + ).once.and_return(lock_writes_manager) + expect(lock_writes_manager).to receive(:unlock_writes) + + subject + end + end + context 'when running on single database' do before do skip_if_multiple_databases_are_setup(:ci) @@ -56,17 +100,27 @@ RSpec.describe Gitlab::Database::TablesLocker, :reestablished_active_record_base describe '#lock_writes' do subject { described_class.new.lock_writes } - it 'does not add any triggers to the main schema tables' do - expect { subject }.not_to change { number_of_triggers(main_connection) } - end + it 'does not call Gitlab::Database::LockWritesManager.lock_writes' do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with(any_args).and_return(lock_writes_manager) + expect(lock_writes_manager).not_to receive(:lock_writes) - it 'will be still able to modify tables that belong to the main two schemas' do subject + end - expect do - User.last.touch - Ci::Build.last.touch - end.not_to raise_error + include_examples "unlock tables", :gitlab_main, 'main' + include_examples "unlock tables", :gitlab_ci, 'ci' + include_examples "unlock tables", :gitlab_shared, 'main' + include_examples "unlock tables", :gitlab_internal, 'main' + end + + describe '#unlock_writes' do + subject { described_class.new.lock_writes } + + it 'does call Gitlab::Database::LockWritesManager.unlock_writes' do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with(any_args).and_return(lock_writes_manager) + expect(lock_writes_manager).to receive(:unlock_writes) + + subject end end end @@ -74,112 +128,72 @@ RSpec.describe Gitlab::Database::TablesLocker, :reestablished_active_record_base context 'when running on multiple databases' do before do skip_if_multiple_databases_not_setup(:ci) - - Gitlab::Database::SharedModel.using_connection(ci_connection) do - Postgresql::DetachedPartition.create!( - table_name: detached_partition_table, - drop_after: Time.zone.now - ) - end end describe '#lock_writes' do subject { described_class.new.lock_writes } - it 'still allows writes on the tables with the correct connections' do - User.touch_all - Ci::Build.touch_all - end - - it 'still allows writing to gitlab_shared schema on any connection' do - connections = [main_connection, ci_connection] - connections.each do |connection| - Gitlab::Database::SharedModel.using_connection(connection) do - LooseForeignKeys::DeletedRecord.create!( - fully_qualified_table_name: "public.users", - primary_key_value: 1, - cleanup_attempts: 0 - ) - end - end - end + include_examples "lock tables", :gitlab_ci, 'main' + include_examples "lock tables", :gitlab_main, 'ci' - it 'prevents writes on the main tables on the ci database' do - subject + include_examples "unlock tables", :gitlab_main, 'main' + include_examples "unlock tables", :gitlab_ci, 'ci' + include_examples "unlock tables", :gitlab_shared, 'main' + include_examples "unlock tables", :gitlab_shared, 'ci' + include_examples "unlock tables", :gitlab_internal, 'main' + include_examples "unlock tables", :gitlab_internal, 'ci' + end - expect do - ci_connection.execute("delete from users") - end.to raise_error(ActiveRecord::StatementInvalid, /Table: "users" is write protected/) - end + describe '#unlock_writes' do + subject { described_class.new.unlock_writes } + + include_examples "unlock tables", :gitlab_ci, 'main' + include_examples "unlock tables", :gitlab_main, 'ci' + include_examples "unlock tables", :gitlab_main, 'main' + include_examples "unlock tables", :gitlab_ci, 'ci' + include_examples "unlock tables", :gitlab_shared, 'main' + include_examples "unlock tables", :gitlab_shared, 'ci' + include_examples "unlock tables", :gitlab_internal, 'main' + include_examples "unlock tables", :gitlab_internal, 'ci' + end - it 'prevents writes on the ci tables on the main database' do - subject + context 'when running in dry_run mode' do + subject { described_class.new(dry_run: true).lock_writes } - expect do - main_connection.execute("delete from ci_builds") - end.to raise_error(ActiveRecord::StatementInvalid, /Table: "ci_builds" is write protected/) - end + it 'passes dry_run flag to LockManger' do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with( + table_name: 'users', + connection: anything, + database_name: 'ci', + with_retries: true, + logger: anything, + dry_run: true + ).and_return(lock_writes_manager) + expect(lock_writes_manager).to receive(:lock_writes) - it 'prevents truncating a ci table on the main database' do subject - - expect do - main_connection.execute("truncate ci_build_needs") - end.to raise_error(ActiveRecord::StatementInvalid, /Table: "ci_build_needs" is write protected/) end + end - it 'prevents writes to detached partitions' do - subject + context 'when running on multiple shared databases' do + subject { described_class.new.lock_writes } - expect do - ci_connection.execute("INSERT INTO gitlab_partitions_dynamic.#{detached_partition_table} DEFAULT VALUES") - end.to raise_error(ActiveRecord::StatementInvalid, /Table: "#{detached_partition_table}" is write protected/) + before do + allow(::Gitlab::Database).to receive(:db_config_share_with).and_return(nil) + ci_db_config = Ci::ApplicationRecord.connection_db_config + allow(::Gitlab::Database).to receive(:db_config_share_with).with(ci_db_config).and_return('main') end - context 'when running in dry_run mode' do - subject { described_class.new(dry_run: true).lock_writes } - - it 'allows writes on the main tables on the ci database' do - subject - - expect do - ci_connection.execute("delete from users") - end.not_to raise_error - end + it 'does not lock any tables if the ci database is shared with main database' do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with(any_args).and_return(lock_writes_manager) + expect(lock_writes_manager).not_to receive(:lock_writes) - it 'allows writes on the ci tables on the main database' do - subject - - expect do - main_connection.execute("delete from ci_builds") - end.not_to raise_error - end - end - - context 'when running on multiple shared databases' do - before do - allow(::Gitlab::Database).to receive(:db_config_share_with).and_return(nil) - ci_db_config = Ci::ApplicationRecord.connection_db_config - allow(::Gitlab::Database).to receive(:db_config_share_with).with(ci_db_config).and_return('main') - end - - it 'does not lock any tables if the ci database is shared with main database' do - subject { described_class.new.lock_writes } - - expect do - ApplicationRecord.connection.execute("delete from ci_builds") - Ci::ApplicationRecord.connection.execute("delete from users") - end.not_to raise_error - end + subject end end end context 'when geo database is configured' do - let(:lock_writes_manager) do - instance_double(Gitlab::Database::LockWritesManager, lock_writes: nil, unlock_writes: nil) - end - let(:geo_table) do Gitlab::Database::GitlabSchema .tables_to_schema.filter_map { |table_name, schema| table_name if schema == :gitlab_geo } @@ -190,8 +204,6 @@ RSpec.describe Gitlab::Database::TablesLocker, :reestablished_active_record_base before do skip "Geo database is not configured" unless Gitlab::Database.has_config?(:geo) - - allow(Gitlab::Database::LockWritesManager).to receive(:new).with(any_args).and_return(lock_writes_manager) end it 'does not lock table in geo database' do diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 0425ba3e631..84794b5f6f8 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::CreateService, '#execute' do +RSpec.describe Groups::CreateService, '#execute', feature_category: :subgroups do let!(:user) { create(:user) } let!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } } @@ -78,10 +78,6 @@ RSpec.describe Groups::CreateService, '#execute' do it { is_expected.to be_persisted } - it 'adds an onboarding progress record' do - expect { subject }.to change(Onboarding::Progress, :count).from(0).to(1) - end - context 'with before_commit callback' do it_behaves_like 'has sync-ed traversal_ids' end @@ -107,10 +103,6 @@ RSpec.describe Groups::CreateService, '#execute' do it { is_expected.to be_persisted } - it 'does not add an onboarding progress record' do - expect { subject }.not_to change(Onboarding::Progress, :count).from(0) - end - it_behaves_like 'has sync-ed traversal_ids' end |