On Thu, Jun 30, 2016 at 11:38:35AM +0200, Florence Blanc-Renaud wrote: > On 06/30/2016 06:29 AM, Fraser Tweedale wrote: > > On Wed, Jun 29, 2016 at 11:30:14AM +0200, Florence Blanc-Renaud wrote: > > > On 06/29/2016 07:25 AM, Fraser Tweedale wrote: > > > > The attached patch fixes > > > > https://fedorahosted.org/freeipa/ticket/5991. > > > > > > > > Thanks, > > > > Fraser > > > > > > > > > > > > > > > Hi Fraser, > > > > > > A few cosmetic comments: > > > > > > PEP8 issues: > > > ./ipalib/errors.py:1399:1: E302 expected 2 blank lines, found 1 > > > ./ipaserver/plugins/cert.py:394:80: E501 line too long (98 > 79 > > > characters) > > > ./ipaserver/plugins/cert.py:496:80: E501 line too long (81 > 79 > > > characters) > > > > > > and there is a typo in ipaserver/plugins/cert.py > > > + doc=_("automatically add the principal if it doesn't exist > > > (service princpals only)"), > > > > > > should be "princ*i*pals only" > > > > > > Otherwise LGTM, > > > Flo > > > > > Thanks for review, Flo. Updated patch attached. > > > > Cheers, > > Fraser > > > Hi Fraser, > > thanks for updated patch. There is still a pep8 error: > ./ipalib/errors.py:1399:1: E302 expected 2 blank lines, found 1 > Whups!
> I am also wondering if the message is clear enough. When running the CLI > it's ok because the user typed --add, but the GUI displays "'add' is not > supported for user principals" > and I feel > "'add' option is not supported for user principals" > would be more user-friendly. What do you think? > Yes, I concur that mentioning "option" is an improvement. Will cut a new patch shortly Thanks! Fraser -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code