-
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 amplify-category-api to CDK v2 #883
Conversation
… CDK v2 - make transformer e2e tests compile.
command: yarn coverage | ||
environment: | ||
NODE_OPTIONS: --max-old-space-size=4096 |
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.
Because of Because of https://app.circleci.com/pipelines/github/aws-amplify/amplify-category-api/1548/workflows/86e8aeb5-342e-4526-9370-2b7789e24d18/jobs/40319 .
I .
Borrowed from https://github.com/aws-amplify/amplify-cli/blob/015187a8b714b96fbb87331383a92664aa6e6f35/.circleci/config.base.yml#L134-L148
import * as apigw2 from '@aws-cdk/aws-apigatewayv2'; | ||
import * as cdk from '@aws-cdk/core'; | ||
import * as apigw2 from 'aws-cdk-lib/aws-apigatewayv2'; | ||
import * as apigw2alpha from '@aws-cdk/aws-apigatewayv2-alpha'; |
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 added this for constant definitions used below. We should be fine as this is internal usage.
The alternative is to copy constants into our codebase and maintain them.
service: new (class extends Construct implements ecs.IBaseService { | ||
// eslint-disable-next-line class-methods-use-this | ||
applyRemovalPolicy(_: cdk.RemovalPolicy): void { | ||
// NO-OP | ||
} | ||
|
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.
This is forced upon us by cdk.IResource
. I have not found an obvious candidate to delegate this call to this anonymous class members (or containing class). Therefore I'm logging follow up for this here . https://app.asana.com/0/1202389680978480/1203167795285200/f .
"name": "amplify-category-api-util-mock", | ||
"version": "5.1.14", | ||
"version": "6.0.0", | ||
"description": "Amplify CLI plugin providing local testing", | ||
"repository": { | ||
"type": "git", |
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.
This one amplify-category-api-util-mock
will still carry CDK v1 references through amplify-provider-awscloudformation
from CLI repo...
Which means we need two (?) cycles of data - cli tag releases (and then normal releases at some point) to get this straight.
See
$ yarn why @aws-cdk/core
yarn why v1.18.0
[1/4] 🤔 Why do we have the module "@aws-cdk/core"...?
[2/4] 🚚 Initialising dependency graph...
[3/4] 🔍 Finding dependency...
[4/4] 🚡 Calculating file sizes...
=> Found "@aws-cdk/core@1.124.0"
info Reasons this module exists
- "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-auth-transformer" depends on it
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-auth-transformer#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-function-transformer#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-http-transformer#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-index-transformer#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-relational-transformer#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-interfaces#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#assert#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-apigatewayv2#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#aws-applicationautoscaling#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-function-transformer#@aws-cdk#aws-appsync#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-autoscaling#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#aws-certificatemanager#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#aws-cloudwatch#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#aws-codeguruprofiler#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#aws-cognito#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-index-transformer#@aws-cdk#aws-dynamodb#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#aws-ec2#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-ecr-assets#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-ecr#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-ecs#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#aws-efs#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-elasticloadbalancing#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-elasticloadbalancingv2#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#aws-elasticsearch#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#aws-events#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#aws-iam#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#aws-kms#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-function-transformer#@aws-cdk#aws-lambda#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#aws-logs#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-interfaces#@aws-cdk#aws-rds#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#aws-route53#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#aws-s3-assets#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#aws-s3#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-interfaces#@aws-cdk#aws-secretsmanager#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-servicediscovery#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-sns#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-core#@aws-cdk#aws-sqs#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#custom-resources#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-amplify#cli-extensibility-helper#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-amplify#graphql-transformer-core#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-amplify#graphql-transformer-interfaces#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-ecr-assets#@aws-cdk#assets#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-amplify#cli-extensibility-helper#@aws-cdk#aws-apigateway#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#aws-applicationautoscaling#@aws-cdk#aws-autoscaling-common#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-ecs#@aws-cdk#aws-autoscaling-hooktargets#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#custom-resources#@aws-cdk#aws-cloudformation#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-sns#@aws-cdk#aws-codestarnotifications#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-index-transformer#@aws-cdk#aws-dynamodb#@aws-cdk#aws-kinesis#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-ecs#@aws-cdk#aws-route53-targets#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-transformer-interfaces#@aws-cdk#aws-secretsmanager#@aws-cdk#aws-sam#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-function-transformer#@aws-cdk#aws-lambda#@aws-cdk#aws-signer#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-graphql-migration-tests#@aws-amplify#graphql-model-transformer#@aws-cdk#aws-ec2#@aws-cdk#aws-ssm#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-category-function#amplify-cli-core#@aws-amplify#graphql-transformer-core#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-category-function#amplify-cli-core#@aws-amplify#graphql-transformer-interfaces#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-ecs#@aws-cdk#aws-route53-targets#@aws-cdk#aws-cloudfront#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-ecs#@aws-cdk#aws-route53-targets#@aws-cdk#aws-globalaccelerator#@aws-cdk#core"
- Hoisted from "_project_#amplify-category-api-util-mock#amplify-provider-awscloudformation#@aws-cdk#aws-ecs#@aws-cdk#aws-autoscaling-hooktargets#@aws-cdk#aws-sns-subscriptions#@aws-cdk#core"
info Disk size without dependencies: "4.56MB"
info Disk size with unique dependencies: "7.49MB"
info Disk size with transitive dependencies: "8.09MB"
info Number of shared dependencies: 17
✨ Done in 0.61s.
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.
IIUC , We can convert cli to use cdkV2 , but this will fetch cdkV1 dependencies from api Repo.
Then a data release removing cdkV1 dependencies , this will flush out cdkV1 dependencies right.
@@ -1,4 +1,3 @@ | |||
{ | |||
"@aws-cdk/core:enableStackNameDuplicates": "true", |
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.
When I briefly debugged http transfomer e2e tests cdk v2 CLI (aws-cdk
v2) failed fast saying that this flag is removed and enabled by default now.
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.
One question , otherwise looks good
import * as cloudmap from 'aws-cdk-lib/aws-servicediscovery'; | ||
import * as cdk from 'aws-cdk-lib'; | ||
import { Construct } from 'constructs'; | ||
import { prepareApp } from 'aws-cdk-lib/core/lib/private/prepare-app'; |
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 think we can remove this import if its just building one cfn file
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.
Adding follow up for this https://app.asana.com/0/1202389680978480/1203186729774450/f . We should revisit this when e2e tests are green.
"name": "amplify-category-api-util-mock", | ||
"version": "5.1.14", | ||
"version": "6.0.0", | ||
"description": "Amplify CLI plugin providing local testing", | ||
"repository": { | ||
"type": "git", |
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.
IIUC , We can convert cli to use cdkV2 , but this will fetch cdkV1 dependencies from api Repo.
Then a data release removing cdkV1 dependencies , this will flush out cdkV1 dependencies right.
Description of changes
This PR:
@aws-amplify/amplify-category-api
Out of scope:
I took a stab at
amplify-category-api-graphql-migration-tests
but I reverted this as this madeyarn test
fail. We'll work on that as part of e2e tests fixing as it's most likely manifestation of common issues.Issue #, if available
Description of how you validated changes
yarn clean && yarn dev-build && yarn test
passes.yarn build-tests
passes.Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.