Merge branch 'delete_tags_service_logging_enhancement' into 'master'

Delete tags service logging enhancement

See merge request gitlab-org/gitlab!46079

app/services/projects/container_repository/delete_tags_service.rb

@@ -36,11 +36,11 @@ module Projects
       def log_response(response)
         log_data = LOG_DATA_BASE.merge(
           container_repository_id: @container_repository.id,
-          message: 'deleted tags'
-        )
+          message: 'deleted tags',
+          deleted_tags_count: response[:deleted]&.size
+        ).compact
 
         if response[:status] == :success
-          log_data[:deleted_tags_count] = response[:deleted].size
           log_info(log_data)
         else
           log_data[:message] = response[:message]

app/services/projects/container_repository/gitlab/delete_tags_service.rb

@@ -14,6 +14,7 @@ module Projects
         def initialize(container_repository, tag_names)
           @container_repository = container_repository
           @tag_names = tag_names
+          @deleted_tags = []
         end
 
         # Delete tags by name with a single DELETE request. This is only supported
@@ -25,7 +26,7 @@ module Projects
           delete_tags
         rescue TimeoutError => e
           ::Gitlab::ErrorTracking.track_exception(e, tags_count: @tag_names&.size, container_repository_id: @container_repository&.id)
-          error('timeout while deleting tags')
+          error('timeout while deleting tags', nil, pass_back: { deleted: @deleted_tags })
         end
 
         private
@@ -33,13 +34,15 @@ module Projects
         def delete_tags
           start_time = Time.zone.now
 
-          deleted_tags = @tag_names.select do |name|
+          @tag_names.each do |name|
             raise TimeoutError if timeout?(start_time)
 
-            @container_repository.delete_tag_by_name(name)
+            if @container_repository.delete_tag_by_name(name)
+              @deleted_tags.append(name)
+            end
           end
 
-          deleted_tags.any? ? success(deleted: deleted_tags) : error('could not delete tags')
+          @deleted_tags.any? ? success(deleted: @deleted_tags) : error('could not delete tags')
         end
 
         def timeout?(start_time)

changelogs/unreleased/delete_tags_service_logging_enhancement.yml

+---
+title: Improving Container Registry Delete Tags Service to log number of successfully
+  deleted tags even if deletion process was interrupted by a timeout
+merge_request: 46079
+author: Maksim Stankevic, @maksimstankevic
+type: changed

spec/services/projects/container_repository/delete_tags_service_spec.rb

@@ -27,13 +27,17 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
     end
   end
 
-  RSpec.shared_examples 'logging an error response' do |message: 'could not delete tags'|
+  RSpec.shared_examples 'logging an error response' do |message: 'could not delete tags', extra_log: {}|
     it 'logs an error message' do
-      expect(service).to receive(:log_error).with(
-        service_class: 'Projects::ContainerRepository::DeleteTagsService',
-        message: message,
-        container_repository_id: repository.id
-      )
+      log_data = {
+          service_class: 'Projects::ContainerRepository::DeleteTagsService',
+          message: message,
+          container_repository_id: repository.id
+      }
+
+      log_data.merge!(extra_log) if extra_log.any?
+
+      expect(service).to receive(:log_error).with(log_data)
 
       subject
     end
@@ -115,7 +119,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
 
         it { is_expected.to include(status: :error, message: 'timeout while deleting tags') }
 
-        it_behaves_like 'logging an error response', message: 'timeout while deleting tags'
+        it_behaves_like 'logging an error response', message: 'timeout while deleting tags', extra_log: { deleted_tags_count: 0 }
       end
     end
   end

spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb

@@ -67,7 +67,7 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do
               stub_delete_reference_requests('A' => 200)
             end
 
-            it { is_expected.to include(status: :error, message: 'timeout while deleting tags') }
+            it { is_expected.to eq(status: :error, message: 'timeout while deleting tags', deleted: ['A']) }
 
             it 'tracks the exception' do
               expect(::Gitlab::ErrorTracking)