6:28 a.m.
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
6:56 a.m.
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
7:18 a.m.
Pierre-Yves Chibon <pingou(a)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
7:41 a.m.
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>
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
7:52 a.m.
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>
> 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
9:22 a.m.
Pierre-Yves Chibon <pingou(a)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(a)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
12:45 p.m.
From 44fd9fe553ad79a1f2ae1eb5e7043dcfbf4da3c7 Mon Sep 17 00:00:00
2001
From: Thomas Spura <thomas.spura(a)gmail.com>
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
3399
days inactive
3419
days old
rel-eng@lists.fedoraproject.org
7 comments
3 participants
participants (3)
-
Kevin Fenzi -
Pierre-Yves Chibon -
Thomas Spura