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