On 10/03/2012 11:49 AM, Tomas Babej wrote: > On 10/03/2012 09:18 AM, Martin Kosek wrote: >> 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 > Styling issues corrected. > > Tomas
ACK. Pushed to master, ipa-3-0. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
