Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

correct logic for table name #10

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

Merged

Conversation

@barrysteyn
Copy link

@barrysteyn barrysteyn commented Nov 18, 2019
edited
Loading

Table names must always be deterministic - if a name is defined, then it MUST not be random since it is probably designed to be used in application code.

This fixes logic in #4 and addresses #6. @dodgeblaster Please look at this over. I have done the following:

  1. If name is defined, then there is no random id added onto the name
  2. Simplified logic greatly in setTableName, and added comment (now easy to understand what's happening).
  3. Removed nameInput state variable (less state means less errors).

In summary: this PR accomplishes what was set out to achieve in #4, but neatens up the logic and allows for a deterministic (i.e. non-random) table name if name is defined.

I can't stress how important it is that this PR be addressed.

Please @eahefnawy can you look at it right away. The logic is right now broken. Why? Because if I define a table name in this component, I can't refer to it in code since the name becomes random. This becomes a bigger problem when you have multiple instances of the same component (i.e. a prod and staging component), and so one needs the name to be deterministic so that you don't write special logic in each case to get to the table name.

Lastly, @danielcondemarin also needs this logic for the global table component.

pwagener and KowalskiTom reacted with thumbs up emoji
Copy link
Author

@eahefnawy Also, please can you merge #9 in as well. Preferably, merge this first, and then #9

Copy link
Contributor

@barrysteyn You can use the generated table name in your application code. This component outputs the name of the generated table, which you can use as an input to another component. Example:

name: example-project
dynamo:
 component: '@serverless/aws-dynamodb'
 inputs:
 name: table-name
 attributeDefinitions:
 - AttributeName: PK
 AttributeType: S
 - AttributeName: SK
 AttributeType: S
 keySchema:
 - AttributeName: PK
 KeyType: HASH
 - AttributeName: SK
 KeyType: RANGE
 region: us-east-1
function:
 component: ./code
 inputs:
 dbName: ${dynamo.name}
 dbArn: ${dynamo.arn}

Copy link
Author

barrysteyn commented Nov 18, 2019
edited
Loading

@barrysteyn You can use the generated table name in your application code. This component outputs the name of the generated table, which you can use as an input to another component.

What you have described is different. You are describing using component inputs in other components. However, I am talking about the actual code that a user would use the component to deploy and run. Sure, if that component is aware of the table name, it could then pass it to the app code. However this is not very generic.

A good example is the amazing next.js component: It does not care about dynamodb (nor should it), since a user should be able to use whatever storage they desire (e.g. MongoDB). And so that component would never inject the name of the table into the app code.

Copy link
Member

@eahefnawy eahefnawy left a comment

Choose a reason for hiding this comment

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

Sorry for the super late response here @barrysteyn ... I think this change is good to go, I was just nervous of making changes to the table name as it's usually destructive, but it seems that you're handling it pretty well.

LGTM

@eahefnawy eahefnawy merged commit 2bf8af1 into serverless-components:master Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eahefnawy eahefnawy eahefnawy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /