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

[Thomas Spura]

Hi all,

the attached patch removes the warnings from process-git-requests, that
will show up, when adding a group to InitialCC or owner list of packages.
See e.g. [1-2].

pingou already reviewed the patch before sending it over here. Yet another
review would still be nice as this wasn't tested yet against a pending
group git request. Maybe [1] would be a good candidate? :)

Greetings,
    Tom

[1] https://bugzilla.redhat.com/1167830
[2] https://bugzilla.redhat.com/1131543

[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

[Thomas Spura]

Pierre-Yves Chibon  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

[Pierre-Yves Chibon]

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

Last tiny nit-pick, we may want to use a different warning message here, just to
know where the problem is coming from (is there no group or is there a group but
just of the wrong type)

+1 for me otherwise,

Pierre

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

[Thomas Spura]

Pierre-Yves Chibon  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 
> > > 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

[Pierre-Yves Chibon]

> From 44fd9fe553ad79a1f2ae1eb5e7043dcfbf4da3c7 Mon Sep 17 00:00:00 2001
> From: Thomas Spura 
> Date: Mon, 15 Dec 2014 21:41:26 +0100
> Subject: [PATCH] Implement fas groups in check_owners
>
> This avoids a warning, so that groups can be processed.
>
> See:
> https://bugzilla.redhat.com/1167830
> https://bugzilla.redhat.com/1131543
>
> scripts/process-git-requests/process-git-requests | 43 +++++++++++++++--------
>  1 file changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/process-git-requests/process-git-requests b/scripts/process-git-requests/process-git-requests
> index 068a481..0c56adb 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
> @@ -496,6 +496,31 @@ def process_no_request(bug, allcomments):
>      return True
>
> +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_name = owner[7:]
>    try:
>        group = fas.group_by_name(group_name)
>        if not group_name.endswith('-sig') and not group['group_type'] == 'pkgdb':
>            warnings.append('WARNING: "%s" is not a valid FAS group.' % group_name)
>    except AppError:
>        warnings.append('WARNING: "%s" could not be found as FAS group.' % group_name)
> 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)
>    else:
>        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
>
> def check_owners(fas, owner, comaintainers, cc_list):
>      print "Checking owners..."
>      warnings = []
> @@ -503,7 +528,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 +538,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))
>
> return warnings

This patch still needs to be merged and as people processing the requests are
still getting invalid warnings when the request includes a group.

Thanks,
Pierre

[Kevin Fenzi]

On Mon, 5 Jan 2015 19:45:36 +0100
Pierre-Yves Chibon  wrote:
> This patch still needs to be merged and as people processing the
> requests are still getting invalid warnings when the request includes
> a group.

Applied. Thanks. ;)

kevin