Gen1 migration #3084
Gen1 migration #3084
Conversation
dpilch
force-pushed
the
gen1-migration
branch
from
c52db51 to
83c3ced
Compare
dpilch
force-pushed
the
gen1-migration
branch
from
a6aa93b to
184503b
Compare
dpilch
force-pushed
the
gen1-migration
branch
from
e901f8e to
bc9854c
Compare
| if (datasource.ds.dynamoDbConfig && !cdk.isResolvableObject(datasource.ds.dynamoDbConfig)) { | ||
| dataSourceMapping[modelName] = datasource.ds.dynamoDbConfig.tableName; | ||
| } else { | ||
| console.warn( | ||
| `Could not resolve table name for ${modelName}. DataSourceMappingOutput is incomplete. Please manually add ${modelName} to the mapping for your migration.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Does this block mean that manually overridden table names aren't properly handled? In what cases would the warning be triggered? Is a warning sufficient? It's super easy to miss warnings in the output of a gen1 flow. Can we allow the customer to specify a manual configuration for the errored configs, and only throw an error if that manual specification isn't present?
There was a problem hiding this comment.
This was to make TypeScript happy. I haven't found a case where the else would actually execute.
Property 'tableName' does not exist on type 'DynamoDBConfigProperty | IResolvable'.
Property 'tableName' does not exist on type 'IResolvable'.
There was a problem hiding this comment.
How do we handle the cases where table name is changed using override api?
There was a problem hiding this comment.
This will correctly get the table name when using overrides. I just tested again to be sure.
type Todo @model {
id: ID!
name: String!
description: String
}Deploy without override adds the correct mapping with the generated table name.
Then I added this override.
// set custom table name
resources.models.Todo.modelDDBTable.addPropertyOverride(
"TableName",
"MyCustomTableName"
);The table mapping output: {"Todo":"MyCustomTableName"}
packages/amplify-graphql-model-transformer/src/graphql-model-transformer.ts
Show resolved
Hide resolved
| }); | ||
| if (context.transformParameters.enableGen2Migration && context.transformParameters.enableTransformerCfnOutputs) { |
There was a problem hiding this comment.
Should we check transformer version in all these places? I think we need a way to stop V1 customers running this command.
There was a problem hiding this comment.
Good idea, I've added a check in transformGraphQLSchemaV1 to prevent this.
| @@ -103,7 +103,7 @@ | |||
| "coverageProvider": "v8", | |||
| "coverageThreshold": { | |||
| "global": { | |||
| "branches": 67, | |||
| "branches": 66, | |||
There was a problem hiding this comment.
Coverage decreased slightly because of the unreachable error message.
Description of changes
enableGen2Migration.CDK / CloudFormation Parameters Changed
New behavior is not exposed through the construct because migration is not supported from construct to gen 2.
Issue #, if available
N/A
Description of how you validated changes
enableGen2Migration.Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.