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

Powershell args #5

Merged
merged 4 commits into from
Jul 11, 2013
Merged

Conversation

ferventcoder
Copy link
Contributor

Adding powershell startup args and surrounding the powershell command in quotes.

Also fixing the specs after the change to add finding powershell.exe from several lookup locations.

Some of this was referenced in #1

@rismoney
Copy link

rismoney commented Jul 2, 2013

I think you need the call operator if the command has spaces, and you are already prepping that scenario with the escaped quotes.

end

PS_ARGS = '-NoProfile -NonInteractive -NoLogo -ExecutionPolicy Bypass'

Copy link

Choose a reason for hiding this comment

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

I feel like when i looked at this a number of months ago you should add -inputformat none here. I can't remember the exact reasoning but I felt like stdin redirection caused hangs or some other bizarre scenario. i worked around it, but figured i'd mention it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like:
http://connect.microsoft.com/PowerShell/feedback/details/572313/powershell-exe-can-hang-if-stdin-is-redirected
http://stackoverflow.com/questions/4238192/running-powershell-from-msdeploy-runcommand-does-not-exit/4239192#4239192

Another option would be to write the powershell script to a tempfile (securely) and then use -file <path>. That might be a good idea anyways, in case the command is long, e.g. from a template, and exceeds the maximum CreateProcess length. Thoughts?

@rismoney
Copy link

rismoney commented Jul 2, 2013

One more idea - obviously needs to be quoted as you did above---

powershell -command " $ErrorActionPreference = "stop"; #{command} "

@ferventcoder
Copy link
Contributor Author

@rismoney I'm not sure I would add more to the command than just leaving it open for folks to pass in whatever they want without specifying error action preference. It looks to be a helper but it could be construed as a little magical since it is doing more than just the command I've passed it. And if I was to take my command in troubleshooting and run it in the PowerShell ISE it would run differently than it would with puppet apply.

Giving someone the command to pass as they want, the world is open for them to pass that error action preference into the command themselves. And if they have a different preference, they can just pass that.

Thoughts?

@rismoney
Copy link

rismoney commented Jul 2, 2013

agree with your sentiments. It is presumptive to inject :), especially if someone wants alternate behavior.
puppet is typically promotes fail early / fail everything. perhaps it should be a recommended usage via a readme tidbit.

@ferventcoder
Copy link
Contributor Author

Perhaps we can set a variable and then offer it automatically. :fail_on_first_error ?

@joshcooper
Copy link
Contributor

Allowing the error behavior to be modified would require adding a parameter to the core exec type, which is undesirable, or creating a new powershell type. Unfortunately, subclassing types doesn't work, so in the later case, you'd end up duplicating a lot of exec functionality, e.g. onlyif/unless.

With that said, would someone really want puppet to continue on error? That seems like a strange thing coming from bash perspective. But maybe that's a common thing in powershell?

This might be a good thing to bring up on the puppet-dev list.

@joshcooper joshcooper merged commit 3193385 into puppetlabs:master Jul 11, 2013
This was referenced Jul 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants