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

[Thomas Spura]

Pierre-Yves Chibon <pingou at pingoured.fr> schrieb am Tue Dec 16 2014 at
2:52:33 PM:

> On Tue, Dec 16, 2014 at 02:41:53PM +0100, Pierre-Yves Chibon wrote:
> > On Tue, Dec 16, 2014 at 01:18:54PM +0000, Thomas Spura wrote:
> > > From 377c783cbb9460bba87fdda1add1251bffe45b79 Mon Sep 17 00:00:00 2001
> > > From: Thomas Spura <thomas.spura at gmail.com>
> > > Date: Tue, 16 Dec 2014 14:12:34 +0100
> > > Subject: [PATCH 2/2] Catch exception from FAS, when group does not
> exist
> > >
> > > Furthermore, the group_type must be 'pkgdb', otherwise pkgdb would
> > > throw an error later on.
> > > ---
> > >  scripts/process-git-requests/process-git-requests | 16
> +++++++++-------
> > >  1 file changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/scripts/process-git-requests/process-git-requests
> b/scripts/process-git-requests/process-git-requests
> > > index bce4a54..99de338 100755
> > > --- a/scripts/process-git-requests/process-git-requests
> > > +++ b/scripts/process-git-requests/process-git-requests
> > > @@ -33,7 +33,7 @@ import time
> > >  import webbrowser
> > >  import socket
> > >  from configobj import ConfigObj, flatten_errors
> > > -from fedora.client import AccountSystem, AuthError
> > > +from fedora.client import AccountSystem, AuthError, AppError
> > >  from pkgdb2client import PkgDB, PkgDBException
> > >  from optparse import OptionParser
> > >  from validate import Validator
> > > @@ -507,12 +507,14 @@ def check_owner(fas, owner):
> > >          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 not 'group_type' in group:
> > > -            warnings.append('WARNING: "%s" is not a valid FAS
> account/group.' % owner)
> > > +        try:
> > > +            group = fas.group_by_name(group)
> > > +            if not group['group_type'] == 'pkgdb':
> > > +                warnings.append('WARNING: "%s" is not a valid FAS
> group.' % group)
> > > +                return warnings
> > > +        except AppError:
> > > +            warnings.append('WARNING: "%s" is not a valid FAS group.'
> % group)
> > +1 for me otherwise,
>

There is now a different warning message.


>
> Actually, one more thing.
>
> If you
> - option a) move the check for the group type outside of the try/except
> (likely
>   going to need something like group = None, try/except if group is not
> None:...
>
> or
>
> - option b) move more of the logic inside the try/except
>
> You can probably keep it so that the method as only 1 return statement :)
>

I took option b, but initially avoided it, because I thought it was more
readable back then ;)

So all changes are now melt into one commit now, see the attachment :)

Greetings,
   Tom


But I could see other people disagreeing with this :)
>
>
> Pierre
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.fedoraproject.org/pipermail/rel-eng/attachments/20141216/d4524b22/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Implement-fas-groups-in-check_owners.patch
Type: text/x-patch
Size: 3465 bytes
Desc: not available
URL: <http://lists.fedoraproject.org/pipermail/rel-eng/attachments/20141216/d4524b22/attachment.bin>