Conversation
Add an optional metadata string tag to runs, allowing users to associate a tag with a collection of runs. Signed-off-by: Aishwarya Mathuria <amathuri@redhat.com>
Support tag in run creation (POST) and query-param filtering (?tag=foo). Add TagsController/TagController for path-based access (/runs/tag/foo/) and wire tag into all existing filter controllers for composable queries. Signed-off-by: Aishwarya Mathuria <amathuri@redhat.com>
Signed-off-by: Aishwarya Mathuria <amathuri@redhat.com>
|
@zmc There's no PUT/PATCH on RunController - it only has GET and DELETE. So if we want to stick to the current workflow, tags can only be added at run creation. |
|
Do I understand correctly this only implements a single tag? I would suggest to implement tags, so a run can have several tags. |
|
@amathuria Thanks! |
Actually, this was a gap in my feature request. I think multiple tags is best. Many thanks to @kshtsk thinking of that. I've updated the ticket: https://tracker.ceph.com/issues/74166 |
@zmc @batrick multiple tags makes sense to me! I'll update the PR soon. |
Replace Run.tag with Run.tags (JSONType list) and add PUT /runs/<name>/ to update tags. Update tag filtering/listing to work with tag arrays, including comma-separated ?tag= We have to add a cast to UnicodeText for filtering to avoid JSONType bind-param JSON encoding. Signed-off-by: Aishwarya Mathuria <amathuri@redhat.com>
|
|
||
| def latest_runs(fields=None, count=conf.default_latest_runs_count, page=1): | ||
| query = Run.query.order_by(Run.posted.desc()) | ||
| def latest_runs(fields=None, count=conf.default_latest_runs_count, page=1, tag=None): |
| # JSON-encoded unless we compare as plain text: | ||
| tags_text = cast(Run.tags, UnicodeText) | ||
| for t in tag.split(','): | ||
| query = query.filter(tags_text.contains('"%s"' % t.strip())) |
There was a problem hiding this comment.
(minor)
for tag in (t.strip() for t in tags.split(',')):
. . . filter(tags_text.contains(tag))
so if tags_text is json "[abc,xyz]" the contains('bc') matches?
There was a problem hiding this comment.
If tags_text is comma separated text wouldn't it be less effective to use:
run_tags = (t.strip() for t in tags_text.split(','))
for tag in (t.strip() for t in tags.split(',')):
query = query.filter(tag in run_tags)
|
Any update on this? |
Yes, I am testing it with the Teuthology changes @anshuman-agarwala is working on. |
Signed-off-by: Aishwarya Mathuria <amathuri@redhat.com>
zmc
left a comment
zmc
left a comment
There was a problem hiding this comment.
sorry I had not seen the update to this PR until today! I have some changes I would like to see:
- we would need a db migration but the PR doesn't include one
- I might rather drop the controller path-based query implementation for this feature; it was never a great pattern
- the tags column should probably have an index since it's being queried
run_tags_text_has_element's approach of manually parsing the json string is fragile and also quietly allows searching by wildcard ("%", "_").
Regarding the column type, I had liked the idea of using the ARRAY type but sadly sqlite doesn't support it.
It looks like if we use the native JSON type, we won't be able to avoid having to use different sqlalchemy APIs for postgres and sqlite. I don't know how much we should care about that, but I don't love it.
I wonder if we should just use a separate tags table - it would be safer and more performant.
4 participants
No description provided.