Refactoring the code base in compliance with idiomatic Go style and best practices#11
Refactoring the code base in compliance with idiomatic Go style and best practices#11
Conversation
Split monolithic cmd/main.go into individual command files for better maintainability: - Extract run, deploy, prune, restart, revisions, list, and completion commands - Reduce main.go from ~220 lines to ~60 lines - Improve code organization and readability - Add .claude directory to .gitignore This refactoring follows Go best practices for CLI application structure and makes the codebase more maintainable and easier to navigate.
Extract all command logic from init() functions into dedicated factory and handler functions: - Create newXxxCommand() factory functions for each command - Extract business logic to dedicated xxxHandler() functions - Minimize init() functions to only command registration - Improve flag variable scoping with proper closure patterns - Replace anonymous functions with named handlers for better testability - Apply consistent patterns across all commands (completion, deploy, list, prune, restart, revisions, run, version) This refactoring follows Go best practices for CLI applications: - Better separation of concerns between setup and business logic - Enhanced testability with named functions instead of closures - Improved maintainability and code organization - Interface-driven design patterns - Consistent code structure across all commands No functional changes - maintains 100% backward compatibility.
…cy files - Remove deploy.go, exec.go, logs.go, prune.go, restart.go, revision.go - Migrate ECS logic into pkg/ecs/service.go - Net: 630 insertions, 655 deletions across 7 files
…matted list in CLI
- cmd/run.go: expand functionality and UX; add 38 lines, minor cleanup - cmd/revisions.go: add listing/details enhancements; +27 lines - pkg/ecs/service.go: update service logic and interfaces; net +135/-85 - Overall: 3 files changed, 150 insertions, 50 deletions
- Add PruneResult struct with TaskDefinitionPruneEntry for structured data return - Update Prune() method to return (*PruneResult, error) instead of direct printing - Refactor deregisterTaskFamily() to collect and return operation results - Move all fmt.Printf/Println calls from pkg/ecs to cmd/prune.go for proper separation - Improve error handling with proper error propagation vs log.Fatalln - Follow existing patterns from cmd/run.go and cmd/list.go for output formatting Maintains same user output while achieving better separation of concerns between business logic and presentation layers.
- Fix CLI command descriptions: remove "of" from "List of..." phrases - Improve flag descriptions for clarity and consistency - Fix capitalization: "Cpu" → "CPU", sentence case for descriptions - Correct verb tenses: "is skipped/deregistered" → "was skipped/deregistered" - Simplify message phrasing for better readability - Remove unnecessary articles: "the task definition" → "task definition"
- Add DeployResult and RestartResult structs for structured data return - Refactor Deploy() method to return (*DeployResult, error) instead of direct output - Refactor Restart() method to return (*RestartResult, error) instead of direct output - Update cmd/deploy.go to handle service results and format user output - Update cmd/restart.go to handle service results and format user output - Remove log.Fatalln and fmt.Printf calls from pkg/ecs/service.go - Improve error handling with proper error wrapping - Maintain same user experience while achieving clean separation of concerns
…s command - Replace github.com/rodaine/table with github.com/charmbracelet/lipgloss for styling - Remove github.com/fatih/color dependency - Change output from table to bullet list format showing created date and docker URI - Add sorting by revision number in descending order - Use locale-aware date formatting with proper Go time methods - Simplify display to show only date and time without timezone/seconds Output format: • 2024-01-15 14:30: docker_uri_here
… command - Add lipgloss table rendering for displayServicesWithDetails function - Implement structured table with headers for Service, Task ID, CPU, Memory, Running Time - Add lipgloss styling for displayServices function with bold service names and enumerated lists - Replace simple printf statements with properly formatted table output - Maintain existing service/cluster naming convention (cluster/service format) - Add empty state handling for cases with no running tasks
- Extract time formatting logic into dedicated formatRunningTime function in utils.go - Improve human readability by showing days when duration exceeds 24 hours - Format as "2d 5h 30m" for long-running tasks, "5h 30m" for shorter tasks - Remove seconds from display for cleaner output - Make time formatting reusable across the codebase
- Remove Service struct that was only bundling cluster/service parameters - Convert all Service methods to standalone package functions - Update cmd layer to use parseServiceFlag() instead of initService() - Replace struct-based method calls with direct function calls - Remove unnecessary validator dependency from main.go - Optimize parameter passing with explicit cluster/service arguments Benefits: - Eliminates unnecessary object-oriented wrapper around stateless operations - More idiomatic Go design using functions for stateless operations - Clearer parameter flow without artificial object boundaries - Better performance by removing struct allocation overhead - Simplified codebase with 100 fewer lines of code All public APIs converted: - Deploy(cluster, service, dockerImageTag) - Execute(cluster, service, cmd, waitForCompletion, dockerImageTag) - Restart(cluster, service, kill) - Prune(cluster, service, keepLast, keepDays, dryRun) - Revisions(cluster, service, lastRevisionNr)
Exclude project-specific Claude instructions from version control
Replace manual loop with Go 1.25+ standard library function for better readability and performance
Remove extra whitespace and normalize code formatting
- Replace duplicate extractProcessID/extractLastPart functions with single extractARNResource - Use official github.com/aws/aws-sdk-go-v2/aws/arn package for proper ARN parsing - Add proper error handling instead of fallback string parsing - Update all callers to handle error returns with descriptive error messages This improves reliability and follows AWS best practices for ARN handling
Exclude temporary build directory from version control.
…ncation Add NextToken handling to getClusterArns, getServiceArns, and getTaskDetails functions to ensure complete results in larger AWS environments. Previously, these functions only returned the first page of results, potentially missing clusters, services, and tasks beyond the initial API response limit. Implements the same pagination pattern used successfully in prune and revisions commands, following AWS SDK best practices for paginated responses.
Refactored getProcessLogs to accept pre-initialized CloudWatch client instead of calling initCfg() on every invocation. This eliminates expensive AWS configuration loading that was happening every 5 seconds during Execute's waitForCompletion loop. Changes: - Modified getProcessLogs function signature to accept *cloudwatchlogs.Client parameter - Updated Execute function to create CloudWatch client once and reuse it - Removed initCfg() call from getProcessLogs inner loop Significantly improves performance for long-running Execute operations while maintaining the same functionality.
- Add generic safeGetFirst/safeGetFirstPtr functions in core.go using Go 1.25+ generics - Fix 30+ unchecked pointer dereferences and index accesses across service.go - Add bounds checking for all slice accesses (Tasks[0], Services[0], etc.) - Add nil checks for all AWS API response pointers (TaskArn, ServiceArn, etc.) - Add validation for nested AWS configuration objects (NetworkConfiguration) - Replace panic-prone direct access with safe error handling patterns - Provide descriptive error messages for all failure scenarios Prevents runtime panics when AWS APIs return empty responses, missing fields, or unexpected data structures. Follows Go idiomatic error handling patterns. Fixes: Critical safety issues in describeTask, wait, Execute, getProcessLogs, stopAll, forceNewDeploy, getFamilyPrefix, latestTaskDefinitionArn, getRevisions
- Updated all exported ECS functions to accept context.Context as first parameter - Removed internal context.Background() calls from Deploy, Execute, Prune, Restart, Revisions, and GetClusters - Enhanced Execute function polling loop with proper context cancellation handling - Added signal-aware context in cmd/run.go and cmd/prune.go for Ctrl+C cancellation - Replaced time.Sleep with context-aware select statements for immediate cancellation - Enabled proper timeout, cancellation, and request tracing capabilities - Follows Go 1.25+ best practices for context propagation This change allows users to cancel long-running operations like 'run --wait' with Ctrl+C and provides the foundation for timeout configuration and request tracing.
- Added signal.NotifyContext to deploy, list, restart, and revisions commands - All commands now support Ctrl+C cancellation for improved user experience - Consistent context propagation across entire CLI application - Users can interrupt any hanging AWS operation gracefully - Maintains uniform signal handling pattern established in run and prune commands This completes the signal-aware context implementation across all CLI commands, providing users with responsive and cancellable operations throughout the application.
…tion Replace repeated AWS config initialization with centralized AWSClients struct. Create NewAWSClients() constructor that holds both ECS and CloudWatch clients, eliminating 6 redundant config calls across service layer functions. Changes: - Add AWSClients struct with ECS and CloudWatchLogs clients - Replace initCfg() function with NewAWSClients() constructor - Update all service functions to accept *AWSClients parameter - Modify cmd layer to create and pass clients to service functions Benefits: - Eliminates code duplication (6 → 1 initialization) - Improves performance by avoiding repeated config loading - Enhances testability with injectable mock clients - Follows Go idiom: accept interfaces, return structs
Update copyright notice from "Copyright 2021 Petr Reichl" to "Copyright (c) Petr Reichl and affiliates" across all source files to reflect broader attribution.
Extract all ECS-related types and constants from service.go and list.go into a dedicated types.go file to improve code organization and follow Go conventions for package structure. - Create pkg/ecs/types.go with 16 types and 1 constant - Remove extracted types from service.go (lines 32-116) - Remove extracted types from list.go (lines 14-32) - Maintain all type documentation and structure
|
Caution Review failedThe pull request is closed. WalkthroughAdds multiple Cobra CLI subcommands and refactors ECS logic into package-level, context-aware functions that return structured results. Introduces an AWS clients factory, new domain types, pagination and parsing utilities, and Charmbracelet terminal deps; updates .gitignore and license text. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (cobra)
participant Factory as NewAWSClients()
participant ECS as ecs.Execute
User->>CLI: run [--wait] [-i tag] --service cluster/service [cmd...]
CLI->>Factory: NewAWSClients()
Factory-->>CLI: *AWSClients
CLI->>ECS: Execute(ctx, clients, cluster, service, cmd, wait, tag)
ECS->>ECS: describeService / describeTask
alt tag provided
ECS->>ECS: cloneTaskDef()
end
ECS->>AWS: RunTask
AWS-->>ECS: TaskArn
opt wait
ECS->>AWS: FilterLogEvents / DescribeTasks (poll)
end
ECS-->>CLI: ExecuteResult
CLI-->>User: print result/logs
sequenceDiagram
participant User
participant CLI
participant Factory as NewAWSClients()
participant Deploy as ecs.Deploy
User->>CLI: deploy -i <tag> --service cluster/service
CLI->>Factory: NewAWSClients()
Factory-->>CLI: *AWSClients
CLI->>Deploy: Deploy(ctx, clients, cluster, service, tag)
Deploy->>AWS: DescribeService / DescribeTaskDefinition
Deploy->>AWS: RegisterTaskDefinition
Deploy->>AWS: UpdateService
Deploy-->>CLI: DeployResult(ARNs)
CLI-->>User: print ARNs
sequenceDiagram
participant User
participant CLI
participant Factory as NewAWSClients()
participant Prune as ecs.Prune
User->>CLI: prune [--dry-run] [--keep-last N] [--keep-days D] --service c/s
CLI->>Factory: NewAWSClients()
Factory-->>CLI: *AWSClients
CLI->>Prune: Prune(ctx, clients, cluster, service, keepLast, keepDays, dryRun)
Prune->>AWS: ListTaskDefinitionFamilies
loop families
Prune->>AWS: ListTaskDefinitions / DescribeTaskDefinition
opt !dry-run
Prune->>AWS: DeregisterTaskDefinition
end
end
Prune-->>CLI: PruneResult
CLI-->>User: print summary
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.2.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
pkg/ecs/prune.go (1)
25-142: Fix kept/skipped counters: “newer than keepDays” entries aren’t counted as kept.Currently, keep is only incremented for the “keepLast” branch. Entries kept due to age are misreported as “skipped” via the final arithmetic. Track skipped explicitly and increment keep for age-kept entries.
Apply this diff:
func deregisterTaskFamily(ctx context.Context, family string, keepLast int, keepDays int, dryRun bool, svc *ecs.Client) (int, int, int, []TaskDefinitionPruneEntry, error) { @@ today := time.Now().UTC() totalCount := 0 deleted := 0 keep := 0 + skipped := 0 var processedTasks []TaskDefinitionPruneEntry @@ if err != nil { - processedTasks = append(processedTasks, TaskDefinitionPruneEntry{ + processedTasks = append(processedTasks, TaskDefinitionPruneEntry{ Arn: def, DaysOld: -1, Action: "skipped", Reason: fmt.Sprintf("Failed to describe: %v", err), Family: family, }) - continue + skipped++ + continue } @@ - if response.TaskDefinition.RegisteredAt == nil { - processedTasks = append(processedTasks, TaskDefinitionPruneEntry{ + if response.TaskDefinition.RegisteredAt == nil { + processedTasks = append(processedTasks, TaskDefinitionPruneEntry{ Arn: def, DaysOld: -1, Action: "skipped", Reason: "Missing registration date", Family: family, }) - continue + skipped++ + continue } @@ - if diffInDays < keepDays { - processedTasks = append(processedTasks, TaskDefinitionPruneEntry{ + if diffInDays < keepDays { + processedTasks = append(processedTasks, TaskDefinitionPruneEntry{ Arn: def, DaysOld: diffInDays, Action: "kept", Reason: fmt.Sprintf("newer than %d days", keepDays), Family: family, }) - continue + keep++ + continue } @@ _, err := svc.DeregisterTaskDefinition(ctx, &ecs.DeregisterTaskDefinitionInput{TaskDefinition: &def}) if err != nil { - processedTasks = append(processedTasks, TaskDefinitionPruneEntry{ + processedTasks = append(processedTasks, TaskDefinitionPruneEntry{ Arn: def, DaysOld: diffInDays, Action: "skipped", Reason: fmt.Sprintf("deregistration failed: %v", err), Family: family, }) deleted-- // Decrement since it wasn't actually deleted - continue + skipped++ + continue } @@ - skipped := totalCount - deleted - keep - return totalCount, deleted, skipped, processedTasks, nil + return totalCount, deleted, skipped, processedTasks, nil
🧹 Nitpick comments (35)
.gitignore (1)
4-6: Prefer directory-anchored ignore for tmpUsing a trailing slash (and optionally repo-root anchoring) avoids accidentally ignoring a file named "tmp" anywhere in the tree.
Apply this diff:
-.claude -CLAUDE.md -tmp +.claude +CLAUDE.md +/tmp/pkg/ecs/core.go (1)
21-28: Document pointer semantics for slice elements to avoid surprisesReturning a pointer to a slice element is fine, but if callers append to the same slice later (causing reallocation), that pointer may no longer reflect the “current” first element and will point to the old backing array. A brief note in the comment helps future readers use this correctly.
Apply this diff to the comment:
-// safeGetFirstPtr returns a pointer to the first element of a slice. -// Returns an error if the slice is empty to prevent panic on index access. +// safeGetFirstPtr returns a pointer to the first element of a slice. +// Returns an error if the slice is empty to prevent panic on index access. +// Note: The returned pointer points into the slice’s backing array. If the caller +// later appends to the same slice and it reallocates, the pointer will reference +// the old array. Prefer safeGetFirst when mutation or pointer semantics are not required.pkg/ecs/service.go (1)
16-29: Confirm breaking change: removal of exported Service typeOur search for references to
ecs.Serviceacross the repo returned no matches—no internal code depends on the removed type. If this package is consumed externally under module pathrunecs.io/v1/pkg/ecs, droppingServiceis a breaking API change. Please confirm your intended usage:
- If this is a CLI-only repo, no further action is required.
- If this is a public library, choose one of the following:
• Add a deprecated alias in v1 (e.g.type Service = …) for backward compatibility
• Document the BREAKING CHANGE and bump the major version to v2 ingo.mod
• Publish clear migration instructions in your release notespkg/ecs/utils.go (2)
21-26: Handle colon-delimited ARN resources as a fallbackSome ARNs use ":" as the resource delimiter (e.g., iam, rds). As written, those will return the entire resource (e.g., "db:instance-id") instead of just the identifier. Adding a colon fallback covers both formats without changing ECS task-definition behavior.
Apply this diff:
- if idx := strings.LastIndex(parsedARN.Resource, "/"); idx != -1 { - return parsedARN.Resource[idx+1:], nil - } - - // If no "/" in resource, return the whole resource - return parsedARN.Resource, nil + if idx := strings.LastIndex(parsedARN.Resource, "/"); idx != -1 { + return parsedARN.Resource[idx+1:], nil + } + // Fallback to ":" delimiter if "/" not present (e.g., "db:instance-id") + if idx := strings.LastIndex(parsedARN.Resource, ":"); idx != -1 { + return parsedARN.Resource[idx+1:], nil + } + // If no known delimiter in resource, return the whole resource + return parsedARN.Resource, nil
33-44: Minor readability: drop else after returnA small style tweak: return early on the >=24h path and avoid the else to reduce nesting.
Apply this diff:
- if totalHours >= 24 { - days := totalHours / 24 - hours := totalHours % 24 - minutes := int(duration.Minutes()) % 60 - return fmt.Sprintf("%dd %dh %dm", days, hours, minutes) - } else { - hours := totalHours - minutes := int(duration.Minutes()) % 60 - return fmt.Sprintf("%dh %dm", hours, minutes) - } + if totalHours >= 24 { + days := totalHours / 24 + hours := totalHours % 24 + minutes := int(duration.Minutes()) % 60 + return fmt.Sprintf("%dd %dh %dm", days, hours, minutes) + } + hours := totalHours + minutes := int(duration.Minutes()) % 60 + return fmt.Sprintf("%dh %dm", hours, minutes)pkg/ecs/types.go (2)
86-91: Avoid magic strings: define typed constants for Restart method and Prune actionThe stringly-typed "kill"/"force_deploy" and "kept"/"deleted"/"skipped" are error-prone. Provide canonical constants to prevent typos and enable discoverability.
Apply this diff to introduce constants without breaking public types:
+// Canonical values for RestartResult.Method +const ( + RestartMethodKill = "kill" + RestartMethodForceDeploy = "force_deploy" +) + +// Canonical values for TaskDefinitionPruneEntry.Action +const ( + PruneActionKept = "kept" + PruneActionDeleted = "deleted" + PruneActionSkipped = "skipped" +)Also applies to: 100-106
93-98: Consider including StoppedAt in StoppedTaskInfoOnly StartedAt is captured, which limits downstream UX (e.g., calculating actual run duration or showing stop time). If available from ECS, add StoppedAt for completeness.
type StoppedTaskInfo struct { TaskArn string StartedAt time.Time + StoppedAt time.Time }pkg/ecs/client.go (1)
28-29: Prefer accepting context.Context to allow cancellation and deadlinesHard-coding context.Background() prevents callers (CLI commands/tests) from controlling timeouts or cancellations.
Minimal change (will require updating call sites):
-func NewAWSClients() (*AWSClients, error) { +func NewAWSClients(ctx context.Context) (*AWSClients, error) { @@ - cfg, err := config.LoadDefaultConfig(context.Background(), configFunctions...) + cfg, err := config.LoadDefaultConfig(ctx, configFunctions...)Also applies to: 35-35
cmd/version.go (1)
25-27: Guard against nil version to avoid potential panicIf SetVersion wasn’t called (e.g., in tests), versionHandler will panic dereferencing version.
-func versionHandler(cmd *cobra.Command, args []string) { - fmt.Printf("runecs %s\n", version.Version) -} +func versionHandler(cmd *cobra.Command, args []string) { + out := "dev" + if version != nil && version.Version != "" { + out = version.Version + } + fmt.Printf("runecs %s\n", out) +}cmd/completion.go (1)
21-38: Add Long help with quick-start usage to improve UXA short “how to use” block saves users from hunting docs, especially for persistent shell setup.
func newCompletionCommand() *cobra.Command { return &cobra.Command{ Use: "completion [bash|zsh|fish|powershell]", Short: "Generate shell completion scripts", + Long: `Generate shell completion scripts for runecs. + +Examples: + Bash: + runecs completion bash > /etc/bash_completion.d/runecs # Linux (root) + runecs completion bash | source # current session + + Zsh: + runecs completion zsh > "${fpath[1]}/_runecs" + + Fish: + runecs completion fish | source + runecs completion fish > ~/.config/fish/completions/runecs.fish + + PowerShell: + runecs completion powershell | Out-String | Invoke-Expression + runecs completion powershell > runecs.ps1`, DisableFlagsInUseLine: true, ValidArgs: []string{"bash", "zsh", "fish", "powershell"}, Args: cobra.MatchAll(cobra.ExactArgs(1), cobra.OnlyValidArgs), Run: completionHandler, } }cmd/deploy.go (3)
19-25: Prefer RunE and return errors instead of exiting the processUsing RunE allows Cobra to handle exit codes and error printing uniformly. It also simplifies testing. Suggest switching from Run to RunE and updating the handler to return error rather than calling log.Fatalf/log.Fatalln.
Apply this diff to the command wiring:
cmd := &cobra.Command{ Use: "deploy", Short: "Deploy a new version of the task", DisableFlagsInUseLine: true, PreRunE: deployPreRunE(&dockerImageTag), - Run: deployHandler(&dockerImageTag), + RunE: deployHandler(&dockerImageTag), }And update the handler signature and error handling (see lines 40-60 for the corresponding changes).
40-60: Return errors from the handler; avoid log.Fatalf in command flowReturning errors improves composability and testability. It also avoids abrupt os.Exit(1) within business logic.
Apply this diff to the handler:
-func deployHandler(dockerImageTag *string) func(*cobra.Command, []string) { - return func(cmd *cobra.Command, args []string) { +func deployHandler(dockerImageTag *string) func(*cobra.Command, []string) error { + return func(cmd *cobra.Command, args []string) error { // Set up context that cancels on interrupt signal for cancellable deploy operations ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer cancel() clients, err := ecs.NewAWSClients() if err != nil { - log.Fatalf("Failed to initialize AWS clients: %v\n", err) + return fmt.Errorf("failed to initialize AWS clients: %w", err) } cluster, service := parseServiceFlag() result, err := ecs.Deploy(ctx, clients, cluster, service, *dockerImageTag) if err != nil { - log.Fatalf("Deploy failed: %v\n", err) + return fmt.Errorf("deploy failed: %w", err) } fmt.Printf("New task revision %s has been created\n", result.TaskDefinitionArn) fmt.Printf("Service %s has been updated.\n", result.ServiceArn) - } + return nil + } }
27-28: Use non-persistent flags here and mark required
- There are no subcommands under deploy, so PersistentFlags is unnecessary. Prefer Flags for clarity.
- Cobra can enforce required flags via MarkFlagRequired, which integrates better with usage/help output and reduces custom PreRun checks.
-cmd.PersistentFlags().StringVarP(&dockerImageTag, "image-tag", "i", "", "docker image tag") +cmd.Flags().StringVarP(&dockerImageTag, "image-tag", "i", "", "docker image tag") +_ = cmd.MarkFlagRequired("image-tag")cmd/prune.go (3)
24-27: Avoid empty short-hands and PersistentFlags; prefer Flags() with long names
- Using BoolP/IntP with an empty shorthand is confusing; prefer Bool/Int with long flag names.
- PersistentFlags are unnecessary since prune has no subcommands; use Flags for readability.
-cmd.PersistentFlags().BoolP("dry-run", "", false, "dry run") -cmd.PersistentFlags().IntP("keep-last", "", defaultLastNumberOfTasks, "keep last N task definitions") -cmd.PersistentFlags().IntP("keep-days", "", defaultLastDays, "keep task definitions older than N days") +cmd.Flags().Bool("dry-run", false, "dry run") +cmd.Flags().Int("keep-last", defaultLastNumberOfTasks, "keep last N task definitions") +cmd.Flags().Int("keep-days", defaultLastDays, "keep task definitions older than N days")
16-23: Prefer RunE with error returns over log.Fatal in handlersStandardizing on RunE improves testability and consistent error handling. Replace log.Fatalf/Fatalln with returned errors.
func newPruneCommand() *cobra.Command { cmd := &cobra.Command{ Use: "prune", Short: "Deregister active task definitions", DisableFlagsInUseLine: true, - Run: pruneHandler, + RunE: pruneHandler, } ... } -func pruneHandler(cmd *cobra.Command, args []string) { +func pruneHandler(cmd *cobra.Command, args []string) error { keepLastNr, _ := cmd.Flags().GetInt("keep-last") keepDays, _ := cmd.Flags().GetInt("keep-days") dryRun, _ := cmd.Flags().GetBool("dry-run") // Set up context that cancels on interrupt signal for cancellable prune operations ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer cancel() clients, err := ecs.NewAWSClients() if err != nil { - log.Fatalf("Failed to initialize AWS clients: %v\n", err) + return fmt.Errorf("failed to initialize AWS clients: %w", err) } cluster, service := parseServiceFlag() result, err := ecs.Prune(ctx, clients, cluster, service, keepLastNr, keepDays, dryRun) if err != nil { - log.Fatalln(err) + return err } ... + return nil }Also applies to: 30-49
78-85: Optional: enrich the summary with kept/skipped countsYou already compute KeptCount/SkippedCount in the result. Consider surfacing them for a fuller summary.
cmd/run.go (3)
1-3: Remove non-standard “ABOUTME” commentsThese aren’t idiomatic GoDoc and add noise. If you want a package-level comment, use a proper doc comment above “package cmd”.
-// ABOUTME: Command-line interface for running ECS tasks -// ABOUTME: Handles user input and output formatting for task execution -Optionally add:
// Package cmd contains Cobra commands for the runecs CLI.
22-28: Adopt RunE and return errors; avoid log.Fatal in command flowThis aligns with Cobra best practices and eases testing.
cmd := &cobra.Command{ Use: "run [cmd]", Short: "Execute a one-off process in an AWS ECS cluster", Args: cobra.MinimumNArgs(1), DisableFlagsInUseLine: true, - Run: runHandler(&dockerImageTag, &execWait), + RunE: runHandler(&dockerImageTag, &execWait), } ... -func runHandler(dockerImageTag *string, execWait *bool) func(*cobra.Command, []string) { - return func(cmd *cobra.Command, args []string) { +func runHandler(dockerImageTag *string, execWait *bool) func(*cobra.Command, []string) error { + return func(cmd *cobra.Command, args []string) error { cluster, service := parseServiceFlag() ... clients, err := ecs.NewAWSClients() if err != nil { - log.Fatalf("Failed to initialize AWS clients: %v\n", err) + return fmt.Errorf("failed to initialize AWS clients: %w", err) } result, err := ecs.Execute(ctx, clients, cluster, service, args, *execWait, *dockerImageTag) if err != nil { - log.Fatalln(err) + return err } ... - } + return nil + } }Also applies to: 35-35, 43-51, 78-78
30-31: Use non-persistent flags on leaf commandsThere are no subcommands under run; use Flags instead of PersistentFlags.
-cmd.PersistentFlags().BoolVarP(&execWait, "wait", "w", false, "wait for task to finish") -cmd.PersistentFlags().StringVarP(&dockerImageTag, "image-tag", "i", "", "docker image tag") +cmd.Flags().BoolVarP(&execWait, "wait", "w", false, "wait for task to finish") +cmd.Flags().StringVarP(&dockerImageTag, "image-tag", "i", "", "docker image tag")cmd/restart.go (2)
16-23: Switch to RunE and return errors; avoid log.FatalfConsistent with other commands, prefer returning errors.
func newRestartCommand() *cobra.Command { cmd := &cobra.Command{ Use: "restart", Short: "Restart the service", DisableFlagsInUseLine: true, - Run: restartHandler, + RunE: restartHandler, } ... } -func restartHandler(cmd *cobra.Command, args []string) { +func restartHandler(cmd *cobra.Command, args []string) error { kill, _ := cmd.Flags().GetBool("kill") ... clients, err := ecs.NewAWSClients() if err != nil { - log.Fatalf("Failed to initialize AWS clients: %v\n", err) + return fmt.Errorf("failed to initialize AWS clients: %w", err) } cluster, service := parseServiceFlag() result, err := ecs.Restart(ctx, clients, cluster, service, kill) if err != nil { - log.Fatalf("Restart failed: %v\n", err) + return fmt.Errorf("restart failed: %w", err) } ... - fmt.Println("Done.") + fmt.Println("Done.") + return nil }Also applies to: 28-44, 54-55
24-25: Avoid empty shorthand and PersistentFlags; use Flags().BoolNo subcommands under restart, and an empty shorthand is confusing.
-cmd.PersistentFlags().BoolP("kill", "", false, "Stops running tasks, ECS starts a new one if the health check is properly set") +cmd.Flags().Bool("kill", false, "Stops running tasks; ECS starts a new one if health checks pass")cmd/main.go (2)
32-48: Service flag validation is simple and effective; consider future-proofing the allowlistThe allowlist approach works. As the CLI grows, consider annotating commands that do not require --service instead of hardcoding names in one place. For example, set cmd.Annotations["serviceRequired"]="false" on list/completion/version and consult it here. This reduces the chance of drift when adding commands.
55-71: Avoid global rootCmd flag lookups and fatal exits inside helpers
- parseServiceFlag tightly couples to rootCmd; accepting *cobra.Command and reading cmd.Flags() decouples and improves testability.
- Prefer returning an error rather than calling log.Fatalf inside a utility function.
Proposed refactor:
-func parseServiceFlag() (cluster, service string) { - serviceValue := rootCmd.Flag("service").Value.String() +func parseServiceFlag(cmd *cobra.Command) (cluster, service string, err error) { + serviceValue, _ := cmd.Flags().GetString("service") parsed := strings.Split(serviceValue, "/") if len(parsed) == 2 { - cluster = parsed[0] - service = parsed[1] + cluster = parsed[0] + service = parsed[1] } else if serviceValue != "" { - log.Fatalf("Invalid service name %s\n", serviceValue) + return "", "", fmt.Errorf("invalid service name %q; expected format cluster/service", serviceValue) } - if cluster == "" || service == "" { - log.Fatalf("Missing cluster or service configuration\n") + if cluster == "" || service == "" { + return "", "", fmt.Errorf("missing cluster or service configuration") } - return cluster, service + return cluster, service, nil }Follow-up: update call sites in command handlers to
cluster, service, err := parseServiceFlag(cmd)and handle err (works best with RunE).pkg/ecs/revisions.go (1)
115-117: Silent error handling may hide API failuresContinuing silently when
DescribeTaskDefinitionfails could hide important API errors like rate limiting or permission issues. Consider at least logging these errors for debugging purposes.Consider logging the error before continuing:
if err != nil { - // Log error but continue processing other revisions + // TODO: Consider logging this error for debugging + // fmt.Fprintf(os.Stderr, "Warning: Failed to describe task definition %s: %v\n", def, err) continue }cmd/list.go (1)
86-96: Service name is repeated for each task unnecessarilyThe current implementation repeats the full service name for every task row, which creates visual clutter when a service has multiple tasks.
Consider showing the service name only once per service group:
for _, cluster := range clusters { for _, service := range cluster.Services { boldServiceName := lipgloss.NewStyle().Bold(true).Render(service.Name) serviceName := fmt.Sprintf("%s/%s", service.ClusterName, boldServiceName) - for _, task := range service.Tasks { + for i, task := range service.Tasks { + displayName := serviceName + if i > 0 { + // Use empty string or placeholder for subsequent tasks + displayName = "" + } rows = append(rows, []string{ - serviceName, + displayName, task.ID, task.CPU, task.Memory, task.RunningTime, }) } } }pkg/ecs/logs.go (1)
65-81: Complex timestamp calculation logic could be simplifiedThe nested conditions for calculating
lastEventTimestampare hard to follow and contain redundant checks.Simplify the timestamp calculation:
var lastEventTimestamp int64 - if len(output.Events) > 0 { - lastEvent := output.Events[len(output.Events)-1] - if lastEvent.Timestamp == nil { - if startTime != nil { - lastEventTimestamp = *startTime - } else { - lastEventTimestamp = 0 - } - } else { - lastEventTimestamp = *lastEvent.Timestamp + 1 - } - } else if startTime != nil { - lastEventTimestamp = *startTime - } else { - lastEventTimestamp = 0 - } + // Default to startTime or 0 + lastEventTimestamp = 0 + if startTime != nil { + lastEventTimestamp = *startTime + } + + // If we have events with valid timestamp, use the last one + 1 + if len(output.Events) > 0 { + lastEvent := output.Events[len(output.Events)-1] + if lastEvent.Timestamp != nil { + lastEventTimestamp = *lastEvent.Timestamp + 1 + } + }pkg/ecs/deploy.go (2)
51-59: Guard against nil/empty container definitions after copy.Depending on copier behavior, ContainerDefinitions might be empty and indexing [0] would panic. Add a sanity check.
newDef := &ecs.RegisterTaskDefinitionInput{} if err := copier.Copy(newDef, response.TaskDefinition); err != nil { return "", err } + if len(newDef.ContainerDefinitions) == 0 { + return "", errors.New("copied task definition has no container definitions") + } + if containerDef.Image == nil { return "", errors.New("container definition has no image specified") }
77-101: Use idiomatic local var naming (lowerCamelCase) for clarity.Local identifiers should be lowerCamelCase. Rename TaskDefinitionArn to taskDefinitionArn.
- TaskDefinitionArn, err := cloneTaskDef(ctx, cluster, service, dockerImageTag, clients.ECS) + taskDefinitionArn, err := cloneTaskDef(ctx, cluster, service, dockerImageTag, clients.ECS) @@ - TaskDefinition: &TaskDefinitionArn, + TaskDefinition: &taskDefinitionArn, @@ return &DeployResult{ - TaskDefinitionArn: TaskDefinitionArn, + TaskDefinitionArn: taskDefinitionArn, ServiceArn: *updateOutput.Service.ServiceArn, }, nilpkg/ecs/execute.go (5)
168-175: Prefer typed SDK enums over raw strings.Use types.LaunchTypeFargate and types.AssignPublicIpEnabled for stronger typing and future-proofing.
- LaunchType: "FARGATE", + LaunchType: types.LaunchTypeFargate, @@ - AssignPublicIp: "ENABLED", // FIXME: Public IP is not needed (mostly) + AssignPublicIp: types.AssignPublicIpEnabled, // FIXME: consider inheriting from service config
41-46: Confirm intent: using latest task definition vs the service’s current one.describeService picks latestTaskDefinitionArn rather than serviceInfo.TaskDefinition. If the latest registered TD is incompatible with the service (e.g., different compatibilities), RunTask may fail. If intentional, consider documenting this in the function comment; otherwise, use the service’s currently set task definition.
81-86: Log config options may be absent; current read is safe but consider explicit presence.Indexing a nil map is safe (zero value), but being explicit can aid readability when options are missing.
217-221: Best-effort log streaming hides errors.Swallowing log retrieval errors may hide useful diagnostics. Consider capturing the error in result metadata or logging it at debug level for operators.
169-175: Network config: inherit AssignPublicIp from service.Hardcoding ENALBED can cause unexpected public exposure. You already fetch NetworkConfiguration; consider adding AssignPublicIp to ServiceDefinition and reusing it.
Would you like a small refactor that adds AssignPublicIp to ServiceDefinition and wires it here?
pkg/ecs/prune.go (1)
31-35: Optional: make time deterministic for tests.Inject “now” via parameter or a clock interface instead of time.Now().UTC() to ease unit testing.
pkg/ecs/restart.go (1)
24-59: Consider surfacing per-task StopTask errors.Currently, errors are silently ignored. If useful, capture them as a separate slice or annotate StoppedTaskInfo to aid diagnostics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
.gitignore(1 hunks)cmd/completion.go(1 hunks)cmd/deploy.go(1 hunks)cmd/list.go(1 hunks)cmd/main.go(4 hunks)cmd/prune.go(1 hunks)cmd/restart.go(1 hunks)cmd/revisions.go(1 hunks)cmd/run.go(1 hunks)cmd/version.go(1 hunks)go.mod(2 hunks)main.go(1 hunks)pkg/ecs/client.go(1 hunks)pkg/ecs/core.go(1 hunks)pkg/ecs/deploy.go(3 hunks)pkg/ecs/exec.go(0 hunks)pkg/ecs/execute.go(1 hunks)pkg/ecs/list.go(1 hunks)pkg/ecs/logs.go(3 hunks)pkg/ecs/prune.go(5 hunks)pkg/ecs/restart.go(1 hunks)pkg/ecs/revision.go(0 hunks)pkg/ecs/revisions.go(1 hunks)pkg/ecs/service.go(2 hunks)pkg/ecs/types.go(1 hunks)pkg/ecs/utils.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/ecs/exec.go
- pkg/ecs/revision.go
🧰 Additional context used
🧬 Code Graph Analysis (15)
pkg/ecs/client.go (1)
pkg/ecs/types.go (1)
AWSClients(28-31)
cmd/restart.go (3)
pkg/ecs/client.go (1)
NewAWSClients(28-44)pkg/ecs/restart.go (1)
Restart(93-114)pkg/ecs/types.go (1)
TaskDefinition(41-47)
pkg/ecs/execute.go (1)
pkg/ecs/types.go (5)
ServiceDefinition(34-38)TaskDefinition(41-47)AWSClients(28-31)ExecuteResult(57-64)LogEntry(50-54)
cmd/run.go (3)
pkg/ecs/client.go (1)
NewAWSClients(28-44)cmd/main.go (1)
Execute(73-78)pkg/ecs/types.go (1)
TaskDefinition(41-47)
cmd/revisions.go (2)
pkg/ecs/client.go (1)
NewAWSClients(28-44)pkg/ecs/revisions.go (1)
Revisions(151-174)
cmd/prune.go (2)
pkg/ecs/client.go (1)
NewAWSClients(28-44)pkg/ecs/prune.go (1)
Prune(144-181)
cmd/list.go (3)
pkg/ecs/client.go (1)
NewAWSClients(28-44)pkg/ecs/list.go (1)
GetClusters(119-166)pkg/ecs/types.go (1)
ClusterInfo(135-138)
cmd/deploy.go (2)
pkg/ecs/client.go (1)
NewAWSClients(28-44)pkg/ecs/deploy.go (1)
Deploy(77-102)
pkg/ecs/revisions.go (1)
pkg/ecs/types.go (3)
RevisionEntry(67-72)AWSClients(28-31)RevisionsResult(75-77)
pkg/ecs/types.go (1)
pkg/ecs/revisions.go (1)
Revisions(151-174)
pkg/ecs/logs.go (1)
pkg/ecs/types.go (1)
LogEntry(50-54)
pkg/ecs/prune.go (1)
pkg/ecs/types.go (4)
TaskDefinitionPruneEntry(100-106)TaskDefinition(41-47)AWSClients(28-31)PruneResult(109-117)
pkg/ecs/list.go (1)
pkg/ecs/types.go (4)
TaskInfo(120-125)AWSClients(28-31)ClusterInfo(135-138)ServiceInfo(128-132)
pkg/ecs/deploy.go (1)
pkg/ecs/types.go (3)
TaskDefinition(41-47)AWSClients(28-31)DeployResult(80-83)
pkg/ecs/restart.go (1)
pkg/ecs/types.go (4)
StoppedTaskInfo(94-97)TaskDefinition(41-47)AWSClients(28-31)RestartResult(86-91)
🔇 Additional comments (16)
pkg/ecs/core.go (1)
32-38: LGTM: value-returning helper is simple and safeClear semantics, avoids pointer pitfalls, and provides an error on empty slices.
pkg/ecs/types.go (1)
27-31: Centralizing shared AWS clients struct is a solid moveDefining AWSClients in one place improves cohesion and reduces duplication across ECS operations.
main.go (1)
1-1: License header update looks goodNo functional changes; wiring to cmd package remains clean and minimal.
cmd/completion.go (1)
10-19: Good use of Cobra arg validation and clean command factoryCommand signature and validation (ExactArgs + OnlyValidArgs) keep UX tight and prevent misuse.
cmd/deploy.go (1)
42-45: Good use of cancellable context with signal.NotifyContextThis is idiomatic and ensures long-running AWS operations can be interrupted cleanly.
cmd/prune.go (1)
35-38: Cancellable context set-up looks goodGraceful cancellation on SIGINT/SIGTERM is correctly handled.
cmd/run.go (1)
39-41: Cancellable context handling is solidClean, idiomatic Ctrl+C handling.
cmd/restart.go (2)
31-34: Cancellable context set-up: goodInterrupt handling is implemented correctly.
46-52: Output UX is clear and helpfulThe humanized timestamp for stopped tasks is a nice touch. Messaging for the force-deploy path is concise.
cmd/main.go (1)
50-53: Root-level service flag and completion settings look goodPlacing the “service” flag at the root ensures consistent availability across subcommands.
pkg/ecs/revisions.go (1)
106-108: Potential off-by-one error in revision count limit checkThe condition
lastRevisionsNr < (total+1)is equivalent tolastRevisionsNr <= total, which means it breaks when the current count equals the limit. This causes the function to return one fewer revision than requested.Apply this fix to correctly limit the number of revisions:
- if lastRevisionsNr != 0 && lastRevisionsNr < (total+1) { + if lastRevisionsNr != 0 && total >= lastRevisionsNr {Likely an incorrect or invalid review comment.
cmd/list.go (1)
55-76: LGTM! Clean separation of display logicThe function properly handles the display of services with clear formatting using lipgloss styles.
pkg/ecs/logs.go (1)
24-30: LGTM! Clean function signature with proper parameter organizationThe refactored function signature properly separates concerns by accepting the CloudWatch client as a parameter, following dependency injection best practices.
pkg/ecs/prune.go (1)
155-181: Result aggregation looks good.The final KeptCount derivation is correct once skipped is tracked explicitly in deregisterTaskFamily.
pkg/ecs/restart.go (2)
61-91: Validate service update response thoroughly.Good defensive checks for nil Service, ServiceArn, and TaskDefinition. LGTM.
93-114: Overall Restart orchestration is clean and cohesive.Clear branching between kill and force_deploy with structured results and error wrapping. Nice.
| fmt.Printf("Task definition %s created %d days ago was skipped (%s)\n", | ||
| arnStyle.Render(task.Arn), task.DaysOld, task.Reason) | ||
| case "deleted": |
There was a problem hiding this comment.
Inconsistent action messaging: “kept” tasks are printed as “skipped”
This will confuse users and misrepresents actions. The “kept” case should say “was kept”, not “was skipped”.
- fmt.Printf("Task definition %s created %d days ago was skipped (%s)\n",
+ fmt.Printf("Task definition %s created %d days ago was kept (%s)\n",
arnStyle.Render(task.Arn), task.DaysOld, task.Reason)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Printf("Task definition %s created %d days ago was skipped (%s)\n", | |
| arnStyle.Render(task.Arn), task.DaysOld, task.Reason) | |
| case "deleted": | |
| fmt.Printf("Task definition %s created %d days ago was kept (%s)\n", | |
| arnStyle.Render(task.Arn), task.DaysOld, task.Reason) | |
| case "deleted": |
🤖 Prompt for AI Agents
In cmd/prune.go around lines 61 to 63, the log message for tasks with action
"kept" currently prints "was skipped"; update the fmt.Printf format string to
say "was kept" (e.g., change "was skipped" to "was kept") so the output
correctly reflects the action and uses the same arnStyle.Render(task.Arn),
task.DaysOld, task.Reason parameters.
| dockerParts := strings.Split(revision.DockerURI, ":") | ||
| if len(dockerParts) >= 2 { | ||
| repo := strings.Join(dockerParts[:len(dockerParts)-1], ":") | ||
| tag := dockerParts[len(dockerParts)-1] | ||
| styledTag := boldStyle.Render(tag) | ||
| fmt.Printf("%s: %s:%s\n", formattedDate, repo, styledTag) | ||
| } else { | ||
| fmt.Printf("%s: %s\n", formattedDate, revision.DockerURI) | ||
| } |
There was a problem hiding this comment.
Potential issue with Docker URI parsing for images without tags
The current logic assumes the last colon separates the repository from the tag, but this breaks for registry URLs with ports (e.g., localhost:5000/repo). Also, some images might not have tags at all.
Use a more robust approach to parse Docker URIs:
// Split DockerURI to extract and style the tag
- dockerParts := strings.Split(revision.DockerURI, ":")
- if len(dockerParts) >= 2 {
- repo := strings.Join(dockerParts[:len(dockerParts)-1], ":")
- tag := dockerParts[len(dockerParts)-1]
- styledTag := boldStyle.Render(tag)
- fmt.Printf("%s: %s:%s\n", formattedDate, repo, styledTag)
+ // Handle registry URLs with ports and images without tags
+ lastColonIdx := strings.LastIndex(revision.DockerURI, ":")
+ lastSlashIdx := strings.LastIndex(revision.DockerURI, "/")
+
+ // If colon appears after the last slash and isn't followed by a port number
+ if lastColonIdx > lastSlashIdx && lastColonIdx > 0 {
+ repo := revision.DockerURI[:lastColonIdx]
+ tag := revision.DockerURI[lastColonIdx+1:]
+ // Check if it's likely a port number (all digits)
+ if !isLikelyPort(tag) {
+ styledTag := boldStyle.Render(tag)
+ fmt.Printf("%s: %s:%s\n", formattedDate, repo, styledTag)
+ } else {
+ fmt.Printf("%s: %s\n", formattedDate, revision.DockerURI)
+ }
} else {
fmt.Printf("%s: %s\n", formattedDate, revision.DockerURI)
}And add a helper function:
func isLikelyPort(s string) bool {
for _, r := range s {
if r < '0' || r > '9' {
return false
}
}
return len(s) > 0 && len(s) <= 5
}🤖 Prompt for AI Agents
In cmd/revisions.go around lines 67 to 75, the Docker URI splitting incorrectly
treats the last colon as a tag separator (breaking URIs with registry ports and
images without tags); change the logic to locate the last '/' and only treat a
colon after that slash as a tag separator if the segment after the colon is not
a numeric port (add the provided isLikelyPort helper), extract repo and tag
accordingly (if no tag present, treat as no tag or use default handling), and
render the tag with boldStyle when present; update the else branch to print the
original DockerURI when no tag is detected.
| github.com/aws/aws-sdk-go-v2/config v1.27.43 | ||
| github.com/aws/aws-sdk-go-v2/service/cloudwatchlogs v1.39.1 | ||
| github.com/aws/aws-sdk-go-v2/service/ecs v1.45.5 | ||
| github.com/charmbracelet/lipgloss v1.1.0 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Terminal styling deps: ensure graceful degradation on non-TTY/CI
With lipgloss and related terminal deps added, please ensure commands detect non-interactive environments (CI, redirected output) and disable styling accordingly (e.g., check isatty, honor NO_COLOR, TERM=dumb).
You can sanity-check for explicit handling with this script:
If missing, consider:
- Disabling colors/styles when not a TTY
- Honoring NO_COLOR (https://no-color.org/) and TERM=dumb
- Providing a global --no-color / --plain flag for all commands
Also applies to: 22-36
🏁 Script executed:
#!/bin/bash
# Look for basic TTY/NO_COLOR handling hints
rg -n -C2 -P '\bNO_COLOR\b|\bTERM\b|isatty|termenv|EnableColor|DisableColor|HasDarkBackground|Supports256Color|SupportsTrueColor|lipgloss\.' \
-g '!**/vendor/**' -g '!**/dist/**'Length of output: 3662
Ensure non-TTY/CI environments disable lipgloss styling
I ran the provided ripgrep check and didn’t find any handling for isatty/termenv detection, NO_COLOR, TERM=dumb or a global “no-color” flag in the code. As it stands, all commands will emit ANSI styling even when output is redirected or running in CI.
Please refactor to disable lipgloss styles when not on a true TTY or when the user opts out. For example, in your root/main package:
import (
"os"
"github.com/muesli/termenv"
"github.com/charmbracelet/lipgloss"
)
func init() {
// disable styling if stdout is not a terminal, NO_COLOR is set, or TERM=dumb
if !termenv.IsTerminal(os.Stdout.Fd()) ||
os.Getenv("NO_COLOR") != "" ||
os.Getenv("TERM") == "dumb" {
lipgloss.SetColorProfile(lipgloss.ColorProfileNone)
}
}Additionally, consider adding a global --no-color (or --plain) flag to override styling in all commands.
- Entry point (e.g.,
cmd/root.goormain.go): add environment/T TY checks and calllipgloss.SetColorProfile(lipgloss.ColorProfileNone). - Update documentation/CLI help to mention NO_COLOR, TERM=dumb, and the
--no-colorflag. - Verify in CI (and with
| tee) that ANSI codes are omitted when expected.
🤖 Prompt for AI Agents
In go.mod around line 12 and in the app entry point (e.g., cmd/root.go or
main.go), ensure lipgloss styling is disabled when not on a real TTY or when the
user opts out: add an init (or early setup) that imports os,
github.com/muesli/termenv and github.com/charmbracelet/lipgloss, then check if
!termenv.IsTerminal(os.Stdout.Fd()) || os.Getenv("NO_COLOR") != "" ||
os.Getenv("TERM") == "dumb" and call
lipgloss.SetColorProfile(lipgloss.ColorProfileNone); also add a global CLI flag
(e.g., --no-color or --plain) that, when set, forces ColorProfileNone and
document NO_COLOR, TERM=dumb and the flag in CLI help and README so CI/non-TTY
use emits no ANSI codes.
| split := strings.Split(*containerDef.Image, ":") | ||
| newDockerURI := fmt.Sprintf("%s:%s", split[0], dockerImageTag) | ||
|
|
There was a problem hiding this comment.
Fix Docker image parsing: handle registries with ports and digests.
Splitting on the first colon breaks images like registry:5000/repo:tag and digest forms name@sha256:.... Use last colon after stripping any digest.
Apply this diff:
- split := strings.Split(*containerDef.Image, ":")
- newDockerURI := fmt.Sprintf("%s:%s", split[0], dockerImageTag)
+ // Strip any digest suffix and replace only the tag portion.
+ base := *containerDef.Image
+ if idx := strings.Index(base, "@"); idx != -1 {
+ base = base[:idx]
+ }
+ if idx := strings.LastIndex(base, ":"); idx != -1 {
+ base = base[:idx]
+ }
+ newDockerURI := fmt.Sprintf("%s:%s", base, dockerImageTag)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| split := strings.Split(*containerDef.Image, ":") | |
| newDockerURI := fmt.Sprintf("%s:%s", split[0], dockerImageTag) | |
| // Strip any digest suffix and replace only the tag portion. | |
| base := *containerDef.Image | |
| if idx := strings.Index(base, "@"); idx != -1 { | |
| base = base[:idx] | |
| } | |
| if idx := strings.LastIndex(base, ":"); idx != -1 { | |
| base = base[:idx] | |
| } | |
| newDockerURI := fmt.Sprintf("%s:%s", base, dockerImageTag) |
🤖 Prompt for AI Agents
In pkg/ecs/deploy.go around lines 60 to 62, the current split on ":" breaks
images with registry ports and digest forms; change the logic to first check for
a digest by looking for "@" and if present separate and keep the name portion,
then find the last index of ":" in the name portion (not the entire string) to
determine if a tag exists; if a last-colon is found treat everything before it
as the image name and replace the tag with dockerImageTag, otherwise append ":"
+ dockerImageTag, and if a digest was present preserve or remove it per intent
(most likely remove digest when replacing tag) so registry:5000/repo:oldtag and
name@sha256:... are handled correctly without splitting on registry ports.
| // Výpočet délky běhu úlohy | ||
| var runningTime string | ||
| if task.StartedAt != nil { | ||
| duration := time.Since(*task.StartedAt) | ||
| runningTime = fmt.Sprintf("%dh %dm %ds", int(duration.Hours()), int(duration.Minutes())%60, int(duration.Seconds())%60) | ||
| runningTime = formatRunningTime(duration) | ||
| } else { | ||
| runningTime = "Unknown" | ||
| } |
There was a problem hiding this comment.
Comment in non-English language
Line 93 contains a comment in Czech ("Výpočet délky běhu úlohy") which should be in English for consistency.
- // Výpočet délky běhu úlohy
+ // Calculate task running time📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Výpočet délky běhu úlohy | |
| var runningTime string | |
| if task.StartedAt != nil { | |
| duration := time.Since(*task.StartedAt) | |
| runningTime = fmt.Sprintf("%dh %dm %ds", int(duration.Hours()), int(duration.Minutes())%60, int(duration.Seconds())%60) | |
| runningTime = formatRunningTime(duration) | |
| } else { | |
| runningTime = "Unknown" | |
| } | |
| // Calculate task running time | |
| var runningTime string | |
| if task.StartedAt != nil { | |
| duration := time.Since(*task.StartedAt) | |
| runningTime = formatRunningTime(duration) | |
| } else { | |
| runningTime = "Unknown" | |
| } |
🤖 Prompt for AI Agents
In pkg/ecs/list.go around lines 93 to 100, the inline comment "Výpočet délky
běhu úlohy" is in Czech and should be converted to English for consistency;
replace it with a concise English comment such as "Calculate task running time"
(or similar), keeping the surrounding logic unchanged and ensuring comment style
matches the file's existing English comments.
| CPU: *task.Cpu, | ||
| Memory: *task.Memory, | ||
| RunningTime: runningTime, |
There was a problem hiding this comment.
Missing nil checks for required task fields
The code dereferences task.Cpu and task.Memory pointers without checking if they're nil, which could cause a panic.
Add nil checks before dereferencing:
+ cpu := "Unknown"
+ if task.Cpu != nil {
+ cpu = *task.Cpu
+ }
+ memory := "Unknown"
+ if task.Memory != nil {
+ memory = *task.Memory
+ }
+
tasks = append(tasks, TaskInfo{
ID: taskID,
- CPU: *task.Cpu,
- Memory: *task.Memory,
+ CPU: cpu,
+ Memory: memory,
RunningTime: runningTime,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CPU: *task.Cpu, | |
| Memory: *task.Memory, | |
| RunningTime: runningTime, | |
| @@ pkg/ecs/list.go | |
| // before appending, coalesce CPU & Memory to avoid nil deref | |
| cpu := "Unknown" | |
| if task.Cpu != nil { | |
| cpu = *task.Cpu | |
| } | |
| memory := "Unknown" | |
| if task.Memory != nil { | |
| memory = *task.Memory | |
| } | |
| tasks = append(tasks, TaskInfo{ | |
| ID: taskID, | |
| - CPU: *task.Cpu, | |
| CPU: cpu, | |
| Memory: memory, | |
| RunningTime: runningTime, | |
| }) |
🤖 Prompt for AI Agents
In pkg/ecs/list.go around lines 109-111, the code dereferences task.Cpu and
task.Memory without nil checks which can panic; add explicit nil checks before
dereferencing and handle the nil case (either return an error with context or
set a safe default like 0 depending on function semantics). Specifically,
validate task.Cpu and task.Memory are non-nil, log or wrap a clear error and
return it if they are required, or use a fallback value when appropriate, then
only dereference after those checks.
| parts := strings.Split(serviceArn, "/") | ||
| link := fmt.Sprintf("%s/%s", parts[1], parts[2]) | ||
| tasks := listTasks(ctx, svc, parts[1], parts[2]) | ||
| clusterName := parts[1] | ||
| serviceName := parts[2] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fragile ARN parsing using string split
The code assumes service ARNs always have exactly 3 parts when split by "/", which could break with different ARN formats.
Use the existing extractARNResource helper for consistent ARN parsing:
- parts := strings.Split(serviceArn, "/")
- clusterName := parts[1]
- serviceName := parts[2]
+ // Service ARN format: arn:aws:ecs:region:account:service/cluster-name/service-name
+ // Extract the resource part: cluster-name/service-name
+ resource, err := extractARNResource(serviceArn)
+ if err != nil {
+ return nil, fmt.Errorf("failed to parse service ARN %s: %w", serviceArn, err)
+ }
+ parts := strings.SplitN(resource, "/", 2)
+ if len(parts) != 2 {
+ return nil, fmt.Errorf("unexpected service ARN format: %s", serviceArn)
+ }
+ clusterName := parts[0]
+ serviceName := parts[1]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parts := strings.Split(serviceArn, "/") | |
| link := fmt.Sprintf("%s/%s", parts[1], parts[2]) | |
| tasks := listTasks(ctx, svc, parts[1], parts[2]) | |
| clusterName := parts[1] | |
| serviceName := parts[2] | |
| // Service ARN format: arn:aws:ecs:region:account:service/cluster-name/service-name | |
| // Extract the resource part: cluster-name/service-name | |
| resource, err := extractARNResource(serviceArn) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse service ARN %s: %w", serviceArn, err) | |
| } | |
| parts := strings.SplitN(resource, "/", 2) | |
| if len(parts) != 2 { | |
| return nil, fmt.Errorf("unexpected service ARN format: %s", serviceArn) | |
| } | |
| clusterName := parts[0] | |
| serviceName := parts[1] |
🤖 Prompt for AI Agents
In pkg/ecs/list.go around lines 134 to 136, the code fragily parses the service
ARN with strings.Split and assumes three parts; replace this with the existing
extractARNResource helper to parse the ARN robustly, call
extractARNResource(serviceArn) and use its returned resource (or check for
error/empty return) to derive clusterName and serviceName safely, and handle any
error/empty result by logging/returning instead of indexing into a split array.
| for _, family := range response { | ||
| revisions, err := getRevisions(ctx, family, lastRevisionNr, clients.ECS) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| allRevisions = append(allRevisions, revisions...) | ||
| } |
There was a problem hiding this comment.
Early termination logic doesn't work correctly across multiple families
The lastRevisionNr parameter limits revisions per family rather than globally. If you have 2 families and request 5 revisions, you could get up to 10 revisions (5 from each family).
To fix this, track the total count across all families:
var allRevisions []RevisionEntry
+ totalCollected := 0
for _, family := range response {
- revisions, err := getRevisions(ctx, family, lastRevisionNr, clients.ECS)
+ remainingCount := 0
+ if lastRevisionNr > 0 {
+ remainingCount = lastRevisionNr - totalCollected
+ if remainingCount <= 0 {
+ break
+ }
+ }
+ revisions, err := getRevisions(ctx, family, remainingCount, clients.ECS)
if err != nil {
return nil, err
}
allRevisions = append(allRevisions, revisions...)
+ totalCollected += len(revisions)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, family := range response { | |
| revisions, err := getRevisions(ctx, family, lastRevisionNr, clients.ECS) | |
| if err != nil { | |
| return nil, err | |
| } | |
| allRevisions = append(allRevisions, revisions...) | |
| } | |
| var allRevisions []RevisionEntry | |
| totalCollected := 0 | |
| for _, family := range response { | |
| remainingCount := 0 | |
| if lastRevisionNr > 0 { | |
| remainingCount = lastRevisionNr - totalCollected | |
| if remainingCount <= 0 { | |
| break | |
| } | |
| } | |
| revisions, err := getRevisions(ctx, family, remainingCount, clients.ECS) | |
| if err != nil { | |
| return nil, err | |
| } | |
| allRevisions = append(allRevisions, revisions...) | |
| totalCollected += len(revisions) | |
| } |
🤖 Prompt for AI Agents
In pkg/ecs/revisions.go around lines 163-169, the loop uses lastRevisionNr as a
per-family limit causing up to (families * lastRevisionNr) results; instead
track total collected across families and stop when the global limit is reached:
before each getRevisions call compute remaining := lastRevisionNr -
len(allRevisions) and if remaining <= 0 break; then either pass remaining into
getRevisions (add a limit parameter to getRevisions) or call getRevisions
normally and append only up to remaining items (slice the returned revisions to
remaining), then break when the global limit is satisfied.
…lity Split the monolithic 837-line service.go file into 7 focused modules, dramatically improving code organization and maintainability: • client.go - AWS client initialization and configuration • deploy.go - Deployment operations and task definition cloning • execute.go - Task execution logic and monitoring • logs.go - CloudWatch logs management (renamed getProcessLogs → getTaskLogs) • prune.go - Task definition cleanup and lifecycle management • restart.go - Service restart and recovery operations • revisions.go - Task definition history and revision management Each module now has a single, clear responsibility with 44-244 lines, making the codebase much more readable and easier to maintain. The original service.go is replaced with documentation explaining the new structure.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
pkg/ecs/client.go (1)
31-33: Retryer configuration fixed — compiles with AWS SDK v2 nowSwitch to retry.NewStandard with options resolves the prior compile error. LGTM.
pkg/ecs/revisions.go (2)
98-146: Stop paginating once the per-call limit is reached in getRevisionsThe inner break exits only the for-range; the outer pagination loop continues issuing requests unnecessarily. Add a top-level limit check and break the paging loop as soon as the limit is reached.
- for { - response, err := svc.ListTaskDefinitions(ctx, definitionInput) + for { + if lastRevisionsNr > 0 && total >= lastRevisionsNr { + break + } + response, err := svc.ListTaskDefinitions(ctx, definitionInput) @@ - for _, def := range response.TaskDefinitionArns { - if lastRevisionsNr != 0 && lastRevisionsNr < (total+1) { - break - } + for _, def := range response.TaskDefinitionArns { + if lastRevisionsNr > 0 && total >= lastRevisionsNr { + break + } @@ - revisions = append(revisions, RevisionEntry{ + revisions = append(revisions, RevisionEntry{ Revision: resp.TaskDefinition.Revision, CreatedAt: *resp.TaskDefinition.RegisteredAt, DockerURI: *containerDef.Image, Family: familyPrefix, }) total++ } - if response.NextToken == nil { + if response.NextToken == nil { break } definitionInput.NextToken = response.NextToken }
162-169: Global “lastRevisionNr” limit is enforced per-family — can over-returnAs previously noted, passing lastRevisionNr to each family yields up to families*lastRevisionNr results. Track a global count and pass a per-family remaining count instead.
- var allRevisions []RevisionEntry - for _, family := range response { - revisions, err := getRevisions(ctx, family, lastRevisionNr, clients.ECS) + var allRevisions []RevisionEntry + totalCollected := 0 + for _, family := range response { + remaining := 0 + if lastRevisionNr > 0 { + remaining = lastRevisionNr - totalCollected + if remaining <= 0 { + break + } + } + revisions, err := getRevisions(ctx, family, remaining, clients.ECS) if err != nil { return nil, err } allRevisions = append(allRevisions, revisions...) + totalCollected += len(revisions) }pkg/ecs/deploy.go (1)
60-63: Fix Docker image parsing: handle registry ports and digestsSplitting on “:” breaks for registry:5000/repo:tag and name@sha256:... forms. Replace only the tag portion after stripping any digest and using the last colon.
- split := strings.Split(*containerDef.Image, ":") - newDockerURI := fmt.Sprintf("%s:%s", split[0], dockerImageTag) + // Strip digest if present and replace only the tag. + base := *containerDef.Image + if idx := strings.Index(base, "@"); idx != -1 { + base = base[:idx] + } + if idx := strings.LastIndex(base, ":"); idx != -1 && !strings.Contains(base[idx+1:], "/") { + // Replace existing tag. + base = base[:idx] + } + newDockerURI := fmt.Sprintf("%s:%s", base, dockerImageTag)
🧹 Nitpick comments (5)
pkg/ecs/client.go (1)
27-37: Prefer passing a context into NewAWSClients (avoid context.Background())This enables cancellation/timeouts at the call site and improves composability.
Apply:
-// NewAWSClients creates and initializes AWS service clients with shared configuration -func NewAWSClients() (*AWSClients, error) { +// NewAWSClients creates and initializes AWS service clients with shared configuration +func NewAWSClients(ctx context.Context) (*AWSClients, error) { @@ - cfg, err := config.LoadDefaultConfig(context.Background(), configFunctions...) + cfg, err := config.LoadDefaultConfig(ctx, configFunctions...)Call sites will need to pass a ctx.
pkg/ecs/logs.go (1)
46-50: Paginate FilterLogEvents to avoid missing logs under high volumeCurrently only the first page is processed. Heavy log streams can exceed a single page; older events would be dropped. Iterate with NextToken and collect all events for this tick.
- output, err := client.FilterLogEvents(ctx, input) - - if err != nil { - return nil, 0, fmt.Errorf("failed to filter log events (%w)", err) - } - - var logs []LogEntry - for _, event := range output.Events { - if event.LogStreamName == nil || event.Message == nil || event.Timestamp == nil { - continue // Skip events with missing required fields - } - logs = append(logs, LogEntry{ - StreamName: *event.LogStreamName, - Message: *event.Message, - Timestamp: *event.Timestamp, - }) - } - - var lastEventTimestamp int64 - if len(output.Events) > 0 { - lastEvent := output.Events[len(output.Events)-1] - if lastEvent.Timestamp == nil { - if startTime != nil { - lastEventTimestamp = *startTime - } else { - lastEventTimestamp = 0 - } - } else { - lastEventTimestamp = *lastEvent.Timestamp + 1 - } - } else if startTime != nil { - lastEventTimestamp = *startTime - } else { - lastEventTimestamp = 0 - } + var logs []LogEntry + var lastEventTimestamp int64 + var anyEvents bool + for { + output, err := client.FilterLogEvents(ctx, input) + if err != nil { + return nil, 0, fmt.Errorf("failed to filter log events (%w)", err) + } + for _, event := range output.Events { + if event.LogStreamName == nil || event.Message == nil || event.Timestamp == nil { + continue // Skip events with missing required fields + } + logs = append(logs, LogEntry{ + StreamName: *event.LogStreamName, + Message: *event.Message, + Timestamp: *event.Timestamp, + }) + anyEvents = true + lastEventTimestamp = *event.Timestamp + 1 + } + if output.NextToken == nil { + break + } + input.NextToken = output.NextToken + } + + if !anyEvents { + if startTime != nil { + lastEventTimestamp = *startTime + } else { + lastEventTimestamp = 0 + } + }Also applies to: 52-62, 64-83
pkg/ecs/deploy.go (1)
77-83: Nit: use lowerCamelCase for local vars (taskDefinitionArn)Small style tweak for idiomatic Go naming.
-func Deploy(ctx context.Context, clients *AWSClients, cluster, service, dockerImageTag string) (*DeployResult, error) { +func Deploy(ctx context.Context, clients *AWSClients, cluster, service, dockerImageTag string) (*DeployResult, error) { @@ - TaskDefinitionArn, err := cloneTaskDef(ctx, cluster, service, dockerImageTag, clients.ECS) + taskDefinitionArn, err := cloneTaskDef(ctx, cluster, service, dockerImageTag, clients.ECS) @@ - updateOutput, err := clients.ECS.UpdateService(ctx, &ecs.UpdateServiceInput{ + updateOutput, err := clients.ECS.UpdateService(ctx, &ecs.UpdateServiceInput{ Cluster: &cluster, Service: &service, - TaskDefinition: &TaskDefinitionArn, + TaskDefinition: &taskDefinitionArn, }) @@ - return &DeployResult{ - TaskDefinitionArn: TaskDefinitionArn, + return &DeployResult{ + TaskDefinitionArn: taskDefinitionArn, ServiceArn: *updateOutput.Service.ServiceArn, }, nilAlso applies to: 84-96, 98-101
pkg/ecs/execute.go (2)
168-175: Use typed enums for LaunchType and AssignPublicIpPrefer SDK constants for type safety.
- LaunchType: "FARGATE", + LaunchType: types.LaunchTypeFargate, @@ - AssignPublicIp: "ENABLED", // FIXME: Public IP is not needed (mostly) + AssignPublicIp: types.AssignPublicIpEnabled, // FIXME: Public IP is not needed (mostly)
206-223: Skip log retrieval when no awslogs config is presentAvoids repeated failed calls and throttling if the task has no logs configured.
- if waitForCompletion { + if waitForCompletion { @@ - for { + for { @@ - logs, newTimestamp, err := getTaskLogs(ctx, tdef.LogGroup, tdef.LogStreamPrefix, *executedTask.TaskArn, tdef.Name, lastTimestamp, clients.CloudWatchLogs) - if err == nil { - result.Logs = append(result.Logs, logs...) - lastTimestamp = &newTimestamp - } + if tdef.LogGroup != "" && tdef.LogStreamPrefix != "" { + logs, newTimestamp, err := getTaskLogs(ctx, tdef.LogGroup, tdef.LogStreamPrefix, *executedTask.TaskArn, tdef.Name, lastTimestamp, clients.CloudWatchLogs) + if err == nil { + result.Logs = append(result.Logs, logs...) + lastTimestamp = &newTimestamp + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
pkg/ecs/client.go(2 hunks)pkg/ecs/deploy.go(3 hunks)pkg/ecs/execute.go(1 hunks)pkg/ecs/logs.go(3 hunks)pkg/ecs/prune.go(5 hunks)pkg/ecs/restart.go(1 hunks)pkg/ecs/revisions.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/ecs/restart.go
- pkg/ecs/prune.go
🧰 Additional context used
🧬 Code Graph Analysis (5)
pkg/ecs/execute.go (1)
pkg/ecs/types.go (5)
ServiceDefinition(34-38)TaskDefinition(41-47)AWSClients(28-31)ExecuteResult(57-64)LogEntry(50-54)
pkg/ecs/revisions.go (1)
pkg/ecs/types.go (3)
RevisionEntry(67-72)AWSClients(28-31)RevisionsResult(75-77)
pkg/ecs/logs.go (1)
pkg/ecs/types.go (1)
LogEntry(50-54)
pkg/ecs/client.go (1)
pkg/ecs/types.go (1)
AWSClients(28-31)
pkg/ecs/deploy.go (1)
pkg/ecs/types.go (3)
TaskDefinition(41-47)AWSClients(28-31)DeployResult(80-83)
🔇 Additional comments (1)
pkg/ecs/client.go (1)
31-33: Confirm retry count vs MaxAttempts semanticsStandardRetryer.MaxAttempts counts the initial request as one attempt. Currently you’re doing:
o.MaxAttempts = defaultNumberOfRetries // 10This yields 10 total attempts (1 initial + 9 retries). If
defaultNumberOfRetriesis meant to be the number of retries, set:- o.MaxAttempts = defaultNumberOfRetries + o.MaxAttempts = defaultNumberOfRetries + 1Otherwise, consider renaming
defaultNumberOfRetriesto something likedefaultMaxAttemptsfor clarity.• pkg/ecs/types.go:24
• pkg/ecs/client.go:31–33
Replace hardcoded string splitting with the existing extractARNResource function for consistent and robust ARN parsing throughout GetClusters. This improves error handling and uses the official AWS SDK ARN parser.
1 participant
This pull request refactors the CLI command structure by moving the implementation of each command (
run,deploy,prune,revisions,restart,list,completion, andversion) into its own dedicated file. Each command now encapsulates its logic and user interaction, improving maintainability and separation of concerns. The main initialization logic incmd/main.gois simplified to only register these commands, and utility functions are updated for clarity and robustness.Command Refactoring and Modularization:
run,deploy,prune,revisions,restart,list,completion,version) is now implemented in its own file undercmd/, encapsulating its logic and user interface, rather than being defined inline inmain.go. This makes the codebase easier to navigate and maintain.Summary:
This refactor brings modularity and clarity to the CLI codebase, making each command's implementation self-contained and easier to maintain, while also modernizing utility code and improving error handling.
Summary by CodeRabbit