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

[Pierre-Yves Chibon]

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(a)gmail.com&gt;
>  > 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, 
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 :)

But I could see other people disagreeing with this :)


Pierre