-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api-server: Support generic labels #949
Conversation
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just 1 question about the query route
@@ -99,10 +99,10 @@ async def query_task_states( | |||
None, description="comma separated list of requester names" | |||
), | |||
pickup: Optional[str] = Query( | |||
None, description="comma separated list of pickup names" | |||
None, description="comma separated list of pickup names", deprecated=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this marked as deprecated? Same for the destination query below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are being replaced with label=name=value
, in main
. Let me see if I can port generic label filtering over (already tested sorting doesn't work due to tortoise bug).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
managed to port label sorting over, this also fixes an issue with filtering where if you try to filter by pickup and destination you get no result even though there is a task that meets the criteria.
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like with the extra fields in the API, anywhere using this API needs to be updated too
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for setting this up! LGTM
What's new
Supports labels without requiring the schema to be strongly defined. This is still a bit hacky, it still assumes the label is a json with a
description
field while labels in RMF are free-form strings. RMF needs to support key value pair labels for us to be able to use them "natively".Another issue is that tortoise-orm is lacking many features which prevent us to implement proper sorting on generic labels so we must still use hacks for the
pickup
anddestination
fields. We could use raw sql for this or drop support for sorting by labels.Self-checks
Discussion