-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: handle updated packer version output from 1.10 #1380
fix: handle updated packer version output from 1.10 #1380
Conversation
I think will be helpful to update packer version in integration tests https://github.com/gruntwork-io/terratest/blob/master/.circleci/config.yml#L7 |
Hm, noticed failing tests:
|
I'm not able to see the build in Circle CI, is there a way to get access? Curious if the packer init ran or not. Also, is there information on running the integration tests locally? Guessing it must be doing something different to the |
Local run of |
Apologies I was talking about the |
Hm,
|
This feels unrelated to the PR and more to do with the AWS account where the test is being run. Does this test run frequently? Even on master it is set to always make the AMI public: https://github.com/gruntwork-io/terratest/blob/master/test/packer_basic_example_test.go#L79 / https://github.com/gruntwork-io/terratest/blob/master/test/packer_basic_example_test.go#L215-L230 So if it is running regularly against an account that has the block public access feature enabled it should be failing constantly. Happy to adjust the test if needed but looking for some guidance on the intent here 🙂 |
This test is executed on each PR, release, and on the master branch, intention is to validate that example is working correctly, since on master it is not failing - I suspect that it is related to this PR. |
When you say the tests are running on master without failing, does that mean this test is actually being executed or is the test being skipped if the most recent commit did not modify the template? My confusion is that the error is originating from an AWS account setting so it has nothing to do with the code being run and everything to do with the configuration of the account. I can modify the tests as part of this e.g. do not create a public AMI or call the API to disable the account level setting before the run but both of these feel like fairly substantial changes for a PR that is essentially doing some string handling so want to be clear on your desired intent. What is it that you would like me to do here? |
@batinicaz can you rebase the master and see whether it passes the test? |
ab01246
to
d3af93b
Compare
Rebased vs master |
Any chance to get this merged? The issue is preventing me from upgrading Packer. |
Happy to make changes here to get it working, but this was blocked from merging due to a failing test. The test was failing due to an AWS account setting. Given that's a Gruntwork owned account, modifying account wide settings from the code does not seem appropriate. To work around that, I could update the code to prevent setting the AMI to be public, but that felt like a sizeable change for a fix that is just string handling. So before doing that I wanted to get confirmation of the desired change, but @denis256 has yet to confirm their preference |
Currently only works with Packer 1.9 or older. Packer 1.10 broke something with Terratest and it needs this PR merged: gruntwork-io/terratest#1380
Description
Fixes #1379.
Minimal fix to handle the version output Packer introduced in hashicorp/packer#12569.
Added a couple of simple unit tests to validate support for both the old version output and the new version output.
Also ran the hello-world test for v1.10:
And for
1.9.4
:TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
fix: handle updated version output from Packer 1.10