On 10/02/2012 02:33 PM, Tomas Babej wrote: > On 09/26/2012 05:44 PM, Martin Kosek wrote: >> On 09/25/2012 02:59 PM, Tomas Babej wrote: >>> On 09/25/2012 02:31 PM, Martin Kosek wrote: >>>> On 09/25/2012 02:22 PM, Tomas Babej wrote: >>>>> Hi, >>>>> >>>>> Group-mod command no longer allows --rename and/or --external >>>>> changes made to the admins group. In such cases, ProtectedEntryError >>>>> is being raised. >>>>> >>>>> https://fedorahosted.org/freeipa/ticket/3098 >>>>> >>>>> Tomas >>>>> >>>> Just based on a quick glance, I see few issues: 1) I would like to see unit >>>> tests for this scenario 2) Some overlooked debug output: + >>>> self.log.info(str(keys)) Martin >>> I removed the unfortunate debug output and added two unit tests to test the >>> scenarios. >>> >>> Tomas >> I finally got to a proper review and here it is: >> >> 1) I think we should use some common global variable containing protected >> groups and not define it in every command separately (duplication -> errors). >> One is already used: >> >> ... >> protected_group_name = u'admins' >> ... >> >> I would like to see that to be used in both group-del and group-mod. I also >> think we should protect "trust admins" too as this group is also encoded in >> trust ACI, i.e. using a global variable like this one: >> >> PROTECTED_GROUPS = (u'admins', u'trust admins') >> >> >> 2) Minor issue, I think instead of: >> >> + is_admins_group = u'admins' in keys >> >> a more common pattern in IPA is the following: >> >> + is_admins_group = keys[-1] == u'admins' >> >> >> 3) We usually do not end our error messages in exceptions with ".": >> >> ... >> + raise errors.ProtectedEntryError(label=u'group', >> key=u'admins', reason=u'Cannot be renamed.') >> ... >> + raise errors.ProtectedEntryError(label=u'group', >> key=u'admins', reason=u'Cannot support external non-IPA members.') >> ... >> >> Otherwise the patch looks OK. >> >> Martin > I fixed the relevant issues and added few more test cases as well. > > Please check the new patch version. > > Tomas
Looks good, works fine. I have just 2 minor styling issues: 1) The following blob can be simplified. From: + is_protected_group = False + if keys[-1] in PROTECTED_GROUPS: + is_protected_group = True to: is_protected_group = keys[-1] in PROTECTED_GROUPS 2) Lines with raised error messages are quite long: + raise errors.ProtectedEntryError(label=u'group', key=keys[-1], reason=u'Cannot be renamed') + raise errors.ProtectedEntryError(label=u'group', key=keys[-1], reason=u'Cannot support external non-IPA members') They should be wrapped. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
