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

Update README to explain when not to use this library #104

Merged
merged 4 commits into from
Sep 13, 2023

Conversation

alisdair
Copy link
Member

By design, the terraform-json library is not always suitable for every application. Its name and prominence may cause those integrating with Terraform to assume otherwise.

This commit expands the README to attempt to clarify situations where the terraform-json library is not appropriate, explaining why and what to do instead.

By design, the `terraform-json` library is not always suitable for every application. Its name and prominence may cause those integrating with Terraform to assume otherwise.

This commit expands the README to attempt to clarify situations where the `terraform-json` library is not appropriate, explaining why and what to do instead.
@alisdair alisdair self-assigned this Sep 11, 2023
@alisdair alisdair requested review from a team as code owners September 11, 2023 14:37
Copy link

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This seems like a good set of guidelines. 👍

README.md Outdated Show resolved Hide resolved
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Perhaps this is just a question of wording rather than misunderstanding of intentions but as one of the consumers we do expect some level of version compatibility I tried to explain in-line.

The remaining part looks reasonable to me.

I'm still not entirely sure that terraform-json and custom decoders have to be mutually exclusive choice though and that the language in the second paragraph has to be as strong as to recommend against entirely, I would just expect some words of caution.

README.md Outdated
Comment on lines 26 to 30
* **Broad version compatibility**: each version this library represents a specific
snapshot of the [Terraform JSON output format](https://developer.hashicorp.com/terraform/internals/json-format),
and it often slightly lags Terraform itself. If you require compatibility with
multiple Terraform versions, we recommend implementing your own custom decoders
for the parts of the JSON format you need.
Copy link
Member

Choose a reason for hiding this comment

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

We could debate about what broad means here but I'd point out that version compatibility is one of the reasons we use this library in terraform-ls and AFAIK that's also a key feature for https://github.com/hashicorp/terraform-plugin-framework or at least the test framework, which lets end-users (plugin developers) test against multiple versions of Terraform. Similarly we cannot dictate end-users what versions of Terraform should they use when using the LS, although it would be reasonable to ignore EOL versions.

The default behaviour of encoding/json's Unmarshal() is to ignore unknown fields. This aligns with the compatibility promises made for all of Terraform's JSON formats, so strictly speaking: (a) custom decoder and terraform-json shouldn't be either/or choice, and (b) most consumers shouldn't need custom decoder anyway.

TL;DR I'm okay with the library being out of date but I do expect it to be backwards-compatible, i.e. if it supports format version 1.5, it will not fail to decode any earlier versions such as 1.0 or 1.1 (assuming default behaviour of encoding/json decoder).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I've revised this paragraph to hopefully clarify what I meant.

Choose a reason for hiding this comment

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

A crucial design problem for this library is that its "implementation" largely consists of declarative struct tags rather than actual code and so its capabilities are limited by what Go's JSON library implements itself.

This caution is based on real-world experience of folks doing the things enumerated here and having surprising (to them) problems when e.g. there was a new version of Terraform but they didn't upgrade the library. The intent here is only to try to make such situations expected rather than surprising, so that anyone using this library has a better idea of what to expect and what maintenance burden they might be taking on by using it (i.e. the need to promptly upgrade if the format has changed in a significant way).

In the long run I'd like to see us take a fresh look at this library as something whose implementation is fully encapsulated (where the JSON decoding behavior is implementation details rather than allowing callers to directly use encoding/json) and thus can make better guarantees about how things will evolve. But this documentation is intended as damage control in the meantime since naive use if this library has now been the cause of several maintenance problems elsewhere.

To your specific point, I think the way we've defined major and minor format revisions means that what you want should happen "for free": a subsequent minor version of the format should be backward compatible with its predecessors in the same major version and so this library will be able to interpret them as a result of the underlying contract, and not due to anything this library is or is not doing. It remains to be seen how this library could manage to evolve across a major format change (i.e. a 2.0 format) but thankfully we have no current plans to produce one and even if we did it would presumably be opt-in at the Terraform CLI level so that clients can still get a reasonable approximation of the latest 1.x format for some reasonable migration period anyway.

In other words, I don't think this documentation changes anything in ways that your comment is concerned about because that's a question of underlying format compatibility rather than this library's own compatibility. It will suffer backward compatibility problems only if Terraform CLI itself breaks its compatibility promises, which seems unlikely in the foreseeable future.

alisdair and others added 2 commits September 12, 2023 10:37
Co-authored-by: Benjamin Bennett <ben.bennett@hashicorp.com>
@alisdair
Copy link
Member Author

I'm still not entirely sure that terraform-json and custom decoders have to be mutually exclusive choice though

Can you say more on this, and perhaps recommend a change to the wording?

and that the language in the second paragraph has to be as strong as to recommend against entirely, I would just expect some words of caution.

The recommendation against using the library (certainly by default) is the Terraform core team's position. We've seen recent usage which has caused compatibility problems and would rather err on the side of avoiding its usage for cases where its value is ambiguous. This doesn't mean that terraform-ls or other users should migrate away.

As a reminder, the Terraform core team's philosophy about compatibility in general has been to publish and adhere to protocol specifications, rather than to supply and support library implementations. While we are now (jointly) maintaining this library, we want to be clear that this doesn't mean that we recommend its usage in all cases.

README.md Outdated Show resolved Hide resolved
@radeksimko
Copy link
Member

I'm still not entirely sure that terraform-json and custom decoders have to be mutually exclusive choice though

Can you say more on this, and perhaps recommend a change to the wording?

My comment was along the lines of what Martin described above

A crucial design problem for this library is that its "implementation" largely consists of declarative struct tags rather than actual code and so its capabilities are limited by what Go's JSON library implements itself.

With that in mind, I find it hard to imagine someone going as far as re-implementing all these struct tags and adding some compatibility layers as part of the decoder (which is instructed by those struct tags). It would seem easier for that someone to simply contribute updates to the struct tags back here.

If minor changes in the format (which are - as mentioned - aligned with compatibility promises) cause problems to a consumer then I'd argue that's a fundamental design problem at that consumer side either way and custom decoders alone likely won't help solve that design problem.

I do understand the intentions about making this clearer though!

Co-authored-by: Radek Simko <radek.simko@gmail.com>
@alisdair alisdair merged commit e58a208 into main Sep 13, 2023
8 checks passed
@alisdair alisdair deleted the alisdair/should-i-use-this-library branch September 13, 2023 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants