Skip to content

fix: Avoid creating unnecessary DebugSettings objects when --debug#2750

Merged
aahung merged 4 commits intoaws:developfrom
aahung:fix-2723
Mar 22, 2021
Merged

fix: Avoid creating unnecessary DebugSettings objects when --debug#2750
aahung merged 4 commits intoaws:developfrom
aahung:fix-2723

Conversation

@aahung
Copy link
Contributor

@aahung aahung commented Mar 22, 2021 •
edited
Loading

Which issue(s) does this change fix?

#2723

Why is this change necessary?

The bug is caused by calling LambdaDebugSettings.parse_go_delve_api_version() even the runtime is not go.

How does it address the issue?

By delaying the creation of DebugSettings

after

 ❯❯❯ sam local invoke StockBuyerFunction -d 5858 --debug-args hey                                                                                                           ✘ 2 
Invoking app.lambdaHandler (nodejs12.x)
Ignoring unrecognized arguments: ['hey']. Only "-delveAPI" is supported.
Skip pulling image and use local one: amazon/aws-sam-cli-emulation-image-nodejs12.x:rapid-1.21.1.

Mounting /Users/xinhol/Downloads/test-20210105/nested/.aws-sam/build/StockBuyerFunction as /var/task:ro,delegated inside runtime container
START RequestId: c249bc8b-8266-4021-8761-b3aacf7d7260 Version: $LATEST
Debugger listening on ws://0.0.0.0:5858/b3e8cf95-2f51-4d0b-8e1e-db6ca7340dfc
For help, see: https://nodejs.org/en/docs/inspector
^C%

before

 ❯❯❯ samdev local invoke StockBuyerFunction -d 5858 --debug-args hey                                                                                                      ✘ 130 
Invoking app.lambdaHandler (nodejs12.x)
Skip pulling image and use local one: amazon/aws-sam-cli-emulation-image-nodejs12.x:rapid-1.21.1.

Mounting /Users/xinhol/Downloads/test-20210105/nested/.aws-sam/build/StockBuyerFunction as /var/task:ro,delegated inside runtime container
START RequestId: f3bcb5ff-b8a6-4cec-946d-b8bd71cef91c Version: $LATEST
Debugger listening on ws://0.0.0.0:5858/73d78b5a-3e85-4feb-ba21-cefb99a8c7f1
For help, see: https://nodejs.org/en/docs/inspector
^C^C
 ❯❯❯

What side effects does this change have?

Checklist

  • Add input/output type hints to new functions/methods
  • Write design document (Do I need to write a design document?)
  • Write unit tests
  • Write/update functional tests
  • Write/update integration tests
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aahung aahung requested a review from CoshUS March 22, 2021 17:23
@sriram-mv sriram-mv self-requested a review March 22, 2021 19:51
sriram-mv
sriram-mv reviewed Mar 22, 2021
View reviewed changes
Copy link
Contributor

@sriram-mv sriram-mv left a comment

Choose a reason for hiding this comment

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

Overall looks good, can we add a test to showcase the fix?

@aahung
Copy link
Contributor Author

aahung commented Mar 22, 2021

Overall looks good, can we add a test to showcase the fix?

good idea, added

@aahung aahung requested a review from sriram-mv March 22, 2021 20:14
@aahung aahung merged commit eeb462e into aws:develop Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@CoshUS CoshUS CoshUS approved these changes

@sriram-mv sriram-mv sriram-mv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr/internal

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aahung @CoshUS @sriram-mv