-
-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
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 using environment variables for the base URL instead of hardcoding
http://localhost:8080
in multiple files. - The changes to the Dockerfile look good, but it might be worth adding a health check to ensure the application is running correctly.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 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.
return s.processManifest(ctx, repo, image, repoPath, tagName, manifest) | ||
} | ||
|
||
func (s *SyncService) processManifest(ctx context.Context, repo *models.Repository, image *models.Image, repoPath string, tagName string, manifest *ManifestResponse) 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 manifest list and single-manifest handling logic into separate helper functions to reduce nesting and recursion in processManifest, improving readability and maintainability without changing functionality..
Consider extracting the manifest list and single-manifest handling logic into separate helper functions. This reduces nesting and recursion in processManifest while keeping the functionality identical. For example:
// New helper to handle manifest lists
func (s *SyncService) handleManifestList(ctx context.Context, repo *models.Repository, image *models.Image, repoPath, tagName string, manifest *ManifestResponse) error {
var targetManifest *ManifestEntry
for _, m := range manifest.Manifests {
if !strings.Contains(m.MediaType, "attestation") &&
m.Platform.OS != "unknown" &&
m.Platform.Architecture != "unknown" {
targetManifest = &m
break
}
}
if targetManifest == nil {
log.Printf("No valid platform manifest found, trying first non-attestation manifest")
for _, m := range manifest.Manifests {
if !strings.Contains(m.MediaType, "attestation") {
targetManifest = &m
break
}
}
}
if targetManifest == nil {
return fmt.Errorf("no valid manifest found in list")
}
actualManifest, err := s.registry.GetManifest(ctx, repoPath, targetManifest.Digest)
if err != nil {
return fmt.Errorf("failed to get actual manifest: %w", err)
}
actualManifest.Platform = targetManifest.Platform
return s.handleSingleManifest(ctx, repo, image, repoPath, tagName, actualManifest)
}
// New helper to process a single manifest
func (s *SyncService) handleSingleManifest(ctx context.Context, repo *models.Repository, image *models.Image, repoPath, tagName string, manifest *ManifestResponse) error {
// ... move single manifest processing logic here (the lower part of processManifest)
// This keeps processManifest concise
return nil // replace with actual logic
}
// Simplified processManifest
func (s *SyncService) processManifest(ctx context.Context, repo *models.Repository, image *models.Image, repoPath, tagName string, manifest *ManifestResponse) error {
if manifest.MediaType == "application/vnd.oci.image.index.v1+json" ||
manifest.MediaType == "application/vnd.docker.distribution.manifest.list.v2+json" {
return s.handleManifestList(ctx, repo, image, repoPath, tagName, manifest)
}
return s.handleSingleManifest(ctx, repo, image, repoPath, tagName, manifest)
}
By refactoring into handleManifestList
and handleSingleManifest
, you reduce the nesting and recursion complexity, making the code easier to follow and maintain without reverting any functionality.
Summary by Sourcery
Migrate the Svelocker UI from a Node.js backend to a Go backend, introducing a more robust and performant architecture with improved separation of concerns
New Features:
Enhancements:
Build:
CI: