On 30.06.2016 15:16, Florence Blanc-Renaud wrote:
On 06/30/2016 01:30 PM, Fraser Tweedale wrote:
On Thu, Jun 30, 2016 at 07:49:04PM +1000, Fraser Tweedale wrote:
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

Updated patch attached.  Third time's the charm? ;)

Cheers,
Fraser

Hi Fraser,

thanks for the updated patch. Yes, 3rd time's the charm :)
Ack,
Flo.

Pushed to master: 3fab1b63502c3206d792b7aeaa12d486612f0137

--
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

Reply via email to