Ensure schema.Set uses custom SetDiff algorithm

[Integralist]

Background: we implemented SetDiff to identify when two elements across distinct sets have the same key. This helps avoid recreating an element (i.e. DELETE, then CREATE) which only needs an UPDATE.

Problem: not all schema.Set objects are using the SetDiff algorithm.

Caveats: there is a separate concern with some resources being deleted/recreated. Specifically resources that require a lookup key of "name" and who are also expecting the name field to be updated (e.g. updating the name of an ACL). This will be addressed in a separate PR.

Notes: I've not updated block_fastly_waf_configuration_v1_active_rule.go as it appears to be doing some kind of filtering of data which would be incompatible with the SetDiff implementation. Nor have I updated the block_fastly_waf_configuration_v1_exclusion.go as the nested structuring of the resources was quite complicated to follow and so it was safer not to mess with it for now.

[triblondon]

I'm not qualified to comment on this so if Patrick can review that would be really appreciated!

[phamann]

Hey @Integralist. Thanks for doing this, its been a very long-running need in the project!

I started reviewing but soon realised you haven't added the Update code path for any of the resources just Add and Delete, hence using the current implementation any updates to resources will not be applied and is the core benefit of moving to this pattern - this may also be why you have failing tests.

Use the backend block as the example, specifically these lines: https://github.com/fastly/terraform-provider-fastly/blob/master/fastly/block_fastly_service_v1_backend.go#L75-L92

Hope that helps :)

[Integralist]

FYI @phamann in the latest commit ee842ed I've added some documentation around the caveats I encountered and for which we discussed offline. I didn't want those to get missed in the review as I feel they're important to call out.

[Integralist]

@phamann I've updated the PR description to reflect the files I'm not including (for various reasons).

[Integralist]

@pschulte @bridgetlane 👋🏻 I'm looking for other engineers experienced with either our terraform provider or terraform in general to give a review of this PR.

[phamann]

👍🏻 Looking good - there are a couple of minor questions, some things to double check and some inconsistencies. However approving now to unblock you.

I left most comments inline, however a couple of other overarching observations:

• It seems there is some inconsistency when you've chosen to note in the public documentation about the lack of ability to update a name. For instance non of the logging endpoints have this note - Is this intentional? I'd suggest we do this everywhere if we are going to do it.
• The original implementation of this pattern in fastly/block_fastly_service_v1_backend.go abstracted the building of the update struct fields to a buildUpdateBackendInput() method, and you've inlined the logic into Process()- I personally feel the former pattern makes process easier to read/grok and enables us to unit test the update logic with mock data if we want in the future.

[Integralist]

re:

The original implementation of this pattern in fastly/block_fastly_service_v1_backend.go abstracted the building of the update struct fields to a buildUpdateBackendInput() method, and you've inlined the logic into Process()- I personally feel the former pattern makes process easier to read/grok and enables us to unit test the update logic with mock data if we want in the future.

That's reasonable. I've made a ticket for doing this work in a separate PR.