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

Validate default template.json #2855

Merged
merged 8 commits into from May 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions samcli/commands/_utils/options.py
Expand Up @@ -14,7 +14,7 @@
from samcli.commands._utils.custom_options.option_nargs import OptionNargs
from samcli.commands._utils.template import get_template_artifacts_format

_TEMPLATE_OPTION_DEFAULT_VALUE = "template.[yaml|yml]"
_TEMPLATE_OPTION_DEFAULT_VALUE = "template.[yaml|yml|json]"
DEFAULT_STACK_NAME = "sam-app"

LOG = logging.getLogger(__name__)
Expand All @@ -35,15 +35,15 @@ def get_or_default_template_file_name(ctx, param, provided_value, include_build)

original_template_path = os.path.abspath(provided_value)

search_paths = ["template.yaml", "template.yml"]
search_paths = ["template.yaml", "template.yml", "template.json"]
Copy link
Contributor

Choose a reason for hiding this comment

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

something to check, if build is included, we always the convert the built template back to yaml (IIRC) and that is referenced template if one is not provided during the deploy command, can we check this behavior?

Copy link
Contributor

@aahung aahung May 6, 2021

Choose a reason for hiding this comment

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

something to check, if build is included, we always the convert the built template back to yaml (IIRC)

If sam build is executed, template.json will be rewritten into .aws-sam/build/template.yaml. And if sam package/deploy is later executed, .aws-sam/build/template.yaml should be picked up. Nice to double check this behavior.

@sriram-mv so we want the priority in sam local and sam package/deploy to be:

  1. .aws-sam/build/template.yaml
  2. ./template.yaml
  3. ./template.yml
  4. ./template.json

is that accurate?

Copy link
Contributor Author

@ssenchenko ssenchenko May 6, 2021

Choose a reason for hiding this comment

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

@sriram-mv Yes, when we run build, template.json is converted to template.yaml in .aws-sam/build after sam build execution. During deploy command that .aws-sam/build/template.yaml from the build directory is used by default. When we run validate in a project with build directory, template.json from the root directory is validated by default. I believe it works as expected

Copy link
Contributor

Choose a reason for hiding this comment

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

ok sounds good, it (template.json) should be the last thing to look up in order to avoid any breaking changes or unwanted behavior from this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something I just realised. the parser is still a yaml_parser.

yaml_helper.py

def yaml_parse(yamlstr):
    """Parse a yaml string"""
    try:
        # PyYAML doesn't support json as well as it should, so if the input
        # is actually just json it is better to parse it with the standard
        # json parser.
        return json.loads(yamlstr, object_pairs_hook=OrderedDict)
    except ValueError:
        yaml.SafeLoader.add_constructor(yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG, _dict_constructor)
        yaml.SafeLoader.add_multi_constructor("!", intrinsics_multi_constructor)
        return yaml.safe_load(yamlstr)

if you look at the comment, it suggest use a json parser instead.


if include_build:
search_paths.insert(0, os.path.join(".aws-sam", "build", "template.yaml"))

if provided_value == _TEMPLATE_OPTION_DEFAULT_VALUE:
# "--template" is an alias of "--template-file", however, only the first option name "--template-file" in
# ctx.default_map is used as default value of provided value. Here we add "--template"'s value as second
# default value in this option, so that the command line paramerters from config file can load it.
# default value in this option, so that the command line parameters from config file can load it.
if ctx and ctx.default_map.get("template", None):
provided_value = ctx.default_map.get("template")
else:
Expand Down
15 changes: 15 additions & 0 deletions tests/integration/testdata/validate/default_json/template.json
@@ -0,0 +1,15 @@
{
"AWSTemplateFormatVersion": "2010-09-09",
"Transform": "AWS::Serverless-2016-10-31",

"Resources": {
"HelloWorldFunction": {
"Type": "AWS::Serverless::Function",
"Properties": {
"CodeUri": "hello-world/",
"Handler": "app.lambdaHandler",
"Runtime": "nodejs14.x"
}
}
}
}
10 changes: 10 additions & 0 deletions tests/integration/testdata/validate/default_yaml/template.yaml
@@ -0,0 +1,10 @@
AWSTemplateFormatVersion: '2010-09-09'
Transform: AWS::Serverless-2016-10-31

Resources:
HelloWorldFunction:
Type: AWS::Serverless::Function
Properties:
CodeUri: HelloWorldFunction
Handler: app.lambdaHandler
Runtime: nodejs14.x
@@ -0,0 +1,15 @@
{
"AWSTemplateFormatVersion": "2010-09-09",
"Transform": "AWS::Serverless-2016-10-31",

"Resources": {
"HelloWorldFunction": {
"Type": "AWS::Serverless::Function",
"Properties": {
"CodeUri": "hello-world/",
"Handler": "app.lambdaHandler",
"Runtime": "nodejs14.x"
}
}
}
}
@@ -0,0 +1,10 @@
AWSTemplateFormatVersion: '2010-09-09'
Transform: AWS::Serverless-2016-10-31

Resources:
HelloWorldFunction:
Type: AWS::Serverless::Function
Properties:
CodeUri: HelloWorldFunction
Handler: app.lambdaHandler
Runtime: nodejs14.x
@@ -0,0 +1,10 @@
AWSTemplateFormatVersion: '2010-09-09'
Transform: AWS::Serverless-2016-10-31

Resources:
HelloWorldFunction:
Type: AWS::Serverless::Function
Properties:
CodeUri: HelloWorldFunction
Handler: app.lambdaHandler
Runtime: nodejs14.x
15 changes: 15 additions & 0 deletions tests/integration/testdata/validate/with_build/template.json
@@ -0,0 +1,15 @@
{
"AWSTemplateFormatVersion": "2010-09-09",
"Transform": "AWS::Serverless-2016-10-31",

"Resources": {
"HelloWorldFunction": {
"Type": "AWS::Serverless::Function",
"Properties": {
"CodeUri": "hello-world/",
"Handler": "app.lambdaHandler",
"Runtime": "nodejs14.x"
}
}
}
}
Empty file.
75 changes: 75 additions & 0 deletions tests/integration/validate/test_validate_command.py
@@ -0,0 +1,75 @@
"""
Integration tests for sam validate
"""

import os
import re
from enum import Enum, auto
from pathlib import Path
from typing import List, Optional
from unittest import TestCase
from unittest.case import skipIf

from parameterized import parameterized
from tests.testing_utils import RUN_BY_CANARY, RUNNING_ON_CI, RUNNING_TEST_FOR_MASTER_ON_CI, run_command

# Validate tests require credentials and CI/CD will only add credentials to the env if the PR is from the same repo.
# This is to restrict package tests to run outside of CI/CD, when the branch is not master or tests are not run by Canary
SKIP_VALIDATE_TESTS = RUNNING_ON_CI and RUNNING_TEST_FOR_MASTER_ON_CI and not RUN_BY_CANARY


class TemplateFileTypes(Enum):
JSON = auto()
YAML = auto()


@skipIf(SKIP_VALIDATE_TESTS, "Skip validate tests in CI/CD only")
class TestValidate(TestCase):
@classmethod
def setUpClass(cls):
cls.patterns = {
TemplateFileTypes.JSON: re.compile(r"^/.+/template[.]json is a valid SAM Template$"),
TemplateFileTypes.YAML: re.compile(r"^/.+/template[.]yaml is a valid SAM Template$"),
}

@staticmethod
def base_command() -> str:
return "samdev" if os.getenv("SAM_CLI_DEV") else "sam"

def command_list(
self,
template_file: Optional[Path] = None,
profile: Optional[str] = None,
region: Optional[str] = None,
config_file: Optional[Path] = None,
) -> List[str]:
command_list = [self.base_command(), "validate"]
if template_file:
command_list += ["--template-file", str(template_file)]
if profile:
command_list += ["--profile", profile]
if region:
command_list += ["--region", region]
if config_file:
command_list += ["--config_file", str(config_file)]
return command_list

@parameterized.expand(
[
("default_yaml", TemplateFileTypes.YAML), # project with template.yaml
("default_json", TemplateFileTypes.JSON), # project with template.json
("multiple_files", TemplateFileTypes.YAML), # project with both template.yaml and template.json
(
"with_build",
TemplateFileTypes.JSON,
), # project with template.json and standard build directory .aws-sam/build/template.yaml
]
)
def test_default_template_file_choice(self, relative_folder: str, expected_file: TemplateFileTypes):
test_data_path = Path(__file__).resolve().parents[2] / "integration" / "testdata" / "validate"
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know / operator can be used in Path, wow!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it's nice to have operator overloading 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

is the "/" similar to using pathlib.join ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I tested, it is

process_dir = test_data_path / relative_folder
command_result = run_command(self.command_list(), cwd=str(process_dir))
pattern = self.patterns[expected_file] # type: ignore
output = command_result.stdout.decode("utf-8")
self.assertEqual(command_result.process.returncode, 0)
self.assertRegex(output, pattern)
13 changes: 12 additions & 1 deletion tests/unit/commands/_utils/test_options.py
Expand Up @@ -53,7 +53,18 @@ def test_must_return_yml_extension(self, os_mock):
def test_must_return_yaml_extension(self, os_mock):
expected = "template.yaml"

os_mock.path.exists.return_value = True
os_mock.path.exists.side_effect = lambda file_name: file_name == expected
ssenchenko marked this conversation as resolved.
Show resolved Hide resolved
os_mock.path.abspath.return_value = "absPath"

result = get_or_default_template_file_name(None, None, _TEMPLATE_OPTION_DEFAULT_VALUE, include_build=False)
self.assertEqual(result, "absPath")
os_mock.path.abspath.assert_called_with(expected)

@patch("samcli.commands._utils.options.os")
def test_must_return_json_extension(self, os_mock):
expected = "template.json"

os_mock.path.exists.side_effect = lambda file_name: file_name == expected
os_mock.path.abspath.return_value = "absPath"

result = get_or_default_template_file_name(None, None, _TEMPLATE_OPTION_DEFAULT_VALUE, include_build=False)
Expand Down