feat(apigateway): WebSocketIntegrationResponse implementation#29661
feat(apigateway): WebSocketIntegrationResponse implementation#29661
WebSocketIntegrationResponse implementation#29661Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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.
| // FIXME change to a warning? | ||
| throw new Error('Setting up integration responses without setting up returnResponse to true will have no effect, and is likely a mistake.'); |
There was a problem hiding this comment.
Not sure which is best here, as far as I can tell the web console (and thus I assume Cfn) allows the user to create responses, with a simple notice:
It does feel like quite a big pitfall though, I don't think an exception is unwarranted
There was a problem hiding this comment.
I feel like a warning would be more suitable here if it's something that's possible to do from the web console, is there a very strong reason you think we should throw an exception instead?
There was a problem hiding this comment.
I can see a stronger reason to allow this in the web console, say if you want to add responses first, before enabling returnResponse. With IaC, you can just block comment both the integration responses and the property. I don't mind just showing a warning, an exception just seemed like it would save people time debugging something that could could be easily missed at deploy time
There was a problem hiding this comment.
Rather than doing this validation after the fact, we should structure these inputs so that they are tightly coupled and so that users can't do this.
There was a problem hiding this comment.
Ultimately, what I think should be done here is to deprecate the returnResponse field and update it to response that takes in a enum like class with two options: DEFAULT, which is what it's doing now, and customResponse() that is a function that creates all the stuff you're creating in this PR. What do you think?
| /** | ||
| * Integration response ID | ||
| */ | ||
| public readonly integrationResponseId?: string; | ||
|
|
There was a problem hiding this comment.
I assume this was added in anticipation of an implementation of integration responses, but serves no purpose and cannot store multiple responses
There was a problem hiding this comment.
Technically, we need to deprecate this, not delete it. While it doesn't actually do anything, the removal is a breaking change. This is a stable module so we can't allow it.
There was a problem hiding this comment.
Also, can we get access to this id in this scope? If so, why not just set it?
There was a problem hiding this comment.
My main issue is that there can be multiple integrationResponseId, it would need to be a string[] to be usable. I don't think setting it to the first value is worth the confusion. I can leave it up and just @deprecrate it, but again, I think it's needlessly confusing given it was never implemented
| // FIXME any better way to generate a unique id? | ||
| Names.nodeUniqueId(this.integration.node) + slugify(responseProps.responseKey.key) + 'IntegrationResponse', | ||
| { ...responseProps, integration: this.integration }, | ||
| ); |
There was a problem hiding this comment.
Unsure this is the best way to generate this ID, feedback would be appreciated
|
|
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
I forgot to add, you added some changes that were actually unrelated to this implementation. Can you please separate those out into a separate PR? |
Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>
Pull request has been modified.
…ebsocket-integration-response
|
…ebsocket-integration-response
…ebsocket-integration-response
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This PR is pending for community review now. |
…0622) ### Issue # (if applicable) None ### Reason for this change Unlike the other integrations, `WebSocketMockIntegration` did not have a props interface, and was missing a few properties. This PR does not include integration responses, they are already being implemented in #29661 ### Description of changes * Added `requestTemplates` and `templateSelectionExpression` to the newly created `WebSocketMockIntegrationProps` ### Description of how you validated changes Unit and integration tests were updated ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
|
This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
None as far as I could tell, related to #29562.
Reason for this change
While it is possible to use the L1
CfnIntegrationResponseconstruct, it's not trivial given theWebSocketRouteIntegrationare currently bound to theWebSocketIntegrationon the fly.Description of changes
WebSocketIntegrationResponseconstructWebSocketIntegrations (capable of settingIntegrationResponse) aresponsesconfig prop, as well as aaddResponsemethod. This allows me to check that there are no repeatresponseKeys, and thatreturnResponseis active if there areresponsessetCustomResponseWebSocketRouteabstract class was created to isolateWebSocketLambdaIntegration, which does not support response customizationWebSocketIntegrationResponseKeyhelper class to access common and to generate customresponseKeysDescription of how you validated changes
Unit tests were added/modified, and existing integration files were extended to include responses
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license