-
Notifications
You must be signed in to change notification settings - Fork 16
feat: Add calloc/crun -Q/--quiet #313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughAdd a global Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
internal/calloc/calloc.go (1)
214-221: Informational print gated correctly; route the failure message to stderrThe “Allocated craned nodes” print is informational and now gated — good. The failure message right below still goes to stdout; it should go to stderr for proper CLI semantics.
Apply:
- fmt.Println("Failed to allocate task resource. Exiting...") + _, _ = fmt.Fprintln(os.Stderr, "Failed to allocate task resource. Exiting...")
🧹 Nitpick comments (3)
internal/calloc/calloc.go (1)
183-186: Gating informational print behind quiet is correctOnly the informational “Task id allocated” goes through fmt.Printf and is now suppressed by --quiet. Control flow and error handling remain unchanged.
Optionally align the message with crun’s wording for consistency across tools:
- fmt.Printf("Task id allocated: %d\n", taskId) + fmt.Printf("Task id allocated: %d, waiting resources.\n", taskId)internal/crun/crun.go (2)
301-303: Allocated-nodes informational print gated — consider punctuation parity with callocBehavior is correct. For a consistent user experience with calloc, consider adding a trailing period.
- fmt.Printf("Allocated craned nodes: %s\n", cforedPayload.AllocatedCranedRegex) + fmt.Printf("Allocated craned nodes: %s.\n", cforedPayload.AllocatedCranedRegex)
349-351: Quiet gating for “io forward ready” is appropriateGood call to suppress this purely informational line in quiet mode.
If you find yourself adding more gated prints, consider a small helper in each package to avoid repetition:
// in package crun func printInfo(format string, a ...any) { if !FlagQuiet { fmt.Printf(format, a...) } }Then replace fmt.Printf(...) with printInfo(...).
📜 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 (4)
internal/calloc/CmdArgParser.go(2 hunks)internal/calloc/calloc.go(2 hunks)internal/crun/CmdArgParser.go(2 hunks)internal/crun/crun.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
internal/calloc/calloc.go (2)
internal/crun/CmdArgParser.go (1)
FlagQuiet(61-61)internal/calloc/CmdArgParser.go (1)
FlagQuiet(55-55)
internal/crun/crun.go (2)
internal/crun/CmdArgParser.go (1)
FlagQuiet(61-61)internal/calloc/CmdArgParser.go (1)
FlagQuiet(55-55)
internal/crun/CmdArgParser.go (1)
internal/calloc/CmdArgParser.go (2)
RootCmd(57-68)FlagQuiet(55-55)
internal/calloc/CmdArgParser.go (1)
internal/crun/CmdArgParser.go (2)
RootCmd(63-73)FlagQuiet(61-61)
🔇 Additional comments (4)
internal/crun/CmdArgParser.go (1)
60-62: Quiet flag state added — looks good and consistent with callocThe new exported FlagQuiet and its placement with other flags is consistent and clear.
internal/calloc/CmdArgParser.go (2)
54-56: Quiet flag state added — mirrors crun and reads wellExported FlagQuiet and its position next to FlagHold is consistent with crun’s CmdArgParser.
103-103: -Q/--quiet registration matches crun — consistent UXBinding and help text match crun. Good consistency across binaries.
internal/crun/crun.go (1)
256-258: TaskId informational print now respects --quiet — goodThis keeps user-facing noise down without altering state handling or error paths.
|
@1daidai1 Rebase 一下就合并吧。 |
8a5c5a4 to
b130c21
Compare
|
@Nativu5 done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/calloc/cmd.go (1)
54-55: Consider not exporting FlagQuiet.It’s only used within this package; making it unexported (flagQuiet) reduces public surface and avoids cross‑pkg name collisions.
Apply:
- FlagHold bool - FlagQuiet bool + FlagHold bool + flagQuiet boolAnd update inits/uses accordingly.
internal/crun/cmd.go (2)
60-61: Limit export surface for FlagQuiet.Same note as calloc: consider unexported flagQuiet unless external packages need it.
Apply:
- FlagHold bool - FlagQuiet bool + FlagHold bool + flagQuiet boolAnd adjust references.
117-117: Make quiet persistent for consistency with debug-level.
Change RootCmd.Flags -> RootCmd.PersistentFlags so -Q is inherited by subcommands. Verified FlagQuiet gates state messages in internal/crun/crun.go (lines 256, 301, 349) and no other -Q flags found under internal/crun. Apply:- RootCmd.Flags().BoolVarP(&FlagQuiet, "quiet", "Q", false, "Quiet mode (suppress informational messages)") + RootCmd.PersistentFlags().BoolVarP(&FlagQuiet, "quiet", "Q", false, "Quiet mode (suppress informational messages)")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/calloc/calloc.go(2 hunks)internal/calloc/cmd.go(2 hunks)internal/crun/cmd.go(2 hunks)internal/crun/crun.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/calloc/calloc.go
- internal/crun/crun.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/calloc/cmd.go (1)
internal/crun/cmd.go (1)
FlagQuiet(61-61)
internal/crun/cmd.go (1)
internal/calloc/cmd.go (2)
RootCmd(57-68)FlagQuiet(55-55)
🔇 Additional comments (2)
internal/calloc/cmd.go (1)
103-103: Quiet flag wiring: looks good.The --quiet/-Q flag registration is correct and consistent with existing flags.
internal/crun/cmd.go (1)
117-117: Quiet flag wiring: looks good.--quiet/-Q registration is correct; uppercase -Q avoids conflict with existing -q (qos).
b130c21 to
a6409e4
Compare
|
need rebase |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.