feat: patch job metadata endpoint #397
Conversation
ryuwd
force-pushed
the
roneil-jobstateupdatehandler
branch
2 times, most recently
from
052332b to
d2106a3
Compare
ryuwd
force-pushed
the
roneil-jobstateupdatehandler
branch
3 times, most recently
from
328cf93 to
46559f7
Compare
ryuwd
force-pushed
the
roneil-jobstateupdatehandler
branch
2 times, most recently
from
46559f7 to
dc8bee3
Compare
|
Rebased and ready to go |
aldbr
force-pushed
the
roneil-jobstateupdatehandler
branch
2 times, most recently
from
a29bf3e to
41cbccf
Compare
|
@aldbr LGTM, please go ahead |
add policy Missing awaits and return results adjust returned output Fixed mypy mypy false positive? and line 34
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
Remove breakpoint pre-commit
aldbr
force-pushed
the
roneil-jobstateupdatehandler
branch
from
06ec05d to
ede0868
Compare
for more information, see https://pre-commit.ci
| if body.parameters: | ||
| body.parameters.remove("LoggingInfo") | ||
| body.parameters = ["JobID"] + (body.parameters or []) | ||
| if not body.parameters: | ||
| body.parameters = None |
There was a problem hiding this comment.
Looks like an impossible thing to happen
There was a problem hiding this comment.
This in essence forces body.parameters to None if removing LoggingInfo leaves an empty list, hence making it as if the default set of parameters (in addition to the logging info) are the ones should be queried when calling job_db.search. I ran into an unfortunate edge case in getting the unit tests to pass and this is what I came up with.
There was a problem hiding this comment.
The default parameter set includes JobID and a bunch of other things, but if you don't keep this then you end up with body.parameters = ["JobID"] only
There was a problem hiding this comment.
Looks like an edge case that can only happen in testing, but whatever let's keep the test.
| possible_attribute_columns = [ | ||
| col.name.lower() for col in _get_columns(Jobs.__table__, None) | ||
| ] |
There was a problem hiding this comment.
Is this (lower() of all column names) and the subsequent check needed? Can't we compare without lowering?
There was a problem hiding this comment.
I'm inclined to agree. I just wonder what would happen if somebody passed a case-sensitive un-matching attribute column that then gets registered in the JobParameterDB instead of throwing an error. Maybe we should throw an exception if the case-sensitive vs. insensitive matches don't line up
There was a problem hiding this comment.
I see, nothing is perfect. Do as you feel.
Todo:
PATCH /metadata/searchCloses #162