chore: run cfn lint on test output #888

keetonian · 2019-04-12T18:16:23Z

Description of changes:
Runs CFN lint on the output files of the tests to make sure that they are valid CFN syntax.

Also fixes a parameter in Serverless::Application from NotificationArns->NotificationARNs. This was previously broken and was a bug.

Description of how you validated changes:
Tests run and pass.

Checklist:

Write/update tests
make pr passes
Update documentation

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

…odel into develop

This reverts commit 5a8b01f.

keetonian · 2019-04-12T18:20:26Z

samtranslator/model/cloudformation.py

@@ -9,7 +9,7 @@ class NestedStack(Resource):
    property_types = {
        'TemplateURL': PropertyType(True, is_str()),
        'Parameters': PropertyType(False, is_type(dict)),
-        'NotificationArns': PropertyType(False, list_of(is_str())),
+        'NotificationARNs': PropertyType(False, list_of(is_str())),


This was a bug- the CFN model had an incorrect property name.

keetonian · 2019-04-12T18:52:04Z

tests/translator/test_translator.py

+        # Only run linter on normal/gov partitions. It errors on china regions
+        if testcase not in LINT_IGNORE_TESTS and partition != 'aws-cn':
+            matches = runner.run()
+        print('cfn-lint ({}): {}'.format(expected_filepath, matches))


This section is really what was added, all the rest is updating output files to conform to lint rules.

keetonian · 2019-04-12T18:53:16Z

Reviewers: look at the test_translator.py file under the tests directory and anything else not under the tests directory. All other things in the tests directory are changes forced by the changes to test_translator.py.

codecov-io · 2019-04-12T20:44:00Z

Codecov Report

Merging #888 into develop will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           develop     #888   +/-   ##
========================================
  Coverage    94.62%   94.62%           
========================================
  Files           69       69           
  Lines         2997     2997           
  Branches       554      554           
========================================
  Hits          2836     2836           
  Misses          85       85           
  Partials        76       76

Impacted Files	Coverage Δ
samtranslator/model/cloudformation.py	`85.71% <ø> (ø)`	⬆️
samtranslator/model/sam_resources.py	`95.5% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92decec...e674b56. Read the comment docs.

keetonian · 2019-04-12T20:54:26Z

tests/translator/test_translator.py

+        else: # deprecation warning catching in py2
+            import warnings
+            with warnings.catch_warnings():
+                warnings.filterwarnings("ignore",category=DeprecationWarning)


CFN lint throws a DeprecationWarning in python2 here for every cfn node object created (possibly dozens per template), and Travis truncates the test output due to the sheer volume of warnings generated. This is to make travis (and the tox command) play nice with these tests and catch the warning.

Travis errors on the output even though the tests pass.

See https://travis-ci.org/awslabs/serverless-application-model/jobs/519380376

"The job exceeded the maximum log length, and has been terminated."

How do we work around this?

keetonian · 2019-04-12T20:57:21Z

tests/translator/test_translator.py

-            if output_value.get("Ref") in deployment_logical_id_dict:
-                output_value["Ref"] = deployment_logical_id_dict[output_value.get("Ref")]
+            if output_value.get("Value").get("Ref") in deployment_logical_id_dict:
+                output_value["Value"]["Ref"] = deployment_logical_id_dict[output_value.get("Value").get("Ref")]


The code was (incorrectly) trying to parse outputs in the following manner:

Outputs: MyOutput: !Ref Deployment34567

I updated it (and the tests) to be correct:

Outputs: MyOutput: Value: !Ref Deployment34567

honglu

Glad this catch a lot of incorrect templates we had!

carvantes · 2019-04-12T21:07:51Z

tests/translator/test_translator.py

        # Only update the deployment Logical Id hash in Py3.
        if sys.version_info.major >= 3:
            self._update_logical_id_hash(expected)
            self._update_logical_id_hash(output_fragment)
+            output_template = cfnlint.decode.cfn_json.load(expected_filepath)
+        else: # deprecation warning catching in py2
+            import warnings


Is it good python style to use import in the middle of the code? Usually those things are at the top of the file I think

Usually they are, but I feel it's fine in this case because we are only using it in py2.

brettstack · 2019-04-13T15:41:06Z

tests/translator/input/api_with_auth_all_minimum.yaml

@@ -32,6 +32,12 @@ Resources:
            Identity:
              Headers:
                - Authorization1
+  MyAuthFn:


Where did these new Resources come from?

The new resources are the resources that are referenced in the template but weren't actually in the template. CFN lint does some resource reference resolution to find broken or missing resource references and dependencies. I added in what was necessary to make all of these templates valid.

keetonian added 6 commits February 25, 2019 19:18

fix: add UpdateReplacePolicy to LayerVersion resource

5a8b01f

Merge branch 'develop' of github.com:awslabs/serverless-application-m…

874d9dc

…odel into develop

Merge branch 'develop' of github.com:awslabs/serverless-application-m…

3e3d14c

…odel into develop

Merge branch 'develop' of github.com:awslabs/serverless-application-m…

1a372c0

…odel into develop

Revert "fix: add UpdateReplacePolicy to LayerVersion resource"

3756aed

This reverts commit 5a8b01f.

run cfn-lint on the translator test output files

24b609f

keetonian changed the title ~~Cfn lint test output~~ chore: run cfn lint on test output Apr 12, 2019

keetonian requested a review from brettstack April 12, 2019 18:16

keetonian commented Apr 12, 2019

View reviewed changes

keetonian closed this Apr 12, 2019

keetonian reopened this Apr 12, 2019

keetonian added 2 commits April 12, 2019 13:27

remove deprecation warning from cfn lint in python 2

4b9c22c

fix py3 error in tests

e674b56

keetonian requested review from carvantes and honglu April 12, 2019 20:41

keetonian commented Apr 12, 2019

View reviewed changes

honglu approved these changes Apr 12, 2019

View reviewed changes

carvantes approved these changes Apr 12, 2019

View reviewed changes

keetonian merged commit d47bb52 into aws:develop Apr 13, 2019

brettstack reviewed Apr 13, 2019

View reviewed changes

keetonian deleted the cfn-lint-test-output branch June 13, 2019 17:52

glaubitz mentioned this pull request Jul 2, 2019

Circular test dependency with cfn-lint #1008

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: run cfn lint on test output #888

chore: run cfn lint on test output #888

keetonian commented Apr 12, 2019

keetonian Apr 12, 2019

keetonian Apr 12, 2019

keetonian commented Apr 12, 2019

codecov-io commented Apr 12, 2019

keetonian Apr 12, 2019

keetonian Apr 12, 2019

keetonian Apr 12, 2019 •

edited

Loading

brettstack Apr 13, 2019

keetonian Apr 12, 2019

honglu left a comment

carvantes Apr 12, 2019

keetonian Apr 13, 2019

brettstack Apr 13, 2019

keetonian Apr 30, 2019

chore: run cfn lint on test output #888

chore: run cfn lint on test output #888

Conversation

keetonian commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keetonian commented Apr 12, 2019

codecov-io commented Apr 12, 2019

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keetonian Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honglu left a 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

keetonian Apr 12, 2019 •

edited

Loading