Skip to content

Conversation

@fmmirel
Copy link

@fmmirel fmmirel commented Aug 5, 2017

Add new --debug-port|-d flag to the "local invoke" command.
When specified, lambda containers will start in suspended debug mode exposing this port on the localhost.
At this moment, this only works for java8 and all nodejs runtimes.

@fmmirel fmmirel requested a review from PaulMaddox August 5, 2017 15:51
@fmmirel fmmirel changed the title Enabling local invoke debug mode for java and all nodejs runtimes Enabling local invoke debug mode for java8 and all nodejs runtimes Aug 5, 2017
@fmmirel fmmirel self-assigned this Aug 5, 2017
@fmmirel fmmirel requested review from sergiu-terman and removed request for sergiu-terman August 5, 2017 16:10
runtime.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

where do these options come from? Can you link to a reference?

Copy link
Author

@fmmirel fmmirel Aug 5, 2017

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, how about the other values like heapsize, GC etc?

Copy link
Author

Choose a reason for hiding this comment

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

Oh, I thought you meant the debug options. There's a comment about where I got these values immediately after the switch statement:

If you go to that repository you can inspect all of the default lambda container Dockerfiles. For example: https://github.com/lambci/docker-lambda/blob/master/nodejs/run/Dockerfile

start.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add debugging to this as well? We should keep behavior consistent between invoke and start-api

Copy link
Author

@fmmirel fmmirel Aug 5, 2017

Choose a reason for hiding this comment

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

The reason why I didn't add this is the fact that adding the debug flag to start-api will map multiple functions to the same debug port in contrast to invoke, where each function is mapped to its own debug port. That might be fine if the intention is to handle the requests sequentially when debugging (i.e. not have multiple suspended lambda debug containers behind the same start-api server) Debugging one lambda function at a time seems reasonable enough to me but I'd like to get your input on this. EDIT: I went ahead and added support for this since it's low hanging fruit).

If the intention is to handle the requests in parallel, then we need to ask the user for a function:debugPort map. Adding this to the CLI input flags might be a bit messy. The ideal setup would be if the debug port could be grabbed from the actual SAM template, for each function (like the timeout), but this is not the case at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense. We should make a note of it in the documentation (README.md)

@sanathkr
Copy link
Contributor

sanathkr commented Aug 5, 2017

@fmmirel Thanks for doing these changes. This is one of the most useful features of the CLI!

@fmmirel
Copy link
Author

fmmirel commented Aug 5, 2017

@sanathkr Thank you for having a look at the changes! EDIT: I've added the debug mode flag to the start-api method as requested.

runtime.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This will kill the CLI process. What happens to the docker container? You should cleanup right?

Copy link
Author

@fmmirel fmmirel Aug 6, 2017

Choose a reason for hiding this comment

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

The docker container does get removed. r.Cleanup() is called before os.Exit(0) and it takes care of the cleanup process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay. I missed it. Thanks

@sanathkr
Copy link
Contributor

sanathkr commented Aug 6, 2017

Can you also update the README to include a section about debugging?

@fmmirel
Copy link
Author

fmmirel commented Aug 6, 2017

Sure. I'll post an update to the pull request tomorrow morning. EDIT: I added the debug information to the readme file.

Copy link
Contributor

@PaulMaddox PaulMaddox left a comment

Choose a reason for hiding this comment

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

LGTM - one small comment

runtime.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of having this as an anon struct (and map), rather than just the map that was there before?

Copy link
Author

@fmmirel fmmirel Aug 7, 2017

Choose a reason for hiding this comment

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

Hi Paul. Thank you for having a look!

The reason why I altered the previous setup is the fact that I need to use the runtime names in the switch I added in the "getDebugEntrypoint" method. Previously, the runtime names were just keys in a map so they could not be used as const values to compare against.

@fmmirel fmmirel merged commit b86245b into master Aug 7, 2017
@fmmirel fmmirel deleted the dev-debug branch August 7, 2017 08:32
moelasmar referenced this pull request in moelasmar/aws-sam-cli Oct 23, 2020
wchengru referenced this pull request in wchengru/aws-sam-cli Oct 24, 2020
mndeveci referenced this pull request in mndeveci/aws-sam-cli Oct 27, 2020
mndeveci referenced this pull request in mndeveci/aws-sam-cli Nov 23, 2021
mndeveci added a commit that referenced this pull request Nov 14, 2022
* # This is a combination of 2 commits.
# This is the 1st commit message:

Add dotnet7 build method for provided

* Added 'BuildMethod: dotnet7' using 'Runtime:provided'
* 'Runtime:provided' is only for the provided runtime and not the dotnet runtime

Testing with sample SAM template:
 ( See BuildMethod and Runtime below )
...
  HelloWorldFunction:
    Type: AWS::Serverless::Function
    Metadata:
      BuildMethod: dotnet7
    Properties:
      CodeUri: ./src/HelloWorld/
      Handler: HelloWorld::HelloWorld.Function::FunctionHandler
      Runtime: provided
      Architectures:
        - x86_64
...

# This is the commit message #2:

Add dotnet7 build method for provided

* Added 'BuildMethod: dotnet7' using 'Runtime:provided'
* 'Runtime:provided' is only for the provided runtime and not the dotnet runtime

Testing with sample SAM template:
 ( See BuildMethod and Runtime below )
...
  HelloWorldFunction:
    Type: AWS::Serverless::Function
    Metadata:
      BuildMethod: dotnet7
    Properties:
      CodeUri: ./src/HelloWorld/
      Handler: HelloWorld::HelloWorld.Function::FunctionHandler
      Runtime: provided
      Architectures:
        - x86_64
...

* run black reformat

Co-authored-by: Samiullah Mohammed <samiull@amazon.com>
Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com>
Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>
mndeveci added a commit that referenced this pull request Nov 15, 2022
* feat: add dotnet7 build method (#428)

* # This is a combination of 2 commits.
# This is the 1st commit message:

Add dotnet7 build method for provided

* Added 'BuildMethod: dotnet7' using 'Runtime:provided'
* 'Runtime:provided' is only for the provided runtime and not the dotnet runtime

Testing with sample SAM template:
 ( See BuildMethod and Runtime below )
...
  HelloWorldFunction:
    Type: AWS::Serverless::Function
    Metadata:
      BuildMethod: dotnet7
    Properties:
      CodeUri: ./src/HelloWorld/
      Handler: HelloWorld::HelloWorld.Function::FunctionHandler
      Runtime: provided
      Architectures:
        - x86_64
...

# This is the commit message #2:

Add dotnet7 build method for provided

* Added 'BuildMethod: dotnet7' using 'Runtime:provided'
* 'Runtime:provided' is only for the provided runtime and not the dotnet runtime

Testing with sample SAM template:
 ( See BuildMethod and Runtime below )
...
  HelloWorldFunction:
    Type: AWS::Serverless::Function
    Metadata:
      BuildMethod: dotnet7
    Properties:
      CodeUri: ./src/HelloWorld/
      Handler: HelloWorld::HelloWorld.Function::FunctionHandler
      Runtime: provided
      Architectures:
        - x86_64
...

* run black reformat

Co-authored-by: Samiullah Mohammed <samiull@amazon.com>
Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com>
Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>

* update provided with provided.al2

* Update test_build_cmd.py

* Update test_workflow_config.py

* Update aws-lambda-tools-defaults.json

Co-authored-by: Beau Gosse <bgosse@amazon.com>
Co-authored-by: Samiullah Mohammed <samiull@amazon.com>
Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants