search

terraform-coop / terraform-provider-foreman

Terraform provider for Foreman
https://registry.terraform.io/providers/terraform-coop/foreman
Mozilla Public License 2.0
33 stars 31 forks source link

Improve compute attributes #35

Closed Fodoj closed 3 years ago

Fodoj commented 3 years ago

My previous implementation https://github.com/HanseMerkur/terraform-provider-foreman/pull/24 was flawed: it works only with simple flat key-value compute attributes. But there are many more of them - for example, volumes in case of vSphere.

The only proper way to make Terraform deal with complex dynamic structures inside single field is to encode and store them as JSON - this is exactly what is done in this PR. All compute attributes are dumped as JSON, and decoded when needed - there is no way to provide a proper schema, because Foreman supports half a dozen compute providers and each of them has absolutely different accepted compute attributes.

The biggest problem in that case is to deal with the diff. Normally one would want to only send a subset of all compute attributes, like CPU and Memory, and may be some volumes (my use case). But Terraform would fetch all compute attributes from the API, which, in case of vSphere, is dozens of fields. As "compute_attributes" is just a string, any change to it will require replacement of the attribute - this is not a problem for an API call to Foreman, but it's a dirty terraform plan every time you run it.

The solution I have is to CustomizeDiff - I take all compute attributes that were fetched from API, and then merged actually defined attributes into it with the https://github.com/imdario/mergo lib. This makes Terraform generate beautiful diffs, that show only the fields inside JSON that were changed, leaving the ones that are not defined or that did not change out (similar to how TypeMap would work).

I've updated example with how it can work now. Also, terraform import now works for hosts with compute attributes and any number of interfaces alike.

P.S.

Autogenerating docs generated docs not for this PR :-\

Fodoj commented 3 years ago

Any review for this one? :) @lhw ?

lhw commented 3 years ago

Don't have a problem with the code at all. But we will need to up the minor version at least for this change as it can and will break existing installations.

Fodoj commented 3 years ago

I did few tests and unless lots of people relies on the previous implementation that I’ve added just a week before, no too many changes should occur. But I agree that version should be bumped. Thanks for merging!