-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat: migrate index and model transformers to CDK v2 #860
feat: migrate index and model transformers to CDK v2 #860
Conversation
@@ -237,7 +237,7 @@ $util.qr($ctx.stash.metadata.put(\\"apiId\\", \\"", | |||
"\\")) | |||
$util.qr($ctx.stash.put(\\"tableName\\", \\"", | |||
Object { | |||
"Ref": "PostTable", |
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.
Follow up: figure out if changing name like this is ok.
https://app.asana.com/0/1202389680978480/1203117329609220/f
…sformer-to-cdk-v2
This pull request introduces 1 alert when merging 30760ce into b3cb9ae - view on LGTM.com new alerts:
|
@@ -24,7 +24,7 @@ | |||
"clean-stale-test-buckets": "ts-node ./src/cleanup-stale-test-buckets.ts" | |||
}, | |||
"dependencies": { | |||
"@aws-amplify/graphql-transformer-core": "0.17.12", | |||
"@aws-amplify/graphql-transformer-core": "1.0.0", |
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.
Until we migrate everything we end up with:
$ yarn tsc --build tsconfig.tests.json
$ /home/circleci/repo/node_modules/.bin/tsc --build tsconfig.tests.json
src/__tests__/BelongsToTransformerV2.e2e.test.ts(49,69): error TS2345: Argument of type '() => import("/home/circleci/repo/packages/amplify-graphql-auth-transformer/node_modules/@aws-amplify/graphql-transformer-core/lib/transformation/transform").GraphQLTransform' is not assignable to parameter of type '() => import("/home/circleci/repo/packages/amplify-graphql-transformer-core/lib/transformation/transform").GraphQLTransform'.
Type 'import("/home/circleci/repo/packages/amplify-graphql-auth-transformer/node_modules/@aws-amplify/graphql-transformer-core/lib/transformation/transform").GraphQLTransform' is not assignable to type 'import("/home/circleci/repo/packages/amplify-graphql-transformer-core/lib/transformation/transform").GraphQLTransform'.
Types have separate declarations of a private property 'options'.
src/__tests__/MapsToTransformer.e2e.test.ts(57,64): error TS2345: Argument of type '() => import("/home/circleci/repo/packages/amplify-graphql-auth-transformer/node_modules/@aws-amplify/graphql-transformer-core/lib/transformation/transform").GraphQLTransform' is not assignable to parameter of type '() => import("/home/circleci/repo/packages/amplify-graphql-transformer-core/lib/transformation/transform").GraphQLTransform'.
Type 'import("/home/circleci/repo/packages/amplify-graphql-auth-transformer/node_modules/@aws-amplify/graphql-transformer-core/lib/transformation/transform").GraphQLTransform' is not assignable to type 'import("/home/circleci/repo/packages/amplify-graphql-transformer-core/lib/transformation/transform").GraphQLTransform'.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
@@ -110,7 +113,7 @@ export interface S3MappingFunctionCodeProvider { | |||
|
|||
export type MappingTemplateProvider = InlineMappingTemplateProvider | S3MappingTemplateProvider; | |||
|
|||
export interface GraphQLAPIProvider { | |||
export interface GraphQLAPIProvider extends IConstruct { |
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.
IConstruct
got node
property in latest version and compiler was unhappy about us passing GraphQLAPIProvider
as a construct.
@alharris-at I have not explored what does this mean to add node
prop to GraphQLAPIProvider
just added IConstruct
as a parent since we use GraphQLAPIProvider
as a construct - is this right direction? I.e. did we think about GraphQLAPIProvider
as a construct or not?
Here
amplify-category-api/packages/amplify-graphql-transformer-core/src/graphql-api.ts
Lines 106 to 107 in b96ca09
public resourceArns(api: GraphQLAPIProvider): string[] { | |
return this.arns.map(arn => Stack.of(api).formatArn({ |
"graphql": "^14.5.8", | ||
"graphql-mapping-template": "4.20.5", | ||
"graphql-transformer-common": "4.24.0" | ||
}, | ||
"devDependencies": { | ||
"@aws-cdk/assert": "~1.124.0" | ||
"@aws-cdk/assert": "~2.41.0" |
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.
Doesn't it comes under 'aws-cdk-lib' ?
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.
It doesn't. It's a testing library. It's expected that this will be separate package.
However, now that I'm looking at https://www.npmjs.com/package/@aws-cdk/assert it says it's deprecated. I'll take a look at what's best option here.
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.
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.
So this is indeed part of core package. I'll work on this then. thanks!
@@ -79,7 +79,7 @@ export class TransformerStackSythesizer extends LegacyStackSynthesizer { | |||
const s3ObjectUrl = `s3://${bucketName}/${rootKey}/${asset.fileName}`; | |||
|
|||
return { | |||
bucketName, objectKey, httpUrl, s3ObjectUrl, s3Url: httpUrl, | |||
bucketName, objectKey, httpUrl, s3ObjectUrl, |
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.
Is this a lint fix ?
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.
No. CDK removed that parameter. It was deprecated and is redundant with httpUrl
.
"CommentTableE9F84112", | ||
"Arn", |
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.
Is the new construct creating new logicalIds for ddbTable?
That can create problem
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 logged a follow up to investigate this when we get to a stage when we can run e2e workflows (tests and manual). https://app.asana.com/0/1202389680978480/1203117329609220/f .
The name isn't pretty but might be ok given that it's consistent change across the whole file.
@@ -7,7 +7,7 @@ import { | |||
Template, | |||
} from '@aws-amplify/graphql-transformer-core'; | |||
import { FeatureFlagProvider } from '@aws-amplify/graphql-transformer-interfaces'; | |||
import { expect as cdkExpect, haveResourceLike } from '@aws-cdk/assert'; | |||
import { Template as AssertionTemplate } from 'aws-cdk-lib/assertions'; |
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.
name collision.
@@ -40,6 +40,7 @@ | |||
}, | |||
"devDependencies": { | |||
"@aws-amplify/graphql-model-transformer": "0.16.0", | |||
"@aws-amplify/graphql-index-transformer": "0.14.0", |
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.
there's index transformer usage in the with-many-to-many.test.ts
. Magically latest version from build was picked up so adding this here for 1. completeness 2. to not bloat this PR with one more transformer.
Description of changes
This PR makes the following packages compile with CDK v2.
@aws-amplify/graphql-index-transformer
@aws-amplify/graphql-model-transformer
@aws-amplify/graphql-transformer-core
@aws-amplify/graphql-transformer-interfaces
Issue #, if available
Description of how you validated changes
This PR compiles and passes
yarn test
, however we won't be able to run e2e tests until we migrate whole category.See https://app.circleci.com/pipelines/github/aws-amplify/amplify-category-api/1412/workflows/79a728ce-2ecd-4822-ae15-35e5ee32cfb0
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.