install: fix help #550
install: fix help #550
Conversation
| sensitive=_missing, deprecated=_missing, description=_missing, | ||
| group=_missing, cli_names=_missing, cli_deprecated_names=_missing, | ||
| cli_metavar=_missing): | ||
| if type is None: |
There was a problem hiding this comment.
Can we avoid shadowing of python builtins?
There was a problem hiding this comment.
+1, why not call it e.g. type_ instead of doing _type = type?
There was a problem hiding this comment.
Because the Knob attribute name is type. IMHO shadowing would be an issue only if type was called in the function, which is not the case here. type is used as an argument name on numerous occasions in the Python standard library too, so I don't see why it should be an issue here.
|
|
||
| permit = knob( | ||
| None, | ||
| description="disable access rules by default, permit all access.", |
There was a problem hiding this comment.
Sorry for nitpicking, could you please keep the casing of the first letter? At least in this new group.
There was a problem hiding this comment.
This is how it was cased before, I did not (intentionally) change any of the descriptions.
There was a problem hiding this comment.
I would consider changing it a small doc fix, similar to PEP-8 fixes for Python. But then again I don't mind this in the end, I would probably ACK this even if you rewrote all the help to Pirate English.
ipapython/install/cli.py
Outdated
| groups = collections.OrderedDict() | ||
| groups[None] = basic_group | ||
| groups[None] = parser |
There was a problem hiding this comment.
I suppose we want to fail in case index is None now. Since the rest of the list will be of type optparser.OptionGroup, wouldn't it be better if this was None instead of parser?
There was a problem hiding this comment.
Group is not guaranteed to be not None, and None does not have .add_option().
There was a problem hiding this comment.
Oh, I see, so this will create some set of basic options. Please, could you add a comment there explaining it so that it's very clear?
| sensitive=_missing, deprecated=_missing, description=_missing, | ||
| group=_missing, cli_names=_missing, cli_deprecated_names=_missing, | ||
| cli_metavar=_missing): | ||
| if type is None: |
There was a problem hiding this comment.
+1, why not call it e.g. type_ instead of doing _type = type?
| if default is not _missing: | ||
| class_dict['default'] = default | ||
| if _order is not _missing: |
There was a problem hiding this comment.
What happens when class_dict is missing the 'order' key? That's one of the reasons why knob and extended_knob came to life but this is getting way too spaghettious for me to decrypt at this stage of development cycle.
There was a problem hiding this comment.
It will be inherited from the base class.
|
|
||
|
|
||
| def extend_knob(base, default=_missing, bases=_missing, group=_missing, | ||
| **kwargs): |
There was a problem hiding this comment.
There shall be at least a short docstring for future humans who try to find out what happened here.
| @@ -451,6 +454,7 @@ class ADTrustInstallInterface(ServiceAdminInstallInterface): | |||
| description="Add IPA masters to a list of hosts allowed to " | |||
| "serve information about users from trusted forests" | |||
| ) | |||
| add_agents = replica_install_only(add_agents) | |||
There was a problem hiding this comment.
Is this fixing some kind of a regression? I don't mind if it's fixed in this commit, I would just like to know.
There was a problem hiding this comment.
This is just a cosmetic fix - the knob was redefined in ServerInstallInterface only to decorate it with replica_install_only, which is a) completely unnecessary b) inconsistent with other component installers.
Remove duplicate -w alias of --admin-password in ipa-server-install and ipa-replica-install. https://pagure.io/freeipa/issue/6392
Remove duplicate definitions of knobs already defined in client install. https://pagure.io/freeipa/issue/6392
Split off SSSD knob definitions from the ClientInstallInterface class into a new SSSDInstallInterface class. https://pagure.io/freeipa/issue/6392
Do not automatically create the "basic options" and "uninstall options" option groups in the CLI code. https://pagure.io/freeipa/issue/6392
Re-introduce option groups in ipa-client-install, ipa-server-install and ipa-replica-install. https://pagure.io/freeipa/issue/6392
|
Works like a charm, ACK. |
|
master:
|
This PR fixes the known issue in the installer refactoring PR #232 and concludes https://pagure.io/freeipa/issue/6392.
server install: remove duplicate -w option
Remove duplicate -w alias of --admin-password in ipa-server-install and
ipa-replica-install.
install: add missing space in realm_name description
server install: remove duplicate knob definitions
Remove duplicate definitions of knobs already defined in client install.
client install: split off SSSD options into a separate class
Split off SSSD knob definitions from the ClientInstallInterface class into
a new SSSDInstallInterface class.
install CLI: remove magic option groups
Do not automatically create the "basic options" and "uninstall options"
option groups in the CLI code.
install: re-introduce option groups
Re-introduce option groups in ipa-client-install, ipa-server-install and
ipa-replica-install.
https://pagure.io/freeipa/issue/6392