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

fix: parse nested yaml GetAtt refs correctly #7220

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

edwardfoyle
Copy link
Contributor

Description of changes

Per the CFN spec: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-getatt.html, the logic for reading YAML templates had a bug where

!GetAtt myResource.output.propName

would be translated into

{
  "Fn::GetAtt": ["myResource", "output", "propName"]
}

instead of

{
  "Fn::GetAtt": ["myResource", "output.propName"]
}

Issue #, if available

Description of how you validated changes

Unit tested

Checklist

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

@edwardfoyle edwardfoyle requested a review from a team as a code owner April 30, 2021 17:58
};
}
// data is a string
const firstPeriodIdx = data.indexOf('.');
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no . in the string, this will generate the wrong output. I'm not sure if it actually matters though since the GetAtt won't work properly.

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 I don't think we need to cover that case here as it's already invalid

@codecov-commenter
Copy link

Codecov Report

Merging #7220 (ff514ac) into master (ec1613e) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7220      +/-   ##
==========================================
- Coverage   52.28%   52.27%   -0.02%     
==========================================
  Files         491      491              
  Lines       25242    25248       +6     
  Branches     4952     4952              
==========================================
  Hits        13199    13199              
- Misses      11091    11097       +6     
  Partials      952      952              
Impacted Files Coverage Δ
packages/amplify-cli-core/src/cfnUtilities.ts 20.00% <0.00%> (-2.45%) ⬇️

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 ec1613e...ff514ac. Read the comment docs.

@attilah attilah merged commit 0b20951 into aws-amplify:master Apr 30, 2021
@siegerts siegerts added the referenced-in-release Issues referenced in a published release changelog label May 4, 2021
@siegerts
Copy link
Contributor

siegerts commented May 4, 2021

👋 Hi, this issue was referenced in the v4.50.2 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v4.50.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
referenced-in-release Issues referenced in a published release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants