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

[Thomas Spura]

Pierre-Yves Chibon <pingou at pingoured.fr> schrieb am Tue Dec 16 2014 at
1:56:57 PM:

> 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?
>

I don't think so. Thanks for catching this. Done in the next patch.


>
> > +        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.
>

Done.


>
> Also the warning message can be tweak, in this case we know it's not a
> valid
> group :)
>

Done.


>
> > +            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
>

Nevertheless, I moved the last 'return warnings' now down to the end of the
function.


>
> > +
> > +
> >  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.
>


Attached is a followup patch to correct the issues from above:
- catching AppError
- group_type must be 'pkgdb'
- moving return to the end of function.

Greetings,
    Tom
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.fedoraproject.org/pipermail/rel-eng/attachments/20141216/2f36a745/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Catch-exception-from-FAS-when-group-does-not-exist.patch
Type: text/x-patch
Size: 2303 bytes
Desc: not available
URL: <http://lists.fedoraproject.org/pipermail/rel-eng/attachments/20141216/2f36a745/attachment-0001.bin>