search

buildkite / terraform-provider-buildkite

Terraform provider for Buildkite
https://registry.terraform.io/providers/buildkite/buildkite/latest
MIT License
56 stars 34 forks source link

SUP-1996: Fix infinite drift with Team Description #531

Closed tomowatt closed 4 months ago

tomowatt commented 4 months ago

GraphQL Responses for Team Resources include the description, when it is not set it will be returned as description: "", currently there is no set a default value when the attribute is not set, so in State it will be nil and when compared for a Plan, will result in a infinite drift for the Team Resource.

jradtilbrook commented 4 months ago

Maybe a little clearer:

If the provider detects that a user has specified description = null, we would send that to the API, but receive a "" back. We would then know to ignore the empty string and put null into state to not trigger tf validation error. And I think for these purposes null === "".

And for the opposite where description is omitted from the tf resource, the API would do the same thing and return "", in this case we would have to put either null or the special not-set value into state (I forget which).

tomowatt commented 4 months ago

I'm not sure I covered this in our pairing but it does look like you've done the right thing. But just to confirm, did you modify the .graphql file and then run make generate? We shouldn't touch the generated.go file as that is handled by the genqlient library for us based on all the .graphql files

Yeah I did, only committing the specific changes related for this PR though after running make generate as there was a lot more and didn't want to make the pull request harder to review, but can add in them on top of this to keep it all up to date.

Otherwise, I think this approach looks okay. So it will change behaviour to add in a default for the description that is an empty string. That should stop the drift as it will always be defined in that case. However, this will cause all customers who upgrade to get a plan with lots of updates (I think). We should check to confirm that and should try to avoid that where possible as terraform output is already verbose enough, and this type of an output is kind of redundant really.

Tested this locally using a Team Resource that was previously defined and had "description": null in the State for the description attribute and there was no update to the Team Resource in any Plans/Applies after running them, and I believe (correct me if I'm wrong) this due to making the attribute Computed so the Provider updates it under the hood and saw that in the State as well, where became "description": "" after an terraform apply

I'm not sure if you tried it, but I think there is a way for the provider to detect the difference between specified and null and not specified at all. If we can hook into that, we should be able to handle the API returning empty strings instead of null and basically ignore it (not put it into state). I think with this approach, the upgrade wont cause diffs for users

I have been looking into this as well as noticed there was other Resources that used the *string data type and haven't been reported as having infinite drift but was not able to repeat the same logic - putting this down to lack of experience - but will check again as I'm with you that it whilst this PR is not a drastic change overall, it might be sorted more subtly.

jradtilbrook commented 4 months ago

@tomowatt okay awesome - very thorough!