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

Support powershell core #59240

Merged
merged 38 commits into from Mar 1, 2021
Merged

Support powershell core #59240

merged 38 commits into from Mar 1, 2021

Conversation

twangboy
Copy link
Contributor

@twangboy twangboy commented Jan 6, 2021

What does this PR do?

Adds support for powershell core. The binary for powershell core is pwsh. Added tests.
Fixes running encoded powershell commands. This broke in Python 3, but we didn't know it. Added tests.
Moves a couple integration tests to pytest/functional

Powershell core is added to the golden image in this PR: saltstack/salt-ci-images#1688

When present, tests will run for pwsh binaries

What issues does this PR fix or reference?

Fixes: #58598

Merge requirements satisfied?

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

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@twangboy twangboy requested a review from a team as a code owner January 6, 2021 21:33
@twangboy twangboy requested review from xeacott and removed request for a team January 6, 2021 21:33
@twangboy twangboy added the Aluminium Release Post Mg and Pre Si label Jan 6, 2021
Copy link
Member

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

My only real concern are f-strings but, if Debian 9 and Ubuntu 16.04 don't choke, I'm cool with it.

salt/modules/cmdmod.py Outdated Show resolved Hide resolved
salt/modules/cmdmod.py Outdated Show resolved Hide resolved
tests/pytests/functional/modules/test_cmdmod.py Outdated Show resolved Hide resolved
tests/pytests/functional/modules/test_cmdmod.py Outdated Show resolved Hide resolved
tests/pytests/functional/modules/test_cmdmod.py Outdated Show resolved Hide resolved
tests/pytests/functional/modules/test_cmdmod.py Outdated Show resolved Hide resolved
@s0undt3ch
Copy link
Member

Yep. Debian 9 choked. We can't, yet, use f-strings

@max-arnold
Copy link
Contributor

Does it fix these issues?

#31351 (comment)
#31351 (comment)

@twangboy
Copy link
Contributor Author

twangboy commented Jan 7, 2021

@max-arnold Yeah, I think so... I'll add some tests around your example

@twangboy
Copy link
Contributor Author

twangboy commented Jan 7, 2021

@max-arnold Your first comment using cmd.powershell with encode_cmd=True works. I fixed the issue with the 2nd comment in the cmd.powershell function by prepending $ProgressPreference='SilentlyContinue' to the command before it is encoded.

With cmd.run and cmd.run_all however, that is impossible as the command is being passed to the function already encoded. So, I added some documentation about the need to prepend $ProgressPreference='SilentlyContinue' to the command before you encode it if you want to remove the raw xml data in the return.

As a note, this seems to only affect older versions of powershell. I don't get the raw xml data with Powershell Core, so maybe they've addressed that issue there.

xeacott
xeacott previously approved these changes Jan 7, 2021
@twangboy twangboy changed the title Support powershell 7 Support powershell core Jan 11, 2021
@Ch3LL Ch3LL added the merge-conflict PR has a merge conflict label Feb 1, 2021
@twangboy twangboy removed the merge-conflict PR has a merge conflict label Feb 8, 2021
@twangboy twangboy force-pushed the support_powershell_7 branch 2 times, most recently from f275888 to c47c3b1 Compare February 8, 2021 18:59
@sagetherage sagetherage added the ZD The issue is related to a Zendesk customer support ticket. label Feb 11, 2021
tests/pytests/functional/modules/test_cmdmod.py Outdated Show resolved Hide resolved
tests/integration/modules/test_cmdmod.py Outdated Show resolved Hide resolved
@twangboy twangboy force-pushed the support_powershell_7 branch 2 times, most recently from 7c8ba59 to f209edc Compare February 19, 2021 18:51
@twangboy
Copy link
Contributor Author

Not sure why Fedora 33 is failing... Everything else is passing

@twangboy twangboy dismissed stale reviews from s0undt3ch, xeacott, and cmcmarrow via 28f128b February 26, 2021 04:46
@twangboy twangboy removed the merge-conflict PR has a merge conflict label Feb 26, 2021
@Ch3LL Ch3LL merged commit e24a1b3 into saltstack:master Mar 1, 2021
@twangboy twangboy deleted the support_powershell_7 branch March 23, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si Tests-Passed ZD The issue is related to a Zendesk customer support ticket.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] unable to use the shell command to switch to powershell 7 which is not the default powershell.
9 participants