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

Bump panicwrap's version to fix issue 2093 #23281

Merged
merged 1 commit into from Nov 6, 2019

Conversation

@DaemonSnake
Copy link
Contributor

DaemonSnake commented Nov 4, 2019

panicwrap now unsets the cookie env variable it uses in Wrapped()
to check if we are in a panic wrapped context.
This correct a false positive when teraform execute itself recurcively.

Fixes #20293.

@hashicorp-cla

This comment has been minimized.

Copy link

hashicorp-cla commented Nov 4, 2019

CLA assistant check
All committers have signed the CLA.

@apparentlymart

This comment has been minimized.

Copy link
Member

apparentlymart commented Nov 5, 2019

Thanks for working on the panicwrap update and on this upgrade change, @DaemonSnake!

Since Terraform uses vendoring, there is one additional step to do here before we can merge this: run go mod vendor to sync the updated version of panicwrap into the vendor directory and include all the resulting changes in the vendor directory in your commit.

There's some additional context on this in our contribution guide section External Dependencies, in case that's helpful.

Thanks again, and please let me know if I can help.

@DaemonSnake

This comment has been minimized.

Copy link
Contributor Author

DaemonSnake commented Nov 5, 2019

Thanks for your kind words.
I've rewritten the commit following the contribution guide

@apparentlymart

This comment has been minimized.

Copy link
Member

apparentlymart commented Nov 6, 2019

Hi @DaemonSnake!

The result of your running go mod vendor included more changes than I was expecting (removal of several files), so I ran go mod vendor myself locally and indeed it restored the files that had been unexpectedly removed. I've pushed up the updated version of it here to merge once the tests pass.

However, I'd also like to understand why you and I are getting different results. One theory I have is that perhaps you are using Go 1.13 and the behavior of go mod vendor has changed in that version. We're currently still developing Terraform using Go 1.12 because 1.13 includes removal of support for some target platforms and so we cannot upgrade until we're ready to phase out support for those.

Can you confirm whether you're using Go 1.13? If that is the explanation, then we'll need to keep that in mind when reviewing future contributions that make changes to vendor to make sure that they don't include vendor changes that would then be undone again by running go mod vendor in Go 1.12, until we're ready to upgrade. That's likely to be a new pressure for moving forward with the upgrade to Go 1.13 if so, because we know that new contributors are likely to be using the latest version of the Go toolchain.

@apparentlymart apparentlymart merged commit e702267 into hashicorp:master Nov 6, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@DaemonSnake

This comment has been minimized.

Copy link
Contributor Author

DaemonSnake commented Nov 6, 2019

Hi,
yes I am indeed using go 1.13.
I was also wondering why these commands produced that much diffs.
Thanks for the information.
Maybe adding a pre commit hook could help inform contributors for the time being and tranfer the stress from the reviewer to the contributor.
Anyway thanks for the feedback it was valuable 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.