feat: add overrideIndexName to configure index name #2738
Conversation
dpilch
force-pushed
the
configure-index-name
branch
from
091def5 to
7c8da3d
Compare
| import { Directive } from './directive'; | ||
|
|
||
| const name = 'belongsTo'; | ||
| // TODO: review overrideIndexName before merging to main |
There was a problem hiding this comment.
Is override prefix necessary here? Can we just keep it indexName and if specified it means an override, otherwise use default?
There was a problem hiding this comment.
We will discuss further in API naming review, but I wouldn't favor overloading the indexName directive with different behavior like that.
| const overrideIndexName = getOverrideIndexName(config); | ||
| if (overrideIndexName) { | ||
| config.indexName = overrideIndexName; | ||
| } |
There was a problem hiding this comment.
- Super nitpicky here & below: why separate this assignment from the config.indexName assignment 2 lines above, as opposed to a single block to handle the default & override cases?
- Can you confirm that even though we're processing the GSI name in
hasOneandhasManytransformers, it's actually created on the relatedbelongsTotable?
There was a problem hiding this comment.
This asserts the GSI is created on the belongsTo table. https://github.com/aws-amplify/amplify-category-api/pull/2738/files#diff-acd0902518c587805b3f7217654188e3cc323f5dc9500f146db4f3673109b1f6R824
| import { Directive } from './directive'; | ||
|
|
||
| const name = 'belongsTo'; | ||
| // TODO: review overrideIndexName before merging to main |
There was a problem hiding this comment.
We will discuss further in API naming review, but I wouldn't favor overloading the indexName directive with different behavior like that.
palpatim
left a comment
palpatim
left a comment
There was a problem hiding this comment.
LGTM, pending addressing nitpicky comments in a future PR
3 participants
Description of changes
Add
overrideIndexname to@belongsToto change the name of the GSI that is automatically created with references.CDK / CloudFormation Parameters Changed
See above.
Issue #, if available
N/A
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.