-
-
Notifications
You must be signed in to change notification settings - Fork 2
feat: migrate to go backend #195
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:8080in 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: