-
-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
Reviewer's Guide by SourceryThis pull request implements the tag deletion functionality across the backend and frontend. The backend changes include database updates and registry manifest deletion. The frontend changes include adding a delete tag button and implementing the deletion logic. A new API endpoint is exposed for deleting tags. Sequence diagram for deleting a tagsequenceDiagram
actor User
participant Frontend
participant Backend API
participant Tag Repository
participant Registry Client
participant Registry
User->>Frontend: Clicks 'Delete Tag' button
Frontend->>Backend API: Sends DELETE request to /repositories/{repo}/images/{image}/tags/{tag}
Backend API->>Tag Repository: Calls DeleteTag(repoName, imageName, tagName)
Tag Repository->>Tag Repository: Starts database transaction
Tag Repository->>Tag Repository: Finds tag by repoName, imageName, tagName
Tag Repository->>Tag Repository: Deletes tag metadata from database
Tag Repository->>Tag Repository: Deletes tag from database
Tag Repository->>Tag Repository: Commits database transaction
Tag Repository->>Registry Client: Calls DeleteManifest(repoPath, digest)
Registry Client->>Registry: Sends DELETE request to /v2/{repoPath}/manifests/{digest}
Registry-->>Registry Client: Returns status
Registry Client->>Tag Repository: Returns result of manifest deletion
Tag Repository->>Backend API: Returns success or error
Backend API->>Frontend: Returns success or error
Frontend->>User: Updates UI with deletion result
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @kmendell - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a unique constraint to the database to prevent duplicate tags.
- The registry client should be injected into the tag repository instead of creating it within the delete function.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -117,8 +124,134 @@ func (r *tagRepository) UpdateTag(ctx context.Context, tag *models.Tag) error { | |||
} | |||
|
|||
func (r *tagRepository) DeleteTag(ctx context.Context, repoName, imageName, tagName string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider extracting the registry deletion logic into a separate service layer to improve testability and maintainability by separating concerns related to database transactions and registry operations within the DeleteTag function.
The updated DeleteTag
function now mixes database transaction management with digest extraction and registry deletion logic, resulting in a single multi-responsibility function. Consider extracting the registry deletion logic (including digest lookup) into its own service layer. This would make the code easier to test and maintain while keeping the database logic isolated.
Actionable steps:
-
Create a new function in a registry service:
Move all registry-related operations including manifest retrieval and deletion into a separate function, e.g.,DeleteRegistryTag
. -
Simplify
DeleteTag
:
LetDeleteTag
handle only the database operations and then call the new registry service function.
Example changes:
Extract registry deletion logic:
// In services/registry.go
func (s *RegistryService) DeleteRegistryTag(ctx context.Context, repository, tagName string, possibleDigests []string) error {
for _, digest := range possibleDigests {
log.Printf("Attempting to delete manifest using digest: %s", digest)
if err := s.client.DeleteManifest(ctx, repository, digest); err == nil {
log.Printf("Successfully deleted manifest with digest: %s", digest)
return nil
} else {
log.Printf("Failed to delete manifest with digest %s: %v", digest, err)
}
}
return errors.New("failed to delete registry tag with any digest")
}
Simplify DeleteTag
:
func (r *tagRepository) DeleteTag(ctx context.Context, repoName, imageName, tagName string) error {
tx := r.db.Begin()
// Find tag and manage transaction...
// [database deletion logic remains unchanged]
if err := tx.Commit().Error; err != nil {
return err
}
log.Printf("Successfully deleted tag %s from database", tagName)
// Construct registry path
registryPath := imageName
if repoName != "library" {
registryPath = fmt.Sprintf("%s/%s", repoName, imageName)
}
// Retrieve possible digests
possibleDigests := extractDigests(tag) // refactor digest extraction into its own helper
// Delegate registry deletion to the new service
registryService := services.NewRegistryService(os.Getenv("PUBLIC_REGISTRY_URL"),
os.Getenv("REGISTRY_USERNAME"),
os.Getenv("REGISTRY_PASSWORD"))
if err := registryService.DeleteRegistryTag(ctx, registryPath, tagName, possibleDigests); err != nil {
log.Printf("Warning: Registry cleanup failed: %v", err)
}
return nil
}
By splitting the concerns, each function becomes focused on a single responsibility, making your code easier to test, maintain, and debug without reverting functionality.
…m user input Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…m user input Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Currently Giving a 500 error
Fixes: #215
Summary by Sourcery
Implement tag deletion functionality across the backend and frontend, allowing users to delete Docker image tags from both the database and container registry
New Features:
Bug Fixes:
Enhancements:
Summary by Sourcery
Implement comprehensive tag deletion functionality across backend and frontend, enabling users to delete Docker image tags from both the database and container registry
New Features:
Bug Fixes:
Enhancements: