use both job and type query on scaling policy list endpoint #9312
Conversation
cgbaker
force-pushed
the
b-9227-scaling-policy-filtering
branch
from
8b84ad8
to
42e5b5b
Compare
cgbaker
force-pushed
the
b-9227-scaling-policy-filtering
branch
from
42e5b5b
to
ece8cde
Compare
cgbaker
requested review from
shoenig and
krishicks
and removed request for
lgfa29
| if !ok { | ||
| return true | ||
| } | ||
| return policyType != p.Type |
There was a problem hiding this comment.
I think my only comment about this is that it seems like this check could be moved into ScalingPoliciesByJobTxn, where the new policyType string would be passed through to it.
Looking at ScalingPoliciesByJobTxn, it already does the same kind of type assertion we see here, so it happening again (specifically the !ok part, which I think is inconceivable) makes me want a single filter.
There was a problem hiding this comment.
yeah, maybe 50/50? this short-circuits the filter on the outside of the loop when policyType == "", as opposed to doing it on every filter operation. merging them avoids duplicate cast assertion checks.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
the
/v1/scaling/policieslist endpoint allows filtering by?joband?type. unfortunately, the code path forjobignored thetypequery. this PR remedies this.unsurprisingly, there weren't any tests protecting this behavior, so those are added as well.
resolves #9227