Fix #1463. Allow to expose multiple ports when running in debug mode #1479

sdubov · 2019-10-22T18:11:08Z

Issue #, if available:
#1463

Description of changes:

Add ability to specify multiple ports to be exposed from docker instance when running lambda in debug mode
Update and add new tests to verify multiple debug ports

Checklist:

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
Write documentation

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

* Add ability to specify multiple ports to be exposed from docker instance when running lambda in debug mode * Update and add new tests to verify multiple debug ports

jfuss · 2019-10-22T18:58:20Z

designs/debug_mode_multiple_exposed_ports.md

+
+Success criteria for the change
+-------------------------------
+All ports (comma separated) specified in ``--debug-port`` SAM CLI parameter should be exposed by docker container.


Why comma separated? This seems like a good use-case for multiple options: --debug-port 5600 --debug-port 5601.

Good point. Updated with multiple=true option parameter.

jfuss

Overall, this looks pretty good. Just a couple requests, nothing major.

jfuss · 2019-10-23T20:54:52Z

samcli/commands/local/cli_common/options.py

@@ -95,6 +95,8 @@ def invoke_common_options(f):
                help="When specified, Lambda function container will start in debug mode and will expose this "
                "port on localhost.",
                envvar="SAM_DEBUG_PORT",
+                type=click.INT,


If -d is passed as a string, would this cause any issues? I understand why you would add this but not sure if this affects if the integer was pass as a string ("33333" for example). I want to make sure we don't accidentally break the existing contract here.

I didn't find any specific information on click, but will the debug-port always be a list when passed to our cli methods? I am having a hard time understanding how type in impacts what click will end up passing us.

No issues with passing integer as a string here. Looked to the click parameters type processing logic - it tries to cast a value to integer. Verified it works well with strings unless you specify an invalid value, e.g. "abc" that sounds like an expected behavior.

Looked through click implementation for handling multiple flag - it wraps values into tuple if flag is set and return them or return an empty tuple if no values. So, I would expect we always have tuple after processing --debug-port option. Verified with single and multiple values.

Perfect. Thanks for digging into that.

jfuss · 2019-10-23T21:03:25Z

samcli/local/docker/lambda_container.py

@@ -96,16 +96,21 @@ def _get_exposed_ports(debug_options):
        bind to same port both inside and outside the container ie. Runtime process is started in debug mode with
        at given port inside the container and exposed to the host machine at the same port

-        :param int debug_port: Optional, integer value of debug port
+        :param list(int) debug_port: Optional, List of integer values of debug ports


This doc is wrong (was wrong previously). The parameter passed in is debug_options not the debug_port. Could you update this to be correct?

Sure. Fixed doc here and in DebugContext to specify that debug port is a collection of ports to be exposed.

jfuss · 2019-10-23T21:04:17Z

samcli/local/docker/lambda_container.py

+
+        # container port : host port
+        ports_map = {}
+        for port in debug_options.debug_port:


Can you update the docs for debug_options.debug_port to reflect changes to the type?

jfuss · 2019-10-24T18:03:03Z

samcli/commands/local/lib/debug_context.py

+        """
+        Initialize the Debug Context with Lambda debugger options
+
+        :param tuple(int) debug_port: Collection of debugger ports to be exposed from a docker container


Nit: Should this be debug_ports instead of debug_port

Agreed. I thought about the same naming change. My original thought here was to persist the original CLI option --debug-port down to usages. Updated parameter names from InvokeContext down to DebugContext.

Ya we can keep the option the same (--debug-port). This is just an internal object, so no concerns on the updates.

Will do a re-review here shortly.

jfuss · 2019-10-24T18:04:10Z

@sdubov I had one last comment. It's purely for readability. Everything else looks good to me.

- Update DebugContext parameter from "debug-port" to "debug_ports" to make it clear about the implementation details - Update related parameters - Fix the logic to set port when running Java and other containers - Update documentation

sdubov · 2019-10-24T22:35:17Z

Not sure why make-pr configuration is failed. Checked agent details and did not find any issues from logs.

jfuss · 2019-10-24T22:39:32Z

@sdubov its failing due to a format check. We run a tool called black to make sure everything is formatted the same way throughout the codebase and avoids us having to do things manually. I can pull a update that part tomorrow if you don’t want to set that up.

sdubov · 2019-10-24T23:00:43Z

@jfuss, Thank you for clarifying. No worries. Done with code formatting.

jfuss · 2019-10-25T12:59:41Z

samcli/local/docker/lambda_container.py

        :return list: List containing the new entry points. Each element in the list is one portion of the command.
            ie. if command is ``node index.js arg1 arg2``, then this list will be ["node", "index.js", "arg1", "arg2"]
        """

        if not debug_options:
            return None

-        debug_port = debug_options.debug_port
+        debug_ports = debug_options.debug_ports
+        if not debug_ports:


This check is fine but I think it is not needed. __bool__ on DebugContext is bool(self.debug_ports). This should never execute given the first if statement (if not debug_options).

jfuss · 2019-10-25T13:04:12Z

@sdubov Thanks for contributing the feature and going through and addressing all the feedback!

sdubov · 2019-10-25T19:31:10Z

@jfuss, no problem :) thank you for your feedback and a quick review 👍

Fix aws#1463. Allow to expose multiple ports when running in debug mode

56f4802

* Add ability to specify multiple ports to be exposed from docker instance when running lambda in debug mode * Update and add new tests to verify multiple debug ports

jfuss reviewed Oct 22, 2019

View reviewed changes

Add multiple=true flag for --debug-port option

030b5ae

jfuss requested changes Oct 23, 2019

View reviewed changes

jfuss added the stage/in-review label Oct 23, 2019

Update documentation for DebugContext to reflect latest changes

249ccec

jfuss reviewed Oct 24, 2019

View reviewed changes

sdubov force-pushed the sdubov-multi-debug-ports branch from 01e1044 to a2b9b05 Compare October 24, 2019 21:01

Merge branch 'develop' into sdubov-multi-debug-ports

c4d52e2

Re-format code with Black

5560d22

jfuss reviewed Oct 25, 2019

View reviewed changes

jfuss approved these changes Oct 25, 2019

View reviewed changes

jfuss merged commit 8d82ec0 into aws:develop Oct 25, 2019

franck-schmidlin mentioned this pull request Apr 21, 2020

sam deploy - Unable to upload artifact - SSL validation failed #1930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1463. Allow to expose multiple ports when running in debug mode #1479

Fix #1463. Allow to expose multiple ports when running in debug mode #1479

sdubov commented Oct 22, 2019

jfuss Oct 22, 2019

sdubov Oct 23, 2019

jfuss left a comment

jfuss Oct 23, 2019

sdubov Oct 24, 2019 •

edited

Loading

jfuss Oct 24, 2019

jfuss Oct 23, 2019

sdubov Oct 24, 2019

jfuss Oct 23, 2019

sdubov Oct 24, 2019

jfuss Oct 24, 2019

sdubov Oct 24, 2019

jfuss Oct 24, 2019

jfuss commented Oct 24, 2019

sdubov commented Oct 24, 2019 •

edited

Loading

jfuss commented Oct 24, 2019

sdubov commented Oct 24, 2019

jfuss Oct 25, 2019

jfuss commented Oct 25, 2019

sdubov commented Oct 25, 2019

Fix #1463. Allow to expose multiple ports when running in debug mode #1479

Fix #1463. Allow to expose multiple ports when running in debug mode #1479

Conversation

sdubov commented Oct 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdubov Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfuss commented Oct 24, 2019

sdubov commented Oct 24, 2019 • edited Loading

jfuss commented Oct 24, 2019

sdubov commented Oct 24, 2019

Choose a reason for hiding this comment

jfuss commented Oct 25, 2019

sdubov commented Oct 25, 2019

sdubov Oct 24, 2019 •

edited

Loading

sdubov commented Oct 24, 2019 •

edited

Loading