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

Make --no-event the default for local invoke #1477

Merged
merged 3 commits into from
Oct 31, 2019
Merged

Make --no-event the default for local invoke #1477

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d1dd92
Make --no-event the default for local invoke
awood45 Oct 17, 2019
4296d68
Merge develop into no-event branch
awood45 Oct 30, 2019
0812b24
Remove Exception Test for Local Invoke
awood45 Oct 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions samcli/commands/local/invoke/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,17 @@

HELP_TEXT = """
You can use this command to execute your function in a Lambda-like environment locally.
You can pass in the event body via stdin or by using the -e (--event) parameter.
Logs from the Lambda function will be output via stdout.\n
You can pass in an event body using the -e (--event) parameter.
Logs from the Lambda function will be written to stdout.\n
\b
Invoking a Lambda function without an input event
$ sam local invoke "HelloWorldFunction"\n
\b
Invoking a Lambda function using an event file
$ sam local invoke "HelloWorldFunction" -e event.json\n
\b
Invoking a Lambda function using input from stdin
$ echo '{"message": "Hey, are you there?" }' | sam local invoke "HelloWorldFunction" \n
$ echo '{"message": "Hey, are you there?" }' | sam local invoke "HelloWorldFunction" --event - \n
awood45 marked this conversation as resolved.
Show resolved Hide resolved
"""
STDIN_FILE_NAME = "-"

Expand All @@ -31,11 +34,10 @@
"--event",
"-e",
type=click.Path(),
default=STDIN_FILE_NAME, # Defaults to stdin
help="JSON file containing event data passed to the Lambda function during invoke. If this option "
"is not specified, we will default to reading JSON from stdin",
"is not specified, no event is assumed. Pass in the value '-' to input JSON via stdin",
awood45 marked this conversation as resolved.
Show resolved Hide resolved
)
@click.option("--no-event", is_flag=True, default=False, help="Invoke Function with an empty event")
@click.option("--no-event", is_flag=True, default=True, help="DEPRECATED: By default no event is assumed.", hidden=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a plan to remove this option all together?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to that, question is if it's worth breaking people who are already using it. But by making it hidden, we're more or less de facto deleting it. The next step would be to drop values on the floor if someone goes to the trouble of doing --no-no-event or similar.

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 actually add a line somewhere "DEPRECATED" onto stdout, before we start looking for values on stdin. Similair to how python2.7 was deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have --no-no-event defined. So customers shouldn't be doing that at least. (https://click.palletsprojects.com/en/7.x/options/#boolean-flags).

I am ok not deleting it fully. You marked it as DEPRECATED in the help text, which would indicate we would remove it at some point. Maybe the right thing to do is not to that at all, and allow people to explicitly define --no-event. Once this is released, I am not sure why people would type more but it really doesn't hurt anything. If we are planning on removing completely, I would like us to have a way to do that was all. This is a non-blocking comment but worth thinking about.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think if someone explicitly defines --no-event there is no harm done - we're basically making it a no-op regardless, and I believe the deprecation flag removes it from help text anyways.

So perhaps the best way forward is to just leave it in place, but have it rendered inert.

@invoke_common_options
@cli_framework_options
@aws_creds_options
Expand Down Expand Up @@ -116,14 +118,10 @@ def do_cli( # pylint: disable=R0914

LOG.debug("local invoke command is called")

if no_event and event != STDIN_FILE_NAME:
# Do not know what the user wants. no_event and event both passed in.
raise UserException("no_event and event cannot be used together. Please provide only one.")

if no_event:
event_data = "{}"
else:
if event:
event_data = _get_event(event)
else:
event_data = "{}"

# Pass all inputs to setup necessary context to invoke function locally.
# Handler exception raised by the processor for invalid args and print errors
Expand Down
42 changes: 4 additions & 38 deletions tests/unit/commands/local/invoke/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def setUp(self):
self.docker_network = "network"
self.log_file = "logfile"
self.skip_pull_image = True
self.no_event = False
self.no_event = True
self.parameter_overrides = {}
self.layer_cache_basedir = "/some/layers/path"
self.force_image_build = True
Expand Down Expand Up @@ -98,7 +98,7 @@ def test_cli_must_setup_context_and_invoke(self, get_event_mock, InvokeContextMo
@patch("samcli.commands.local.cli_common.invoke_context.InvokeContext")
@patch("samcli.commands.local.invoke.cli._get_event")
def test_cli_must_invoke_with_no_event(self, get_event_mock, InvokeContextMock):
self.no_event = True
self.event = None

ctx_mock = Mock()
ctx_mock.region = self.region_name
Expand All @@ -111,7 +111,7 @@ def test_cli_must_invoke_with_no_event(self, get_event_mock, InvokeContextMock):
ctx=ctx_mock,
function_identifier=self.function_id,
template=self.template,
event=STDIN_FILE_NAME,
event=self.event,
no_event=self.no_event,
env_vars=self.env_vars,
debug_port=self.debug_port,
Expand Down Expand Up @@ -144,44 +144,10 @@ def test_cli_must_invoke_with_no_event(self, get_event_mock, InvokeContextMock):
aws_profile=self.profile,
)

get_event_mock.assert_not_called()
context_mock.local_lambda_runner.invoke.assert_called_with(
context_mock.function_name, event="{}", stdout=context_mock.stdout, stderr=context_mock.stderr
)
get_event_mock.assert_not_called()

@patch("samcli.commands.local.cli_common.invoke_context.InvokeContext")
@patch("samcli.commands.local.invoke.cli._get_event")
def test_must_raise_user_exception_on_no_event_and_event(self, get_event_mock, InvokeContextMock):
self.no_event = True

ctx_mock = Mock()
ctx_mock.region = self.region_name
ctx_mock.profile = self.profile

with self.assertRaises(UserException) as ex_ctx:

invoke_cli(
ctx=ctx_mock,
function_identifier=self.function_id,
template=self.template,
event=self.eventfile,
no_event=self.no_event,
env_vars=self.env_vars,
debug_port=self.debug_port,
debug_args=self.debug_args,
debugger_path=self.debugger_path,
docker_volume_basedir=self.docker_volume_basedir,
docker_network=self.docker_network,
log_file=self.log_file,
skip_pull_image=self.skip_pull_image,
parameter_overrides=self.parameter_overrides,
layer_cache_basedir=self.layer_cache_basedir,
force_image_build=self.force_image_build,
)

msg = str(ex_ctx.exception)
self.assertEqual(msg, "no_event and event cannot be used together. Please provide only one.")

@parameterized.expand(
[
param(FunctionNotFound("not found"), "Function id not found in template"),
Expand Down