fix(cli): reorganize internal CLI packages#115
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the CLI infrastructure by introducing a core package to centralize shared dependencies, configuration, and utilities, reducing circular dependencies. It also introduces dedicated packages for certmanager, kube, platformstatus, registry, and setup/plan to improve modularity. Several improvements were suggested: replacing global Swap functions with dependency injection to avoid race conditions in tests, using f.Chmod instead of os.Chmod for security, and ensuring all output goes through the centralized core.DefaultPrinter rather than fmt.Print.
| func SwapExecCommand(f func(string, ...string) *exec.Cmd) (restore func()) { | ||
| prev := execCommand | ||
| execCommand = f | ||
| return func() { execCommand = prev } | ||
| } |
There was a problem hiding this comment.
Similar to SwapDefaultKubectlClient, SwapExecCommand modifies a global variable execCommand, leading to race conditions in parallel tests. This pattern should be avoided in favor of dependency injection through struct fields or context.
| if err := f.Close(); err != nil { | ||
| return fmt.Errorf("write output file: %w", err) | ||
| } | ||
| if err := os.Chmod(absPath, 0o600); err != nil { | ||
| return fmt.Errorf("write output file: %w", err) | ||
| } |
There was a problem hiding this comment.
Using os.Chmod on a file path after closing it is less secure and less efficient than using f.Chmod on the open file descriptor. os.Chmod follows symbolic links and is subject to race conditions if the file is replaced between the close and chmod operations. Since you have an open file handle f, use f.Chmod(0o600) before closing it.
| if err := f.Close(); err != nil { | |
| return fmt.Errorf("write output file: %w", err) | |
| } | |
| if err := os.Chmod(absPath, 0o600); err != nil { | |
| return fmt.Errorf("write output file: %w", err) | |
| } | |
| if err := f.Chmod(0o600); err != nil { | |
| _ = f.Close() | |
| return fmt.Errorf("write output file: %w", err) | |
| } | |
| if err := f.Close(); err != nil { | |
| return fmt.Errorf("write output file: %w", err) | |
| } |
| if dryRun { | ||
| core.Info(fmt.Sprintf("[dry-run] would write kind config and run: kind create cluster --name %s --config <tmp.yaml>", clusterName)) | ||
| core.Info("[dry-run] kind config that would be written:") | ||
| fmt.Print(config) |
There was a problem hiding this comment.
Using fmt.Print directly bypasses the centralized CLI printer and its configuration (e.g., quiet mode, output redirection). For consistency with other output in this package, use core.DefaultPrinter.Println(config).
| fmt.Print(config) | |
| core.DefaultPrinter.Println(config) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec08cb1fac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Runtime errors should not trigger Cobra's usage/help dump; flag/arg | ||
| // validation errors still do (those happen before RunE). main() prints | ||
| // the error itself, so silence Cobra's own error print to avoid duplicates. | ||
| SilenceUsage: true, |
There was a problem hiding this comment.
Setting SilenceUsage: true on the root command suppresses usage text for all subcommands, including flag/arg validation failures (e.g., missing required flags), because Cobra checks the root command's SilenceUsage before printing cmd.UsageString(). This regresses CLI ergonomics compared to prior behavior and contradicts the comment above this field; users now get only Error: ... without command-specific guidance when they make common input mistakes.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #115 +/- ##
==========================================
- Coverage 51.64% 49.16% -2.48%
==========================================
Files 72 86 +14
Lines 10679 10998 +319
==========================================
- Hits 5515 5407 -108
- Misses 4580 5011 +431
+ Partials 584 580 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
No description provided.