Skip to content

fix(apigatewayv2): WebSocketAwsIntegration ignores requestParameters#28859

Closed
FayezX wants to merge 3 commits intoaws:mainfrom
FayezX:patch-1
Closed

fix(apigatewayv2): WebSocketAwsIntegration ignores requestParameters#28859
FayezX wants to merge 3 commits intoaws:mainfrom
FayezX:patch-1

Conversation

@FayezX
Copy link
Copy Markdown

@FayezX FayezX commented Jan 25, 2024

#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
Copy link
Copy Markdown
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 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
Copy link
Copy Markdown
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
Copy Markdown
Member

GavinZZ commented Jan 26, 2024

@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.

integrationUri: `arn:aws:apigateway:${stack.region}:dynamodb:action/PutItem`,
integrationMethod: HttpMethod.POST,
credentialsRole: apiRole,
requestParameters: {
Copy link
Copy Markdown
Member

@GavinZZ GavinZZ Jan 26, 2024

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
Copy Markdown
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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

FayezX commented Jan 29, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

4 participants