note: the query for getUserGroups is lifted almost verbatim from the existing kojihub.auth:get_user_groups function. We're simply skipping the step of converting that into a single dictionary.

I've rebased this onto master and that required reworking some tests to mock correctly. I've also reworked this a bit to remove duplication of queries, so now the original get_user_groups is being fed from an iter_user_groups. iter_user_groups is what's used to populate the data from the hub call getUserGroups.

Sorry, can you rebase it once more? I'll put it into test-ready then.

I've got a rebase here, lemme post it

https://pagure.io/fork/mikem/koji/commits/pr3912updates

The overall pattern here is fine. A couple things though

It doesn't feel right to use query.iterate here. We're always querying a specific user_id and never expect a large number of rows to be returned. This turns a simple query into four.

DECLARE qp_cursor_139890124317536_2146560_0 NO SCROLL CURSOR FOR 
SELECT group_id, name
  FROM user_groups
  JOIN users ON group_id = users.id
 WHERE (active IS TRUE)
   AND (user_id=1)
   AND (users.usertype=2)
FETCH 1000 FROM qp_cursor_139890124317536_2146560_0
FETCH 1000 FROM qp_cursor_139890124317536_2146560_0
CLOSE qp_cursor_139890124317536_2146560_0

Also, the PR adds tests for the new function in multiple places. Seems like there might be overlap. Perhaps the single test in test_user_groups.py was meant to be dropped in favor of test_get_user_groups.py ?

You're right. I'd started adding the new tests into the existing test file, and then migrated them to their own, and neglected to commit the removal from the original. I'm also probably copying the wrong/outdated method for setting up a QueryProcessor test, since I think that was reworked since I originally hacked on this.

I will poke at this again later today, and see if I can't simplify it in so doing. In particular I think I'm going to just abandon the iterative refactor and make the exposed hub api reveal a staticmethod wrapper on the auth module's existing function.

uinfo = get_user(user)
if not uinfo or uinfo['usertype'] != koji.USERTYPES['NORMAL']:
    raise koji.GenericError("No such user: %s" % user)

This reports a misleading error for other user types. Better to call get_user with strict=True for basic validation.

I'm not sure if we even need to restrict the call to normal users. Simpler to let the query just report [], which is a true answer. Also it's not 100% clear to me that a host can't have groups. I wouldn't recommend it, but add_group_member doesn't block it.

If we were to check usertype here, probably best only assert that the user is not a group (as add_group_member does).

rebased, simplified.

I fought for a bit with the get_user api -- it worked when I ran my tests alone, but failed when running the whole suite. Turns out that the TestGetUserByKrbPrincipal wasn't ever tearing itself down, so I fixed that.

I also identified a problem in the api json while adding my own change, simply that the version had changed and the Retry type had as well.

Thank you very much for your reviews

Turns out that the TestGetUserByKrbPrincipal wasn't ever tearing itself down

Thanks. I thought I'd found all of those. I'll have to search deeper.

I also identified a problem in the api json while adding my own change, simply that the version had changed and the Retry type had as well.

This is actually causing the test to fail for me here. This value is an import, and the check-api does detect that it's external to the code, but still tries to enforce a type check. Unfortunately the type varies depending on the version of requests that is installed.

I'll make a separate PR for the script. In the meantime, probably best to leave the api.json change out of this PR.

filed #4132 for the api test

fyi, previous tearDown cleanup in #4068 and #4082

More tearDown fixes -- #4133

The current version uncovers the reason that you initially wanted to separate out the underlying query. xmlrpc can't handle integer keys.

TypeError: dictionary key must be string

Options:

• just return the names, e.g. with .values()
• reconstruct the list of dicts, e.g. dict(zip(('id', 'name'), *gdict.items()))
• split out the query portion of the function similar to before

Went with the list of dicts using the id and name keys, since that's a pattern used elsewhere in the API.

Set the api.json back to the state from master.

Thanks for the quick fix!

:thumbsup:

Commit 3dd76652 fixes this pull-request