-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Only set associate_public_ip_address if it's explicitly set #10157
Conversation
Hey @ryndaniels 👋 This resource has been a good candidate for highlighting some limitations in the Terraform Plugin SDK type system. In EC2 Launch Templates, many of the boolean type fields actually have three states:
To work around this limitation, we introduced a workaround with Is this type of workaround more appropriate in this case? Removing the ability to specify For acceptance testing here, we should try to ensure there is covering of all attribute states (using |
Hello, and thank you for your contribution! This project recently migrated to the standalone Terraform Plugin SDK. While the migration helps speed up future feature requests and bug fixes to the Terraform AWS Provider's interface with Terraform, it has the unfortunate consequence of requiring minor changes to pull requests created using the old SDK. This pull request appears to include the Go import path To resolve this situation without losing any existing work, you may be able to Git rebase your branch against the current master branch (example below); replacing any remaining old import paths with the newer ones. $ git fetch --all
$ git rebase origin/master Another option is to create a new branch from the current master with the same code changes (replacing the import paths), submit a new pull request, and close this existing pull request. We apologize for this inconvenience and appreciate your effort. Thank you for contributing and helping make the Terraform AWS Provider better for everyone. |
1dcd64d
to
5080007
Compare
@bflad it took a minute but I think I've got this working using the |
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.
Other than the testing changes noted below, I think this is good to go. 👍
@@ -1077,6 +1125,80 @@ resource "aws_launch_template" "test" { | |||
} | |||
` | |||
|
|||
const testAccAWSLaunchTemplateConfig_associatePublicIpAddressTrue = ` |
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.
Nit: This can be simplified into a single test configuration function by passing in the parameter, e.g.
func testAccAWSLaunchTemplateConfig_associatePublicIpAddress(associatePublicIpAddress string) string {
return fmt.Sprintf(`
# ...
resource "aws_launch_template" "test" {
name = "network-interface-launch-template"
network_interfaces {
associate_public_ip_address = %[1]s
network_interface_id = "${aws_network_interface.test.id}"
ipv4_address_count = 2
}
}
`, associatePublicIpAddress)
Where that is populated via:
Config: testAccAWSLaunchTemplateConfig_associatePublicIpAddress("true"),
Config: testAccAWSLaunchTemplateConfig_associatePublicIpAddress("false"),
Config: testAccAWSLaunchTemplateConfig_associatePublicIpAddress("null"),
} | ||
|
||
resource "aws_launch_template" "test" { | ||
name = "network-interface-launch-template" |
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.
This name needs to be randomized to prevent issues like:
--- FAIL: TestAccAWSLaunchTemplate_networkInterface (23.50s)
testing.go:635: Step 0 error: errors during apply:
Error: InvalidLaunchTemplateName.AlreadyExistsException: Launch template name already in use.
Combining with the suggested update above:
func testAccAWSLaunchTemplateConfig_associatePublicIpAddress(rName, associatePublicIpAddress string) string {
return fmt.Sprintf(`
# ...
resource "aws_launch_template" "test" {
name = %[1]q
network_interfaces {
associate_public_ip_address = %[2]s
network_interface_id = "${aws_network_interface.test.id}"
ipv4_address_count = 2
}
}
`, rName, associatePublicIpAddress)
This has been released in version 2.41.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! |
Community Note
Closes #6777
Release note for CHANGELOG:
Output from acceptance testing: