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.

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

samconfig debug level logging fixed; documentation updated #2891

Merged
merged 10 commits into from
May 28, 2021
23 changes: 20 additions & 3 deletions designs/sam-config.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Today users of SAM CLI need to invoke the CLI directly with all parameters suppl

for e.g: `sam build --use-container --debug`

But often, during the lifecycle of building and deploying a serverless application. the same commands get run repeatedly to build, package and deploy, before solidifying into the final application.
But often, during the lifecycle of building and deploying a serverless application. The same commands get run repeatedly to build, package and deploy, before solidifying into the final application.

These CLI commands are often long and have many changing parts.

Expand Down Expand Up @@ -45,16 +45,33 @@ The suite of commands supported by SAM CLI would be aided by looking for a confi

This configuration would be used for specifiying the parameters that each of SAM CLI commands use and would be in TOML format.

Running a SAM CLI command now automatically looks for `samconfig.toml` file and if its finds it goes ahead with parameter passthroughs to the CLI.
Running a SAM CLI command now automatically looks for `samconfig.toml` file and if it finds it, it passes parameter through to the CLI.

Every command which uses parameters from the configuration file, prints out the location of `samconfig.toml` file it parses.

```
sam build
Default Config file location: samconfig.toml
Config file location: /home/xxxxxxxxxx/projects/app-samconfig/samconfig.toml
..
..
..
```

If no configuration file can be found at the project root directory, the command output will contain a warning.

```
sam local invoke -t ./out/build/template.yaml
Config file '/home/xxxxxxxxx/projects/app-samconfig/out/build/samconfig.toml' does not exist
..
..
..
```

Where is my `samconfig.toml`?
---------------------------------

SAM CLI always expects `samconfig.toml` to be in the project root directory (where the template file is located). When neither template nor config file is specified through cli options, both of them are expected to be in the current working directory where SAM CLI command is running. However, if you use `--template-file` to point to the directory without config file, you need to use `--config-file` option to be able to use `samconfig.toml`.

Why `samconfig.toml` not under `.aws-sam` directory?
---------------------------------

Expand Down
20 changes: 10 additions & 10 deletions samcli/cli/cli_config_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,12 @@ def __call__(self, config_path, config_env, cmd_names):

samconfig = SamConfig(config_file_dir, config_file_name)

# Enable debug level logging by environment variable "SAM_DEBUG"
if os.environ.get("SAM_DEBUG", "").lower() == "true":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It never worked unless SAM_DEBUG was set manually apart from --debug flag because click doesn't create/set environment variables

Copy link
Contributor

Choose a reason for hiding this comment

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

--debug also loads from this envvar. In case this callback is executed before callback of --debug, the log level will not be properly set and here the debug log will not be shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to be the case but now it is not. Now --debug callback is executed before. I kept SAM_DEBUG as a back door though. Hopefully, we won't ever need it because it makes UX confusing

LOG.setLevel(logging.DEBUG)

LOG.debug("Config file location: %s", samconfig.path())

if not samconfig.exists():
LOG.debug("Config file '%s' does not exist", samconfig.path())
# bringing samconfig file location up to info level,
# to improve UX and make it clear where we're looking for samconfig file
if samconfig.exists():
click.echo(f"Config file location: {samconfig.path()}")
else:
click.secho(f"Config file '{samconfig.path()}' does not exist", fg="yellow")
return resolved_config

try:
Expand Down Expand Up @@ -236,8 +234,10 @@ def decorator_customize_config_file(f):
config_file_param_decls = ("--config-file",)
config_file_attrs["help"] = (
"The path and file name of the configuration file containing default parameter values to use. "
"Its default value is 'samconfig.toml' in project directory. For more information about configuration files, "
"see: "
"Its default value is 'samconfig.toml' in project root directory. Project root directory is defined by the "
Copy link
Contributor

Choose a reason for hiding this comment

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

Final tweak, I think the help text being in 3rd person makes it sound more uniform, apologies, I didnt note this on my first pass. But I'm approving the PR, feel free to change the help text to be in 3rd person format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing it. Thanks

"template file location. When you use config file and specify --template-file SAM CLI expects samconfig.toml "
"and the template file to be in the same directory. Alternatively, if --config-file is explicitly specified, "
"it can point to a custom samconfig.toml location. For more information about configuration files, see "
"https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/serverless-sam-cli-config.html."
)
config_file_attrs["default"] = "samconfig.toml"
Expand Down
15 changes: 10 additions & 5 deletions samcli/cli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,6 @@ def print_cmdline_args(func):
"""

def wrapper(*args, **kwargs):
if kwargs.get("config_file") and kwargs.get("config_env"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was inaccurate. When the template file was used outside of the project source directory, instead of giving a warning that the config was not found, it output samconfig.toml which created an illusion that the file was found.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with your point that it gives an impression that the file is found. Here it gives the debugging information about the config file and config env. If the file is not found, the debug will show up later. I suggest changing the log to better describe this. Or combine it into the expanded cli parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted this code because I added file directory echo when config file is parsed and because the config env name was already in debug. It didn't get to output without SAM_DEBUG but now it does.

config_file = kwargs["config_file"]
config_env = kwargs["config_env"]
LOG.debug("Using config file: %s, config environment: %s", config_file, config_env)
LOG.debug("Expand command line arguments to:")
cmdline_args_log = ""
for key, value in kwargs.items():
Expand Down Expand Up @@ -115,8 +111,17 @@ def cli(ctx):

The AWS Serverless Application Model extends AWS CloudFormation to provide a simplified way of defining the
Amazon API Gateway APIs, AWS Lambda functions, and Amazon DynamoDB tables needed by your serverless application.

SAM CLI commands run in the project root directory which is the directory with SAM template file
(template.{yml|yaml|json}). If no template file is specified explicitly, SAM CLI looks it up in the current
working directory (where SAM CLI is running).

SAM CLI options can be either passed directly to the commands and/or stored in the config file (samconfig.toml),
which is expected to be in the project root directory by default. It is also possible to specify a custom directory
for the config file if necessary.

You can find more in-depth guide about the SAM specification here:
https://github.com/awslabs/serverless-application-model.
https://github.com/aws/serverless-application-model.
"""
if global_cfg.telemetry_enabled is None:
enabled = True
Expand Down
6 changes: 6 additions & 0 deletions samcli/cli/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,14 @@ def callback(ctx, param, value):
state.debug = value
return value

# NOTE: --debug option should be eager to be evaluated before other parameters and to set log level to DEBUG
# before any other option/parameter processing will require to output debug info.
# Otherwise parameters are evaluated according to their order and if --debug is specified at the end of the command
# some debug output can be lost
# https://click.palletsprojects.com/en/7.x/advanced/#callback-evaluation-order
return click.option(
"--debug",
is_eager=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

We are using is_eager in some other options. Can you please check the callback order is correct if more than one is_eager options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The order is okay. There is no problem in this case because --debug doesn't do anything but sets the log level.
Here is the list of eager options:

expose_value=False,
is_flag=True,
envvar="SAM_DEBUG",
Expand Down
7 changes: 6 additions & 1 deletion samcli/commands/_utils/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,12 @@ def template_click_option(include_build=True):
callback=partial(get_or_default_template_file_name, include_build=include_build),
show_default=True,
is_eager=True,
help="AWS SAM template which references built artifacts for resources in the template. (if applicable)"
help="AWS SAM template which references built artifacts for resources in the template (if applicable). "
"Template file defines the root directory of the project and allows to point SAM CLI to the directory "
"for build, local invocation etc. If template file is not specified explicitly SAM CLI expects it to be "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"for build, local invocation etc. If template file is not specified explicitly SAM CLI expects it to be "
"for build, local invocation, etc. If template file is not specified explicitly SAM CLI expects it to be "

"in the current working directory (where it is running). When you use config file and specify --template-file "
"SAM CLI expects samconfig.toml and the template file to be in the same directory."
Copy link
Contributor

Choose a reason for hiding this comment

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

"When you use the default config file and specify --template-file SAM CLI expects samconfig.toml and the template file to be in the same directory. Alternatively, if --config-file is explicitly specified, it can point to a custom samconfig.toml location"

How does that sound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that's better 😃

"Alternatively, if --config-file is explicitly specified, it can point to a custom samconfig.toml location."
Copy link
Contributor

Choose a reason for hiding this comment

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

"explicitly" seems unnecessary because --config-file, the CLI option, is either specified or not specified. There is no implicitly specified.

Suggested change
"Alternatively, if --config-file is explicitly specified, it can point to a custom samconfig.toml location."
"Alternatively, if --config-file is specified, it can point to a custom samconfig.toml location."

if include_build
else "AWS SAM template file.",
)
Expand Down
2 changes: 2 additions & 0 deletions tests/integration/buildcmd/build_integ_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ def _verify_invoke_built_function(self, template_path, function_logical_id, over
process_execute.process.wait()

process_stdout = process_execute.stdout.decode("utf-8")
if process_stdout.startswith("Config file"):
*_, process_stdout = process_stdout.partition("\n")
Comment on lines +175 to +176
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 need "if" here since "Config file" is always printed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it's not exactly every time. There are commands which don't use it so it's better be on the safe side

self.assertEqual(json.loads(process_stdout), expected_result)


Expand Down
47 changes: 25 additions & 22 deletions tests/integration/buildcmd/test_build_cmd.py
Original file line number Diff line number Diff line change
@@ -1,32 +1,34 @@
import re
import shutil
import sys
import os
"""
Integration tests for Build comand
"""

import logging
import os
import random
from unittest import skipIf
import shutil
import sys
from pathlib import Path
from parameterized import parameterized, parameterized_class
from subprocess import Popen, PIPE, TimeoutExpired
from unittest import skipIf

import pytest

from parameterized import parameterized, parameterized_class
from samcli.lib.utils import osutils
from .build_integ_base import (
BuildIntegBase,
DedupBuildIntegBase,
CachedBuildIntegBase,
BuildIntegRubyBase,
NestedBuildIntegBase,
IntrinsicIntegBase,
)
from tests.testing_utils import (
CI_OVERRIDE,
IS_WINDOWS,
RUNNING_ON_CI,
CI_OVERRIDE,
run_command,
SKIP_DOCKER_TESTS,
SKIP_DOCKER_MESSAGE,
SKIP_DOCKER_TESTS,
run_command,
)

from .build_integ_base import (
BuildIntegBase,
BuildIntegRubyBase,
CachedBuildIntegBase,
DedupBuildIntegBase,
IntrinsicIntegBase,
NestedBuildIntegBase,
)

LOG = logging.getLogger(__name__)
Expand Down Expand Up @@ -188,8 +190,8 @@ def test_unsupported_runtime(self):
LOG.info(cmdlist)
process_execute = run_command(cmdlist, cwd=self.working_dir)
self.assertEqual(1, process_execute.process.returncode)

self.assertIn("Build Failed", str(process_execute.stdout))
output = "\n".join(process_execute.stdout.decode("utf-8").strip().splitlines())
self.assertIn("Build Failed", output)


@skipIf(
Expand Down Expand Up @@ -1141,7 +1143,8 @@ def test_with_wrong_builder_specified_python_runtime(self, use_container):
# This will error out.
command = run_command(cmdlist, cwd=self.working_dir)
self.assertEqual(command.process.returncode, 1)
self.assertEqual(command.stdout.strip(), b"Build Failed")
output = "\n".join(command.stdout.decode("utf-8").strip().splitlines())
self.assertIn("Build Failed", output)

def _verify_built_artifact(self, build_dir, function_logical_id, expected_files):

Expand Down
30 changes: 16 additions & 14 deletions tests/integration/init/test_init_command.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
from unittest import TestCase

from parameterized import parameterized
from subprocess import Popen, TimeoutExpired, PIPE
"""
Integration tests for init command
"""
import os
import shutil
import tempfile
from samcli.lib.utils.packagetype import IMAGE, ZIP

from pathlib import Path
from subprocess import PIPE, Popen, TimeoutExpired
from unittest import TestCase

from parameterized import parameterized
from samcli.lib.utils.packagetype import IMAGE, ZIP

TIMEOUT = 300

Expand Down Expand Up @@ -225,7 +227,7 @@ def test_init_command_no_interactive_missing_name(self):
You can also re-run without the --no-interactive flag to be prompted for required values.
"""

self.assertEqual(errmsg.strip(), "\n".join(stderr.strip().splitlines()))
self.assertIn(errmsg.strip(), "\n".join(stderr.strip().splitlines()))

def test_init_command_no_interactive_apptemplate_location(self):
stderr = None
Expand Down Expand Up @@ -263,7 +265,7 @@ def test_init_command_no_interactive_apptemplate_location(self):
--location
"""

self.assertEqual(errmsg.strip(), "\n".join(stderr.strip().splitlines()))
self.assertIn(errmsg.strip(), "\n".join(stderr.strip().splitlines()))

def test_init_command_no_interactive_runtime_location(self):
stderr = None
Expand Down Expand Up @@ -301,7 +303,7 @@ def test_init_command_no_interactive_runtime_location(self):
--location
"""

self.assertEqual(errmsg.strip(), "\n".join(stderr.strip().splitlines()))
self.assertIn(errmsg.strip(), "\n".join(stderr.strip().splitlines()))

def test_init_command_no_interactive_base_image_location(self):
stderr = None
Expand Down Expand Up @@ -339,7 +341,7 @@ def test_init_command_no_interactive_base_image_location(self):
--location
"""

self.assertEqual(errmsg.strip(), "\n".join(stderr.strip().splitlines()))
self.assertIn(errmsg.strip(), "\n".join(stderr.strip().splitlines()))

def test_init_command_no_interactive_base_image_no_dependency(self):
stderr = None
Expand Down Expand Up @@ -379,7 +381,7 @@ def test_init_command_no_interactive_base_image_no_dependency(self):
You can also re-run without the --no-interactive flag to be prompted for required values.
"""

self.assertEqual(errmsg.strip(), "\n".join(stderr.strip().splitlines()))
self.assertIn(errmsg.strip(), "\n".join(stderr.strip().splitlines()))

def test_init_command_no_interactive_packagetype_location(self):
stderr = None
Expand Down Expand Up @@ -417,7 +419,7 @@ def test_init_command_no_interactive_packagetype_location(self):
--location
"""

self.assertEqual(errmsg.strip(), "\n".join(stderr.strip().splitlines()))
self.assertIn(errmsg.strip(), "\n".join(stderr.strip().splitlines()))

def test_init_command_no_interactive_base_image_no_packagetype(self):
stderr = None
Expand Down Expand Up @@ -455,7 +457,7 @@ def test_init_command_no_interactive_base_image_no_packagetype(self):
You can also re-run without the --no-interactive flag to be prompted for required values.
"""

self.assertEqual(errmsg.strip(), "\n".join(stderr.strip().splitlines()))
self.assertIn(errmsg.strip(), "\n".join(stderr.strip().splitlines()))

def test_init_command_wrong_packagetype(self):
stderr = None
Expand Down Expand Up @@ -489,7 +491,7 @@ def test_init_command_wrong_packagetype(self):
_get_command()
)

self.assertEqual(errmsg.strip(), "\n".join(stderr.strip().splitlines()))
self.assertIn(errmsg.strip(), "\n".join(stderr.strip().splitlines()))


class TestInitWithArbitraryProject(TestCase):
Expand Down