-
Notifications
You must be signed in to change notification settings - Fork 89
chore: migrate pg model dynamic auth e2e test in gen2 cdk #2920
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
Conversation
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
| userPoolConfig: { | ||
| userPool, | ||
| }, | ||
| apiKeyConfig: { expires: Duration.days(2) }, |
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.
Do we need the API key to be valid for 2 days? Especially since we teardown these apps post test run, I would recommend setting it to 1 hr to begin with.
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.
Line 75 of file authorization-modes.ts in amplify-graphql-api-construct enforces the Duration would be converted to days with following definition: apiKeyExpirationDays: authMode.expires.toDays()
Setting expiration to hours would cause CDK deployment to fail with error message:
- "Error: '1 hours' cannot be converted into a whole number of days."
We could adjust the construct time conversion if needed to give test a more tight time boundary.
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.
We could adjust the construct time conversion if needed to give test a more tight time boundary.
That might be the right way to go forward here.
packages/amplify-graphql-api-construct-tests/src/sql-datatabase-controller.ts
Show resolved
Hide resolved
packages/amplify-graphql-api-construct-tests/src/utils/sql-crudl-helper.ts
Show resolved
Hide resolved
| } | ||
|
|
||
| enum SCHEMA { | ||
| DEFAULT = /* GraphQL */ ` |
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.
Why would we represent this as an enum vs a string?
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.
Both options are viable here. My thought originally was that there could be multiple existing schemas available in the stack which could be used without providing schema through stack-config but more like an option. But that could be an overdesign here, as almost every test's schema is different, and it's impossible to store all those schemas in the CDK 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.
I think maybe a similar question is why enum versus an object with a string field default?
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.
Do we have to re-define these Auth modes and set a default for schema? Instead we could just fail if either of them is not defined in the test using it.
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 could keep the Auth mode to sync with the stack config interface that is using the AUTH_TYPE from AppSync, while schema default could be discarded to avoid overdesign, since schema is already required when defining the stack config interface.
Signed-off-by: Kevin Shan <[email protected]>
| } | ||
|
|
||
| enum SCHEMA { | ||
| DEFAULT = /* GraphQL */ ` |
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.
Do we have to re-define these Auth modes and set a default for schema? Instead we could just fail if either of them is not defined in the test using it.
| userPoolConfig: { | ||
| userPool, | ||
| }, | ||
| apiKeyConfig: { expires: Duration.days(2) }, |
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.
We could adjust the construct time conversion if needed to give test a more tight time boundary.
That might be the right way to go forward here.
packages/amplify-graphql-api-construct-tests/src/utils/sql-provider-helper.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-api-construct-tests/src/utils/sql-provider-helper.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-api-construct-tests/src/utils/sql-provider-helper.ts
Show resolved
Hide resolved
packages/amplify-graphql-api-construct-tests/src/utils/sql-crudl-helper.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
Signed-off-by: Kevin Shan <[email protected]>
* chore: initial commit * chore: debugging * chore: debugging cleaning * chore: debugging * fix: revising cdk and auth * fix: tests except subscription * chore: userpool test except subscription * chore: debugging * chore: debugging * fix: ttl duration unit conversion + e2e test * chore: revert back construct changes to another PR * chore: initial cdk refactor * chore: cleanup * refactor: major refactor to sql confogurable stack and config write * refactor: configurable stack for prev tests * chore: naming + trimming * refactor: test setup + stack config * chore: cleanup * fix: async cleanup fix * refactor: appsync client refactor * refactor: appsync client helper * refactor: cognito helper + initial test * refactor: tester + test pattern extract * refactor: auth tester generic function + crudl helper * refactor: add pg auto increment test from main * refactor: test conflict resolved + merge main * chore: naming + schema change * refactor: sql provider helper to support my directives and mysql * fix: postgres schema formatting * fix: correct escape when converting sql create statements --------- Signed-off-by: Kevin Shan <[email protected]>
Description of changes
Migrate the Gen1 model dynamic auth E2E tests to Gen2 using CDK construct, with pre-provision of Aurora clusters and Cognito resources. Plus fix/change on resource pre-provision process and CDK setup.
Update: this PR also includes refactor of the Gen2 Postgres auto increment E2E test under same pattern.
CDK / CloudFormation Parameters Changed
Issue #, if available
Description of how you validated changes
CI checks and local pre-provision RDS clusters.
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.