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