Conversation
|
/review |
1 similar comment
|
/review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
/review |
Review SummaryIssues found: 0 critical, 7 important Important Issues
VerdictREQUEST CHANGES — Several important issues around concurrency safety, API surface correctness, and Cobra hook inheritance should be addressed before merge. |
Review Metrics
|
zmotso
force-pushed
the
refactor
branch
2 times, most recently
from
7350784 to
d63c647
Compare
|
/review |
Review SummaryIssues found: 2 critical, 7 important Critical Issues
Important Issues
VerdictREQUEST CHANGES — Two |
Review Metrics
|
Replaces the monolithic internal/cli package with a modular pkg/cmd layout (auth, deployment, project, root, version). Introduces a cmdutil.Factory for dependency injection and an iostreams abstraction for testable I/O. Dependency changes: - Replace github.com/charmbracelet/lipgloss with charm.land/lipgloss/v2 - Remove github.com/fatih/color (replaced by lipgloss v2 styling) - Update charmbracelet indirect deps (colorprofile, x/ansi, x/term)
|
/review |
| } | ||
|
|
||
| var ( | ||
| muCfg sync.Mutex |
There was a problem hiding this comment.
Config memoization is inconsistent with TokenProvider and K8sClient
Config uses a sync.Mutex + nil-check pattern that does not cache errors: if config.Resolve() fails, cachedConfig stays nil and every subsequent caller retries the filesystem I/O. Both TokenProvider and K8sClient use sync.Once, which correctly caches the failure on the first call and skips all retries.
Suggestion: convert Config to sync.Once for consistency:
| muCfg sync.Mutex | |
| onceConfig sync.Once | |
| cachedConfig *config.Config | |
| cachedConfigErr error |
Then update the closure:
f.Config = func() (*config.Config, error) {
onceConfig.Do(func() {
cfg, err := config.Resolve()
if err != nil {
cachedConfigErr = fmt.Errorf("loading config: %w", err)
return
}
cachedConfig = cfg
})
return cachedConfig, cachedConfigErr
}
|
|
||
| // ResolveFormat returns the explicit format if provided, otherwise defaults to table. | ||
| // Use -o json for JSON output. | ||
| func ResolveFormat(explicit string) string { |
There was a problem hiding this comment.
ResolveFormat always returns "table" — piped output is no longer machine-readable
When no -o flag is passed, this unconditionally returns FormatTable regardless of whether stdout is a TTY. The previous implementation auto-detected TTY and defaulted to JSON for piped output, which is important for scripting (krci deployment list | jq ...).
Consider restoring TTY-based auto-detection:
| func ResolveFormat(explicit string) string { | |
| func ResolveFormat(explicit string, isTTY bool) string { | |
| if explicit != "" { | |
| return explicit | |
| } | |
| if !isTTY { | |
| return FormatJSON | |
| } | |
| return FormatTable | |
| } |
Callers (RenderList, RenderDetail) already receive ios which exposes IsStdoutTTY(), so isTTY can be passed through without changes to the public API surface.
| lines = append(lines, output.DetailLine{Label: "Status", Value: "Authenticated"}) | ||
|
|
||
| if expiry := info.ExpiresAt; !expiry.IsZero() { | ||
| remaining := time.Until(expiry).Round(time.Second) |
There was a problem hiding this comment.
time.Until evaluated twice — color threshold can disagree with displayed value
remaining is computed here for the display string, then computed again at line 111 inside the Styled closure. The two calls observe different wall-clock instants. In practice the delta is small, but if the token has e.g. 5m01s remaining when the display string is built and 4m59s when the closure runs, the output will show "5m1s" in the Expires line but color it yellow — a confusing inconsistency.
Fix: capture remaining once and close over it:
if expiry := info.ExpiresAt; !expiry.IsZero() {
remaining := time.Until(expiry).Round(time.Second) // ← single evaluation
expiresVal := fmt.Sprintf("%s (%s)", expiry.Local().Format(time.RFC822), remaining)
lines = append(lines, output.DetailLine{Label: "Expires", Value: expiresVal})
}Then in the Styled closure, reference the captured remaining variable directly instead of calling time.Until again.
| lines = append(lines, output.DetailLine{Label: "Groups", Value: strings.Join(info.Groups, ", ")}) | ||
| } | ||
|
|
||
| return output.RenderDetail(opts.IO, "", lines, output.DetailRenderer[[]output.DetailLine]{ |
There was a problem hiding this comment.
outputFormat hardcoded to "" — -o json is impossible for auth status
RenderDetail accepts an outputFormat parameter, but it is hardcoded to "" here and no -o flag is registered on the status command. This means users cannot request JSON output from krci auth status, and the outputFormat parameter of RenderDetail is permanently dead for this call site.
Either register a persistent -o flag and wire it through StatusOptions, or — if JSON output is intentionally not supported here — use a simpler renderer that does not imply format selection.
| } | ||
|
|
||
| return output.RenderDetail(opts.IO, "", lines, output.DetailRenderer[[]output.DetailLine]{ | ||
| Styled: func(w io.Writer, ls []output.DetailLine) error { |
There was a problem hiding this comment.
Styled closure mutates the caller's slice in-place — fragile side-effect pattern
ls[i].Styled = ... modifies the backing array of the slice passed into RenderDetail. This works today because PrintStyledDetailLines only iterates the slice once. However, if RenderDetail is ever refactored to call Styled more than once (e.g., for buffering or retry), the second invocation will see already-populated Styled fields, silently bypassing the conditional logic.
Compare with deployment/get and project/get, which build a pre-styled slice before passing it to RenderDetail — a cleaner pattern with no hidden mutation. Consider doing the same here by computing styled values eagerly before calling RenderDetail.
| return err | ||
| } | ||
|
|
||
| svc := k8s.NewDeploymentService(dynClient, cfg.Namespace) |
There was a problem hiding this comment.
Service construction duplicated across 4 command packages
k8s.NewDeploymentService(dynClient, cfg.Namespace) is constructed verbatim here and again in pkg/cmd/deployment/get/get.go:75. The same pattern repeats for k8s.NewProjectService in pkg/cmd/project/list/list.go:71 and pkg/cmd/project/get/get.go:73.
Each of the four *Run functions calls opts.Config() and opts.K8sClient() solely to feed the service constructor — 8 lines of identical boilerplate. Adding a DeploymentService and ProjectService factory func to cmdutil.Factory (similar to how K8sClient is handled) would eliminate the duplication and make the constructors a single source of truth.
| Version: v, | ||
| SilenceUsage: true, | ||
| SilenceErrors: true, | ||
| PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { |
There was a problem hiding this comment.
PersistentPreRunE is not automatically chained by Cobra
Cobra does not chain PersistentPreRunE hooks. If any future subcommand (e.g., a new auth flow) defines its own PersistentPreRunE or PreRunE, this config warm-up will be silently bypassed — no compile error, no runtime warning.
Consider documenting this constraint explicitly, or switching to a pattern that is chain-safe (e.g., a middleware wrapper around each command's RunE, or calling f.Config() at the top of each RunE that needs it). At minimum, add a comment warning future contributors that adding PersistentPreRunE to a subcommand will override this hook.
Review SummaryIssues found: 0 critical, 7 important Important Issues
VerdictREQUEST CHANGES — The refactor introduces several important issues: broken TTY-detection for piped output format, inconsistent memoization in the factory, and duplicated service construction patterns that should be resolved before merging. |
Review Metrics
|
2 participants
Replaces the monolithic internal/cli package with a modular pkg/cmd layout (auth, deployment, project, root, version). Introduces a cmdutil.Factory for dependency injection and an iostreams abstraction for testable I/O.
Dependency changes: