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

feat: add new LocalStack AWS session via env var #1211

Closed

Conversation

blairham
Copy link

@blairham blairham commented Dec 3, 2022

Description

Issues related

Pull Requests related

This adds a new create session that points to the URL specified in the env variable TERRATEST_LOCALSTACK. Since we don't need any credentials with LocalStack, we assume some random credentials and use the rest of Terratest. This was inspired by @ffernandezcast PR and the comments in it. This should be a much simpler approach and much more maintainable, which was one of the questions made by @brikis98

Usage

define the env var TERRATEST_LOCALSTACK

TERRATEST_LOCALSTACK=http://localhost:4566

TODOs

Read the Gruntwork contribution guidelines.

  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added support for custom AWS Endpoint.

@blairham blairham changed the title Creating a new LocalStack session if the env var TERRATEST_LOCALSTACK is Create a new LocalStack session env env var is set Dec 3, 2022
@blairham blairham changed the title Create a new LocalStack session env env var is set Create a new LocalStack session env var is set Dec 3, 2022
@blairham blairham changed the title Create a new LocalStack session env var is set feat: add new LocalStack AWS session via env var Dec 3, 2022
@blairham
Copy link
Author

blairham commented Dec 9, 2022

Hi, @zackproser or @denis256, or @brikis98. What does one need to do to get someone to take a look and approve or review the code and merge this? I took a look at the failing tests, and they are not related to these changes.

@@ -32,6 +33,21 @@ func NewAuthenticatedSession(region string) (*session.Session, error) {
func NewAuthenticatedSessionFromDefaultCredentials(region string) (*session.Session, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Could you please update the comment above this function to describe this behavior?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Etiene
Copy link
Contributor

Etiene commented Dec 14, 2022

I had a look at the tests, and confirm that the failures indeed dont seem related to this PR. However, there isn't a test for the NewAuthenticatedSessionFromDefaultCredentials, so it would be better if there was one.

@blairham
Copy link
Author

Yeah, I didn't see one either, let me take a crack at adding one.

awsSecretAccessKey = AWS_SECRET_ACCESS_KEY
}

awsConfig = awsConfig.WithEndpoint(localStackUrl).WithDisableSSL(true).WithCredentials(credentials.NewStaticCredentials(awsAccessKeyId, awsSecretAccessKey, ""))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, will be helpful to have a test case to keep track that handling of TERRATEST_LOCALSTACK will continue to work

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately, that would require a lot more than you think as there aren't any tests for this for good reason. I don't think there is a reasonable way to invoke the auth to begin with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants