Remove legacy registry tags when deleting a project

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