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
andProjects::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 acontainer_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.
- In addition, test files have been cleaned up by removing several duplicated
- 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
- [-] Changelog entry
- [-] Documentation (if required)
-
Code review guidelines -
Merge request performance guidelines -
Style guides - [-] Database guides
-
Separation of EE specific content
Availability and Testing
- [-] Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
- [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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