Skip to content

Conversation

@marekaiv
Copy link
Contributor

@marekaiv marekaiv commented Oct 5, 2021

Issue #, if available:
n/a

Description of changes:
Check that AutoPublishCodeSha256 resolves to a string and fail gracefully if it doesn't.

Description of how you validated changes:
Added a unit test to check the failure and ran existing unit tests to confirm there's no regression.

Checklist:

  • Add/update tests using:
    • Correct values - existing tests cover this field
    • Bad/wrong values (None, empty, wrong type, length, etc.)
    • Intrinsic Functions
  • make pr passes
  • Update documentation - not needed
  • Verify transformed template deploys and application functions as expected

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2021

Codecov Report

Merging #2173 (1e710a5) into develop (e7a1496) will increase coverage by 0.70%.
The diff coverage is 97.64%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2173      +/-   ##
===========================================
+ Coverage    93.58%   94.29%   +0.70%     
===========================================
  Files           90       95       +5     
  Lines         6124     6430     +306     
  Branches      1260     1291      +31     
===========================================
+ Hits          5731     6063     +332     
+ Misses         183      169      -14     
+ Partials       210      198      -12     
Impacted Files Coverage Δ
samtranslator/model/lambda_.py 93.10% <ø> (ø)
samtranslator/plugins/globals/globals.py 99.05% <ø> (ø)
samtranslator/translator/logical_id_generator.py 100.00% <ø> (+9.09%) ⬆️
samtranslator/region_configuration.py 77.77% <63.63%> (-22.23%) ⬇️
samtranslator/translator/translator.py 97.26% <90.00%> (+0.07%) ⬆️
samtranslator/swagger/swagger.py 93.65% <91.89%> (+0.28%) ⬆️
samtranslator/model/sam_resources.py 92.19% <96.66%> (+0.95%) ⬆️
samtranslator/__init__.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/dialup.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/feature_toggle.py 100.00% <100.00%> (+12.16%) ⬆️
... and 26 more

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 35ed6d0...1e710a5. Read the comment docs.

if not isinstance(code_sha256, string_types):
raise InvalidResourceException(
self.logical_id,
"AutoPublishCodeSha256 must be a string",
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we support intrinsic Fn::Ref but not Fn::Sub, as shown in the test added.
If I understand correctly, Fn::Sub also resolves to a string. My question is - do we also want to support support Fn::Sub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do want to support Fn::Sub but would prefer to deal with that when we tackle intrinsics support more broadly.

Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

Any reason we didn't add unit tests? :)

@jfuss jfuss merged commit 11f9fef into aws:develop Oct 7, 2021
@marekaiv marekaiv deleted the detect_non_string_codesha256 branch October 7, 2021 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants