Merge branch '219915-cleanup-policy-not-working' into 'master'

Update DeleteTagsService to allow policy to work

See merge request gitlab-org/gitlab!43359

app/services/projects/container_repository/cleanup_tags_service.rb

@@ -25,7 +25,10 @@ module Projects
         tag_names = tags.map(&:name)
 
         Projects::ContainerRepository::DeleteTagsService
-          .new(container_repository.project, current_user, tags: tag_names)
+          .new(container_repository.project,
+               current_user,
+               tags: tag_names,
+               container_expiration_policy: params['container_expiration_policy'])
           .execute(container_repository)
       end
 

app/services/projects/container_repository/delete_tags_service.rb

@@ -7,7 +7,10 @@ module Projects
 
       def execute(container_repository)
         @container_repository = container_repository
-        return error('access denied') unless can?(current_user, :destroy_container_image, project)
+
+        unless params[:container_expiration_policy]
+          return error('access denied') unless can?(current_user, :destroy_container_image, project)
+        end
 
         @tag_names = params[:tags]
         return error('not tags specified') if @tag_names.blank?

changelogs/unreleased/219915-cleanup-policy-not-working.yml

+---
+title: Fix bug to allow container cleanup policies to properly run
+merge_request: 43359
+author:
+type: fixed

spec/services/projects/container_repository/cleanup_tags_service_spec.rb

@@ -245,7 +245,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
         end
 
         it 'succeeds without a user' do
-          expect_delete(%w(Bb Ba C))
+          expect_delete(%w(Bb Ba C), container_expiration_policy: true)
 
           is_expected.to include(status: :success, deleted: %w(Bb Ba C))
         end
@@ -287,10 +287,10 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
     end
   end
 
-  def expect_delete(tags)
+  def expect_delete(tags, container_expiration_policy: nil)
     expect(Projects::ContainerRepository::DeleteTagsService)
       .to receive(:new)
-      .with(repository.project, user, tags: tags)
+      .with(repository.project, user, tags: tags, container_expiration_policy: container_expiration_policy)
       .and_call_original
 
     expect_any_instance_of(Projects::ContainerRepository::DeleteTagsService)

spec/services/projects/container_repository/delete_tags_service_spec.rb

@@ -85,6 +85,41 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
     end
   end
 
+  RSpec.shared_examples 'supporting fast delete' do
+    context 'when the registry supports fast delete' do
+      before do
+        allow(repository.client).to receive(:supports_tag_delete?).and_return(true)
+      end
+
+      it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::Gitlab::DeleteTagsService
+
+      it_behaves_like 'handling invalid params'
+
+      context 'with the real service' do
+        before do
+          stub_delete_reference_requests(tags)
+          expect_delete_tag_by_names(tags)
+        end
+
+        it { is_expected.to include(status: :success) }
+
+        it_behaves_like 'logging a success response'
+      end
+
+      context 'with a timeout error' do
+        before do
+          expect_next_instance_of(::Projects::ContainerRepository::Gitlab::DeleteTagsService) do |delete_service|
+            expect(delete_service).to receive(:delete_tags).and_raise(::Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError)
+          end
+        end
+
+        it { is_expected.to include(status: :error, message: 'timeout while deleting tags') }
+
+        it_behaves_like 'logging an error response', message: 'timeout while deleting tags'
+      end
+    end
+  end
+
   describe '#execute' do
     let(:tags) { %w[A Ba] }
 
@@ -103,47 +138,30 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
         project.add_developer(user)
       end
 
-      context 'when the registry supports fast delete' do
+      it_behaves_like 'supporting fast delete'
+
+      context 'when the registry does not support fast delete' do
         before do
-          allow(repository.client).to receive(:supports_tag_delete?).and_return(true)
+          allow(repository.client).to receive(:supports_tag_delete?).and_return(false)
         end
 
-        it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::Gitlab::DeleteTagsService
+        it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::ThirdParty::DeleteTagsService
 
         it_behaves_like 'handling invalid params'
+      end
+    end
 
-        context 'with the real service' do
-          before do
-            stub_delete_reference_requests(tags)
-            expect_delete_tag_by_names(tags)
-          end
-
-          it { is_expected.to include(status: :success) }
-
-          it_behaves_like 'logging a success response'
-        end
-
-        context 'with a timeout error' do
-          before do
-            expect_next_instance_of(::Projects::ContainerRepository::Gitlab::DeleteTagsService) do |delete_service|
-              expect(delete_service).to receive(:delete_tags).and_raise(::Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError)
-            end
-          end
-
-          it { is_expected.to include(status: :error, message: 'timeout while deleting tags') }
+    context 'without user' do
+      let_it_be(:user) { nil }
 
-          it_behaves_like 'logging an error response', message: 'timeout while deleting tags'
-        end
+      context 'when not run by a cleanup policy' do
+        it { is_expected.to include(status: :error) }
       end
 
-      context 'when the registry does not support fast delete' do
-        before do
-          allow(repository.client).to receive(:supports_tag_delete?).and_return(false)
-        end
-
-        it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::ThirdParty::DeleteTagsService
+      context 'when run by a cleanup policy' do
+        let(:params) { { tags: tags, container_expiration_policy: true } }
 
-        it_behaves_like 'handling invalid params'
+        it_behaves_like 'supporting fast delete'
       end
     end
   end