fix: address Copilot review nits on prune scope flags and dry-run
- Reword --dangling/--registry help to clarify they restrict scope, not exclusive - Skip ListImages call in dry-run when pruneDangling is false (registry-only) - Use API field name keep_last in remote client validation error - Make service log message conditional on PruneRegistry when runtime prune fails
- commit
- 9de4117901a91c18104e4472a5efc3118766e6be
- author
- brice · · 2026-02-13 06:27 UTC
- parent
- 24a521d
- signature
- unsigned
3 files · +22 -20
M internal/adapters/in/cli/images.go +16 -18
| @@ -111,8 +111,8 @@ release tags per repository. Use --dangling or --registry to restrict scope.`, | |||
| 111 | 111 | cmd.Flags().BoolVar(&opts.DryRun, "dry-run", false, "Show what would be pruned without applying changes") | |
| 112 | 112 | cmd.Flags().IntVar(&opts.KeepReleases, "keep-releases", domain.DefaultImagePruneKeepLast, | |
| 113 | 113 | "Number of previous non-latest tags to keep per repository (latest is always kept)") | |
| 114 | - | cmd.Flags().BoolVar(&opts.Dangling, "dangling", false, "Prune dangling runtime images only") | |
| 115 | - | cmd.Flags().BoolVar(&opts.Registry, "registry", false, "Prune old registry tags only") | |
| 114 | + | cmd.Flags().BoolVar(&opts.Dangling, "dangling", false, "Include dangling runtime images (default: both scopes)") | |
| 115 | + | cmd.Flags().BoolVar(&opts.Registry, "registry", false, "Include old registry tags (default: both scopes)") | |
| 116 | 116 | cmd.Flags().BoolVar(&opts.NoConfirm, "no-confirm", false, "Skip confirmation prompt") | |
| 117 | 117 | ||
| 118 | 118 | return cmd | |
| @@ -231,23 +231,23 @@ func runImagesPrune(ctx context.Context, client imagesClient, opts imagesPruneOp | |||
| 231 | 231 | } | |
| 232 | 232 | ||
| 233 | 233 | func runImagesPruneDryRun(ctx context.Context, client imagesClient, opts imagesPruneOptions, pruneDangling, pruneRegistry bool, out io.Writer) error { | |
| 234 | - | images, err := client.ListImages(ctx) | |
| 235 | - | if err != nil { | |
| 236 | - | return fmt.Errorf("failed to list images: %w", err) | |
| 237 | - | } | |
| 238 | - | ||
| 239 | - | danglingCount := 0 | |
| 240 | - | for _, img := range images { | |
| 241 | - | if img.Dangling { | |
| 242 | - | danglingCount++ | |
| 243 | - | } | |
| 244 | - | } | |
| 245 | - | ||
| 246 | 234 | if err := cliWriteLine(out, cliRenderWarning("Dry run: no changes applied")); err != nil { | |
| 247 | 235 | return err | |
| 248 | 236 | } | |
| 249 | 237 | ||
| 250 | 238 | if pruneDangling { | |
| 239 | + | images, err := client.ListImages(ctx) | |
| 240 | + | if err != nil { | |
| 241 | + | return fmt.Errorf("failed to list images: %w", err) | |
| 242 | + | } | |
| 243 | + | ||
| 244 | + | danglingCount := 0 | |
| 245 | + | for _, img := range images { | |
| 246 | + | if img.Dangling { | |
| 247 | + | danglingCount++ | |
| 248 | + | } | |
| 249 | + | } | |
| 250 | + | ||
| 251 | 251 | if err := cliWritef(out, "Runtime: would prune %d dangling runtime images\n", danglingCount); err != nil { | |
| 252 | 252 | return err | |
| 253 | 253 | } | |
| @@ -258,12 +258,10 @@ func runImagesPruneDryRun(ctx context.Context, client imagesClient, opts imagesP | |||
| 258 | 258 | } | |
| 259 | 259 | ||
| 260 | 260 | if pruneRegistry { | |
| 261 | - | err = cliWritef(out, "Registry: would keep latest + %d previous tags per repository\n", opts.KeepReleases) | |
| 262 | - | return err | |
| 261 | + | return cliWritef(out, "Registry: would keep latest + %d previous tags per repository\n", opts.KeepReleases) | |
| 263 | 262 | } | |
| 264 | 263 | ||
| 265 | - | err = cliWriteLine(out, cliRenderMuted("Registry cleanup skipped (--dangling)")) | |
| 266 | - | return err | |
| 264 | + | return cliWriteLine(out, cliRenderMuted("Registry cleanup skipped (--dangling)")) | |
| 267 | 265 | } | |
| 268 | 266 | ||
| 269 | 267 | func buildPruneRequest(keepReleases int, pruneDangling, pruneRegistry bool) dto.ImagePruneRequest { | |
| @@ -111,8 +111,8 @@ release tags per repository. Use --dangling or --registry to restrict scope.`, | |||||
| 111 | cmd.Flags().BoolVar(&opts.DryRun, "dry-run", false, "Show what would be pruned without applying changes") | 111 | cmd.Flags().BoolVar(&opts.DryRun, "dry-run", false, "Show what would be pruned without applying changes") | ||
| 112 | cmd.Flags().IntVar(&opts.KeepReleases, "keep-releases", domain.DefaultImagePruneKeepLast, | 112 | cmd.Flags().IntVar(&opts.KeepReleases, "keep-releases", domain.DefaultImagePruneKeepLast, | ||
| 113 | "Number of previous non-latest tags to keep per repository (latest is always kept)") | 113 | "Number of previous non-latest tags to keep per repository (latest is always kept)") | ||
| 114 | - | cmd.Flags().BoolVar(&opts.Dangling, "dangling", false, "Prune dangling runtime images only") | 114 | + | cmd.Flags().BoolVar(&opts.Dangling, "dangling", false, "Include dangling runtime images (default: both scopes)") |
| 115 | - | cmd.Flags().BoolVar(&opts.Registry, "registry", false, "Prune old registry tags only") | 115 | + | cmd.Flags().BoolVar(&opts.Registry, "registry", false, "Include old registry tags (default: both scopes)") |
| 116 | cmd.Flags().BoolVar(&opts.NoConfirm, "no-confirm", false, "Skip confirmation prompt") | 116 | cmd.Flags().BoolVar(&opts.NoConfirm, "no-confirm", false, "Skip confirmation prompt") | ||
| 117 | 117 | ||||
| 118 | return cmd | 118 | return cmd | ||
| @@ -231,23 +231,23 @@ func runImagesPrune(ctx context.Context, client imagesClient, opts imagesPruneOp | |||||
| 231 | } | 231 | } | ||
| 232 | 232 | ||||
| 233 | func runImagesPruneDryRun(ctx context.Context, client imagesClient, opts imagesPruneOptions, pruneDangling, pruneRegistry bool, out io.Writer) error { | 233 | func runImagesPruneDryRun(ctx context.Context, client imagesClient, opts imagesPruneOptions, pruneDangling, pruneRegistry bool, out io.Writer) error { | ||
| 234 | - | images, err := client.ListImages(ctx) | |||
| 235 | - | if err != nil { | |||
| 236 | - | return fmt.Errorf("failed to list images: %w", err) | |||
| 237 | - | } | |||
| 238 | - | ||||
| 239 | - | danglingCount := 0 | |||
| 240 | - | for _, img := range images { | |||
| 241 | - | if img.Dangling { | |||
| 242 | - | danglingCount++ | |||
| 243 | - | } | |||
| 244 | - | } | |||
| 245 | - | ||||
| 246 | if err := cliWriteLine(out, cliRenderWarning("Dry run: no changes applied")); err != nil { | 234 | if err := cliWriteLine(out, cliRenderWarning("Dry run: no changes applied")); err != nil { | ||
| 247 | return err | 235 | return err | ||
| 248 | } | 236 | } | ||
| 249 | 237 | ||||
| 250 | if pruneDangling { | 238 | if pruneDangling { | ||
| 239 | + | images, err := client.ListImages(ctx) | |||
| 240 | + | if err != nil { | |||
| 241 | + | return fmt.Errorf("failed to list images: %w", err) | |||
| 242 | + | } | |||
| 243 | + | ||||
| 244 | + | danglingCount := 0 | |||
| 245 | + | for _, img := range images { | |||
| 246 | + | if img.Dangling { | |||
| 247 | + | danglingCount++ | |||
| 248 | + | } | |||
| 249 | + | } | |||
| 250 | + | ||||
| 251 | if err := cliWritef(out, "Runtime: would prune %d dangling runtime images\n", danglingCount); err != nil { | 251 | if err := cliWritef(out, "Runtime: would prune %d dangling runtime images\n", danglingCount); err != nil { | ||
| 252 | return err | 252 | return err | ||
| 253 | } | 253 | } | ||
| @@ -258,12 +258,10 @@ func runImagesPruneDryRun(ctx context.Context, client imagesClient, opts imagesP | |||||
| 258 | } | 258 | } | ||
| 259 | 259 | ||||
| 260 | if pruneRegistry { | 260 | if pruneRegistry { | ||
| 261 | - | err = cliWritef(out, "Registry: would keep latest + %d previous tags per repository\n", opts.KeepReleases) | 261 | + | return cliWritef(out, "Registry: would keep latest + %d previous tags per repository\n", opts.KeepReleases) |
| 262 | - | return err | |||
| 263 | } | 262 | } | ||
| 264 | 263 | ||||
| 265 | - | err = cliWriteLine(out, cliRenderMuted("Registry cleanup skipped (--dangling)")) | 264 | + | return cliWriteLine(out, cliRenderMuted("Registry cleanup skipped (--dangling)")) |
| 266 | - | return err | |||
| 267 | } | 265 | } | ||
| 268 | 266 | ||||
| 269 | func buildPruneRequest(keepReleases int, pruneDangling, pruneRegistry bool) dto.ImagePruneRequest { | 267 | func buildPruneRequest(keepReleases int, pruneDangling, pruneRegistry bool) dto.ImagePruneRequest { | ||
M internal/adapters/in/cli/remote/client.go +1 -1
| @@ -503,7 +503,7 @@ func (c *Client) ListImages(ctx context.Context) ([]dto.Image, error) { | |||
| 503 | 503 | // PruneImages prunes runtime and registry images. | |
| 504 | 504 | func (c *Client) PruneImages(ctx context.Context, req dto.ImagePruneRequest) (*dto.ImagePruneResponse, error) { | |
| 505 | 505 | if req.KeepLast != nil && *req.KeepLast < 0 { | |
| 506 | - | return nil, fmt.Errorf("keepLast must be >= 0") | |
| 506 | + | return nil, fmt.Errorf("keep_last must be >= 0") | |
| 507 | 507 | } | |
| 508 | 508 | ||
| 509 | 509 | resp, err := c.request(ctx, http.MethodPost, "/images/prune", req) | |
| @@ -503,7 +503,7 @@ func (c *Client) ListImages(ctx context.Context) ([]dto.Image, error) { | |||||
| 503 | // PruneImages prunes runtime and registry images. | 503 | // PruneImages prunes runtime and registry images. | ||
| 504 | func (c *Client) PruneImages(ctx context.Context, req dto.ImagePruneRequest) (*dto.ImagePruneResponse, error) { | 504 | func (c *Client) PruneImages(ctx context.Context, req dto.ImagePruneRequest) (*dto.ImagePruneResponse, error) { | ||
| 505 | if req.KeepLast != nil && *req.KeepLast < 0 { | 505 | if req.KeepLast != nil && *req.KeepLast < 0 { | ||
| 506 | - | return nil, fmt.Errorf("keepLast must be >= 0") | 506 | + | return nil, fmt.Errorf("keep_last must be >= 0") |
| 507 | } | 507 | } | ||
| 508 | 508 | ||||
| 509 | resp, err := c.request(ctx, http.MethodPost, "/images/prune", req) | 509 | resp, err := c.request(ctx, http.MethodPost, "/images/prune", req) | ||
M internal/usecase/images/service.go +5 -1
| @@ -347,7 +347,11 @@ func (s *Service) Prune(ctx context.Context, opts domain.ImagePruneOptions) (dom | |||
| 347 | 347 | if opts.PruneDangling { | |
| 348 | 348 | runtimeReport, err := s.PruneRuntime(ctx) | |
| 349 | 349 | if err != nil { | |
| 350 | - | log.Warn().Err(err).Msg("runtime prune failed; continuing with registry prune") | |
| 350 | + | msg := "runtime prune failed" | |
| 351 | + | if opts.PruneRegistry { | |
| 352 | + | msg += "; continuing with registry prune" | |
| 353 | + | } | |
| 354 | + | log.Warn().Err(err).Msg(msg) | |
| 351 | 355 | } else { | |
| 352 | 356 | report.Runtime = runtimeReport.Runtime | |
| 353 | 357 | } | |
| @@ -347,7 +347,11 @@ func (s *Service) Prune(ctx context.Context, opts domain.ImagePruneOptions) (dom | |||||
| 347 | if opts.PruneDangling { | 347 | if opts.PruneDangling { | ||
| 348 | runtimeReport, err := s.PruneRuntime(ctx) | 348 | runtimeReport, err := s.PruneRuntime(ctx) | ||
| 349 | if err != nil { | 349 | if err != nil { | ||
| 350 | - | log.Warn().Err(err).Msg("runtime prune failed; continuing with registry prune") | 350 | + | msg := "runtime prune failed" |
| 351 | + | if opts.PruneRegistry { | |||
| 352 | + | msg += "; continuing with registry prune" | |||
| 353 | + | } | |||
| 354 | + | log.Warn().Err(err).Msg(msg) | |||
| 351 | } else { | 355 | } else { | ||
| 352 | report.Runtime = runtimeReport.Runtime | 356 | report.Runtime = runtimeReport.Runtime | ||
| 353 | } | 357 | } | ||