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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
