chore: check config policies on 'det notebook set priority' #10047
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10047 +/- ##
==========================================
- Coverage 54.42% 54.41% -0.01%
==========================================
Files 1262 1262
Lines 158880 158886 +6
Branches 3631 3632 +1
==========================================
- Hits 86463 86455 -8
- Misses 72283 72297 +14
Partials 134 134
Flags with carried forward coverage won't be shown. Click here to find out more.
|
master/internal/command/command.go
Outdated
| @@ -278,6 +279,16 @@ func (c *Command) garbageCollect() { | |||
| } | |||
|
|
|||
| func (c *Command) setNTSCPriority(priority int, forward bool) error { | |||
| if smallerHigher, err := c.rm.SmallerValueIsHigherPriority(); err == nil { | |||
| ok, err := configpolicy.PriorityAllowed(int(c.Metadata.WorkspaceID), model.NTSCType, priority, smallerHigher) | |||
There was a problem hiding this comment.
I think we need to be calling
| ok, err := configpolicy.PriorityAllowed(int(c.Metadata.WorkspaceID), model.NTSCType, priority, smallerHigher) | |
| ok, err := configpolicy.PriorityUpdateAllowed(int(c.Metadata.WorkspaceID), model.NTSCType, priority, smallerHigher) |
That function is implemented in the only commit on main that this branch doesn't have lol 😆. PriorityAllowed checks against constraints but we also want to check that this field isn't set in an invariant config, which is done in findAllowedPriority, a helper in PriorityUpdateAllowed!
There was a problem hiding this comment.
I changed the name of the function (by request) in a recent PR. Sorry to change the code out from under you.
There was a problem hiding this comment.
Makes sense, this veriy likely would have had merge conflicts and would have been discovered when rebasing either way
There was a problem hiding this comment.
I've changed it. I didn't have any conflicts from git (merge button is still green), it was a semantic conflict. So thanks for the catch.
There was a problem hiding this comment.
Awesome! Change LGTM, approved!
stoksc
force-pushed
the
check-contraints-set-nb-prio
branch
from
fc90ff2
to
36773ea
Compare
stoksc
force-pushed
the
check-contraints-set-nb-prio
branch
from
454e573
to
acd760b
Compare
Ticket
CM-562
Description
Add a constraint check to a stray priority change API.
Test Plan
TBD.