Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enumerate available options in IPA installer #209

Closed
wants to merge 1 commit into from

Conversation

Akasurde
Copy link
Member

@Akasurde Akasurde commented Nov 2, 2016

Fix adds enumerated list of available options in IPA server
installer and IPA CA installer help options

Fixes https://fedorahosted.org/freeipa/ticket/5435

Signed-off-by: Abhijeet Kasurde akasurde@redhat.com

@Akasurde
Copy link
Member Author

ping

@MartinBasti
Copy link
Contributor

Hi, we changed a lot of code during refactoring, PR doesn't apply. IMO list of choices shown in --help should be handled in knob() if metavar is not specified and type is choice

@MartinBasti MartinBasti self-assigned this Nov 15, 2016
@HonzaCholasta
Copy link
Contributor

@mbasti-rh: knob() already handles choices, it's the built-in optparse module which does not display them. Once the installer code is migrated to argparse, this problem will go away.

@MartinBasti
Copy link
Contributor

@jcholast I know, but it doesn't fill metavar with choices. I don't know when we will migrate to argparse, so I think until that we can extend it to show choices with optparse too

@MartinBasti
Copy link
Contributor

@jcholast any update? Should reject this PR and wait for argparse or fix it with optparse as well? IMO fixing it now is better for UX, we dont know when or if we migrate to argparse.

@HonzaCholasta
Copy link
Contributor

@mbasti-rh, I don't care as long as it's done right (i.e. without hardcoding cli_metavar in knob definitions).

@Akasurde
Copy link
Member Author

Akasurde commented Dec 7, 2016

@jcholast @mbasti-rh I will work on modifying Knob() to handle metavar

@HonzaCholasta
Copy link
Contributor

@Akasurde, Knob() already handles metavar properly, you need to work on the interface between the installer and optparse - ipapython.install.cli.

@@ -179,6 +179,7 @@ def add_options(cls, parser, positional=False):
elif issubclass(knob_scalar_type, enum.Enum):
kwargs['type'] = 'choice'
kwargs['choices'] = [i.value for i in knob_scalar_type]
kwargs['metavar'] = "[%s]" % ("|".join(kwargs['choices']))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please use the same format as argparse: {a,b,c}.
  • str.format() is the preferred way of formatting strings.

@@ -179,6 +179,8 @@ def add_options(cls, parser, positional=False):
elif issubclass(knob_scalar_type, enum.Enum):
kwargs['type'] = 'choice'
kwargs['choices'] = [i.value for i in knob_scalar_type]
kwargs['metavar'] = "{{{0}}}".format(
"|".join(kwargs['choices']))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use , rather than |, as argparse does.

@HonzaCholasta
Copy link
Contributor

Works for me, although you should probably keep the changes to ipa-ca-install from the original patch (using the argparse format, of course).

parser.add_option("--ca-signing-algorithm", dest="ca_signing_algorithm",
type="choice",
choices=('SHA1withRSA', 'SHA256withRSA', 'SHA512withRSA'),
type="choice", choices=ca_alogs,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: ca_alogs.

Fix adds enumerated list of available options in IPA server
installer and IPA CA installer help options

Fixes https://fedorahosted.org/freeipa/ticket/5435

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@HonzaCholasta HonzaCholasta added the ack Pull Request approved, can be merged label Jan 3, 2017
@HonzaCholasta
Copy link
Contributor

@HonzaCholasta HonzaCholasta added the pushed Pull Request has already been pushed label Jan 3, 2017
@Akasurde
Copy link
Member Author

Akasurde commented Jan 3, 2017

@HonzaCholasta Thanks for your help and reviews.

@Akasurde Akasurde deleted the tkt-5435 branch January 3, 2017 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants