-
Notifications
You must be signed in to change notification settings - Fork 28
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
Unify command-line options and configuration #227
Conversation
Added a unit test for verifying that the configuration file options take precedence. |
@@ -63,7 +63,7 @@ def merge(self, d): | |||
""" | |||
Merge variables from dict ``d`` into the configuration | |||
|
|||
The last one wins. | |||
The first one wins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it the last one? If both dicts have the same key, the value will be updated to the value for the key in the passed dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct. The reason is because of the default cli options. I wasn't able to find a reliable way to tell whether the option had been passed in vs the argparse default value. Since pki also uses the framework I can't drop all default values because they may use them as well.
The tests verify this in an indirect way. e.g. setting output_type=human vs the default value of json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I don't understand the comment. Isn't the last one winning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because of the object.__setattr__(self, key, value)
in setitem. It basically makes it immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, missed that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below, otherwise LGTM.
This makes it possible to add command-line options to the configuration file. The config file is loaded then the command-line options are merged in. The first one option set takes precedence. So if an option, say output_type, is in the configuration file then passing output-type on the command-line will not override it. The workaround is to pass --config= to ipa-healthcheck in order to not load the configuration file. This will allow for greater flexibility when running this automatically without having to manually change test timer scripting directly. https://bugzilla.redhat.com/show_bug.cgi?id=1872467 Signed-off-by: Rob Crittenden <[email protected]>
This adds command-line options to the configuration, but not vice-versa.
It will allow for greater flexibility when running this automatically
without having to change any scripting.
https://bugzilla.redhat.com/show_bug.cgi?id=1872467
Signed-off-by: Rob Crittenden [email protected]