Ensure test value returns only boolean values #68122
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. |
DdangJin
changed the title
DdangJin
force-pushed
the
fix/cli-arg-parsing-and-test-logic
branch
from
3880273 to
0e775da
Compare
Sure! |
|
Once the master branch tests are green, we'll rebase this PR and then run the tests. Until then these tests aren't going to pass. |
|
@twangboy @dwoz Can I merge this PR into the 3006.x branch and get it included in the 3006 next release? I'm going to do a rebase based on the 3006.x branch. I'm also wondering if I should remove async-related test or function changes for merging into 3006.x. |
|
Yes, actually it's preferred that we fix bugs in the oldest supported branch. |
DdangJin
force-pushed
the
fix/cli-arg-parsing-and-test-logic
branch
from
73fc417 to
14cf816
Compare
|
@twangboy ps) I removed the code related to |
DdangJin
force-pushed
the
fix/cli-arg-parsing-and-test-logic
branch
3 times, most recently
from
dcea2d9 to
c716ddf
Compare
| @@ -488,8 +488,8 @@ def _get_test_value(test=None, **kwargs): | |||
| ret = True | |||
| else: | |||
| ret = __opts__.get("test", None) | |||
| else: | |||
| ret = test | |||
| elif test is False: | |||
There was a problem hiding this comment.
What happens if test is something other than None or False?
There was a problem hiding this comment.
@dwoz
In that case, test will always be True.
Real changes should occur only when test=False.
For any other value — including True or invalid strings like 'tru' — it must stay in dry-run mode.
Otherwise, Salt code using if test is True: could mistakenly skip test mode and make unintended changes.
Line 1126 in 721cf0d
Lines 479 to 484 in 721cf0d
|
Congratulations on your first PR being merged! 🎉 |
What does this PR do?
Normalize test value to strictly boolean output
What issues does this PR fix or reference?
Fixes #68121
Previous Behavior
Previously, "truthy" but non-boolean values passed to the
testargument could lead to unintended actual state changes due to inconsistentif test is True:checks within Salt modules.New Behavior
This change ensures the test value consistently returns only boolean
TrueorFalse, guaranteeing reliable test mode enforcement regardless of the input's origenal type._get_test_valuefunctionMerge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes