-
Notifications
You must be signed in to change notification settings - Fork 50
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: added common helpers like redis,appsyncmergeapi,event bridge an… #13
Conversation
…d updated vpc helper
…d updated vpc helper
src/patterns/gen-ai/aws-summarization-appsync-stepfn/summary_architecture.gif
Outdated
Show resolved
Hide resolved
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.
Good for v1
Next pass we will add unit tests on helpers
src/patterns/gen-ai/aws-summarization-appsync-stepfn/architecture.png
Outdated
Show resolved
Hide resolved
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.
What keeps the package-lock.json
and yarn.lock
in sync?
} | ||
|
||
function getMergedAPIRole(scope: Construct, props: AppsyncMergedApiProps) { | ||
return new iam.Role(scope, 'mergedapirole', { |
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.
A small nit pick, can we use camel case for resource names?
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.
ack.
* @param AppsyncMergedApiProps The props to be used by the construct | ||
* @returns App sync merge api | ||
*/ | ||
export function buildMergedAPI(scope: Construct, props: AppsyncMergedApiProps, logProps: LogConfigProps) { |
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 it a common convention to have two prop arguments? consider one Prop with a key-value of type LogConfigProps
import { Construct } from 'constructs'; | ||
|
||
|
||
export interface AppsyncMergedApiProps { |
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 some of these be strongly typed? then perhaps getting the ID's from the object properties later in the code (foo.cognitoUserPoolId
)
const redisclustername = props.redisclustername || 'redisCluster'; | ||
const cacheNodeType = props.cacheNodeType || 'cache.r6g.xlarge'; | ||
|
||
let redisCulster = new elasticache.CfnCacheCluster(scope, 'redisCluster', { |
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 great! love the way you used the local functions 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.
remove package-lock.json
for now
Removed package-lock.json from the PR. Managed by projen but need to review it again to make sure there is only one package manager. Signed-off-by: Dinesh Sajwan <[email protected]>
9a53381
Fixes #
Added following common/helpers construct.
# redis-helper.ts,
# appsyncmergedapi-helper.
# eventbridge-helper.ts
# updated vpc-helper.ts
Added Pattern aws-summarization-appsync-stepfn
# Initial Readme draft for summarization pattern construct.
# Architecture diagram.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.