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.

Sign up for GitHub

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

Jump to bottom

Fixes for configuration file support for cli options #257

Merged
merged 4 commits into from May 4, 2022
Merged

Fixes for configuration file support for cli options #257

merged 4 commits into from May 4, 2022

Conversation

rcritten
Copy link
Collaborator

Three small fixes to allowing the configuration file to set command-line options discovered by QE during review process

@rcritten
Validate that a known output-type has been selected
95c683c 
A user may pass an unknown value in via the configuration file.

https://bugzilla.redhat.com/show_bug.cgi?id=2079698

Signed-off-by: Rob Crittenden <rcritten@redhat.com>
@rcritten
Limit config file delimiters to =, catch empty values
4214423 
ConfigParser allows both = and : as a delimiter. Limit to
just = to match the configuration file man page.

Don't allow empty values in options in the config file.

https://bugzilla.redhat.com/show_bug.cgi?id=2079739

Signed-off-by: Rob Crittenden <rcritten@redhat.com>
@rcritten
Relocate eval of debug/verbose in case they are set in config file
1fd0e27 
Since the configuration file allows options to be set we need
to evaluate them after the merge.

Leaving version handling pre-config load since it makes no sense
within the config file.

https://bugzilla.redhat.com/show_bug.cgi?id=2079861

Signed-off-by: Rob Crittenden <rcritten@redhat.com>
@flo-renaud
Copy link
Contributor

Hi @rcritten
thanks for the PR, the new code LGTM.
Tested the following:

  • add output_type=other in conf file, ipa-healthcheck exits with error Unknown output-type 'other'
  • add output_type=human in conf file, works fine
  • use invalid format output_type: human in conf file, ipa-healthcheck exits with Unable to parse /etc/ipahealthcheck/ipahealthcheck.conf and prints the line with the format error
  • use empty value output_type= in conf file, ipa-healthcheck exits with Empty value for output_type in /etc/ipahealthcheck/ipahealthcheck.conf [default]
  • comment a setting in conf file #output_type=human, ipa-healthcheck doesn't take it into account and uses default JSON output type

My testing detected other issues, not related to this PR:

  • add debug=False in the conf file, ipa-healthcheck runs in debug mode. This is because if options.debug on line 366 is equivalent to if 'False' (the test compares a string value, not a boolean).
  • add verbose=False in the conf file, same issue as above with debug.

@flo-renaud flo-renaud self-assigned this Apr 29, 2022
@rcritten
Copy link
Collaborator Author

Thanks for the review. I added a function to try to convert the configuration strings into native python data types.

flo-renaud
flo-renaud reviewed May 3, 2022
View reviewed changes
src/ipahealthcheck/core/config.py
"true",
"false",
):
return value.lower() == 'true'
Copy link
Contributor

Choose a reason for hiding this comment

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

With the convert_string function, debug=other still triggers the debug mode. The boolean options should probably be handled separately (with something like if key in ('debug', 'verbose'):...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was trying to avoid that because I can't control what other users of healthcheck may configure as variables (pki has its own pki-healthcheck command). I suppose I can add it for the currently known booleans. I don't think I have access to the argparse options directly at this point so I can't auto-detect the type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even with access to argparse I can't find a way to get the expected types so I added a list of known booleans and integers.

@rcritten
Convert configuration option strings into native data types
5ea4ae3 
When loading options from the configuration file they will all
be strings. This breaks existing boolean checks (if <something>)
and some assumptions about integer types (e.g. timeout, indent).

So try to detect the data type, defaulting to remain as a string.

Also hardcode some type validation for known keys to prevent
things like debug=foo (which would evaluate to True).

https://bugzilla.redhat.com/show_bug.cgi?id=2079861

Signed-off-by: Rob Crittenden <rcritten@redhat.com>
@flo-renaud
Copy link
Contributor

Hi @rcritten
thanks for the updated PR, LGTM.
Tested the following:

  • add output_type=other in conf file, ipa-healthcheck exits with error Unknown output-type 'other'
  • add output_type=human in conf file, works fine
  • use invalid format output_type: human in conf file, ipa-healthcheck exits with Unable to parse /etc/ipahealthcheck/ipahealthcheck.conf and prints the line with the format error
  • use empty value output_type= in conf file, ipa-healthcheck exits with Empty value for output_type in /etc/ipahealthcheck/ipahealthcheck.conf [default]
  • comment a setting in conf file #output_type=human, ipa-healthcheck doesn't take it into account and uses default JSON output type
  • add debug=False / verbose=False in the conf file, no debug/verbose output
  • add debug=true in the conf file, debug output

@rcritten
Copy link
Collaborator Author

rcritten commented May 4, 2022

Thanks for the careful review.

@rcritten rcritten merged commit 195adff into freeipa:master May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flo-renaud flo-renaud flo-renaud approved these changes

Assignees

@flo-renaud flo-renaud

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rcritten @flo-renaud