-
Notifications
You must be signed in to change notification settings - Fork 4k
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(apigatewayv2-integrations): sqs integrations #29646
base: main
Are you sure you want to change the base?
feat(apigatewayv2-integrations): sqs integrations #29646
Conversation
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 pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
Thank you for your contribution! I really like the thought put in here to default values and abstracting away some of the parameters you know users will need every time. I have a few questions/comments inline regarding the contract. I'm not sure it's quite right but this is a great start.
* | ||
* @default HttpIntegrationSubtype.SQS_SEND_MESSAGE | ||
*/ | ||
readonly subtype?: apigwv2.HttpIntegrationSubtype; |
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.
Since only a couple of these are relevant here, I think that this isn't quite the right type to use here. Of the 10 values, only 4 work.
Additionally, per the comments above where you mention that certain arns need to be added based off the subtype here, I think that these props aren't quite the right contract in general. For instance, in the case that this subtype is START_EXECUTION
, the parameter mapping shouldn't be optional because it must include at least the StateMachineArn
.
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.
Wait, I actually went back and read through the SubTypes more and it looks like the subtypes listed in the parameterMapping
docstring above don't start with SQS_
so those comments don't mesh with the ones here in subType
.
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'm really sorry for describing crazy comments for parameterMapping
.
I have updated it and could you please re-confirm 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.
Why not narrow down the type to the sane choices? Ah damn, it's an enum...
type PickEnum<T, K extends T> = {
[P in keyof K]: P extends K ? P : never;
};
type HttpSqsIntegrationProps = {
readonly subtype: PickEnum<HttpIntegrationSubtype,
| HttpIntegrationSubtype.SQS_SEND_MESSAGE
| HttpIntegrationSubtype.SQS_RECEIVE_MESSAGE
| HttpIntegrationSubtype.SQS_DELETE_MESSAGE
| HttpIntegrationSubtype.SQS_PURGE_QUEUE
>;
readonly parameterMapping?: ParameterMapping;
};
Would that work?
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.
@chriswilty Thank you for your suggestion.
This PickEnum
type will work correctly in TypeScript. However, it may not function properly when converted to other languages using jsii.
For example, I believe even TypeScript's union types couldn't be used in CDK.
I would like to check with the maintainer if it is usable.
* | ||
* @see https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-parameter-mapping.html | ||
* | ||
* @default - specify only `StateMachineArn` |
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.
Below in the code you're using this.props.queue.queueUrl
so this doesn't seem accurate.
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 have fixed it.
* | ||
* @default - specify only `StateMachineArn` | ||
*/ | ||
readonly parameterMapping?: apigwv2.ParameterMapping; |
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.
Are these parameter mappings typically defined or can they be whatever the user adds 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.
Default parameter mappings are based on this docs.
I defined only required parameter such as QueueUrl
for SQS_SEND_MESSAGE
.
If a user wants to set optional parameters or change the values of required parameters, it is assumed that they will provide a custom-defined parameterMapping
.
Is this response addressing your question? Please feel free to correct me if my understanding is incorrect.
Pull request has been modified.
@TheRealAmazonKendra Thank you for your review! I deeply apologize for the nonsensical comments regarding the |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
d3c3c88
to
d8a7760
Compare
d8a7760
to
73ad0d0
Compare
This looks awesome, can't wait to see it in a release! |
@TheRealAmazonKendra Could you please review this PR again? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #29646 +/- ##
=======================================
Coverage 78.46% 78.46%
=======================================
Files 106 106
Lines 7208 7208
Branches 1323 1323
=======================================
Hits 5656 5656
Misses 1364 1364
Partials 188 188
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #24785.
Reason for this change
HTTP API supports AWS service integrations with SQS but CDK does not support this.
Description of changes
Added
HttpSqsIntegration
classDescription of how you validated changes
I've added both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license