-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
migrate to AWS SDKv2, updating only singnatures and making sure tests are passing #1451
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
Conversation
6646d68 to
83f34d2
Compare
72d9de8 to
22a3870
Compare
|
Hi @wakeful, thanks for making this contribution. It seems like some of the relevant tests are failing. Can you please run them and make sure they don't fail? |
22a3870 to
e2ce5d3
Compare
|
hey @james03160927 I updated tests, rebase with latest master & bump all aws SDK v2 pkg - please try again if you get any errors 🙏 share the output log. |
|
this is awesome @wakeful - but this should probably come with a major bump, for example for s3 fileupload I had to replace s3manager... and I can't remember other situations where the change in terratest bled through into my code (which could be more a problem with my code)... Until this gets merged, I'm using your fork of terratest I'd like to see this merged (and I'd like to contribute some more utility functions such as testing Cloudfront Functions, Sfn State Machines, ...) ref all the utilities I wrote here but I'm not sure if there's an interest for this by gruntworks (not sure if there's a slack or discord where this can be discussed?) |
|
thanks for testing @so0k! I also have few more ideas / new helpers but first we need to merge this one.
recently there was a blog post about A stronger Terragrunt community with official Discord server - I'm hoping at some point other tools will also get a dedicated channel there. |
|
Seeing this error on packer unit test |
e2ce5d3 to
cc1368f
Compare
|
hey @james03160927.
Based on the error above, it seems that in your AWS account (or AWS organisation) there is a policy blocking the sharing of the AMI with everyone. I’ve decided to modify the test to check if the AMI is private. This way, we won’t require any changes in your testing environment. The fix has been pushed, and the branch has been rebased with the latest master. |
cc1368f to
6e6ba19
Compare
|
Can you rebase your change with the latest commit? It requires bigger circleCi resource to run the tests which I submitted here. #1472 |
6e6ba19 to
468b41b
Compare
|
@james03160927 done 👍 |
047b0ac to
826eccc
Compare
|
Still seeing failures in the following tests:
|
826eccc to
dbfbabd
Compare
|
I resolved the conflicts and will work on the failing tests later today. |
dbfbabd to
63c1d7a
Compare
|
The issue in TestTerraformPackerExample and TestPackerDockerExampleLocal was related to a missing HTTP server. It seems that the Sinatra gem no longer installs an HTTP server by default, or it may require extra configuration. I added puma and configured Sinatra to use it. Both tests are now passing for me. The TestTerraformAwsRdsExample/mysql issue seems more like a Terraform cache problem. I reran it locally, and it passed without any issues |
|
@james03160927 Can you rerun the tests again? After my investigation, they are not related to my changes. They also failed for me on the master branch, so I fixed them. |
|
HI @wakeful, would it be possible to create a separate PR for fixing the already failing tests? I still get the same error for |
|
hey @james03160927
This seems to be an issue with Terraform. I'll try to reproduce it. Could you share the Terraform version used in your CI? Unfortunately, the log output is not accessible to external contributors.
Are you asking me to fix all tests on the |
63c1d7a to
b1b65d8
Compare
james00012
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Just a few comments on the test changes.
|
|
||
| // website::tag::3::Check AMI's properties. | ||
| // Check if AMI is public | ||
| MakeAmiPublic(t, amiID, ec2Client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we deleting this line of code here and elsewhere? Does migrating to AWS SDK v2 require this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as mention in this comment #1451 (comment) To prevent this test from failing in your AWS account, I decided to remove the part that makes it public.
I think this is a transient error. Please ignore for now. We are using TERRAFORM_VERSION: 1.5.7 right now for CircleCi.
I was suggesting to make this solely focus on migrating to AWS SDK v2 changes. Your fix for HTTP server could be in a different PR. |
@james03160927 Should I create a new PR with just the test fix? I added the HTTP fix here mainly because the tests were failing, and I wanted to ensure my branch was "green." |
b1b65d8 to
f5425c1
Compare
|
I think it's all good now. Thanks for all the back and forth and addressing even the test failure issues. Let's merge the code now. |
james00012
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM

Description
This PR includes changes to upgrade the outdated
AWS SDKto the newerSDKv2version.AWS SDKv1.SDKv2.What's missing:- Finalise portingmodules/aws/auth.go(support for MFA and assume role). I complete this over the next few days.Fixes #1432.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
migrated to AWS SDK v2.