-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
r/launch_template: modify default_version #5225
r/launch_template: modify default_version #5225
Conversation
aws/resource_aws_launch_template.go
Outdated
} | ||
} | ||
|
||
if v, vOk := d.GetOk("update_default_version"); vOk && v.(bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe if you update only update_default_version
in your Terraform configuration (even true
to false
), the logic above will always call CreateLaunchTemplateVersion
. We should prevent this behavior and verify this doesn't occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh yes, I'll take a look at this and reply back once it's complete
@bflad made some changes. It looks a bit clunky to me so appreciate any input.
|
Is there any way of speeding up this pull request, if I were to fix the merge conflict would I need to create a new PR including this work? |
ae82070
to
7c7d127
Compare
|
7c7d127
to
e5d52ab
Compare
@kl4w Thanks! |
Is there a release date for this PR ? |
This seems like a critical piece of missing functionality to make launch templates truly useful, please consider integrating. |
Do we know when this will be merged? |
@kl4w Can you bring this branch up to date to see if we can get this merged? |
e5d52ab
to
8a3a042
Compare
rebased
|
@bflad Could you check this PR and merge it if everything looks good? Or just lets us know what else is needed to get this out the door. Thanks! |
hi @kl4w, thank you again for this contribution and your diligence in keeping your PR up-to-date! Given more recent changes in the codebase, do you mind rebasing again? |
70e7cf5
to
60a01b1
Compare
@anGie44 apologies for the late reply. it's been a long while since I've played around with launch_templates as well as this PR. I've rebased and below are the tests and the outputs. Due to how long it's been since I've viewed this PR, I'm not 100% confident in the behaviour and more testing should be done. Happy to add more, I just need some time to refresh my memory. The failure in the test below is due my test AWS account.
|
No worries @kl4w, thanks for commenting back here! overall the behavior looks good, I did a bit of refactoring with what you last committed and updated the test cases to use the new map syntax for Before you just rebased, I ran the tests again and they looked 👍 will re-run them and comment back here if any more changes are needed :) Update: Output of acceptance tests:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kl4w and @anGie44 👋 Thank for you working on this functionality! Its pretty non-trivial since there are multiple things happening during potential updates. I left some initial feedback below with what I would consider the correct behavior here, but please do reach out if you think I'm wrong or if you have any questions. 👍
c24a3cd
to
7104da7
Compare
7104da7
to
4d1f5c1
Compare
b5a1a77
to
b75b3b0
Compare
b75b3b0
to
2be2311
Compare
Output of re-run acceptance tests: @bflad
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, LGTM 🚀
Output from acceptance testing:
--- PASS: TestAccAWSLaunchTemplate_disappears (15.58s)
--- PASS: TestAccAWSLaunchTemplate_capacityReservation_preference (19.99s)
--- PASS: TestAccAWSLaunchTemplate_cpuOptions (20.86s)
--- PASS: TestAccAWSLaunchTemplate_creditSpecification_t2 (21.77s)
--- PASS: TestAccAWSLaunchTemplate_creditSpecification_nonBurstable (21.75s)
--- PASS: TestAccAWSLaunchTemplateDataSource_basic (21.59s)
--- PASS: TestAccAWSLaunchTemplate_basic (21.90s)
--- PASS: TestAccAWSLaunchTemplate_data (21.83s)
--- PASS: TestAccAWSLaunchTemplateDataSource_metadataOptions (22.27s)
--- PASS: TestAccAWSLaunchTemplateDataSource_filter_basic (22.24s)
--- PASS: TestAccAWSLaunchTemplateDataSource_filter_tags (23.14s)
--- PASS: TestAccAWSLaunchTemplate_capacityReservation_target (24.22s)
--- PASS: TestAccAWSLaunchTemplate_tags (31.66s)
--- PASS: TestAccAWSLaunchTemplate_description (32.87s)
--- PASS: TestAccAWSLaunchTemplate_ElasticInferenceAccelerator (33.11s)
--- PASS: TestAccAWSLaunchTemplate_creditSpecification_t3 (20.08s)
--- PASS: TestAccAWSLaunchTemplate_IamInstanceProfile_EmptyConfigurationBlock (15.56s)
--- PASS: TestAccAWSLaunchTemplate_networkInterface_ipv6Addresses (17.96s)
--- PASS: TestAccAWSLaunchTemplate_networkInterface_ipv6AddressCount (18.40s)
--- PASS: TestAccAWSLaunchTemplate_metadataOptions (17.86s)
--- PASS: TestAccAWSLaunchTemplateDataSource_associatePublicIPAddress (42.82s)
--- PASS: TestAccAWSLaunchTemplate_networkInterface (21.59s)
--- PASS: TestAccAWSLaunchTemplate_networkInterfaceAddresses (23.89s)
--- PASS: TestAccAWSLaunchTemplate_placement_partitionNum (25.46s)
--- PASS: TestAccAWSLaunchTemplate_update (51.67s)
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS (52.53s)
--- PASS: TestAccAWSLaunchTemplate_BlockDeviceMappings_EBS_DeleteOnTermination (52.75s)
--- PASS: TestAccAWSLaunchTemplate_hibernation (30.33s)
--- PASS: TestAccAWSLaunchTemplate_EbsOptimized (56.29s)
--- PASS: TestAccAWSLaunchTemplate_defaultVersion (29.33s)
--- PASS: TestAccAWSLaunchTemplate_associatePublicIPAddress (38.54s)
--- PASS: TestAccAWSLaunchTemplate_updateDefaultVersion (32.46s)
--- PASS: TestAccAWSLaunchTemplate_instanceMarketOptions (50.10s)
Added handling of the above use-case, such that latest version is determined at
|
This has been released in version 2.70.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Fixes #4655
Changes proposed in this pull request:
default_version
attribute modifyableupdate_default_version
attributeOutput from acceptance testing: