Skip to content

Conversation

@xsmile
Copy link
Contributor

@xsmile xsmile commented Sep 1, 2025

What does this PR do?

Change the behavior of the cmd state module.

What issues does this PR fix or reference?

#68156
#68298

Previous Behavior

  • shell functionality was enforced
  • no clear separation between the program to be executed and the arguments in cmd.run
  • integration tests were skipped on Windows
  • broken argument processing

New Behavior

  • shell functionality can be changed via the python_shell parameter and is disabled by default for cmd.script (handled in the execution module)
  • cmd.run supports the args parameter to avoid quoting and escaping issues when running programs
  • additional integration tests
  • fix argument processing

Merge requirements satisfied?

Commits signed with GPG?

No

@xsmile xsmile requested a review from a team as a code owner September 1, 2025 19:48
@xsmile
Copy link
Contributor Author

xsmile commented Sep 10, 2025

Reverted the change to disable shell functionality by default when running commands via the state module. It can be disabled manually if required.

@dwoz dwoz added the test:full Run the full test suite label Sep 10, 2025
args=["foo bar", "baz qux"],
)
self.assertSaltTrueReturn(ret)
self.assertSaltStateChangesEqual(ret, expected, "stdout")
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to either migrate this file to pytests or create these tests in pytest.

@@ -0,0 +1,2 @@
Expose paramter python_shell to allow disabling shell functionality for the cmd state module.
Copy link
Contributor

Choose a reason for hiding this comment

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

parameter python_shell

@jpmsilva
Copy link
Contributor

Just adding some information that may be relevant to the scope of these changes.

Currently, according to the documentation for the cmd.script state, there are multiple combinations of source, name and args (list or space separated string) that may affect how the command to be executed is constructed.

However, the stateful argument can complicate things further in that it accepts an optional key test_name (as a parallel to the name key) that overrides the command to be run when in test mode.
The construction of the test command lacks a similar args key (test_args?), which makes it a bit more difficult to reason with.
So it may be worth considering extending the args support to the stateful mechanism (at least in the cmd.script case, where args are supported).

One might even see a rationale for a parallel to the source key (test_source?) if a different script source were to be used when in test mode, but this might be a step too far.

@dwoz
Copy link
Contributor

dwoz commented Oct 21, 2025

@xsmile @twangboy
If this is changing any existing functionality it needs to go into master instead of 3006.x

@bdrx312
Copy link
Contributor

bdrx312 commented Oct 31, 2025

@xsmile @twangboy If this is changing any existing functionality it needs to go into master instead of 3006.x

@dwoz Everything in this PR appears to be fixing the non-backwards compatible changes (and bug) from the previous PR which did add changes to existing functionality that should have gone to master. There were several issues written shortly after the previous as a result so I think it would be good to get fixes in before even more people run into it. @xsmile appears to have disappear though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants