ec2: Add AWS-managed launch template tags to EC2 instances#116
ec2: Add AWS-managed launch template tags to EC2 instances#116nik-localstack wants to merge 3 commits intolocalstack:localstackfrom
Conversation
nik-localstack
force-pushed
the
fix/ec2/launch-template-tags-on-instance
branch
from
5cb7d12 to
1696f9b
Compare
viren-nadkarni
left a comment
viren-nadkarni
left a comment
There was a problem hiding this comment.
Moto-Ext is frozen for all changes to backend, which this PR is doing. Until the state management work is ongoing though, we may be able to make some changes to Moto backends. That said, wherever possible, we want to prefer adding logic to LocalStack providers. We won't be able to accept any changes after that.
tests/test_ec2/test_instances.py
Outdated
| assert instance["Tags"] == [{"Key": "k", "Value": "v"}] | ||
| finally: | ||
| # Clean up launch template | ||
| ec2_client.delete_launch_template(LaunchTemplateId=template_id) |
There was a problem hiding this comment.
- I think the test is trying to do too many things. It is hard for me to form a mental model of what's happening considering how many parameters there are. Please restructure this into separate tests per scenario.
- Why not use fixture setup/teardown instead of try-finally blocks?
There was a problem hiding this comment.
Good point, I split it in multiple tests.
Fixture with setup / teardown similar to localstack is not straightforward for aws validated tests in moto.
I would prefer to keep things simple here and invest in better testing in localstack side that we have a better setup for this kind of testing. WDYT ?
| "aws:ec2launchtemplate:id": template_version.template.id, | ||
| "aws:ec2launchtemplate:version": str(template_version.number), | ||
| } | ||
| ) |
There was a problem hiding this comment.
Can't this change be made in LS EC2 provider?
There was a problem hiding this comment.
It can, but we won't have access to the template_version that moto actually picks for this instance creation.
viren-nadkarni
left a comment
viren-nadkarni
left a comment
There was a problem hiding this comment.
Thanks for reorganising the tests. I was wondering whether we could get rid of the nested try-finally blocks...
|
|
||
| finally: | ||
| # Clean up launch template | ||
| ec2_client.delete_launch_template(LaunchTemplateId=template_id) |
There was a problem hiding this comment.
It should be possible to add an equivalent of the cleanups fixture from LS to avoid this nesting, no?
2 participants
Motivation
The only way to verify that the correct launch template is used when creating an instance, is through the tags
"aws:ec2launchtemplate:id"and"aws:ec2launchtemplate:version"Changes
"aws:ec2launchtemplate:id"and"aws:ec2launchtemplate:version"tags to an ec2 instance when created from a launch template$Defaultversion instead of$Latest