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

[bug] suppress gzip diff unless fields are explicitly set #441

Merged
merged 2 commits into from
Jul 14, 2021

Conversation

smaeda-ks
Copy link
Contributor

@smaeda-ks smaeda-ks commented Jul 10, 2021

We recently made some changes to our Gzip API endpoint.

Previously, the endpoint accepted an empty value for content_types and extensions parameters (ie., "" is stored in DB as the value). But after the change, API will set their default values if an empty value is sent for these parameters from clients.

This results in unexpected (and potentially infinite) diff as the TF state file contains no data for these parameters but the API returns different data upon refresh.

Since Terraform Plugin SDK currently does not support a way to configure a Default for TypeList/TypeSet, the only workaround seems to be ignoring these fields in the API response IF those values are not set explicitly in the TF file by the user. Or, we can also make these fields Required (+ disallow empty value) even though they're optional in the API spec.

This PR also introduces a downside where remote changes for those particular fields will be ignored unless users set them in TF file.

For example, with this gzip block setting:

  gzip {
    name = "Test gzip"
  }

and if I change content_types or extensions value on the UI, the subsequent TF runs won't report any diff. But I think that's acceptable.

@smaeda-ks
Copy link
Contributor Author

smaeda-ks commented Jul 10, 2021

@Integralist This fixes the failing TestAccFastlyServiceV1_gzips tests, but needs some discussion...

do some hack vs breaking change (change them to Required)

Copy link
Collaborator

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

Thanks @smaeda-ks !

remote changes for those particular fields will be ignored unless users set them in TF file.

I think this is acceptable considering users should be using TF to manage these resources.

/cc @bengesoff would be good to get your review and thoughts.

@bengesoff
Copy link
Contributor

This looks like a good temporary fix although I think we should move to replace it with the breaking change when we can. Other than that, I agree with @Integralist's comments and think it looks good otherwise 👌🏼

- more code comments
- use block scope for added special handling logic
@smaeda-ks
Copy link
Contributor Author

Thank you @Integralist and @bengesoff.
I've applied your feedback.

@smaeda-ks smaeda-ks requested a review from Integralist July 12, 2021 16:34
@Integralist Integralist merged commit 9782f3d into fastly:main Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants