[codex] Improve dmesg tail flag error#196
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a special-case error wrapper for the ChangesDmesg --tail error wrapping
Sequence DiagramsequenceDiagram
participant User
participant wrapTalosCommand
participant wrapDmesgCommand
participant isTailBooleanParseError
participant dmesgTailLineCountFromError
participant dmesgTailActionableHintErrorText
User->>wrapTalosCommand: dmesg --tail=3
wrapTalosCommand->>wrapDmesgCommand: Detect dmesg, apply wrapper
wrapDmesgCommand->>isTailBooleanParseError: Check error pattern
isTailBooleanParseError-->>wrapDmesgCommand: true, --tail parse error
wrapDmesgCommand->>dmesgTailLineCountFromError: Extract line count from error
dmesgTailLineCountFromError-->>wrapDmesgCommand: "3"
wrapDmesgCommand->>dmesgTailActionableHintErrorText: Format hint message
dmesgTailActionableHintErrorText-->>wrapDmesgCommand: actionable error with suggestions
wrapDmesgCommand-->>User: Enhanced error with tail -n and --follow --tail examples
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Signed-off-by: Stell Hub <xiaoyaoyunlian@gmail.com>
WenLong Chen (stellhub)
force-pushed
the
codex/dmesg-tail-hint
branch
from
6a81712 to
5d79f06
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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 enhances the dmesg command wrapper by intercepting boolean parsing errors for the --tail flag and providing users with helpful hints for correct usage. It also includes a new test suite to validate this behavior. The review identified a potential panic in the error handling logic when the original flag error function is undefined, suggesting a safety check to ensure robust execution.
| originalFlagErrorFunc := cmd.FlagErrorFunc() | ||
| cmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { | ||
| if isDmesgTailBoolParseError(err) { | ||
| //nolint:wrapcheck // return a replacement error so the operator sees the actionable talm hint instead of the pflag internals. | ||
| return errors.WithHint( | ||
| errors.WithHint( | ||
| errors.New("talm dmesg: --tail is a boolean toggling tail-mode for --follow, not a line count"), | ||
| dmesgTailLineCountHint(err), | ||
| ), | ||
| "to stream only new messages on a follow, run: talm dmesg --follow --tail", | ||
| ) | ||
| } | ||
|
|
||
| return originalFlagErrorFunc(cmd, err) | ||
| }) |
There was a problem hiding this comment.
The originalFlagErrorFunc variable will be nil if no custom flag error function was previously set on the command. Calling a nil function in the else block (line 207) will cause a panic whenever a user provides an invalid flag other than the specific --tail boolean error (e.g., talm dmesg --unknown-flag).
You should check if originalFlagErrorFunc is non-nil before calling it, or simply return the error to let Cobra handle it with its default behavior.
func wrapDmesgCommand(cmd *cobra.Command) {
originalFlagErrorFunc := cmd.FlagErrorFunc()
cmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error {
if isDmesgTailBoolParseError(err) {
//nolint:wrapcheck // return a replacement error so the operator sees the actionable talm hint instead of the pflag internals.
return errors.WithHint(
errors.WithHint(
errors.New("talm dmesg: --tail is a boolean toggling tail-mode for --follow, not a line count"),
dmesgTailLineCountHint(err),
),
"to stream only new messages on a follow, run: talm dmesg --follow --tail",
)
}
if originalFlagErrorFunc != nil {
return originalFlagErrorFunc(cmd, err)
}
return err
})
}
|
Hi WenLong Chen (@stellhub) — apologies for the timing collision. I iterated on this very quickly and didn't surface the duplicate before you put work into it: a On top of the duplication, the existing cushion (and consequently the hint in your PR) is going to be rewritten anyway: the upstream maintainer's response on siderolabs/talos#13333 points at Closing this PR. Thanks again for the effort and the careful test coverage — please don't let this timing turn you off contributing more. |
2 participants
Fixes #195.
Summary
strconv.ParseBool-styletalm dmesg --tail=Nfailure with a talm-specific actionable error.tail -n Nand for the boolean--follow --tailstreaming use case.Validation
$env:GOSUMDB='sum.golang.org'; go test ./pkg/commands$env:GOSUMDB='sum.golang.org'; go build -o .\talm-test.exe ..\talm-test.exe dmesg --tail=3prints:talm dmesg: --tail is a boolean toggling tail-mode for --follow, not a line counthint: for the last 3 lines, run: talm dmesg --nodes <node> | tail -n 3hint: to stream only new messages on a follow, run: talm dmesg --follow --tailAlso tried
$env:GOSUMDB='sum.golang.org'; go test ./...locally on Windows. Packages outsidepkg/enginepassed, butpkg/enginefailed before reaching this change because the checked-out Git symlink filescharts/cozystack/charts/talmandcharts/generic/charts/talmare plain 10-byte files on this Windows checkout, so Helm reportsChart.yaml file is missingfor those chart tests.Summary by CodeRabbit
Bug Fixes
dmesgcommand when--tailis used with invalid values, providing clearer guidance to users.Tests
dmesgcommand argument parsing and ensure other flag errors remain unchanged.