Validate default template.json

[ssenchenko]

Which issue(s) does this change fix?

#2355

Why is this change necessary?

Allows users to validate template.json (in json instead of yaml format) without specifying --template argument with a path to the file. Improves user experience

How does it address the issue?

Adding template.json to the list of default file names when looking up a template file.

What side effects does this change have?

None

Checklist

• Add input/output type hints to new functions/methods
• 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
• make update-reproducible-reqs if dependencies were changed
• Write documentation

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

[sriram-mv]

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?

[aahung]

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?

[ssenchenko]

@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

[sriram-mv]

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.

[sriram-mv]

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.

[aahung]

Thanks for adding the tests!

[aahung]

Can we use this way to define the testdata path? So that integration tests can be started from any directory (also using / might not work on Windows)

aws-sam-cli/tests/integration/buildcmd/test_build_cmd.py

Line 1467 in 283e546

test_data_path = Path(__file__).resolve().parents[2].joinpath("integration", "testdata")

[ssenchenko]

Agree, that's more bullet proof. Thanks! Though direct slash / works for windows path as well

[aahung]

oh really? that's cool, does it work in normal CMD as well, apart from WSL?

[ssenchenko]

it works in PowerShell not sure about an archaic cmd though 😄 I haven't been using cmd for a long time. But in this case it will work because python supports both \\ and / for windows paths

[aahung]

LGTM! 🚢

[aahung]

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

[ssenchenko]

yeah, it's nice to have operator overloading 😄

[sriram-mv]

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

[aahung]

yeah I tested, it is

[sriram-mv]

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.

[sriram-mv]

I didnt realize we are calling json.loads() first, I focused on the docstring instead.