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

schema: add versioned schemas for all blocks #1

Merged
merged 3 commits into from
Nov 10, 2020
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Nov 4, 2020

This PR is adding core schemas for the following blocks:

  • data
  • locals
  • module
  • output
  • provider
  • resource
  • variable
  • terraform

This addresses majority of hashicorp/terraform-ls#121, hashicorp/terraform-ls#122 and hashicorp/terraform-ls#123

@radeksimko radeksimko force-pushed the f-versioned-schema branch 7 times, most recently from c12c56f to fdf63a2 Compare November 5, 2020 10:17
@radeksimko radeksimko changed the title schema: add versioned schemas for some blocks schema: add versioned schemas for all blocks Nov 5, 2020
@radeksimko radeksimko marked this pull request as ready for review November 6, 2020 18:11
@radeksimko radeksimko requested a review from a team November 6, 2020 18:12
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

Overall LGTM! although I can't vet some of the minute details of the schema as I'm hardly a power user these days 😂 I think you captured it all

Body: &schema.BodySchema{
Attributes: map[string]*schema.AttributeSchema{
"description": {
ValueType: cty.String,
Copy link
Contributor

@appilon appilon Nov 6, 2020

Choose a reason for hiding this comment

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

the Terraform SDK was hesitant to expose the zclconf.cty namespace (or rather, commit to supporting cty as the typesystem for the SDK) so we forked and owned a hashicorp namespaced "cty". I'm assuming though, this project shouldn't concern itself the same way?

Because it's a very specific library, and there could be some reintegration with TF core down the road, I'm assuming that's why it's a good idea to work and expose cty?

Just bringing it up, not really presenting any opinion

Copy link
Member Author

@radeksimko radeksimko Nov 9, 2020

Choose a reason for hiding this comment

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

🤔 That's certainly a good question to ask.

The library is entirely decoupled from Terraform for now and I don't know if it will integrate with core in any way yet, but it's possible that in the future we may want to move functions here and these make very heavy use of cty, so 100% compatibility may be beneficial, if not necessary.

As far as I remember the SDK's main motivation behind using the fork was that SDK has a high number of consumers (providers) and so any change in dependencies and generally anything that can be perceived as breaking change can be painful to overcome.

I don't expect this particular library (terraform-schema) to have that many consumers beyond language server, certainly not as many as the SDK.

This would be a valid question to ask about hcl-lang, but even there I would lean towards keeping the dependency as is, just because I think about hcl-lang as a complimentary library to hcl itself, which does depend on zclconf/go-cty.

"under the given local name. The name is used to refer to this resource from elsewhere in the same " +
"Terraform module, but has no significance outside of the scope of a module."),
Body: &schema.BodySchema{
Attributes: map[string]*schema.AttributeSchema{
Copy link
Contributor

Choose a reason for hiding this comment

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

May not be worth the trouble, but in general the Terraform schema is additive and not breaking. With that being the main case, would be be better to instead mutate/add onto the existing schema definitions you defined in 0.12, instead of copypasting the same schema in multiple places, should there need to be a bugfix to the "base" schema, we will need to make sure every versioned definition get's updated.

Not likely a huge problem now, but could become prone to problems/annoyance later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was kinda torn between the two approaches and I'm still unsure which one is better.

Having entirely "stacked up" schemas, where the latest schema is basically a combination of all stacks below and modelling it that way up to 0.14 would mostly work.

However there are also arguments for some separation:

  • deprecations will happen, no matter how well you plan - so from that perspective the schema is not strictly additive. e.g. provider.version attribute will be removed some future versions
  • readability of the code - the more conditionals are involved, the harder it is to understand what the real latest schema actually is and the more likely it is that someone will misread it

It's fair to say that we will need to change the old schemas soon as we add support for expressions, so to make these changes easier I will try to reduce the repetition. I would really prefer to have a clean 1.0 schema (as we get closer to Terraform v1.0) in its own package though for the reasons mentioned above.

@radeksimko
Copy link
Member Author

I reduced the repetition a bit + I found a small bug in cty, which has a PR here: zclconf/go-cty#76

PTAL @appilon

@radeksimko radeksimko merged commit 5f405b2 into main Nov 10, 2020
@radeksimko radeksimko deleted the f-versioned-schema branch November 10, 2020 07:18
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.

2 participants