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

force creating new condition if type is changed #518

Merged
merged 3 commits into from
Dec 20, 2021

Conversation

smaeda-ks
Copy link
Contributor

@smaeda-ks smaeda-ks commented Dec 17, 2021

#517

background: Fastly API doens't support updating the type field once it's created.

And because fastly_service_v1 is a mono resource, the usual ForceNew approach doesn't work as it will attempt to re-create a fastly_service_v1 resource as a whole. CustomizeDiff is also not an option either as condition is a nested block.

So we unfortunately implement it in this way, where we silently call DELETE and CREATE if type attribute is changed. Otherwise, call UPDATE.

@smaeda-ks smaeda-ks added the enhancement New feature or request label Dec 17, 2021
@smaeda-ks smaeda-ks self-assigned this Dec 17, 2021
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

The implementation makes sense to me.

Of course it would be preferable for the API to support this feature rather than us implementing this in the Terraform provider (as has been mentioned by others this can lead to fragmentation across API clients).

That said, this feature is currently broken for users. Due to the potential time it would take to get the API to support this, I'm OK seeing this PR merged/released, with the intention that we actively push for the API to be updated and then we can come back and update this logic accordingly.

Question: Are there any downsides/side-effects that I'm not considering that would make us what to tread cautiously before merging?

@smaeda-ks smaeda-ks marked this pull request as ready for review December 20, 2021 15:28
@smaeda-ks
Copy link
Contributor Author

@Integralist

Question: Are there any downsides/side-effects that I'm not considering that would make us what to tread cautiously before merging?

No, I don't expect any side-effects by this change. Thanks

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.

LGTM. Thanks for adding the test.

@smaeda-ks smaeda-ks merged commit 678097f into main Dec 20, 2021
@smaeda-ks smaeda-ks deleted the shohei-update-condition-block branch December 20, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants