Conversation
gtsiolis
requested review from
carole-lavillonniere and
silv-io
as code owners
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds custom Cobra help templates and configures default help/version flags; updates short/long help texts across multiple CLI commands; adds tests for help output. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/root.go (1)
49-64: Template customization via string replacement is fragile against Cobra upgrades
strings.ReplaceonUsageTemplate()silently no-ops if Cobra ever changes its default template wording (e.g., changes"Available Commands:"or reorders sections). There's no error or warning if a replacement finds nothing, so a future Cobra bump could silently revert the help output to the un-customized form.A note on line 51:
"Flags:"is replaced with count1, which works correctly today because Cobra places the local flags section (Flags:) before the inherited section (Global Flags:) in its template — so the first substring hit is the intended one. This ordering is a Cobra implementation detail, not a contract.Consider defining the usage template as a complete string literal rather than patching the default, which makes the intent explicit and immune to upstream template changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 49 - 64, The current approach mutates Cobra's default UsageTemplate via multiple strings.Replace calls (usageTemplate variable and the block that patches Usage: / Available Commands: / Flags:), which is fragile; replace this patching by defining a complete, explicit usage template string and pass it to rootCmd.SetUsageTemplate instead of performing replacements on rootCmd.UsageTemplate(), keeping the same customized sections (Commands:, Options:, the special lstk Usage header and the removed "Use ... --help" line) so the behavior no longer depends on Cobra's upstream template wording or ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Line 59: Update the user-facing help string that contains the typo "interace"
to "interface" — locate the string literal containing "LSTK - LocalStack
command-line interace{{end}}`" in cmd/root.go (e.g., the root command's
description/long help) and correct the spelling to "LSTK - LocalStack
command-line interface{{end}}`" so the CLI help displays the proper word.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 49-64: The current approach mutates Cobra's default UsageTemplate
via multiple strings.Replace calls (usageTemplate variable and the block that
patches Usage: / Available Commands: / Flags:), which is fragile; replace this
patching by defining a complete, explicit usage template string and pass it to
rootCmd.SetUsageTemplate instead of performing replacements on
rootCmd.UsageTemplate(), keeping the same customized sections (Commands:,
Options:, the special lstk Usage header and the removed "Use ... --help" line)
so the behavior no longer depends on Cobra's upstream template wording or
ordering.
597f760 to
ffb8144
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/root.go (1)
49-64: String-replacement on Cobra's internal default template is fragile.The five chained
strings.Replace/ReplaceAll/TrimRightcalls mutate the text of Cobra's built-inUsageTemplate(). Any change to that template in a future Cobra release (including patch versions) would cause one or more replacements to silently no-op, producing a malformed or partially-branded help output with no compile-time or runtime error.Consider defining the full usage template as a single, self-contained string literal instead of patching the default one. This removes the coupling to Cobra internals entirely and makes the intended layout explicit and auditable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 49 - 64, The current approach mutates Cobra's default UsageTemplate by applying multiple strings.Replace/ReplaceAll/TrimRight calls on usageTemplate returned from rootCmd.UsageTemplate(), which is fragile; instead replace this whole mutation sequence by constructing a single explicit usage template string literal (containing the desired "Usage", "Commands"/"Options" labels and the LSTK branding/conditional logic) and call rootCmd.SetUsageTemplate(...) with that literal; update or remove references to the intermediate usageTemplate variable and ensure the new template uses the same template variables (e.g., .UseLine, .CommandPath, .HasAvailableSubCommands) so behavior of rootCmd remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Around line 52-61: The usage/help templates currently check for a hardcoded
binary name via `{{if eq .CommandPath "lstk"}}` which breaks when the binary is
renamed; update the template logic in the `usageTemplate` and the help template
to use Cobra's root detection by replacing those instances with `{{if not
.HasParent}}` so the branded header renders for the root command regardless of
the executable name (update occurrences in the `usageTemplate` and the help
template where `CommandPath` is compared to "lstk").
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 49-64: The current approach mutates Cobra's default UsageTemplate
by applying multiple strings.Replace/ReplaceAll/TrimRight calls on usageTemplate
returned from rootCmd.UsageTemplate(), which is fragile; instead replace this
whole mutation sequence by constructing a single explicit usage template string
literal (containing the desired "Usage", "Commands"/"Options" labels and the
LSTK branding/conditional logic) and call rootCmd.SetUsageTemplate(...) with
that literal; update or remove references to the intermediate usageTemplate
variable and ensure the new template uses the same template variables (e.g.,
.UseLine, .CommandPath, .HasAvailableSubCommands) so behavior of rootCmd remains
identical.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/root.gocmd/version.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/version.go
gtsiolis
force-pushed
the
george/des-145
branch
2 times, most recently
from
f7221fe to
4c01111
Compare
cmd/root.go
Outdated
|
|
||
| LSTK - LocalStack command-line interface{{end}}`, | ||
| 1, | ||
| ) |
There was a problem hiding this comment.
issue: it's a bit fragile to replace certain strings in the help template (if we upgrade cobra, it could change and break). However I see why it makes sense: if we were instead writing the template from scratch, new commands would not be automatically added and we could forget to add them.
So I would suggest to add a unit test to make sure the help output is what we expect.
Also I would move the code to cmd/help.go
There was a problem hiding this comment.
Actually, there is a range over the commands in the original template which we also could just do.
I think writing our own template might actually be a better solution in this case
There was a problem hiding this comment.
The templating is using the built-in https://pkg.go.dev/text/template, so we're quite free in what we can do there.
There was a problem hiding this comment.
Thanks, both! Whoa I didn't even notice that template in the file.
Moved in d1f2fdd. Let me know if this looks better now. 🏓 🏓
gtsiolis
force-pushed
the
george/des-145
branch
from
4c01111 to
60254b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/help_test.go (1)
31-57: Consider adding one integration-level help-output test.These unit checks are good, but one CLI integration test for
lstk --helpwould reduce regressions in the real binary output path.As per coding guidelines "
**/*_test.go: Prefer integration tests to cover most cases. Use unit tests when integration tests are not practical."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/help_test.go` around lines 31 - 57, Add an integration test that runs the real CLI binary for the root help output rather than the unit helper used by TestRootHelpOutputTemplate; create a new test (e.g., TestRootHelpIntegration) that uses os/exec to run the compiled `lstk --help` (or runs the built test binary's main via exec) and assert the same key strings ("Usage:", "lstk [options] [command]", "LSTK - LocalStack command-line interface", "Commands:", "Options:") are present and that deprecated templates are not present, mirroring the assertions in TestRootHelpOutputTemplate/TestSubcommandHelpUsesSubcommandUsageLine so regressions in the real binary output path are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/help_test.go`:
- Around line 10-29: executeWithArgs currently mutates the global rootCmd
causing flag/parse state to leak between tests; change it to create and use a
fresh command instance per call (e.g., call the constructor/factory that builds
the CLI such as NewRootCmd()/newRootCmd() or a Clone of rootCmd), set
args/out/err on that new instance, call ExecuteContext on it, and return its
output/error so the global rootCmd is no longer mutated by SetArgs/SetOut/SetErr
(if no constructor exists, instead fully reset all flag state on rootCmd before
executing by clearing args and resetting all flags via PersistentFlags()/Flags()
traversal).
---
Nitpick comments:
In `@cmd/help_test.go`:
- Around line 31-57: Add an integration test that runs the real CLI binary for
the root help output rather than the unit helper used by
TestRootHelpOutputTemplate; create a new test (e.g., TestRootHelpIntegration)
that uses os/exec to run the compiled `lstk --help` (or runs the built test
binary's main via exec) and assert the same key strings ("Usage:", "lstk
[options] [command]", "LSTK - LocalStack command-line interface", "Commands:",
"Options:") are present and that deprecated templates are not present, mirroring
the assertions in
TestRootHelpOutputTemplate/TestSubcommandHelpUsesSubcommandUsageLine so
regressions in the real binary output path are caught.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/help.gocmd/help_test.gocmd/root.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/root.go
| Short: "Show container logs", | ||
| Long: "Show logs from the LocalStack container. Use --follow to stream in real-time.", | ||
| Short: "Show emulator logs", | ||
| Long: "Show logs from the emulator. Use --follow to stream in real-time.", |
There was a problem hiding this comment.
Updated also the terminology from #51.
Lines 14 to 15 in a90fc63
Cc @anisaoshafi @silv-io @carole-lavillonniere for visibility and because of relevant discussion.
| flag.Changed = false | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
thought: it's a little strange we have to do that. ATM rootCmd is a package-level singleton and I wonder if we could construct a fresh one for each test. Anyways that would be a refactoring outside the scope of this PR.
cc @silv-io
There was a problem hiding this comment.
Thanks, @carole-lavillonniere! Merging this one. Opened PRO-226 to keep track of this comment.
3 participants
This will improves
helpcommand output readability and design.What changed?