Skip to content

Split the DeleteTagsService into two smaller services

What does this MR do?

Preparation work before implementing #208193 (comment 362910703) changes.

The current DeleteTagsService has currently two modes:

  • When the Gitlab Container Registry is used.
  • When a Third Party Container Registry is used. This is also used as a fallback.

Each mode implements a different way to delete tags.

With #208193 (comment 362910703), we will starting adding limits for the "Gitlab Container Registry" mode behind a feature flag ("throttling"). So with these changes, the DeleteTagsService will need to handle 3 modes:

  • "Gitlab Container Registry" + "throttling" enabled
  • "Gitlab Container Registry" + "throttling" disabled
  • "Third Party Container Registry"

That starts to be a bit too much for a single service. On the RSpec side, imagine the amount of contexts used to properly test all these modes. A glimpse of that can be seen in this commit: 31860314.

This MR tries to untangle the code by:

  • Splitting the service into two smaller ones:
    • Projects::ContainerRepository::DeleteTagsService is the main one. It's a "facade" service: it will validate the parameters, check the user permissions, select the proper service class, execute it and log the response.
    • Projects::ContainerRepository::Gitlab::DeleteTagsService is the service used when the "Gitlab Container Registry" is available. This is the one that will be mainly updated by #208193 (comment 362910703) changes.
    • Projects::ContainerRepository::ThirdParty::DeleteTagsService is the service used when the "Third Party Container Registry" is used.
    • The above split is also explicitly done by using two different namespaces: Projects::ContainerRepository::Gitlab and Projects::ContainerRepository::ThirdParty. This was not mandatory but it will come handy when we're going to implement more requests for the "Gitlab Container Registry" while maintaining compatibility with the "Third Party Container Registry".
  • Cleaning up the Projects::ContainerRepository::DeleteTagsService implementation by using instance variables. This is to avoid passing a container_repository parameter to all private functions. That's a small code smell to me.
  • Splitting the test files so that each service has its own dedicate test files
    • In addition, test files have been cleaned up by removing several duplicated let_it_be vars, they will now use only one set of them.
  • Not adding, updating or removing any feature
  • This MR adds a quick success return if the underlying service receives an empty list of tags to delete.

-> This way, we are in a better place to handle future changes from #208193 (comment 362910703)

Screenshots

n/a

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by David Fernandez

Merge request reports