-
Notifications
You must be signed in to change notification settings - Fork 71
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
Make creating the table optional #88
base: master
Are you sure you want to change the base?
Conversation
It's pretty bad form to create cloud resources automatically, especially with such a generic default name. I would make it default to false but that would change the existing behaviour to much
Hi @nodefortytwo, Thanks for the PR!
Good point! I wanted it to be as easy as possible for users, including automatically creating the table so they wouldn't have to use the AWS dashboard, but didn't think about a flag to disable that. So with the default being We can think about making There's one syntax error though, I'll comment it in the PR code. |
dynamodb/dynamodb.go
Outdated
@@ -184,6 +186,7 @@ var DefaultOptions = Options{ | |||
TableName: "gokv", | |||
ReadCapacityUnits: 5, | |||
WriteCapacityUnits: 5, | |||
CreateTable: true |
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.
trailing "," is missing here, see the Travis CI build: https://travis-ci.org/philippgille/gokv/jobs/638423386#L2320
dynamodb/dynamodb.go
Outdated
@@ -261,7 +264,7 @@ func NewClient(options Options) (Client, error) { | |||
awsErr, ok := err.(awserr.Error) | |||
if !ok { | |||
return result, err | |||
} else if awsErr.Code() == awsdynamodb.ErrCodeResourceNotFoundException { | |||
} else if awsErr.Code() == awsdynamodb.ErrCodeResourceNotFoundException && options.CreateTable{ |
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 gofmt would add a space between the options.CreateTable
and the {
Sorry you are right, i'll make those updates. I did the change straight into github :) i'll update later. I am planning on a much bigger overhaul of this backend to add flexibility around role assumption and things like that (useful for lambdas) but i'll do that as a separate pr. |
That sounds great! When I implemented this I didn't dig very deep into AWS and DynamoDB, so it's awesome if you have ideas for improvements and could even implement them! Regarding the failing CI build: https://travis-ci.org/philippgille/gokv/jobs/639494585#L2596 gokv/dynamodb/dynamodb_test.go Line 231 in f277b58
false is used for CreateTable , now leading to these errors:
This also means that the PR would break existing code that doesn't use the DefaultOptions. But maybe that's okay, because 1) with Go modules or even a "previous generation package manager" or vendoring and a proper SemVer change in gokv this is acceptable, and 2) it's better to break by not creating a table anymore than the other way around. The absolute safest option (no break at all) would be to change What do you think? |
Yeah agreed, thats a nice change, i'll update the PR tomorrow |
It's pretty bad form to create cloud resources automatically, especially with such a generic default name.
I would make it default to false but that would change the existing behaviour to much