-
Notifications
You must be signed in to change notification settings - Fork 534
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
[report] Honour -o/-e/-n from cmdline over preset #3195
[report] Honour -o/-e/-n from cmdline over preset #3195
Conversation
Particular reproducer: on RHOCP preset (which disables |
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
When preset disables a plugin that user wants to manually enable, current merging of options ends up with -e PLUG -n PLUG which makes the plugin still disabled. apply_options_from_cmdline method must remove the preset options for any plugin that interfere with cmdline options for the plugin. Resolves: sosreport#3195 Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
784e327
to
dc6fa02
Compare
Looks good to me |
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.
Ack for the goal of this change. Just one comment below.
# A plugin that is [enabled|disabled|only] in cmdopts must | ||
# overwrite these three options of itself in opts - reset it first | ||
if opt in ["enable_plugins", "skip_plugins", "only_plugins"]: | ||
for oopt in ["enable_plugins", "skip_plugins", "only_plugins"]: |
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.
Do we actually need the loop here? We have the previous conditional for checking that we're operating on one of those keys, so I don't think we need to loop through each of those keys each time we meet the conditional?
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.
We need to evaluate pairs of options+values, if present. Like "-e logs
in cmdopts AND -n logs
in preset". So for each cmdopts
that alters plugin enablement, we must check if it is not in a conflict of a different option for the same plugin.
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.
Ah, ok - that tracks. Thanks for clearing that up for me.
When preset disables a plugin that user wants to manually enable, current merging of options ends up with -e PLUG -n PLUG which makes the plugin still disabled.
apply_options_from_cmdline method must remove the preset options for any plugin that interfere with cmdline options for the plugin.
Resolves: #3195
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines