salt icon indicating copy to clipboard operation
salt copied to clipboard

[WIP] cmd state module fixes

Open xsmile opened this issue 5 months ago • 6 comments

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?

  • [x] Docs
  • [x] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
  • [x] Tests written/updated

Commits signed with GPG?

No

xsmile avatar Sep 01 '25 19:09 xsmile

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

xsmile avatar Sep 10 '25 14:09 xsmile

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.

jpmsilva avatar Sep 18 '25 21:09 jpmsilva

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

dwoz avatar Oct 21 '25 20:10 dwoz

@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.

bdrx312 avatar Oct 31 '25 00:10 bdrx312

What's the status of this PR? The issues introduced in 3007.7 still seem to exist in 3007.8.

papegaaij avatar Nov 06 '25 15:11 papegaaij

Is this still WIP?

twangboy avatar Dec 09 '25 20:12 twangboy

Would you please rebase?

twangboy avatar Dec 16 '25 21:12 twangboy