Add cbatch --signal #381
Add cbatch --signal #381
Conversation
WalkthroughAdds a new CLI Changes
sequenceDiagram
participant CLI as CLI (calloc/cbatch/crun)
participant Parser as util.ParseSignalParamString
participant Builder as Job/Task Builder
participant Proto as protobuf (TaskToCtld / StepToD)
CLI->>Parser: pass FlagSignal string
Parser-->>CLI: signalNumber, secondsBeforeKill / error
CLI->>Builder: set SignalParam on job/task (SignalNumber, SecondsBeforeKill)
Builder->>Proto: marshal job/task with optional signal_param
Proto-->>Builder: marshalled payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
1daidai1
force-pushed
the
dev/cbatchsignalnew
branch
from
85eafa4 to
2c22680
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cbatch/cbatch.go (1)
324-329: Missing CLI flag handling for --signal.The
FlagSignalis registered in cmd.go but never processed in the CLI flag handling section (lines 214-339). This means--signalpassed on the command line won't take effect, only script directives will work.Add this code after line 329 (after FlagHold handling):
if FlagHold { task.Hold = true } + if FlagSignal != "" { + sig, sec, err := util.ParseSignalParamString(FlagSignal) + if err != nil { + return nil, fmt.Errorf("invalid argument: invalid --signal value '%s': %w", FlagSignal, err) + } + task.SignalParam = &protos.SignalParam{ + SignalNumber: sig, + SecondsBeforeKill: sec, + } + } // Set and check the extra attributes
🧹 Nitpick comments (2)
internal/util/string.go (2)
44-58: Consider expanding SignalMap or documenting the limited set.The
SignalMapincludes only 13 signals. While these cover common use cases, users might expect support for additional signals likeSIGSTOP,SIGCONT,SIGCHLD, etc.Consider either:
- Expanding
SignalMapto include all commonly-used signals- Adding a comment documenting why only these signals are supported
1490-1491: Document the default 60-second timeout.The default value of 60 seconds for
secondsBeforeKillappears in two places but isn't documented. Consider adding a comment explaining why 60 seconds was chosen as the default.Also applies to: 1502-1503
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/calloc/calloc.go(1 hunks)internal/calloc/cmd.go(2 hunks)internal/cbatch/cbatch.go(1 hunks)internal/cbatch/cmd.go(2 hunks)internal/crun/cmd.go(2 hunks)internal/crun/crun.go(1 hunks)internal/util/string.go(3 hunks)protos/PublicDefs.proto(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/crun/crun.go (5)
internal/calloc/cmd.go (1)
FlagSignal(54-54)internal/cbatch/cmd.go (1)
FlagSignal(66-66)internal/crun/cmd.go (1)
FlagSignal(67-67)internal/util/string.go (1)
ParseSignalParamString(1457-1507)internal/util/err.go (2)
NewCraneErr(60-65)ErrorCmdArg(36-36)
internal/calloc/calloc.go (5)
internal/calloc/cmd.go (1)
FlagSignal(54-54)internal/cbatch/cmd.go (1)
FlagSignal(66-66)internal/crun/cmd.go (1)
FlagSignal(67-67)internal/util/string.go (1)
ParseSignalParamString(1457-1507)internal/util/err.go (2)
NewCraneErr(60-65)ErrorCmdArg(36-36)
internal/calloc/cmd.go (1)
internal/crun/cmd.go (1)
FlagSignal(67-67)
internal/cbatch/cbatch.go (1)
internal/util/string.go (1)
ParseSignalParamString(1457-1507)
🪛 GitHub Actions: Go Code Quality Check
internal/crun/cmd.go
[error] 1-1: gofmt formatting check failed. File is not properly formatted. Run 'gofmt -w .' to fix formatting.
internal/util/string.go
[error] 1-1: gofmt formatting check failed. File is not properly formatted. Run 'gofmt -w .' to fix formatting.
🔇 Additional comments (6)
internal/calloc/cmd.go (1)
54-54: LGTM! Signal flag integration is clean.The flag variable declaration and registration follow the established patterns in this codebase. The description text is clear and consistent with the format used in crun and cbatch.
Also applies to: 107-107
protos/PublicDefs.proto (1)
135-138: LGTM! Proto definitions are well-structured.The
SignalParammessage definition is clean, and the optional fields are correctly added to bothTaskToCtldandStepToD. Field tags (42 and 30) don't conflict with existing fields in the respective messages.Also applies to: 188-188, 345-345
internal/crun/cmd.go (1)
67-67: LGTM! Flag registration is consistent.The implementation follows the same pattern as calloc and cbatch.
Note: The pipeline indicates a gofmt formatting issue for this file. Please run
gofmt -w internal/crun/cmd.goto fix.Also applies to: 124-124
internal/calloc/calloc.go (1)
476-484: LGTM! Signal parameter handling is correct.The implementation correctly parses the signal flag, handles errors appropriately, and initializes the
SignalParambefore marshalling extra attributes. This follows the established pattern used in other components.internal/cbatch/cmd.go (1)
66-66: LGTM! Flag declaration and registration are consistent.The implementation aligns with the patterns used in calloc and crun.
Note: The pipeline indicates a gofmt formatting issue. Please run
gofmt -w internal/cbatch/cmd.goto fix.Also applies to: 159-159
internal/cbatch/cbatch.go (1)
196-203: LGTM! Script-based signal parameter handling is correct.The signal parameter is properly parsed from
#CBATCHdirectives and assigned totask.SignalParam.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/calloc/calloc.go (1)
476-484: Signal parameter parsing looks correct.The implementation correctly parses the
--signalflag, handles errors appropriately, and populates theSignalParamprotobuf message. The error handling is consistent with other flag parsing in this file.Optional: Consider using a composite literal for initialization.
Lines 481-483 can be simplified:
- task.SignalParam = &protos.SignalParam{} - task.SignalParam.SignalNumber = sig - task.SignalParam.SecondsBeforeKill = sec + task.SignalParam = &protos.SignalParam{ + SignalNumber: sig, + SecondsBeforeKill: sec, + }Note: cbatch does not implement signal parameter handling. The consistency check should only cover
callocandcrun. Additionally, there's a minor inconsistency in error message formatting between them:crunincludes a trailing period whilecallocdoes not.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/calloc/calloc.go(1 hunks)internal/calloc/cmd.go(2 hunks)internal/cbatch/cbatch.go(1 hunks)internal/cbatch/cmd.go(2 hunks)internal/crun/cmd.go(2 hunks)internal/crun/crun.go(1 hunks)internal/util/string.go(3 hunks)protos/PublicDefs.proto(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/cbatch/cmd.go
- internal/calloc/cmd.go
- protos/PublicDefs.proto
- internal/util/string.go
- internal/crun/crun.go
- internal/crun/cmd.go
- internal/cbatch/cbatch.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/calloc/calloc.go (5)
internal/calloc/cmd.go (1)
FlagSignal(54-54)internal/cbatch/cmd.go (1)
FlagSignal(66-66)internal/crun/cmd.go (1)
FlagSignal(67-67)internal/util/string.go (1)
ParseSignalParamString(1456-1506)internal/util/err.go (2)
NewCraneErr(60-65)ErrorCmdArg(36-36)
| RootCmd.Flags().BoolVar(&FlagExclusive, "exclusive", false, "Exclusive node resources") | ||
| RootCmd.Flags().BoolVarP(&FlagHold, "hold", "H", false, "Hold the job until it is released") | ||
| RootCmd.Flags().StringVarP(&FlagLicenses, "licenses", "L", "", "Licenses used for the job") | ||
| RootCmd.Flags().StringVarP(&FlagSignal, "signal", "s", "", "send signal when time limit within time seconds, format: signum[@time]") |
Summary by CodeRabbit
@seconds(default: 60s).✏️ Tip: You can customize this high-level summary in your review settings.