provider/aws: Change ELB access_logs to list type

[tpounds]

Changes the access_logs type from set to list to makes it easier to compare updates when executing a plan.

[apparentlymart]

Thanks for this, @tpounds!

It seems like you're primarily interested in this because Terraform doesn't do well at rendering diffs for sets. If this change is motivated entirely by diff readability, do you think we should instead try to find a better way for Terraform to present set diffs?

For example, Terraform could, in theory, detect the .# suffix and render the subsequent rows with the collection member prefix hidden, and a more traditional line-oriented diff:

    access_logs.#:       '2' => '2'
        * interval:      "60"
          bucket:        "logs_1"
          bucket_prefix: ""
      --- interval:      "60"
          bucket:        "logs_2"
          bucket_prefix: ""
      +++ interval:      "120"
          bucket:        "logs_2"
          bucket_prefix: ""

The intent here is to indicate that the set contained two items and one of them is unchanged while the other has been replaced by a new item.

Lists could be treated in the same way, since in that case too the indices are not really important. The relative ordering of the items is the main interesting thing in a List diff, so I think you'd just order the items by their prefix and then process the list in a similar way to how you'd diff lines of source code.

Of course changing the diff rendering is a more intense Terraform change, but it'd have the benefit of fixing the sub-optimal rendering across all resources.

What do you think? I'm really just thinking aloud here rather than suggesting that you need to implement this alternative, but thought it was worth discussing whether there's a more general fix we can apply .

[tpounds]

@apparentlymart Yes. My change here and in #5065 is primarily motivated around plan readability. I ran into this today when updating a few ELBs and was initially thrown for a loop because it looked like more was changing that expected.

If this change is motivated entirely by diff readability, do you think we should instead try to find a better way for Terraform to present set diffs?

I think it would be great to think about a longer term solution to improving diff readability for collections. This discussion sort of applies in this situation but mostly because Terraform's schema doesn't have a way to define a single structured type (e.g. Type: TypeStruct). The hack is to put this into a list/set and restrict the size to 1. 😞

For example, Terraform could, in theory, detect the .# suffix and render the subsequent rows with the collection member prefix hidden, and a more traditional line-oriented diff:

Personally, I really like the side-by-side diff comparison since it makes it a lot easier to scan changes line by line at quick glance. I think this starts to get trickier for anything more complex than standard collections of primitive types. I suspect this will start to get kind of messy for anything non-trivial.

Lists could be treated in the same way, since in that case too the indices are not really important. The relative ordering of the items is the main interesting thing in a List diff, so I think you'd just order the items by their prefix and then process the list in a similar way to how you'd diff lines of source code.

I agree. It would be kind of neat to preserve the ordering of the list with side-by-side views and show gaps where items were added or removed.

    => bar
foo => foo
baz => 
zoo => zoo

Of course changing the diff rendering is a more intense Terraform change, but it'd have the benefit of fixing the sub-optimal rendering across all resources.

Agreed but I don't think it's minor surgery. 😄

What do you think? I'm really just thinking aloud here rather than suggesting that you need to implement this alternative, but thought it was worth discussing whether there's a more general fix we can apply .

I think improving the diff capabilities would be awesome. It's probably worthwhile opening a separate design issue to discuss ideas in more detail and/or solicit feedback from the community.

[apparentlymart]

Oh, I totally missed that this is one of those "list of zero or one items" scenarios. In that case, I agree that a list makes more sense than a set because it's clearer to present it as the user having changed the zeroth item rather than adding and removing an item.

But I will take my diff suggestion and make a separate issue for it anyway. 😀

[tpounds]

Sounds good to me! In the meantime let me know if there is anything else I should address to get the zero or one item pull requests (#5065, #5066) merged.

[tpounds]

@phinze Any feedback on this pull request?

[tpounds]

Rebased after #5066 was merged into master.

cc @stack72

[tpounds]

Just noticed this pull request is still open. Anything else I should address to get this merged?

[stack72]

Hi @tpounds,

apologies for not picking this up around the time of #5066 - thanks for the PR. The test run looks as follows:

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSELB_' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSELB_ -timeout 120m
=== RUN   TestAccAWSELB_basic
--- PASS: TestAccAWSELB_basic (26.20s)
=== RUN   TestAccAWSELB_fullCharacterRange
--- PASS: TestAccAWSELB_fullCharacterRange (23.82s)
=== RUN   TestAccAWSELB_AccessLogs
--- PASS: TestAccAWSELB_AccessLogs (77.49s)
=== RUN   TestAccAWSELB_generatedName
--- PASS: TestAccAWSELB_generatedName (27.28s)
=== RUN   TestAccAWSELB_availabilityZones
--- PASS: TestAccAWSELB_availabilityZones (42.71s)
=== RUN   TestAccAWSELB_tags
--- PASS: TestAccAWSELB_tags (44.27s)
=== RUN   TestAccAWSELB_iam_server_cert
--- PASS: TestAccAWSELB_iam_server_cert (31.55s)
=== RUN   TestAccAWSELB_InstanceAttaching
--- PASS: TestAccAWSELB_InstanceAttaching (125.65s)
=== RUN   TestAccAWSELB_HealthCheck
--- PASS: TestAccAWSELB_HealthCheck (26.22s)
=== RUN   TestAccAWSELB_Timeout
--- PASS: TestAccAWSELB_Timeout (26.44s)
=== RUN   TestAccAWSELB_ConnectionDraining
--- PASS: TestAccAWSELB_ConnectionDraining (23.78s)
=== RUN   TestAccAWSELB_SecurityGroups
--- PASS: TestAccAWSELB_SecurityGroups (56.78s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    532.191s

[ghost]

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.