docs(cognito): fix up code samples so that they compile #11034
Conversation
Added a default fixture with the imports needed to compile snippets. Fixed up a few parameter names, required parameters, references that were incorrect. Verified that these snippets (with fixture) compile by running `compile-samples` in the `scripts` directory. Closes #9185
|
|
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
|
||
| new UserPool(this, 'myuserpool', { | ||
| new cognito.UserPool(this, 'myuserpool', { |
There was a problem hiding this comment.
Not a big fan of prefixing cognito everywhere. Would've preferred if the fixture had individually imported what's required, so the README stays clean.
OTOH doing so would mean that any new type being used here would have the fixture be updated. Given that we have no validation for this, maybe this is ok.
| userpool.addTrigger(UserPoolOperation.USER_MIGRATION, new lambda.Function(this, 'userMigrationFn', { | ||
| // ... | ||
| userpool.addTrigger(cognito.UserPoolOperation.USER_MIGRATION, new lambda.Function(this, 'userMigrationFn', { | ||
| runtime: lambda.Runtime.NODEJS_10_X, |
| const authChallengeFn = new lambda.Function(this, 'authChallengeFn', { | ||
| // ... | ||
| runtime: lambda.Runtime.NODEJS_10_X, | ||
| handler: 'index.handler', | ||
| code: lambda.Code.fromInline('auth challenge'), | ||
| }); |
There was a problem hiding this comment.
Is this necessary? I thought rosetta translations work as long as it was valid typescript. Does it have to actually compile and work?
I would've preferred to leave this open for the user to define and rosetta translation snippet does the same.
|
|
||
| pool.addDomain('CognitoDomain', { | ||
| cognitoDomain: { | ||
| domainPrefix: 'my-awesome-app', | ||
| }, | ||
| }); | ||
|
|
||
| const domainCert = new acm.Certificate.fromCertificateArn(this, 'domainCert', certificateArn); | ||
| const certificateArn = 'arn:aws:acm:us-east-1:123456789012:certificate/11-3336f1-44483d-adc7-9cd375c5169d'; |
There was a problem hiding this comment.
Why not move this into the fixture?
| clientId: 'amzn-client-id', | ||
| clientSecret: 'amzn-client-secret', | ||
| userPool: userpool, | ||
| attributeMapping: { |
There was a problem hiding this comment.
Not a fan of this change either. Now the focus is diffused and doesn't focus on just the attributeMapping property which this section is talking about.
How about defining the whole set of properties in the rosetta fixture and doing something like
new UserPoolIdentityProviderAmazon(stack, 'Amazon', {
...otherIdentityProviderAttrs, // this is defined in the fixture
attributeMapping: {
}
});
Added a default fixture with the imports needed to compile snippets.
Fixed up a few parameter names, required parameters, references that were incorrect.
Verified that these snippets (with fixture) compile by running
compile-samplesinthe
scriptsdirectory.Closes #9185
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license