Improve code related to removing container image tags

5 files changed, 36 insertions, 34 deletions

diff --git a/app/models/container_repository.rb b/app/models/container_repository.rb
index 5663b3db92f..052d93c3bdc 100644
--- a/app/models/container_repository.rb
+++ b/app/models/container_repository.rb
@@ -45,15 +45,14 @@ class ContainerRepository < ActiveRecord::Base
   # TODO, specs needed
   #
   def has_tags?
-    tags.any?
+    tags.to_a.any?
   end
 
-  # TODO, add bang to this method
-  #
-  def delete_tags
-    return unless tags
+  def delete_tags!
+    return unless has_tags?
+
+    digests = tags.map { |tag| tag.digest }.to_set
 
-    digests = tags.map {|tag| tag.digest }.to_set
     digests.all? do |digest|
       client.delete_repository_tag(self.path, digest)
     end
diff --git a/spec/factories/container_repositories.rb b/spec/factories/container_repositories.rb
index 295b3596ee9..4919a03cdf2 100644
--- a/spec/factories/container_repositories.rb
+++ b/spec/factories/container_repositories.rb
@@ -8,13 +8,21 @@ FactoryGirl.define do
     end
 
     after(:build) do |repository, evaluator|
-      if evaluator.tags.any?
+      next if evaluator.tags.to_a.none?
+
+      allow(repository.client)
+        .to receive(:repository_tags)
+        .and_return({
+          'name' => repository.path,
+          'tags' => evaluator.tags
+        })
+
+      evaluator.tags.each do |tag|
         allow(repository.client)
-          .to receive(:repository_tags)
-          .and_return({
-            name: repository.path,
-            tags: evaluator.tags
-          })
+          .to receive(:repository_tag_digest)
+          .with(repository.path, tag)
+          .and_return('sha256:4c8e63ca4cb663ce6c688cb06f1c3' \
+                      '72b088dac5b6d7ad7d49cd620d85cf72a15')
       end
     end
   end
diff --git a/spec/features/container_registry_spec.rb b/spec/features/container_registry_spec.rb
index 73b22fa1b7d..530e6af92d3 100644
--- a/spec/features/container_registry_spec.rb
+++ b/spec/features/container_registry_spec.rb
@@ -38,7 +38,8 @@ describe "Container Registry" do
     end
 
     it do
-      expect_any_instance_of(ContainerRepository).to receive(:delete_tags).and_return(true)
+      expect_any_instance_of(ContainerRepository)
+        .to receive(:delete_tags!).and_return(true)
 
       click_on 'Remove image'
     end
diff --git a/spec/models/container_repository_spec.rb b/spec/models/container_repository_spec.rb
index 296b9e713a8..92dccf76d71 100644
--- a/spec/models/container_repository_spec.rb
+++ b/spec/models/container_repository_spec.rb
@@ -57,38 +57,30 @@ describe ContainerRepository do
     it { is_expected.not_to be_empty }
   end
 
-  # TODO, improve these specs
-  #
-  describe '#delete_tags' do
-    let(:tag) { ContainerRegistry::Tag.new(container_repository, 'tag') }
-
-    before do
-      allow(container_repository).to receive(:tags).twice.and_return([tag])
-      allow(tag).to receive(:digest)
-        .and_return('sha256:4c8e63ca4cb663ce6c688cb06f1c3672a172b088dac5b6d7ad7d49cd620d85cf')
+  describe '#delete_tags!' do
+    let(:container_repository) do
+      create(:container_repository, name: 'my_image',
+                                    tags: %w[latest rc1],
+                                    project: project)
     end
 
     context 'when action succeeds' do
-      before do
-        allow(container_repository.client)
+      it 'returns status that indicates success' do
+        expect(container_repository.client)
           .to receive(:delete_repository_tag)
           .and_return(true)
-      end
 
-      it 'returns status that indicates success' do
-        expect(container_repository.delete_tags).to be_truthy
+        expect(container_repository.delete_tags!).to be_truthy
       end
     end
 
     context 'when action fails' do
-      before do
-        allow(container_repository.client)
+      it 'returns status that indicates failure' do
+        expect(container_repository.client)
           .to receive(:delete_repository_tag)
           .and_return(false)
-      end
 
-      it 'returns status that indicates failure' do
-        expect(container_repository.delete_tags).to be_falsey
+        expect(container_repository.delete_tags!).to be_falsey
       end
     end
   end
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index 44e0286350b..193ccd17282 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -100,7 +100,8 @@ describe Projects::DestroyService, services: true do
 
     context 'images deletion succeeds' do
       it do
-        expect_any_instance_of(ContainerRepository).to receive(:delete_tags).and_return(true)
+        expect_any_instance_of(ContainerRepository)
+          .to receive(:delete_tags!).and_return(true)
 
         destroy_project(project, user, {})
       end
@@ -108,7 +109,8 @@ describe Projects::DestroyService, services: true do
 
     context 'images deletion fails' do
       before do
-        expect_any_instance_of(ContainerRepository).to receive(:delete_tags).and_return(false)
+        expect_any_instance_of(ContainerRepository)
+          .to receive(:delete_tags!).and_return(false)
       end
 
       subject { destroy_project(project, user, {}) }