diff options
11 files changed, 144 insertions, 13 deletions
diff --git a/app/assets/javascripts/registry/components/collapsible_container.vue b/app/assets/javascripts/registry/components/collapsible_container.vue index 4116c4a0489..cea409aa130 100644 --- a/app/assets/javascripts/registry/components/collapsible_container.vue +++ b/app/assets/javascripts/registry/components/collapsible_container.vue @@ -6,6 +6,7 @@ import tooltip from '../../vue_shared/directives/tooltip'; import tableRegistry from './table_registry.vue'; import { errorMessages, errorMessagesTypes } from '../constants'; + import { __ } from '../../locale'; export default { name: 'CollapsibeContainerRegisty', @@ -46,7 +47,10 @@ handleDeleteRepository() { this.deleteRepo(this.repo) - .then(() => this.fetchRepos()) + .then(() => { + Flash(__('This container registry has been scheduled for deletion.'), 'notice'); + this.fetchRepos(); + }) .catch(() => this.showError(errorMessagesTypes.DELETE_REPO)); }, diff --git a/app/controllers/projects/registry/repositories_controller.rb b/app/controllers/projects/registry/repositories_controller.rb index 32c0fc6d14a..ef0433795f4 100644 --- a/app/controllers/projects/registry/repositories_controller.rb +++ b/app/controllers/projects/registry/repositories_controller.rb @@ -18,14 +18,10 @@ module Projects end def destroy - if image.destroy - respond_to do |format| - format.json { head :no_content } - end - else - respond_to do |format| - format.json { head :bad_request } - end + DeleteContainerRepositoryWorker.perform_async(current_user.id, image.id) + + respond_to do |format| + format.json { head :no_content } end end @@ -41,10 +37,10 @@ module Projects # Needed to maintain a backwards compatibility. # def ensure_root_container_repository! - ContainerRegistry::Path.new(@project.full_path).tap do |path| + ::ContainerRegistry::Path.new(@project.full_path).tap do |path| break if path.has_repository? - ContainerRepository.build_from_path(path).tap do |repository| + ::ContainerRepository.build_from_path(path).tap do |repository| repository.save! if repository.has_tags? end end diff --git a/app/services/projects/container_repository/destroy_service.rb b/app/services/projects/container_repository/destroy_service.rb new file mode 100644 index 00000000000..a8e7eab6068 --- /dev/null +++ b/app/services/projects/container_repository/destroy_service.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Projects + module ContainerRepository + class DestroyService < BaseService + def execute(container_repository) + return false unless can?(current_user, :update_container_image, project) + + container_repository.destroy + end + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index ae9dc8d4857..1eeb972cee9 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -87,6 +87,7 @@ - authorized_projects - background_migration - create_gpg_signature +- delete_container_repository - delete_merged_branches - delete_user - email_receiver diff --git a/app/workers/delete_container_repository_worker.rb b/app/workers/delete_container_repository_worker.rb new file mode 100644 index 00000000000..b703530d3a0 --- /dev/null +++ b/app/workers/delete_container_repository_worker.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class DeleteContainerRepositoryWorker + include ApplicationWorker + include ExclusiveLeaseGuard + + LEASE_TIMEOUT = 1.hour + + attr_reader :container_repository + + def perform(current_user_id, container_repository_id) + current_user = User.find_by(id: current_user_id) + @container_repository = ContainerRepository.find_by(id: container_repository_id) + project = container_repository&.project + + return unless current_user && container_repository && project + + # If a user accidentally attempts to delete the same container registry in quick succession, + # this can lead to orphaned tags. + try_obtain_lease do + Projects::ContainerRepository::DestroyService.new(project, current_user).execute(container_repository) + end + end + + # For ExclusiveLeaseGuard concern + def lease_key + @lease_key ||= "container_repository:delete:#{container_repository.id}" + end + + # For ExclusiveLeaseGuard concern + def lease_timeout + LEASE_TIMEOUT + end +end diff --git a/changelogs/unreleased/sh-delete-container-registry-async.yml b/changelogs/unreleased/sh-delete-container-registry-async.yml new file mode 100644 index 00000000000..dfe0e812112 --- /dev/null +++ b/changelogs/unreleased/sh-delete-container-registry-async.yml @@ -0,0 +1,5 @@ +--- +title: Delete a container registry asynchronously +merge_request: 21553 +author: +type: fixed diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index dc49403aca1..0e723cdeb9c 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -46,6 +46,7 @@ - [project_service, 1] - [delete_user, 1] - [todos_destroyer, 1] + - [delete_container_repository, 1] - [delete_merged_branches, 1] - [authorized_projects, 1] - [expire_build_instance_artifacts, 1] diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8b19e2bec61..d4d7913e65a 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5832,6 +5832,9 @@ msgstr "" msgid "This branch has changed since you started editing. Would you like to create a new branch?" msgstr "" +msgid "This container registry has been scheduled for deletion." +msgstr "" + msgid "This diff is collapsed." msgstr "" diff --git a/spec/controllers/projects/registry/repositories_controller_spec.rb b/spec/controllers/projects/registry/repositories_controller_spec.rb index 17769a14def..d11e42b411b 100644 --- a/spec/controllers/projects/registry/repositories_controller_spec.rb +++ b/spec/controllers/projects/registry/repositories_controller_spec.rb @@ -86,9 +86,10 @@ describe Projects::Registry::RepositoriesController do stub_container_registry_tags(repository: :any, tags: []) end - it 'deletes a repository' do - expect { delete_repository(repository) }.to change { ContainerRepository.all.count }.by(-1) + it 'schedules a job to delete a repository' do + expect(DeleteContainerRepositoryWorker).to receive(:perform_async).with(user.id, repository.id) + delete_repository(repository) expect(response).to have_gitlab_http_status(:no_content) end end diff --git a/spec/services/projects/container_repository/destroy_service_spec.rb b/spec/services/projects/container_repository/destroy_service_spec.rb new file mode 100644 index 00000000000..307ccc88865 --- /dev/null +++ b/spec/services/projects/container_repository/destroy_service_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::ContainerRepository::DestroyService do + set(:user) { create(:user) } + set(:project) { create(:project, :private) } + + subject { described_class.new(project, user) } + + before do + stub_container_registry_config(enabled: true) + end + + context 'when user does not have access to registry' do + let!(:repository) { create(:container_repository, :root, project: project) } + + it 'does not delete a repository' do + expect { subject.execute(repository) }.not_to change { ContainerRepository.all.count } + end + end + + context 'when user has access to registry' do + before do + project.add_developer(user) + end + + context 'when root container repository exists' do + let!(:repository) { create(:container_repository, :root, project: project) } + + before do + stub_container_registry_tags(repository: :any, tags: []) + end + + it 'deletes the repository' do + expect { described_class.new(project, user).execute(repository) }.to change { ContainerRepository.all.count }.by(-1) + end + end + end +end diff --git a/spec/workers/delete_container_repository_worker_spec.rb b/spec/workers/delete_container_repository_worker_spec.rb new file mode 100644 index 00000000000..8c40611a959 --- /dev/null +++ b/spec/workers/delete_container_repository_worker_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DeleteContainerRepositoryWorker do + let(:registry) { create(:container_repository) } + let(:project) { registry.project } + let(:user) { project.owner } + + subject { described_class.new } + + describe '#perform' do + it 'executes the destroy service' do + service = instance_double(Projects::ContainerRepository::DestroyService) + expect(service).to receive(:execute) + expect(Projects::ContainerRepository::DestroyService).to receive(:new).with(project, user).and_return(service) + + subject.perform(user.id, registry.id) + end + + it 'does not raise error when user could not be found' do + expect do + subject.perform(-1, registry.id) + end.not_to raise_error + end + + it 'does not raise error when registry could not be found' do + expect do + subject.perform(user.id, -1) + end.not_to raise_error + end + end +end |