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
Merged

Bump panicwrap's version to fix issue 2093 #23281

merged 1 commit into from
Nov 6, 2019

Conversation

DaemonSnake
Copy link
Contributor

@DaemonSnake 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
Copy link

hashicorp-cla commented Nov 4, 2019

CLA assistant check
All committers have signed the CLA.

@apparentlymart
Copy link
Contributor

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.

@apparentlymart apparentlymart self-assigned this Nov 5, 2019
@apparentlymart apparentlymart added the dependencies Auto-pinning label Nov 5, 2019
@DaemonSnake
Copy link
Contributor Author

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

@apparentlymart
Copy link
Contributor

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
@DaemonSnake
Copy link
Contributor Author

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 🙂

@ghost
Copy link

ghost commented Mar 29, 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 Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Auto-pinning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform commands prepend "o:" and "e:" to lines when run recursively inside local-exec
3 participants