Change SchedulingIntents.Priority from bool to int#15
Conversation
Co-authored-by: ianchen0119 <42661015+ianchen0119@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aligns the decisionmaker’s internal/REST scheduling-intent representation with the existing integer Intent.Priority, avoiding the previously lossy boolean collapse (intent.Priority > 0) and preserving the full priority value end-to-end.
Changes:
- Update
SchedulingIntents.Priorityfrombooltointin the decisionmaker domain model. - Preserve full priority values when constructing stored scheduling intents in
ProcessIntents. - Update the decisionmaker REST list/response struct to expose
priorityas an integer.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| decisionmaker/domain/pod.go | Changes SchedulingIntents.Priority to int to match intent priority semantics. |
| decisionmaker/service/service.go | Removes lossy > 0 conversion and stores the full intent.Priority. |
| decisionmaker/rest/intent_handler.go | Updates the REST-facing scheduling intent representation to return integer priority. |
Comments suppressed due to low confidence (1)
decisionmaker/rest/intent_handler.go:62
decisionmaker/rest/intent_handler.godefines localSchedulingIntents/LabelSelectortypes that mirrordecisionmaker/domaintypes, which already required updating both places forPriority. Consider reusing the domain types directly in the list response (or type-aliasing) to reduce drift and maintenance overhead.
type SchedulingIntents struct {
Priority int `json:"priority"` // Priority value; higher value means higher priority
ExecutionTime uint64 `json:"execution_time"` // Time slice for this process in nanoseconds
PID int `json:"pid,omitempty"` // Process ID to apply this strategy to
Selectors []LabelSelector `json:"selectors,omitempty"` // Label selectors to match pods
CommandRegex string `json:"command_regex,omitempty"` // Regex to match process command
}
| schedulingIntent := &domain.SchedulingIntents{ | ||
| Priority: intent.Priority > 0, | ||
| Priority: intent.Priority, | ||
| ExecutionTime: uint64(intent.ExecutionTime), | ||
| PID: process.PID, |
There was a problem hiding this comment.
This change fixes the lossy mapping of Intent.Priority into the stored scheduling intent, but there isn’t a focused test that would fail if the value regressed back to a boolean collapse. Consider adding a small unit test around the scheduling-intent representation (e.g., JSON marshal/unmarshal or list response construction) to assert the full integer priority is preserved.
| @@ -54,7 +54,7 @@ func (h *Handler) HandleIntents(w http.ResponseWriter, r *http.Request) { | |||
|
|
|||
| // SchedulingStrategy represents a strategy for process scheduling | |||
There was a problem hiding this comment.
The comment above type SchedulingIntents still refers to a "SchedulingStrategy", but the struct represents an individual scheduling intent returned by the decisionmaker. Please update the comment to match the type/endpoint to avoid confusing API consumers and maintainers.
| // SchedulingStrategy represents a strategy for process scheduling | |
| // SchedulingIntents represents a scheduling intent returned by the decisionmaker, | |
| // including process-level parameters and selectors used for process scheduling. |
SchedulingIntents.Prioritywas typed asbool, inconsistent withIntent.Priority intand silently lossy — the conversionintent.Priority > 0collapsed the full integer value to a binary flag.Changes
decisionmaker/domain/pod.go—SchedulingIntents.Priority:bool→intdecisionmaker/service/service.go— assignment:intent.Priority > 0→intent.Prioritydecisionmaker/rest/intent_handler.go— local REST mirror struct:Priority bool→Priority int💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.