#2591 DNM: hub: add "extra_key" argument to listTags
Closed by ktdreyer. Opened by ktdreyer.
ktdreyer/koji dnm-list-tags-extras  into  master

Download 2591.patch

Add an extra_key argument to the listTags RPC. This allows clients to list all tags that have an "extra" key set, for example mock.yum.module_hotfixes or a future one like osbs.priority.

I'm marking this DNM for now, just for discussion. I've not tested this at all yet, and we want to plumb this through the list-tags CLI at least (and maybe kojiweb)

One advantage to OSBS using tag_extra for an osbs.priority setting is that the basic tag_extra table has existed on the hub for years. It's already deployed to production, so users can set the extra fields like so:

koji edit-tag my-tag-1.0-containers-priority -x osbs.priority=True

(or with koji-ansible, etc.)

and we can implement the koji-containerbuild-hub "priority" feature today without changes to the hub.

This pull request just makes it easier to query all the tags that have a particular key set.

I've added #2594 to track it.

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready

I see no problem with adding it as is. Maybe there could be some confusion that in other places extra field is composed from inherited tree while here it is exactly what is in the tag. So, maybe some expansion of help text.

CLI/Web could be other PRs?

@mikem?

Works fine in a casual test.

It feels a little odd to query on the extra field, but not have the extra field in the return. I wonder if listTags should have the ability to include the extra field in the return. Otherwise, if the client cares about the actual value of this field, it's going to have to take this resulting list and query each tag returned to check that, say, osbs.priority is actually set to True and not a different value.

For that matter, i wonder if users might want a more flexible query here, e.g. a way to query the tags where mock.package_manager is equal to dnf instead of merely being set.

there could be some confusion that in other places extra field is composed from inherited tree

agreed.

Side note, with #2589, you can find such tags with something like:

queryHistory(['tag_extra'], active=True, xkey='osbs.priority')

Of course this is a bit more advanced usage than listTags and you're only getting the tag_extra row returned. So this is not really a good substitute, but it is possible.

So, this seems reasonable and works fine, but my concern is that it feels like we're going to want to expand on this and it might be worth considering what the rest of the picture looks like lest we accidentally limit ourselves with this partial solution.

Yeah, that is a good summary of the limitations here. I'm going to close this for now.

Did we ever decide if tag_extra will always remain a simple key-value field with both columns as plain TEXT? I seem to recall someone saying that we might use JSON there eventually.

Pull-Request has been closed by ktdreyer

The tag_extra field is a TEXT field in the database, but the contexts of that field are always json. So, you can set tag extra fields to complex values (though you'd have to use the api as the cli doesn't have command line syntax for that).

$ lkoji add-tag xkey-test
$ lkoji call editTag2 '"xkey-test"' --kwargs '{"extra": {"a": 1, "b": {"c":2}}}'
None
$ lkoji call getTag xkey-test
{'arches': '',
 'extra': {'a': 1, 'b': {'c': 2}},
 'id': 1497,
 'locked': False,
 'maven_include_all': False,
 'maven_support': False,
 'name': 'xkey-test',
 'perm': None,
 'perm_id': None}

What we may do in the future is shift the underlying field from TEXT to JSONB.

The reason that we treat the top level keys differently is for versioning. This way, if you change the value of, say, rpm.macro.dist for a tag, that change is tracked separately.

Thanks Mike.

For posterity, Tomas is implementing OSBS's task priority policies at PR #2711

Metadata