Skip to content

Conversation

@kp2099
Copy link
Collaborator

@kp2099 kp2099 commented Mar 17, 2025

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

@kp2099 kp2099 requested a review from a team as a code owner March 17, 2025 03:56
@kp2099 kp2099 changed the title [WIP] Powershell Provisioner Exit Code Handling [WIP] Powershell Provisioner Error Handling Mar 18, 2025
@kp2099 kp2099 changed the title [WIP] Powershell Provisioner Error Handling Powershell Provisioner Error Handling Mar 19, 2025
Copy link
Contributor

@anshulsharma-hashicorp anshulsharma-hashicorp left a 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.

@anurag5sh
Copy link
Collaborator

Overall the changes look good to me. But we can test this in few more scenarios or probably add test cases.
Some of which I can think of:

  1. Any form of syntax errors in the powershell script
  2. Certain powershell commands like Get-Item "C:\nonexistent.txt" . External programs will error out, but we can test if the powershell cmdlet also behaves the same
  3. A nested try/catch scenario

@kp2099
Copy link
Collaborator Author

kp2099 commented Mar 28, 2025

Overall the changes look good to me. But we can test this in few more scenarios or probably add test cases. Some of which I can think of:

  1. Any form of syntax errors in the powershell script
  2. Certain powershell commands like Get-Item "C:\nonexistent.txt" . External programs will error out, but we can test if the powershell cmdlet also behaves the same
  3. A nested try/catch scenario

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:

  1. Any invalid syntax.
  2. CommandNotFoundException (occurs when attempting to run a non-existent command).
  3. Runtime exceptions (e.g., division by zero).
  4. Explicitly thrown exceptions.

Regarding the second test case, Get-Item "C:\nonexistent.txt" produces a non-terminating error by default. This means the script would continue execution without failing and return an exit status of zero. However, if users want to explicitly handle non-terminating errors and return a non-zero exit code, they can do so using the ErrorActionPreference variable. Added it in the acceptance test for reference.

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.

@kp2099
Copy link
Collaborator Author

kp2099 commented Mar 28, 2025

LGTM Also please test the case where ExecutionPolicy is set to None.

Added the test case

Copy link
Collaborator

@JenGoldstrich JenGoldstrich left a 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"
Copy link
Collaborator

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 kp2099 merged commit 089df02 into hashicorp:main Apr 25, 2025
10 of 11 checks passed
@mloskot
Copy link

mloskot commented Apr 25, 2025

@kp2099 Thank very much you for this fix!

@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PowerShell provisioner silently ignores some types of errors

5 participants