Skip to content

provisioner/powershell: Update default execute command to handle script errors#9040

Merged
SwampDragons merged 4 commits intomasterfrom
powershell-exit-code-fix-4916
May 21, 2020
Merged

provisioner/powershell: Update default execute command to handle script errors#9040
SwampDragons merged 4 commits intomasterfrom
powershell-exit-code-fix-4916

Conversation

@nywilken
Copy link
Copy Markdown
Contributor

@nywilken nywilken commented Apr 10, 2020

This change sets the ErrorActionPreference and wraps the script execution in a Try/Catch statement so that the provisioner can capture any errors encountered when running the script. In addition to the try/catch the & operator is replaced by the . sourcing operator to ensure the script is executed in the same scope as the parent command (so that errors bubble up properly).

Tests after change

⇶  ACC_TEST_BUILDERS=amazon-ebs ACC_TEST_PROVISIONERS=powershell go test
./provisioner/powershell/... -timeout=1h ok
github.com/hashicorp/packer/provisioner/powershell      915.865s

Todo

  • Test against Elevated command execution
  • Rename acceptance test fixtures
  • Validate issue is resolved for reporters on 4916
  • Validate that LastExitCode is set for scripts with native Windows application failures

Closes #4916
Closes #9161

@nywilken nywilken requested a review from a team as a code owner April 10, 2020 11:19
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2020

Codecov Report

Merging #9040 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
provisioner/powershell/provisioner.go 75.21% <100.00%> (+0.07%) ⬆️
packer/communicator.go 76.59% <0.00%> (+1.06%) ⬆️
builder/azure/dtl/tempname.go 75.00% <0.00%> (+6.25%) ⬆️

@SwampDragons
Copy link
Copy Markdown
Contributor

This probably needs to go into 1.6 since it's going to change behavior.

@SwampDragons
Copy link
Copy Markdown
Contributor

I tested this and it looks good. Not merging yet since this is targeted for v1.6

@azr azr added this to the 1.6.0 milestone Apr 15, 2020
@nywilken nywilken force-pushed the powershell-exit-code-fix-4916 branch from 3cdfb89 to e396f4e Compare April 20, 2020 20:57
@nywilken nywilken force-pushed the powershell-exit-code-fix-4916 branch 3 times, most recently from aee26ca to 92d8ba0 Compare May 20, 2020 19:38
@nywilken nywilken changed the title [WIP] provisioner/powershell: Update default execute command to handle script errors provisioner/powershell: Update default execute command to handle script errors May 20, 2020
Copy link
Copy Markdown
Contributor

@SwampDragons SwampDragons left a comment

Choose a reason for hiding this comment

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

one small question but otherwise this looks great :)

}

baseCmd += `. {{.Vars}}; &'{{.Path}}'; exit $LastExitCode }`
baseCmd = fmt.Sprintf(baseCmd, debugLine)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this doesn't look like a valid fmt.Sprintf string to me.

https://play.golang.org/p/tvHz_A_xWqL

Copy link
Copy Markdown
Contributor Author

@nywilken nywilken May 21, 2020

Choose a reason for hiding this comment

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

In my PR I added a hard to see string interpolation verb at the beginning of the line %s. {{.Vars}}; ... }. But that is confusing for sure so I reformatted it back to how it was.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh yeah I totally missed that, sorry.

nywilken added 4 commits May 21, 2020 09:05
…pt errors

This change sets the ErrorActionPreference and wraps the script execution in a Try/Catch statement so that the provisioner can capture any errors encountered when running the script. In addition to the try/catch the `&` operator is replaced by the `.` sourcing operator to ensure the script is executed in the same scope as the parent command (so that errors bubble up properly).

Tests after change
```
⇶  ACC_TEST_BUILDERS=amazon-ebs ACC_TEST_PROVISIONERS=powershell go test ./provisioner/powershell/... -timeout=1h
ok      github.com/hashicorp/packer/provisioner/powershell      915.865s
```
* Add broken requires statement test case
* Add test case to reproduce invalid LastExitCode
* Ensure child scope doesn't conflict with parent scope
* Add elevated user options to tests case.
@nywilken nywilken force-pushed the powershell-exit-code-fix-4916 branch from e188ce9 to 2e326ef Compare May 21, 2020 13:08
@SwampDragons SwampDragons merged commit 334f399 into master May 21, 2020
@SwampDragons SwampDragons deleted the powershell-exit-code-fix-4916 branch May 21, 2020 16:55
nywilken added a commit that referenced this pull request Jun 9, 2020
…fix-4916"

This reverts commit 334f399, reversing
changes made to 45a5d28.

When testing against Windows SSH the Powershell script fails to parse
the newly added if statement.
SwampDragons added a commit that referenced this pull request Jun 9, 2020
…er-9040

Revert "Merge pull request #9040 from hashicorp/powershell-exit-code-fix-4916"
@nywilken nywilken added the tech-debt Issues and pull requests related to addressing technical debt or improving the codebase label Jun 9, 2020
@ghost
Copy link
Copy Markdown

ghost commented Jun 21, 2020

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jun 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

provisioner/powershell tech-debt Issues and pull requests related to addressing technical debt or improving the codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Powershell provisioner and #requires directive PowerShell provisioner silently ignores some types of errors

3 participants