-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implemented SequenceRunManager app deployment #155
Conversation
* Added Shared API Gateway as stateful resource * Introduced param constants that go across stateful & stateless * Rearranged Python dependencies into deps directory * Removed no longer used constants * Removed now deprecated API Gateway Alpha constructs * Updated CDK nag validation accordingly Resolves #139
1d1d44f
to
18c0fae
Compare
Anybody else pls feel free look into and chime in, if any. |
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.
LGTM
import { SHARED_HTTP_API_ID } from '../../../../config/param'; | ||
import { CorsHttpMethod, HttpApi } from 'aws-cdk-lib/aws-apigatewayv2'; | ||
|
||
export class SharedApiGatewayConstruct extends Construct { |
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 the idea to reuse this for all microservices? For some reason I thought the API gateway would be stateless?
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.
Yes, agreed. Maybe refactor it out as a "common" construct that can be reused in (stateless) stacks?
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.
Possibly to put the construct directly on the top level of the stateless stack instead?
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.
Sound good, folks. I will move it back into its own service for now.
The common construct sound like the plan. But let do that a little bit down the track - until we get to the point where we can collect few use cases to generalise. Then, we shall refactor their commonality from the specificness.
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.
Have moved the APIGW into the service boundary. Pls see, if any.
@brainstorm @williamputraintan Heads up. This PR removes the Alpha constructs importing (which is deprecated upstream) and, they are available now in the main cdk library. i.e. It can be 1-1 replace import: from |
813cdd7
to
94a5dbd
Compare
Merging... |
Resolves #139