From cb2ce8452fe2e9e156add5ccfe8fd2ec5cda9ace Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 4 Apr 2017 12:57:38 +0200 Subject: [PATCH] Remove legacy registry tags when deleting a project --- app/models/container_repository.rb | 4 ++ app/services/projects/destroy_service.rb | 23 ++++++- spec/models/container_repository_spec.rb | 18 +++++ .../services/projects/destroy_service_spec.rb | 68 ++++++++++++++----- 4 files changed, 92 insertions(+), 21 deletions(-) diff --git a/app/models/container_repository.rb b/app/models/container_repository.rb index 36158d75ae8..463eb5b7d69 100644 --- a/app/models/container_repository.rb +++ b/app/models/container_repository.rb @@ -70,4 +70,8 @@ class ContainerRepository < ActiveRecord::Base def self.create_from_path!(path) build_from_path(path).tap(&:save!) end + + def self.build_root_repository(project) + self.new(project: project, name: '') + end end diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 4e1964f79dd..a47e74ba9b0 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -29,15 +29,20 @@ module Projects Project.transaction do project.team.truncate - project.destroy! + + unless remove_legacy_registry_tags + raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.') + end unless remove_repository(repo_path) - raise_error('Failed to remove project repository. Please try again or contact administrator') + raise_error('Failed to remove project repository. Please try again or contact administrator.') end unless remove_repository(wiki_path) - raise_error('Failed to remove wiki repository. Please try again or contact administrator') + raise_error('Failed to remove wiki repository. Please try again or contact administrator.') end + + project.destroy! end log_info("Project \"#{project.path_with_namespace}\" was removed") @@ -64,6 +69,18 @@ module Projects end end + ## + # This method makes sure that we correctly remove registry tags + # for legacy image repository (when repository path equals project path). + # + def remove_legacy_registry_tags + return true unless Gitlab.config.registry.enabled + + ContainerRepository.build_root_repository(project).tap do |repository| + return repository.delete_tags! if repository.has_tags? + end + end + def raise_error(message) raise DestroyError.new(message) end diff --git a/spec/models/container_repository_spec.rb b/spec/models/container_repository_spec.rb index 1a29cc9a096..3e6082ec326 100644 --- a/spec/models/container_repository_spec.rb +++ b/spec/models/container_repository_spec.rb @@ -195,4 +195,22 @@ describe ContainerRepository do end end end + + describe '.build_root_repository' do + let(:repository) do + described_class.build_root_repository(project) + end + + it 'fabricates a root repository object' do + expect(repository).to be_root_repository + end + + it 'assignes it to the correct project' do + expect(repository.project).to eq project + end + + it 'does not persist it' do + expect(repository).not_to be_persisted + end + end end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 5ef07c8275e..4b8589b2736 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -7,6 +7,11 @@ describe Projects::DestroyService, services: true do let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") } let!(:async) { false } # execute or async_execute + before do + stub_container_registry_config(enabled: true) + stub_container_registry_tags(repository: :any, tags: []) + end + shared_examples 'deleting the project' do it 'deletes the project' do expect(Project.unscoped.all).not_to include(project) @@ -89,37 +94,64 @@ describe Projects::DestroyService, services: true do it_behaves_like 'deleting the project with pipeline and build' end - context 'container registry' do - let(:container_repository) { create(:container_repository) } + describe 'container registry' do + context 'when there are regular container repositories' do + let(:container_repository) { create(:container_repository) } - before do - stub_container_registry_config(enabled: true) - stub_container_registry_tags(repository: :any, tags: ['tag']) - project.container_repositories << container_repository - end + before do + stub_container_registry_tags(repository: project.full_path + '/image', + tags: ['tag']) + project.container_repositories << container_repository + end - context 'images deletion succeeds' do - it do - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_return(true) + context 'when image repository deletion succeeds' do + it 'removes tags' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(true) - destroy_project(project, user, {}) + destroy_project(project, user) + end + end + + context 'when image repository deletion fails' do + it 'raises an exception' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(false) + + expect{ destroy_project(project, user) } + .to raise_error(ActiveRecord::RecordNotDestroyed) + end end end - context 'images deletion fails' do + context 'when there are tags for legacy root repository' do before do - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_return(false) + stub_container_registry_tags(repository: project.full_path, + tags: ['tag']) + end + + context 'when image repository tags deletion succeeds' do + it 'removes tags' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(true) + + destroy_project(project, user) + end end - subject { destroy_project(project, user, {}) } + context 'when image repository tags deletion fails' do + it 'raises an exception' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(false) - it { expect{subject}.to raise_error(ActiveRecord::RecordNotDestroyed) } + expect { destroy_project(project, user) } + .to raise_error(Projects::DestroyService::DestroyError) + end + end end end - def destroy_project(project, user, params) + def destroy_project(project, user, params = {}) if async Projects::DestroyService.new(project, user, params).async_execute else -- GitLab