Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(apigatewayv2): WebSocketAwsIntegration ignores requestParameters #28859

Closed
wants to merge 3 commits into from
Closed

fix(apigatewayv2): WebSocketAwsIntegration ignores requestParameters #28859

wants to merge 3 commits into from

Conversation

FayezX
Copy link

@FayezX FayezX commented Jan 25, 2024 •
edited by kaizencc
Loading

#28718 missed adding requestParameters to the WebSocketAwsIntegration.

@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jan 25, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team January 25, 2024 07:19
aws-cdk-automation
aws-cdk-automation previously requested changes Jan 25, 2024
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@FayezX FayezX mentioned this pull request Jan 25, 2024
Merged
@FayezX FayezX changed the title Adding requestParameters field to the WebSocketRouteIntegrationConfig Added requestparameters field to the websocketrouteIntegration Jan 25, 2024
@FayezX FayezX changed the title Added requestparameters field to the websocketrouteIntegration added requestparameters field to the websocketrouteIntegration Jan 25, 2024
@FayezX FayezX changed the title added requestparameters field to the websocketrouteIntegration added requestparameters field to the websocketawsintegration Jan 25, 2024
@FayezX FayezX changed the title added requestparameters field to the websocketawsintegration feat(apigatewayv2): added requestparameters field to the websocketawsintegration Jan 25, 2024
@FayezX FayezX changed the title feat(apigatewayv2): added requestparameters field to the websocketawsintegration fix(apigatewayv2): added requestparameters field to the websocketawsintegration Jan 25, 2024
@FayezX FayezX changed the title fix(apigatewayv2): added requestparameters field to the websocketawsintegration feat(apigatewayv2): added requestparameters field to the websocketawsintegration Jan 25, 2024
@FayezX FayezX changed the title feat(apigatewayv2): added requestparameters field to the websocketawsintegration fix(apigatewayv2): added requestparameters field to the websocketawsintegration Jan 25, 2024
kaizencc
kaizencc previously requested changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @FayezX, thanks for the PR! Can we get a unit test to demonstrate that things are getting piped through correctly?

@kaizencc kaizencc changed the title fix(apigatewayv2): added requestparameters field to the websocketawsintegration fix(apigatewayv2): WebSocketAwsIntegration ignores requestParameters Jan 25, 2024
@kaizencc kaizencc added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jan 25, 2024
@GavinZZ GavinZZ self-requested a review January 26, 2024 00:12
@GavinZZ
Copy link
Contributor

GavinZZ commented Jan 26, 2024 •
edited
Loading

@FayezX Thanks for making this fix. I apologize that I missed the property on my last PR. Would be happy to approve once a small unit test is added to silent the linter.

@aws-cdk-automation aws-cdk-automation dismissed their stale review January 26, 2024 21:54

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

GavinZZ
GavinZZ reviewed Jan 26, 2024
View reviewed changes
...s-cdk-testing/framework-integ/test/aws-apigatewayv2-integrations/test/websocket/integ.aws.ts
@@ -42,6 +42,9 @@ webSocketApi.addRoute('$connect', {
integrationUri: `arn:aws:apigateway:${stack.region}:dynamodb:action/PutItem`,
integrationMethod: HttpMethod.POST,
credentialsRole: apiRole,
requestParameters: {
Copy link
Contributor

@GavinZZ GavinZZ Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FayezX This won't be enough unfortunately, you'll need to build this change and run yarn integ <path-to-this-file> --update-on-failed to update the snapshots as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Gavin, I think i will need assistance on this. Unfortunately I don't know this library very well. Can you help out please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

npx lerna run build --scope=@aws-cdk-testing/framework-integ
cd packages/@aws-cdk-testing/framework-integ
yarn integ test/aws-apigatewayv2-integrations/test/websocket/integ.aws.js --update-on-failed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am having some typescript compilation issues and the commands fail for me. Please feel free to add to the pr as you see fit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the command itself is not working. Try yarn integ --update-on-failed instead and it should work for you.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running the command yarn integ --update-on-failed gives me yarn integ --update-on-failed Error: Cannot find module './integ-runner.js'. The command seems to be trying to run /packages/@aws-cdk/integ-runner/bin/integ-runner but cannot find the ./integ-runner.js file and fails.

@FayezX
Copy link
Author

FayezX commented Jan 26, 2024

Hi @kaizencc @GavinZZ can you please take a look to see if this is good or am I missing anything else? Not that familiar with the code base / test strategy.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: ffc12ed
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot dismissed kaizencc’s stale review January 29, 2024 01:36

Pull request has been modified.

@FayezX
Copy link
Author

FayezX commented Jan 29, 2024

Looks like there is another Pr which fixes this. Please take a look here #28905

@FayezX FayezX closed this Jan 29, 2024
@github-actions github-actions bot mentioned this pull request Feb 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GavinZZ GavinZZ GavinZZ left review comments

@kaizencc kaizencc kaizencc left review comments

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

Assignees
No one assigned
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@FayezX @GavinZZ @aws-cdk-automation @kaizencc