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

Migrate tfsdk package Config, Plan, and State into schema package #78

Closed
wants to merge 1 commit into from

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Jul 20, 2021

Reference: #76

These types are tightly coupled to Schema and Attribute handling and migrating them will allow for future enhancements with Attribute plan modifications and validations to reference these types without an import cycle.

Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

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

I think this is fundamentally the right idea, because our Config, State, and Plan types are "schema-rich" - they are the abstraction the framework creates to marry the concepts of "value that comes over the wire from Terraform" and "user-defined schema" to provide a way of accessing those values as values of the schema type.

The validation work adds an irreducible dependency in the other direction, since the schema validation takes the config (and therefore schema) as one of its inputs.

The only way I see to resolve this inherent circularity would be to abstract the schema dependency from the Config type, so instead of

type Config struct {
	Raw    tftypes.Value
	Schema schema.Schema
}

you would have

type Config struct {
	Raw    tftypes.Value
	Schema elsewhere.Typer
}

where Typer is an interface Schema fulfills. Then you'd have to use dependency injection to fill that with the actual Schema type when creating the Schema-specific version of the validation functionality inside the schema package. I don't think this is really feasible given the shape of the APIs we want to offer.

Here's the package dependency graph of main right now:

godepgraph3

Graph of branch f-schema-validation:
godepgraph5

And finally, what happens if you move the proto6 functionality that depends on schema into schema (isolating the circular dependency, so we can ignore proto6):

godepgraph4

I therefore think this PR is the right idea, but we should rename the schema package if we do this, so we don't end up with types schema.Plan, schema.Config, etc, which are semantically off.

The package name should be something that captures the concerns we've grouped together here, i.e. the resource values Terraform sends to us in the protocol, in a "schema-rich" form. I'm struggling to think of anything less abstract than values, vals, tfvals...

@kmoe kmoe requested a review from paddycarver July 22, 2021 16:19
@paddycarver
Copy link
Contributor

I agree with @kmoe that an interface is likely not the route we want here.

I'm wondering if bringing the schema and internal/proto6 packages into the tfsdk don't resolve this. I think, fundamentally, we're no longer able to separate the package that describes the shape of resources (schema) from the package that describes uses values for those resources, because the schema package itself uses those values. schema is going to, unless we contort ourselves weirdly, require access to Config, State, and Plan. and Config, State, and Plan are going to need access to the Schema type to understand the data they're holding. So I think we can't escape having them in the same package. I'm not sure what we gain from keeping those types separate from the resource, data source, and provider interfaces at that point, or why we'd want them separate from the request/response types or the Serve function. I think the Schema type is just more central to the framework than we originally anticipated when we separated it into its own package.

Benefits:

  • Our nested attributes code can be simplified, unexporting many implementation details that were exposed so Serve/proto6 could get to them.
  • If the package gets too large, we can still start drawing semantic boundaries between packages that make sense, and it may be easier to see those boundaries without other boundaries standing in the way.
  • I worry that combining Config/Plan/State with Schema in a separate package of their own will muddy the distinction between describing the shape of a resource/data source/provider and accessing values of that shape, for little gain.

I have a quick sketch of this PR with everything moved to tfsdk, and it compiles (no import cycles) and tests pass, so I think this strategy would work.

@bflad
Copy link
Contributor Author

bflad commented Jul 27, 2021

#77 has the version of this with schema/* -> tfsdk/ and internal/proto6.Schema()/internal/proto6.Attribute() -> tfsdk/

Reference: #76

These types are tightly coupled to Schema and Attribute handling and migrating them will allow for future enhancements with Attribute plan modifications and validations to reference these types without an import cycle.
@bflad bflad force-pushed the td-tfsdk-to-schema branch from 7cd864d to 5a17857 Compare July 27, 2021 16:11
@kmoe
Copy link
Member

kmoe commented Jul 29, 2021

Happy to close in favour of #77

@bflad
Copy link
Contributor Author

bflad commented Jul 29, 2021

We have opted for #77 -- closing this PR.

@bflad bflad closed this Jul 29, 2021
@bflad bflad deleted the td-tfsdk-to-schema branch July 29, 2021 20:45
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants