-
Notifications
You must be signed in to change notification settings - Fork 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
resource/aws_lb: Fix ELBv2 attributes on create #3854
Conversation
Nice catch, looks very good to me. |
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.
a couple of minor comments, otherwise LGTM 👍
@@ -707,7 +707,7 @@ func flattenAwsLbResource(d *schema.ResourceData, meta interface{}, lb *elbv2.Lo | |||
protectionEnabled := (*attr.Value) == "true" | |||
log.Printf("[DEBUG] Setting LB Deletion Protection Enabled: %t", protectionEnabled) | |||
d.Set("enable_deletion_protection", protectionEnabled) | |||
case "enable_http2": |
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.
is the old key still valid, or is this a bug?
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.
bug 🐛
aws/resource_aws_lb_test.go
Outdated
if *attr.Value == value { | ||
return nil | ||
} else { | ||
return fmt.Errorf(`LB attribute %s expected: "%s" actual: "%s"`, key, value, *attr.Value) |
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.
minor you can use %q
rather than %s
to do the same thing as "%s"
:)
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.
TIL!
add acceptance checks on attribute tests
This has been released in version 1.13.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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! |
Fix #3848
Load balancer attributes can only be added after creation with a Modify sdk call. Our Update function does not appear to detect the initial configuration with
HasChange
, usingIsNewResource()
appears to ensure on create the attributes are configured.Specific to HTTP2 our internal state would have been wrong as the attribute keystring was incorrect (used our variable name and not amazon's strange dot notation). Also I noticed the acceptance tests were only checking internal state, as a more explicit check we now read the alb attributes and check the returned value.
My first initial implementation of the acceptance check is very inefficient for aggregate tests that want to validate multiple attributes we are wasting time fetching and iterating over and over. If feedback from others supports it I contemplated an acceptance tests that accepts a map or list of expected values and can check all of them in one pass.
Tests: