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

Reflect v0.15 schema changes #28

Merged
merged 2 commits into from
Mar 22, 2021

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Mar 15, 2021

This reflects a change introduced a while ago in hashicorp/terraform#20949 and also changes introduced as part of hashicorp/terraform#28055, hashicorp/terraform#27699 and hashicorp/terraform#27793

The mentioned PRs do not change anything about the underlying syntax that couldn't be already expressed using the SchemaAttribute.AttributeType, however these changes allow the SDK and providers to communicate some additional metadata that cannot be communicated via "pure" cty.Type, such as sensitivity, or description for each individual attribute.

These new features are not in use by the SDK nor the built-in terraform provider yet (AFAIK), but this allows us to support it when the time comes.

Closes #27

@radeksimko
Copy link
Member Author

Marking this as a draft temporarily until a version bump is decided/implemented per hashicorp/terraform#28050 (comment)

Base automatically changed from master to main March 15, 2021 20:12
@radeksimko
Copy link
Member Author

Version bump implemented in hashicorp/terraform#28115

@radeksimko radeksimko marked this pull request as ready for review March 17, 2021 08:38
@radeksimko
Copy link
Member Author

Given that this package mostly doesn't attempt to interpret the data, I think the job of deeper validation - such as whether AttributeNestedType and AttributeType conflict with each other should be left up to the consumer. I'm open to alternative ideas/opinions though.

@vancluever
Copy link
Contributor

vancluever commented Mar 17, 2021

Updating notes based on internal discussions:

NOW:

  • Root schema has an initial SchemaBlock
    *That block has a set of non-block SchemaAttributes (as the Attributes field) of zero or more length
  • It also has a set of SchemaBlockType (as the NestedBlocks field) of zero or more length.
  • To walk, you need to populate off of Attributes non-recursively, and NestedBlocks recursively.

AFTER:

  • Root schema has an initial SchemaBlock - this is now the only instance of this type in the schema.
  • That block has a set of Attributes of zero or more length
  • NestedBlocks is still used. The existing (above) needs to be honored/co-existed with this workflow.
  • Within a particular SchemaAttribute there, there will either be a SchemaNestedAttributeType (as the AttributeNestedType field) or a cty.Type (as the AttributeType field).
  • If the AttributeNestedType field exists, it needs to be walked for recursive fields.
  • Otherwise it's non-recursive population of the set of SchemaAttributes at the root level.

@@ -38,8 +38,10 @@ func (p *ProviderSchemas) Validate() error {
return errors.New("unexpected provider schema data, format version is missing")
}

if ProviderSchemasFormatVersion != p.FormatVersion {
return fmt.Errorf("unsupported provider schema data format version: expected %q, got %q", PlanFormatVersion, p.FormatVersion)
oldVersion := "0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor assuming that this is going to be logic that will need to be changed in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I assume so, but from what I understand it most likely won't happen until the next major version of Terraform itself.

@radeksimko
Copy link
Member Author

Your notes above look accurate.

FWIW naming-wise I think that calling the root thing a block was and is inaccurate, because "block" includes the header (with type and labels). Calling it a "body" would've been IMO better. This is also how HCL internally refers to these things. But even the docs call it a "block" apparently https://www.terraform.io/docs/cli/commands/providers/schema.html#block-representation

This might be something to address prior to v1 though, certainly not now and not in this PR.

@radeksimko
Copy link
Member Author

btw. I don't seem to have write access and so I can't merge this PR - would you do that for me, please?

@vancluever
Copy link
Contributor

Your notes above look accurate.

FWIW naming-wise I think that calling the root thing a block was and is inaccurate, because "block" includes the header (with type and labels). Calling it a "body" would've been IMO better. This is also how HCL internally refers to these things. But even the docs call it a "block" apparently https://www.terraform.io/docs/cli/commands/providers/schema.html#block-representation

This might be something to address prior to v1 though, certainly not now and not in this PR.

Not opposed to this, I'll leave it to you 🙂 the rationale around most of the field names was to ensure they matched what came in from the JSON, where possible.

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.

Add support for protocol v6 schema changes
2 participants