Re: [PATCH] Implement fas groups in process-git-requests

[Pierre-Yves Chibon]

On Tue, Dec 16, 2014 at 12:28:53PM +0000, Thomas Spura wrote:
> +def check_owner(fas, owner):
> """Check if owner is a valid FAS account or group.
> If owner is a valid FAS account, also check if owner is a packager.
> """
> warnings = []
> if owner.startswith('group::'):
>    group = owner[7:]
>    if not group.endswith('-sig'):
>        warnings.append('WARNING: "%s" is not a valid FAS group.' % group)
>        return warnings
>    group = fas.group_by_name(group)

If the group does not exist, fas will throw an exception. Is this catch
somewhere else?
>    if not 'group_type' in group:
>        warnings.append('WARNING: "%s" is not a valid FAS account/group.' % owner)

I think rather than checking if there is *a* group_type, we should check if the
group_type is `pkgdb` as otherwise pkgdb will refuse it.

Also the warning message can be tweak, in this case we know it's not a valid
group :)
>        return warnings
>    # Valid group
>    return warnings
> else: # Check if owner is a valid user
>    person = fas.person_by_username(owner)
>    if not 'status' in person:
>        warnings.append('WARNING: "%s" is not a valid FAS account.' % i)
>        return warnings
>    groups = [g['name'] for g in person.approved_memberships]
>    if not 'packager' in groups:
>        warnings.append('WARNING: "%s" is not in the packager group.' % i)
>    return warnings

pep8 or pylint might complain that you could have a single return statement, but
that shouldn't have any impact vs the current behavior
> def check_owners(fas, owner, comaintainers, cc_list):
>      print "Checking owners..."
>      warnings = []
> @@ -503,7 +531,7 @@ def check_owners(fas, owner, comaintainers, cc_list):
>      for i in [owner] + comaintainers:
>          for retry in range(1, config['pkgdb.retries'] + 1):
>              try:
>            person = fas.person_by_username(i)
>            warnings.extend(check_owner(fas, i))
>        except AuthError, e:
>            if retry >= config['pkgdb.retries']:
>                break
>
> @@ -513,20 +541,8 @@ def check_owners(fas, owner, comaintainers, cc_list):
>              else:
>                  break
>    if not 'status' in person:
>        warnings.append('WARNING: "%s" is not a valid FAS account.' % i)
>        break
>    groups = [g['name'] for g in person.approved_memberships]
>    if not 'packager' in groups:
>        warnings.append('WARNING: "%s" is not in the packager group.' % i)
> for i in cc_list:
>    person = fas.person_by_username(i)
>    if not 'status' in person:
>        warnings.append('WARNING: "%s" is not a valid FAS account.' % i)
>        break
>    warnings.extend(check_owner(fas, i))

Rest looks good to me.

Thanks!
Pierre