-
Notifications
You must be signed in to change notification settings - Fork 825
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: storage, functions, data codegen integration tests #13952
Conversation
packages/amplify-migration-codegen-e2e/src/__tests__/migration_codegen_e2e.test.ts
Outdated
Show resolved
Hide resolved
packages/amplify-migration-codegen-e2e/src/__tests__/migration_codegen_e2e.test.ts
Outdated
Show resolved
Hide resolved
packages/amplify-migration-codegen-e2e/src/__tests__/migration_codegen_e2e.test.ts
Outdated
Show resolved
Hide resolved
const gen2Meta = getProjectOutputs(projRoot); | ||
const gen2Region = gen2Meta.auth.aws_region; | ||
|
||
// Workaround as amplify_outputs.json file doesn't have function metadata |
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.
can the gen2 lib BackendOutputClientFactory
be use here? The function name is in the root stack output.
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.
Importing gen2 lib would require making modifications to the gen2 package just for e2e tests as they are setup only for ES6 modules.
|
||
return ['amplify', namespace, name, backendId.type, hash].join('-'); | ||
} | ||
|
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.
The logic to get the gen2 stackName from the backendId is the same and used from the gen2 library. Importing gen2 library would require making modifications to the gen2 packages as it is setup for ES6 modules only. Not worth to make changes just for e2e tests.
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.
Instead of replicating the code which is error prone and not future-proof, you can pipe the output of npx ampx amplify --once
to a tmp file, there is a line Stack: <stackname>
in there.
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.
or you can search for arn:aws:cloudformation:<region>:<accountid>:stack
prefix, it is a stack ARN . it is better since Stack: <stackname>
may get changed
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.
another alternative is using amplify sandbox --identifier <someId>
then cfn.listStack() and substring match the stackname on the identifier. No dependency on internal stack construction code or sandbox cmd output format.
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.
Addressed. d310c32
I have piped the output of npx ampx sandbox
and searched for the prefix arn:aws:cloudformation:<region>:<accountid>:stack
having stack name.
await setupAndPushGen1Project(projRoot, 'CodegenTest'); | ||
const { gen1UserPoolId, gen1Region } = await assertGen1Setup(projRoot); | ||
const { gen1UserPoolId, gen1FunctionName, gen1BucketName, gen1GraphQLAPIId, gen1Region } = await assertGen1Setup(projRoot); |
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.
gen1GraphQLAPIId -> gen1GraphqlApiId
gen1Region -> should it be a static variable instead of passing back arg? How the region is set up in the existing gen1 integ test?
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.
Existing tests in gen1 fetch the region from the meta file. That is what is done in theassertGen1Setup()
and being passed back.
} | ||
|
||
export async function copyGen1Schema(projRoot: string): Promise<void> { | ||
const gen1SchemaPath = path.join(projRoot, '.amplify', 'migration', 'amplify', 'backend', 'api', 'codegentest', 'schema.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.
codegentest
-> make it a constant?
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.
Addressed. d310c32
await fs.writeFile(backendPath, backendContent, 'utf-8'); | ||
} | ||
|
||
async function setEnableGen2MigrationFeatureFlag(projectRoot: string): Promise<void> { |
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 we need a feature flag? Is it temporary?
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 need to set this feature flag to true for the data category codegen to work, to get the table mapping information.
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.
Removed this function and reused already existing function to do the same from amplify-e2e-core package.
await fs.writeFile(packageJsonPath, updatedContent, 'utf-8'); | ||
} | ||
|
||
async function getAppSyncDataSource(apiId: string, dataSourceName: string, region: 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.
this helpers.ts file is growing in size. you may consider splitting it to auth_helpers, data_helpers...or something similar.
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.
Best if we don't have any helpers
files.
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.
Addressed. d310c32
const getHash = (backendId: BackendIdentifier): string => | ||
backendId.hash ?? | ||
// md5 would be sufficient here because this hash does not need to be cryptographically secure, but this ensures that we don't get unnecessarily flagged by some security scanner | ||
createHash('sha512').update(backendId.namespace).update(backendId.name).digest('hex').slice(0, HASH_LENGTH); | ||
|
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 are we copying this from backend repo?
Can't we just use this package https://github.com/aws-amplify/amplify-backend/blob/main/packages/platform-core/API.md ?
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.
Tried importing the package, but it is supported only for ES6 modules and importing this package in gen1 cli which is commonJS module, gives import errors due to syntax mismatch.
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.
:(.
In that case, please move the copied code to separate file (class ?) and add verbose comments about where it came from and why.
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.
Addressed. d310c32
I have piped the output of npx ampx sandbox and searched for the prefix arn:aws:cloudformation:::stack having stack name.
Description of changes
Added Integration tests for storage, function, data category along with assert functions for the respective category.
Description of how you validated changes
Manually checking the deployed resources on console.
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.