-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Powershell Provisioner Error Handling #13334
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 Provisioner Error Handling #13334
Conversation
anshulsharma-hashicorp
left a comment
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.
LGTM
Also please test the case where ExecutionPolicy is set to None.
|
Overall the changes look good to me. But we can test this in few more scenarios or probably add test cases.
|
To clarify, PowerShell errors are generally categorized into two types: terminating and non-terminating errors. The wrapper introduced here is designed to capture terminating errors and return their corresponding exit codes. A terminating error includes:
Regarding the second test case, There was some discussion (#6449) about whether Packer should enforce ErrorActionPreference='Stop' by default. However, since this behavior differs from PowerShell's default setting, we have opted to leave it up to the user to configure based on their needs. Additionally, I have incorporated test cases for the nested try/catch scenario. |
Added the test case |
JenGoldstrich
left a comment
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 looks good to me, I've tested it with some basic templates, thank you for your work on this one @kp2099!
Let's plan to release this in the next minor release since it is a slight breaking change
| // File contents should contain 2 lines concatenated by newlines: foo\nbar | ||
| readFile, err := os.ReadFile(file) | ||
| expectedContents := "foo\nbar\n" | ||
| expectedContents := "if (Test-Path variable:global:ProgressPreference) {\n set-variable -name variable:global:ProgressPreference -value 'SilentlyContinue'\n }\n \n $exitCode = 0\n try {\n $env:PACKER_BUILDER_TYPE=\"\"; $env:PACKER_BUILD_NAME=\"\"; \n foobar\n $exitCode = 0\n } catch {\n Write-Error \"An error occurred: $_\"\n $exitCode = 1\n }\n \n if ($LASTEXITCODE -ne $null -and $LASTEXITCODE -ne 0) {\n $exitCode = $LASTEXITCODE\n }\n \n Write-Host $result\n exit $exitCode" |
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 is just a nit pick, but I would break this up into several lines of text joined together, just to make this expected contents a bit more readable
|
@kp2099 Thank very much you for this fix! |
|
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. |
This change is to add a wrapper around inline powershell commands that are executed in wrapper to catch and fail the packer build if there is any error when executing the commands
Added Acc Test , Fixed Unit tests
Closes: #4916, #11198, #9161