-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix bug where cmd.powershell fails to return #36958
Conversation
We should make it explicit that the |
Oops, sorry, accidentally clicked to close and comment instead of just to comment. |
@terminalmage Added |
**kwargs): | ||
''' | ||
Execute the passed PowerShell command and return the output as a string. | ||
Execute the passed PowerShell command and return the output as a dictionary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look accurate. This is executing a cmd.run
, which most certainly does not return a dictionary. There is a difference between a dictionary, which is a data structure, and a string of JSON, whichis not. JSON looks like a Python dictionary, but it is not a dictionary. Please change this back to what it was originally, or at least clarify and say "return the output in JSON format".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disregard, I wrongly thought that we were returning the result of the run()
command.
.. versionadded:: 2016.3.4 | ||
|
||
:returns: | ||
:dict: A dictionary of data returned by the powershell command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, wrong. Not a dictionary. It would be accurate to say "A JSON-formatted string of data returned by the powershell command."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disregard, I wrongly thought that we were returning the result of the run()
command.
Could you make |
@damon-atkins that should not be necessary. It can be called on the Salt using |
It creates a support issue with the order of the parameters, as you can not control how user base will use it. If you expect to always call it with
use_vt is marked experimental, let say SaltStack decide its no good and should be removed. If most of them were kwargs, then it could be ignored, or you could provide a warning for it when its set. I have started an issue for it. |
I believe that the @thatch45 thoughts? I personally think that if you're going to be worried about argument order, then why not make everything with a default value a kwarg, everywhere in Salt? |
It is better this way, having it be an explicit argument like this makes it detectable by salt and the state system which makes it easier for our automated argument forwarding to better report on it. |
@terminalmage What else do I need to do on this one? |
@terminalmage The last thing the function does is run |
You're right, I misread and thought that the result of the |
What does this PR do?
Fixes a bug where some powershell commands would take hours to complete due to the extreme
depth
setting forConvertTo-JSON
. Reverted to the Windows default (2) and added the option to pass a different value.Added additional notes to the documentation explaining the purpose of the
cmd.powershell
function as the name is misleading. Made recommendations to usecmd.run
withshell=powershell
instead unless they need the data output to be a dictionary.What issues does this PR fix or reference?
#31509
Previous Behavior
Basic powershell commands such as
dir
take exponentially longer times to complete as the depth value forConvertTo-JSON
increases. Anything above 4 is painful. It was hard-coded to 32.New Behavior
Changed the depth value to use Windows defaults (2) with the option to pass a different value.
Tests written?
No