On 07/31/2015 02:22 PM, Martin Basti wrote:
On 30/07/15 06:22, Fraser Tweedale wrote:
On Thu, Jul 30, 2015 at 10:19:19AM +1000, Fraser Tweedale wrote:
On Wed, Jul 29, 2015 at 03:48:47PM +0200, Jan Cholasta wrote:
Dne 29.7.2015 v 15:46 Martin Basti napsal(a):
On 29/07/15 15:41, Martin Basti wrote:
On 25/07/15 03:40, Fraser Tweedale wrote:
On Fri, Jul 24, 2015 at 05:53:56PM +0200, Tomas Babej wrote:
On 07/24/2015 05:34 PM, Martin Basti wrote:
On 24/07/15 16:52, Tomas Babej wrote:
On 07/24/2015 03:40 PM, Fraser Tweedale wrote:
The attached patch adds --out option to user-show for saving user's
certificate(s) to file.

Thanks,
Fraser



I hate to nitpick here, but is "out" really a descriptive option name
here? I'd prefer something more explicit, like '--save-cert-to', or
maybe even have this operation implemented as a separate command
altogether.

Tomas

This keyword was already used with several commands. For consistency
might be better to have it the same.

True. I see this options is being used in the following commands:

  - cert-show
  - vault-retrieve
  - host-show
  - service-show
  - user-show (proposed)

While the first two seem to be an acceptable fit for an option called
--out, as they mainly deal with cert/secret, using the '--out' for the
latter three is a poor decision imho.

I agree the consistency is important, I'm just not happy to see this
spread further.

Tomas
Perhaps we should go with something like `--certout' instead, and
support `--certout' in addition to `--out' in host-show and
service-show, esentially deprecating `--out' for those commands.

Cheers,
Fraser
Good idea, but we should do this for all commands, at the same time.
IMO this is not for 4.2, you may file a ticket to deprecate --out
option and replace it by --certout or something.
The "in" option is named --certificate, so it should be --certificate-out.

I will do review is nobody is against this patch :)
Martin^2
LGTM

Is a ticket somewhere for this?

No ticket; I just wanted it so I wrote the patch :)

I'll file the ticket for future change to `--certificate-out'
though.

Ticket: https://fedorahosted.org/freeipa/ticket/5166


ACK

Before pushing, please create a ticket for this CLI addition and link it to this patch - so that this change/improvement can be references from other sources and/or downstream distros.

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