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

Make creating the table optional #88

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nodefortytwo
Copy link

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

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
@philippgille
Copy link
Owner

Hi @nodefortytwo,

Thanks for the PR!

It's pretty bad form to create cloud resources automatically

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 true and your flag it's perfect in my opinion.

We can think about making false the default in the future, I don't mind breaking changes as long as the version is < 1.0.0, and with Go modules there shouldn't be issues with people accidentally getting a new version with a go get.

There's one syntax error though, I'll comment it in the PR code.

@@ -184,6 +186,7 @@ var DefaultOptions = Options{
TableName: "gokv",
ReadCapacityUnits: 5,
WriteCapacityUnits: 5,
CreateTable: true
Copy link
Owner

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

@@ -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{
Copy link
Owner

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 {

@nodefortytwo
Copy link
Author

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.

@philippgille
Copy link
Owner

philippgille commented Jan 20, 2020

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
=> The client created during the tests doesn't use the DefaultOptions (see

options := dynamodb.Options{
), which is why false is used for CreateTable, now leading to these errors:

ResourceNotFoundException: Cannot do operations on a non-existent table

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 CreateTable to SkipCreateTable - that way the default value would be false and a client can use an empty Options struct or the DefaultOptions and the behavior would be the same.

What do you think?

@nodefortytwo
Copy link
Author

Yeah agreed, thats a nice change, i'll update the PR tomorrow

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.

None yet

2 participants