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

r/lb+elb: Suppress diff for LBs w/ empty name #2314

Merged
merged 2 commits into from
Nov 16, 2017
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Nov 16, 2017

This is to address the following test failures we started observing since upgrading the schema in #2185

=== RUN   TestAccAWSELB_generatesNameForZeroValue
--- FAIL: TestAccAWSELB_generatesNameForZeroValue (7.30s)
    testing.go:503: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        DESTROY/CREATE: aws_elb.foo
          arn:                                   "arn:aws:elasticloadbalancing:us-west-2:*******:loadbalancer/tf-lb-20171116064611950500000001" => "<computed>"
          availability_zones.#:                  "3" => "3"
          availability_zones.2050015877:         "us-west-2c" => "us-west-2c"
          availability_zones.221770259:          "us-west-2b" => "us-west-2b"
          availability_zones.2487133097:         "us-west-2a" => "us-west-2a"
          connection_draining:                   "false" => "false"
          connection_draining_timeout:           "300" => "300"
          cross_zone_load_balancing:             "true" => "true"
          dns_name:                              "tf-lb-20171116064611950500000001-1860373339.us-west-2.elb.amazonaws.com" => "<computed>"
          health_check.#:                        "1" => "<computed>"
          idle_timeout:                          "60" => "60"
          instances.#:                           "0" => "<computed>"
          internal:                              "false" => "<computed>"
          listener.#:                            "1" => "1"
          listener.206423021.instance_port:      "8000" => "8000"
          listener.206423021.instance_protocol:  "http" => "http"
          listener.206423021.lb_port:            "80" => "80"
          listener.206423021.lb_protocol:        "http" => "http"
          listener.206423021.ssl_certificate_id: "" => ""
          name:                                  "tf-lb-20171116064611950500000001" => "<computed>" (forces new resource)
          security_groups.#:                     "1" => "<computed>"
          source_security_group:                 "*******/default_elb_486905dd-70a9-3149-9400-be14d7a94e54" => "<computed>"
          source_security_group_id:              "sg-68ed4611" => "<computed>"
          subnets.#:                             "3" => "<computed>"
          zone_id:                               "Z1H1FL5HABSF5" => "<computed>"
=== RUN   TestAccAWSLB_generatesNameForZeroValue
--- FAIL: TestAccAWSLB_generatesNameForZeroValue (304.46s)
    testing.go:503: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        DESTROY/CREATE: aws_lb.lb_test
          access_logs.#:              "0" => "<computed>"
          arn:                        "arn:aws:elasticloadbalancing:us-west-2:*******:loadbalancer/app/tf-lb-20171116072237141200000001/824b60ad8b68d1b2" => "<computed>"
          arn_suffix:                 "app/tf-lb-20171116072237141200000001/824b60ad8b68d1b2" => "<computed>"
          dns_name:                   "internal-tf-lb-20171116072237141200000001-721876436.us-west-2.elb.amazonaws.com" => "<computed>"
          enable_deletion_protection: "false" => "false"
          idle_timeout:               "30" => "30"
          internal:                   "true" => "true"
          ip_address_type:            "ipv4" => "<computed>"
          load_balancer_type:         "application" => "application"
          name:                       "tf-lb-20171116072237141200000001" => "<computed>" (forces new resource)
          security_groups.#:          "1" => "1"
          security_groups.3318027541: "sg-ae979dd3" => "sg-ae979dd3"
          subnets.#:                  "2" => "2"
          subnets.3559705844:         "subnet-47014d21" => "subnet-47014d21"
          subnets.3908916108:         "subnet-fdf499b5" => "subnet-fdf499b5"
          tags.%:                     "1" => "1"
          tags.Name:                  "TestAccAWSALB_basic" => "TestAccAWSALB_basic"
          vpc_id:                     "vpc-f03c3996" => "<computed>"
          zone_id:                    "Z1H1FL5HABSF5" => "<computed>"

I'm not sure if the behaviour change was intentional (@apparentlymart ?), but I'll just make it work the same way as it did before. Admittedly wanting to create LB with name = "" is rather a workaround for the bug related to conditionals than a real scenario, so we may just remove this eventually once that bug is addressed.

Original PR: hashicorp/terraform#14304

Test Results (after patch)

TF_ACC=1 go test ./aws -v -run="(TestAccAWSLB_generatesNameForZeroValue|TestAccAWSELB_generatesNameForZeroValue)" -timeout 120m
=== RUN   TestAccAWSELB_generatesNameForZeroValue
--- PASS: TestAccAWSELB_generatesNameForZeroValue (70.58s)
=== RUN   TestAccAWSLB_generatesNameForZeroValue
--- PASS: TestAccAWSLB_generatesNameForZeroValue (342.22s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	412.832s

@radeksimko radeksimko added the bug Addresses a defect in current functionality. label Nov 16, 2017
@radeksimko radeksimko added this to the v1.3.0 milestone Nov 16, 2017
@radeksimko radeksimko requested a review from catsby November 16, 2017 08:42
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This workaround seems reasonable until we figure out the root cause.

It'd be good to have a comment in both places here explaining a little about why we're doing this, since I think if I ran across this code in future I'd be confused as to why we'd do such a thing... it's pretty un-idiomatic.

@radeksimko
Copy link
Member Author

Good point, explanation added.

@radeksimko radeksimko merged commit 33df43f into master Nov 16, 2017
@radeksimko radeksimko deleted the b-lb-empty-name-diff branch November 16, 2017 17:21
@ghost
Copy link

ghost commented Apr 10, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants