[master] Fix options parsing of highstate returner #66828
Conversation
|
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. |
salt-project-bot-prod-environment
bot
changed the title
| @@ -164,7 +164,7 @@ def _options_browser(cfg, ret_config, defaults, virtualname, options): | |||
| # default place for the option in the config | |||
| value = _fetch_option(cfg, ret_config, virtualname, options[option]) | |||
|
|
|||
| if value: | |||
| if value != "": | |||
There was a problem hiding this comment.
I suspect value is not None also should be in this condition, but I am not sure what the the previous condition was trying to achieve.
There was a problem hiding this comment.
The previous condition try to avoid parameters without value and then, not record them in the dict. But the previous code catchs parameters with "False" value which souldn't be, causing thoose parameters to be excluded of the dict. value is not None could be an alternative, not tested it.
There was a problem hiding this comment.
value is not None would miss value = "". The original code if value would evaluate True for both scenarios(None and ""). Not sure what this code is actually fixing.
There was a problem hiding this comment.
Hello twangboy,
Have you read the issue #66816 ?
"if value:" doesn't work for any configuration directives that is equal to False, like this
highstate.report_failures: True
highstate.report_changes: False
highstate.report_everything: False
Which gives this logs :
[DEBUG ] highstate setup {'report_format': 'html', 'report_delivery': 'smtp', 'smtp_sender': '[email protected]', 'smtp_recipients': '[email protected]', 'smtp_failure_subject': 'failure minion {id} on host {host}', 'smtp_success_subject': 'succes minion {id} on host {host}', 'smtp_server': 'smtpunix.it.noname.fr'}
Fixing code with "if value != """ gives this log :
[DEBUG ] highstate setup {'report_everything': False, 'report_changes': False, 'report_failures': True, 'report_format': 'html', 'report_delivery': 'smtp', 'smtp_sender': '[email protected]', 'smtp_recipients': '[email protected]', 'smtp_failure_subject': 'failure minion {id} on host {host}', 'smtp_success_subject': 'succes minion {id} on host {host}', 'smtp_server': 'smtpunix.it.noname.fr'}
This proceed reports to be send as desired.
Regards.
There was a problem hiding this comment.
OK, so we're just checking if value is set to either True or False. Do we need to handle None here?
This will also need a changelog and some tests. You can you caplog to test debug output.
| @@ -164,7 +164,7 @@ def _options_browser(cfg, ret_config, defaults, virtualname, options): | |||
| # default place for the option in the config | |||
| value = _fetch_option(cfg, ret_config, virtualname, options[option]) | |||
|
|
|||
| if value: | |||
| if value != "": | |||
There was a problem hiding this comment.
OK, so we're just checking if value is set to either True or False. Do we need to handle None here?
This will also need a changelog and some tests. You can you caplog to test debug output.
What does this PR do?
Fix Issue #66816
What issues does this PR fix or reference?
Fixes #66816
Previous Behavior
Highstate returner doesn't take in configuration of some "report_" options
New Behavior
Now the returner use configuration of options report_failures, report_changes and report_everything and send report accordingly.
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
No
Please review Salt's Contributing Guide for best practices, including the
PR Guidelines.
See GitHub's page on GPG signing for more information about signing commits with GPG.