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

file modified
+8 -1
@@ -1192,7 +1192,8 @@ 

      return packages

  

  

- def list_tags(build=None, package=None, perms=True, queryOpts=None, pattern=None):

+ def list_tags(build=None, package=None, perms=True, queryOpts=None,

+               pattern=None, extra_key=None):

      """List tags according to filters

  

      :param int|str build: If build is specified, only return tags associated with
@@ -1210,6 +1211,8 @@ 

      :param bool perms: If perms is True, perm_id and perm is added to resulting maps.

      :param dict queryOpts: hash with query options for QueryProcessor

      :param pattern: If glob pattern is specified, only return tags matching that pattern.

+     :param str extra_key: If specified, only return tags that have this key

+                           configured as an "extra" setting.

  

      :returns list of dicts: Each map contains id, name, arches and locked keys and

                              additional keys as specified via package or perms options.
@@ -1258,6 +1261,10 @@ 

          pattern = pattern.replace(

              '\\', '\\\\').replace('_', r'\_').replace('?', '_').replace('*', '%')

          clauses.append('tag.name ILIKE %(pattern)s')

+     if extra_key is not None:

+         joins.append('tag_extra ON tag.id = tag_extra.tag_id')

+         clauses.append('tag_extra.active = true')

+         clauses.append('tag_extra.key = %(extra_key)s')

  

      query = QueryProcessor(columns=fields, aliases=aliases, tables=tables,

                             joins=joins, clauses=clauses, values=locals(),

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.

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

3 years ago

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

3 years ago

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

3 years ago

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