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

CloudFormation: Implement Fn::GetAtt for AWS::EC2::LaunchTemplate #7121

Merged
merged 5 commits into from Dec 17, 2023

Conversation

skinitimski
Copy link
Contributor

@skinitimski skinitimski commented Dec 13, 2023

Purpose

  • Add support for getting attributes of AWS::EC2::LaunchTemplate resources.
  • Fix bug. Sorta. The real bug is that moto's cloudformation parsing does not honor dependencies between resources -- instead it retries resource updates that raise errors (up to 5 times). This is fine if the update raises errors, but if no error is raised, the updates may process out of order. This was discovered while writing tests for the previous bullet. In our case, we have an AutoScalingGroup that depends on a LaunchTemplate. If the updates to the AutoScalingGroup are handled first, it picks up the current/old version of the LaunchTemplate instead of the new one (because the new one is not created yet). Honoring dependency order appears hard (probably why it is not yet implemented). As a fallback, this PR makes the order of updates deterministic, so that at least if you put the resources in the right order in your template, they will be updated in that order. This is done by using lists rather than sets. The result should be the same -- since previously we were taking the set of a dict, the collection by definition has unique elements (that is, I cannot find any discernable difference between set(some_dict) and list(some_dict) except that the latter preserves order).

@skinitimski
Copy link
Contributor Author

FAILED tests/test_ec2/test_launch_templates_cloudformation.py::test_asg_with_default_launch_template_version - botocore.exceptions.ClientError: An error occurred (InvalidLaunchTemplateName.AlreadyExistsException) when calling the CreateStack operation: Launch template name already in use.

Heh, that sounds like the bug I am working on in another branch! Stay tuned.

@bblommers
Copy link
Collaborator

This looks good to me so far @skinitimski. I cannot see the test results for some reason, so I'll just close and re-open this PR to force-start them.

Did you mean to open the other PR against our repo?

@bblommers bblommers closed this Dec 15, 2023
@bblommers bblommers reopened this Dec 15, 2023
Copy link

codecov bot commented Dec 16, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e422e95) 95.88% compared to head (5ed9d37) 95.88%.
Report is 4 commits behind head on master.

Files Patch % Lines
moto/ec2/models/launch_templates.py 75.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7121      +/-   ##
==========================================
- Coverage   95.88%   95.88%   -0.01%     
==========================================
  Files         834      834              
  Lines       82202    82215      +13     
==========================================
+ Hits        78819    78828       +9     
- Misses       3383     3387       +4     
Flag Coverage Δ
servertests 35.79% <25.00%> (-0.01%) ⬇️
unittests 95.82% <75.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

I'll merge this as is, because it already looks ready to me.

Thank you for adding this to Moto @skinitimski!

@bblommers bblommers changed the title Implement Fn::GetAtt for AWS::EC2::LaunchTemplate CloudFormation: Implement Fn::GetAtt for AWS::EC2::LaunchTemplate Dec 17, 2023
@bblommers bblommers added this to the 4.2.12 milestone Dec 17, 2023
@bblommers bblommers merged commit 692b475 into getmoto:master Dec 17, 2023
31 of 32 checks passed
@skinitimski
Copy link
Contributor Author

I cannot see the test results for some reason, so I'll just close and re-open this PR to force-start them.

Did you mean to open the other PR against our repo?

😅 GitHub is confusing. I had trouble with both things. I have opened the right PR: #7135

@skinitimski skinitimski deleted the feature/timski-lt-getcfnattr branch December 18, 2023 18:04
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.

None yet

3 participants