Merge branch '340278-improve-cleanup-tags-service-readability' into 'master'

Improve readability of the cleanup tags service

See merge request gitlab-org/gitlab!71293

app/services/container_expiration_policies/cleanup_service.rb

@@ -24,8 +24,8 @@ module ContainerExpirationPolicies
 
       begin
         service_result = Projects::ContainerRepository::CleanupTagsService
-                           .new(project, nil, policy_params.merge('container_expiration_policy' => true))
-                           .execute(repository)
+                           .new(repository, nil, policy_params.merge('container_expiration_policy' => true))
+                           .execute
       rescue StandardError
         repository.cleanup_unfinished!
 

app/services/projects/container_repository/cleanup_tags_service.rb

@@ -2,148 +2,152 @@
 
 module Projects
   module ContainerRepository
-    class CleanupTagsService < BaseService
+    class CleanupTagsService
+      include BaseServiceUtility
       include ::Gitlab::Utils::StrongMemoize
 
-      def execute(container_repository)
-        return error('access denied') unless can_destroy?
-        return error('invalid regex') unless valid_regex?
+      def initialize(container_repository, user = nil, params = {})
+        @container_repository = container_repository
+        @current_user = user
+        @params = params.dup
 
-        tags = container_repository.tags
-        original_size = tags.size
+        @project = container_repository.project
+        @tags = container_repository.tags
+        tags_size = @tags.size
+        @counts = {
+          original_size: tags_size,
+          cached_tags_count: 0
+        }
+      end
 
-        tags = without_latest(tags)
-        tags = filter_by_name(tags)
+      def execute
+        return error('access denied') unless can_destroy?
+        return error('invalid regex') unless valid_regex?
 
-        before_truncate_size = tags.size
-        tags = truncate(tags)
-        after_truncate_size = tags.size
+        filter_out_latest
+        filter_by_name
 
-        cached_tags_count = populate_tags_from_cache(container_repository, tags) || 0
+        truncate
+        populate_from_cache
 
-        tags = filter_keep_n(container_repository, tags)
-        tags = filter_by_older_than(container_repository, tags)
+        filter_keep_n
+        filter_by_older_than
 
-        delete_tags(container_repository, tags).tap do |result|
-          result[:original_size] = original_size
-          result[:before_truncate_size] = before_truncate_size
-          result[:after_truncate_size] = after_truncate_size
-          result[:cached_tags_count] = cached_tags_count
-          result[:before_delete_size] = tags.size
+        delete_tags.merge(@counts).tap do |result|
+          result[:before_delete_size] = @tags.size
           result[:deleted_size] = result[:deleted]&.size
 
-          result[:status] = :error if before_truncate_size != after_truncate_size
+          result[:status] = :error if @counts[:before_truncate_size] != @counts[:after_truncate_size]
         end
       end
 
       private
 
-      def delete_tags(container_repository, tags)
-        return success(deleted: []) unless tags.any?
-
-        tag_names = tags.map(&:name)
+      def delete_tags
+        return success(deleted: []) unless @tags.any?
 
         service = Projects::ContainerRepository::DeleteTagsService.new(
-          container_repository.project,
-          current_user,
-          tags: tag_names,
-          container_expiration_policy: params['container_expiration_policy']
+          @project,
+          @current_user,
+          tags: @tags.map(&:name),
+          container_expiration_policy: container_expiration_policy
         )
 
-        service.execute(container_repository)
+        service.execute(@container_repository)
       end
 
-      def without_latest(tags)
-        tags.reject(&:latest?)
+      def filter_out_latest
+        @tags.reject!(&:latest?)
       end
 
-      def order_by_date(tags)
+      def order_by_date
         now = DateTime.current
-        tags.sort_by { |tag| tag.created_at || now }.reverse
+        @tags.sort_by! { |tag| tag.created_at || now }
+             .reverse!
       end
 
-      def filter_by_name(tags)
-        regex_delete = ::Gitlab::UntrustedRegexp.new("\\A#{params['name_regex_delete'] || params['name_regex']}\\z")
-        regex_retain = ::Gitlab::UntrustedRegexp.new("\\A#{params['name_regex_keep']}\\z")
+      def filter_by_name
+        regex_delete = ::Gitlab::UntrustedRegexp.new("\\A#{name_regex_delete || name_regex}\\z")
+        regex_retain = ::Gitlab::UntrustedRegexp.new("\\A#{name_regex_keep}\\z")
 
-        tags.select do |tag|
+        @tags.select! do |tag|
           # regex_retain will override any overlapping matches by regex_delete
           regex_delete.match?(tag.name) && !regex_retain.match?(tag.name)
         end
       end
 
-      def filter_keep_n(container_repository, tags)
-        return tags unless params['keep_n']
+      def filter_keep_n
+        return unless keep_n
 
-        tags = order_by_date(tags)
-        cache_tags(container_repository, tags.first(keep_n))
-        tags.drop(keep_n)
+        order_by_date
+        cache_tags(@tags.first(keep_n_as_integer))
+        @tags = @tags.drop(keep_n_as_integer)
       end
 
-      def filter_by_older_than(container_repository, tags)
-        return tags unless older_than
+      def filter_by_older_than
+        return unless older_than
 
         older_than_timestamp = older_than_in_seconds.ago
 
-        tags, tags_to_keep = tags.partition do |tag|
+        @tags, tags_to_keep = @tags.partition do |tag|
           tag.created_at && tag.created_at < older_than_timestamp
         end
 
-        cache_tags(container_repository, tags_to_keep)
-
-        tags
+        cache_tags(tags_to_keep)
       end
 
       def can_destroy?
-        return true if params['container_expiration_policy']
+        return true if container_expiration_policy
 
-        can?(current_user, :destroy_container_image, project)
+        can?(@current_user, :destroy_container_image, @project)
       end
 
       def valid_regex?
         %w(name_regex_delete name_regex name_regex_keep).each do |param_name|
-          regex = params[param_name]
+          regex = @params[param_name]
           ::Gitlab::UntrustedRegexp.new(regex) unless regex.blank?
         end
         true
       rescue RegexpError => e
-        ::Gitlab::ErrorTracking.log_exception(e, project_id: project.id)
+        ::Gitlab::ErrorTracking.log_exception(e, project_id: @project.id)
         false
       end
 
-      def truncate(tags)
-        return tags unless throttling_enabled?
-        return tags if max_list_size == 0
+      def truncate
+        @counts[:before_truncate_size] = @tags.size
+        @counts[:after_truncate_size] = @tags.size
+
+        return unless throttling_enabled?
+        return if max_list_size == 0
 
         # truncate the list to make sure that after the #filter_keep_n
         # execution, the resulting list will be max_list_size
-        truncated_size = max_list_size + keep_n
+        truncated_size = max_list_size + keep_n_as_integer
 
-        return tags if tags.size <= truncated_size
+        return if @tags.size <= truncated_size
 
-        tags.sample(truncated_size)
+        @tags = @tags.sample(truncated_size)
+        @counts[:after_truncate_size] = @tags.size
       end
 
-      def populate_tags_from_cache(container_repository, tags)
-        cache(container_repository).populate(tags) if caching_enabled?(container_repository)
+      def populate_from_cache
+        @counts[:cached_tags_count] = cache.populate(@tags) if caching_enabled?
       end
 
-      def cache_tags(container_repository, tags)
-        cache(container_repository).insert(tags, older_than_in_seconds) if caching_enabled?(container_repository)
+      def cache_tags(tags)
+        cache.insert(tags, older_than_in_seconds) if caching_enabled?
       end
 
-      def cache(container_repository)
-        # TODO Implement https://gitlab.com/gitlab-org/gitlab/-/issues/340277 to avoid passing
-        # the container repository parameter which is bad for a memoized function
+      def cache
         strong_memoize(:cache) do
-          ::Projects::ContainerRepository::CacheTagsCreatedAtService.new(container_repository)
+          ::Projects::ContainerRepository::CacheTagsCreatedAtService.new(@container_repository)
         end
       end
 
-      def caching_enabled?(container_repository)
-        params['container_expiration_policy'] &&
+      def caching_enabled?
+        container_expiration_policy &&
           older_than.present? &&
-          Feature.enabled?(:container_registry_expiration_policies_caching, container_repository.project)
+          Feature.enabled?(:container_registry_expiration_policies_caching, @project)
       end
 
       def throttling_enabled?
@@ -155,7 +159,11 @@ module Projects
       end
 
       def keep_n
-        params['keep_n'].to_i
+        @params['keep_n']
+      end
+
+      def keep_n_as_integer
+        keep_n.to_i
       end
 
       def older_than_in_seconds
@@ -165,7 +173,23 @@ module Projects
       end
 
       def older_than
-        params['older_than']
+        @params['older_than']
+      end
+
+      def name_regex_delete
+        @params['name_regex_delete']
+      end
+
+      def name_regex
+        @params['name_regex']
+      end
+
+      def name_regex_keep
+        @params['name_regex_keep']
+      end
+
+      def container_expiration_policy
+        @params['container_expiration_policy']
       end
     end
   end

app/workers/cleanup_container_repository_worker.rb

@@ -28,8 +28,8 @@ class CleanupContainerRepositoryWorker
     end
 
     result = Projects::ContainerRepository::CleanupTagsService
-      .new(project, current_user, params)
-      .execute(container_repository)
+      .new(container_repository, current_user, params)
+      .execute
 
     if run_by_container_expiration_policy? && result[:status] == :success
       container_repository.reset_expiration_policy_started_at!

spec/services/container_expiration_policies/cleanup_service_spec.rb

@@ -24,8 +24,8 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
 
       it 'completely clean up the repository' do
         expect(Projects::ContainerRepository::CleanupTagsService)
-          .to receive(:new).with(project, nil, cleanup_tags_service_params).and_return(cleanup_tags_service)
-        expect(cleanup_tags_service).to receive(:execute).with(repository).and_return(status: :success)
+          .to receive(:new).with(repository, nil, cleanup_tags_service_params).and_return(cleanup_tags_service)
+        expect(cleanup_tags_service).to receive(:execute).and_return(status: :success)
 
         response = subject
 

spec/services/projects/container_repository/cleanup_tags_service_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_
   let_it_be(:project, reload: true) { create(:project, :private) }
 
   let(:repository) { create(:container_repository, :root, project: project) }
-  let(:service) { described_class.new(project, user, params) }
+  let(:service) { described_class.new(repository, user, params) }
   let(:tags) { %w[latest A Ba Bb C D E] }
 
   before do
@@ -39,7 +39,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_
   end
 
   describe '#execute' do
-    subject { service.execute(repository) }
+    subject { service.execute }
 
     shared_examples 'reading and removing tags' do |caching_enabled: true|
       context 'when no params are specified' do

spec/workers/cleanup_container_repository_worker_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_stat
 
       it 'executes the destroy service' do
         expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new)
-          .with(project, user, params.merge('container_expiration_policy' => false))
+          .with(repository, user, params.merge('container_expiration_policy' => false))
           .and_return(service)
         expect(service).to receive(:execute)
 
@@ -49,7 +49,7 @@ RSpec.describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_stat
         expect(repository).to receive(:start_expiration_policy!).and_call_original
         expect(repository).to receive(:reset_expiration_policy_started_at!).and_call_original
         expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new)
-          .with(project, nil, params.merge('container_expiration_policy' => true))
+          .with(repository, nil, params.merge('container_expiration_policy' => true))
           .and_return(service)
 
         expect(service).to receive(:execute).and_return(status: :success)
@@ -62,7 +62,7 @@ RSpec.describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_stat
         expect(repository).to receive(:start_expiration_policy!).and_call_original
         expect(repository).not_to receive(:reset_expiration_policy_started_at!)
         expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new)
-          .with(project, nil, params.merge('container_expiration_policy' => true))
+          .with(repository, nil, params.merge('container_expiration_policy' => true))
           .and_return(service)
 
         expect(service).to receive(:execute).and_return(status: :error, message: 'timeout while deleting tags')