Skip to content
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

Pass cmd.run arguments to onlyif/unless cmd execution #57825

Merged
merged 6 commits into from Sep 8, 2020

Conversation

mchugh19
Copy link
Contributor

What does this PR do?

Extends the global onlyif/unless cmd handling to accept arguments defined in the cmd.run state.

What issues does this PR fix or reference?

Fixes: #57760

Previous Behavior

Previous to #55974, the cmd.run state handled its own unless and onlyif support. This included passing any of the cmd.run arguments for the state into the onlyif and unless cmd as well (https://github.com/saltstack/salt/pull/55974/files#diff-6617770c26f25c18a6adeeaf4ccdebd3L336)

When this behavior was removed in favor of using the global onlyif/unless requisites this behavior was not included, so arguments like shell: /bin/bash which were defined for cmd.run were not honored by the cmd.retcode execution used by onlyif/unless.

New Behavior

Arguments accepted by cmd.run (which make sense) are now passed to cmd.retcode used by onlyif and unless.

  • cwd
  • root
  • runas
  • env
  • prepend_path
  • umask
  • timeout
  • success_retcodes

Other possible arguments such as name, output_loglevel, or creates, don't make sense for the unless/onlyif cmd.retcode use, while the arguments bg and use_vt would break functionality.

Support of shell is a special case, and will be pulled first from the state arguments, then from the value of the shell grain if either exist.

Risks: it is possible that generic terms like cwd, env, or root, may exist in other states with different meanings than is intended for the cmd.retcode use in onlyif/unless. If desired, a gate could be added to optionally ignore inclusion of arguments to cmd.retcode, but has not yet been included as I don't know if the risk is worth the additional option complexity.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No

cmcmarrow
cmcmarrow previously approved these changes Jul 15, 2020
@jdelic
Copy link
Contributor

jdelic commented Aug 10, 2020

From reading this patch it brings back env for onlyif. Removing the setting of environment variables for cmd.run requisites has broken my whole setup across multiple machines and required a lot of manual fixes.

Can this land as a (in my case urgent) bugfix for 3001 in the short term?

garethgreenaway
garethgreenaway previously approved these changes Aug 24, 2020
@krionbsd
Copy link
Contributor

@mchugh19 LGTM, could you please run pre-commit run and re-submit the patch?

@Silvenga
Copy link

I would also agree with @jdelic - this breaks a lot for us. PowerShell is a lot different from cmd.

Can we get this backported to 3001?

@mchugh19
Copy link
Contributor Author

@krionbsd @garethgreenaway since pre-commit ran the py2 cleanup process, and this touches state.py, it'd be nice to get lots of eyes on this.

dwoz
dwoz approved these changes Sep 8, 2020
Ch3LL
Ch3LL approved these changes Sep 8, 2020
dwoz
dwoz approved these changes Sep 8, 2020
@dwoz dwoz merged commit 452f190 into saltstack:master Sep 8, 2020
28 checks passed
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-failing-test Magnesium Mg release after Na prior to Al
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] random "bad signature" error on minions after upgrade to 3001
9 participants