-
-
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
migrate to AWS SDKv2, updating only singnatures and making sure tests are passing #1451
base: master
Are you sure you want to change the base?
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
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.
@@ -76,10 +78,9 @@ func TestPackerBasicExample(t *testing.T) { | |||
assert.Contains(t, accountsWithLaunchPermissions, requestingAccount) | |||
|
|||
// 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.
@@ -37,13 +36,6 @@ func TestTerraformPackerExample(t *testing.T) { | |||
undeployUsingTerraform(t, workingDir) | |||
}) | |||
|
|||
// At the end of the test, fetch the most recent syslog entries from each Instance. This can be useful for |
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? 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.
You are right! I went too far with the refactoring and removed part of this logic. The change has been reverted.
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
Description
This PR includes changes to upgrade the outdated
AWS SDK
to the newerSDKv2
version.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.