-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws: Allow to change the DynamoDB Endpoint #2825
Conversation
…B Endpoint for example to connect to dynamodb-local
Region: c.Region, | ||
MaxRetries: c.MaxRetries, | ||
Endpoint: c.DynamoDBEndpoint, | ||
} |
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.
I didn't like this duplication of configs, but we can't set Endpoint
for all other connections. I've also tried to copy awsConfig
, but go didn't like it:
awsDynamoDBConfig := *awsConfig
awsDynamoDBConfig.Endpoint = c.DynamoDBEndpoint
Any ideas?
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.
I think this is fine for now - the duplication is super local. If we get more duplication here we can investigate cleanup options. 👌
"dynamodb_endpoint": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Default: "", |
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.
Is ""
equivalent to "use upstream default" here?
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.
@phinze that's correct based on their docs: https://github.com/aws/aws-sdk-go/blob/67dc9f948602be9f85cb640f89b0adec994ccbda/aws/config.go#L49-L55 and I've also confirmed that on my local tests.
But I can remove the Default: ""
above and make it more explicit:
awsDynamoDBConfig := &aws.Config{
Credentials: creds,
Region: c.Region,
MaxRetries: c.MaxRetries,
}
if c.DynamoDBEndpoint != nil {
awsDynamoDBConfig.Endpoint = c.DynamoDBEndpoint
}
wdyt?
Thanks for this, @phstc - one inline question, and think you could add the attribute to the provider docs too? |
@@ -55,5 +55,7 @@ The following arguments are supported in the `provider` block: | |||
to prevent you mistakenly using a wrong one (and end up destroying live environment). | |||
Conflicts with `allowed_account_ids`. | |||
|
|||
* `dynamodb_endpoint` - (Optional) Use this to override the default endpoint URL constructed from the :region. It's typically used to connect to dynamodb-local" | |||
|
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.
@phinze provider docs ^^^ updated 🍻
…ency with the provider docs
@phinze not sure why the tests are failing |
@phstc there have been upstream changes to the awsDynamoDBConfig := &aws.Config{
Credentials: creds,
Region: aws.String(c.Region),
MaxRetries: aws.Int(c.MaxRetries),
Endpoint: aws.String(c.DynamoDBEndpoint),
} |
@catsby thanks! all set then:
|
? |
provider/aws: Allow to change the DynamoDB Endpoint
Thanks @phstc ! |
🤘 @catsby is there a new release on the schedule? or a mailing list to subscribe to get release updates? I will wait for a new release to update my work colleagues to start using terraform with dynamodb-local. |
I think next week, I'll double check
There is a mailing list here: I don't know that we've been very diligent about announcing there. We typically tweet the announcement here https://twitter.com/hashicorp , e.g. https://twitter.com/hashicorp/status/623250629247520768 That said, we probably should announce on the list... |
* master: (720 commits) Update CHANGELOG.md Update CHANGELOG.md dynamodb-local Update AWS config #2825 (comment) Make target_pools optional Update CHANGELOG.md code formatting Update CHANGELOG.md providers/google: Fix reading account_file path providers/google: Fix error appending providers/google: Return if we could parse JSON providers/google: Change account_file to JSON providers/google: Default account_file* to empty providers/google: Add account_file/account_file_contents ConflictsWith providers/google: Document account_file_contents providers/google: Use account_file_contents if provided providers/google: Add account_file_contents to provider Update CHANGELOG.md Update CHANGELOG.md dynamodb-local Use ` instead of : to refer region to keep the consistency with the provider docs dynamodb-local Update aws provider docs to include the `dynamodb_endpoint` argument ...
@catsby thanks! You got a new follower 😄 |
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. |
Hi
This PR allows us to change the DynamoDB Endpoint, which super useful to run "migrations" against dynamodb-local.
With that, we can use the same scripts we use in production (AWS DynamoDB), in development (dynamodb-local).
The way I'm using it:
cd tf/local ls table1.tf table2.tf config.tf terraform plan terraform apply
In the folder
local
besides the table definitions, I have also aconfig.tf
with:which changes the DynamoDB Endpoint.
I've intentionally set
access
andsecret
to 123, as we don't need those to connect to dynamodb-local.Any code feedback would be more than appreciated 🍻
Fix #2763.