-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
feat: support pwsh in powershell provisioner #11841
Conversation
* set `pwsh` to true in config to run `pwsh.exe`, default is false. * add docs
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.
Just a nit regarding a condition in the test, outside of that LGTM!
Thanks for the contribution, and sorry for the late review
t.Fatalf("Should not be error: %s", err) | ||
} | ||
|
||
if p.config.Pwsh != true { |
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.
if p.config.Pwsh != true { | |
if !p.config.Pwsh { |
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.
Coming back on this PR, after a re-reading of the code, we think it'd be beneficial to address a few remaining comments before we can merge this, any chance you can address those @edeustace?
if(p.config.Pwsh){ | ||
return fmt.Sprintf(`pwsh -executionpolicy %s -command "%s"`, p.config.ExecutionPolicy, baseCmd) | ||
} else { | ||
return fmt.Sprintf(`powershell -executionpolicy %s "%s"`, p.config.ExecutionPolicy, baseCmd) |
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.
Re-reading through this PR, I notice the indentation is off here, could you replace the two-spaces by a tab? That'd help it stay consistent with the rest of the code and the Go guidelines.
@@ -89,6 +89,9 @@ type Config struct { | |||
// A duration of how long to pause after the provisioner | |||
PauseAfter time.Duration `mapstructure:"pause_after"` | |||
|
|||
// Run pwsh.exe instead of powershell.exe - latest version of powershell. | |||
Pwsh bool `mapstructure:"pwsh"` |
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.
As noted by @JenGoldstrich in a out-of-band discussion, UsePwsh
would be a clearer name for the attribute, could you amend this?
HI @edeustace thanks for the contribution. I'm going to close this PR as your changes, along with commit history, have been rolled into #11950. The team requested a few changes a bit late so we thought it best to make the changes along with yours in a separate PR. Thanks again for making the fix. We look forward to the continued contributions and feedback. Cheers! |
Sorry - I was on vacation - thanks! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
pwsh
to true in config to runpwsh.exe
, default is false.Closes #7164