Massive refactor #237
Massive refactor #237
Conversation
Try and make our provider serving code a little bit more atomic. A first step, unfinished.
Refactor the reflect code, continue refactoring serving.
|
Oh, also, this refactors Plan/Config/State to use new types, hopefully cutting down on the amount of code needed. I deleted the plan/state/config tests, but they'll be replaced with non-duplicated tests for the |
| // Schema is the schema for the Attribute | ||
| AttributeSchema Attribute |
There was a problem hiding this comment.
Drive-by note: I believe ModifyAttributePlanRequest is also used during block plan modification.
| return nil | ||
| } | ||
|
|
||
| func (d *Data) RemoveResource(ctx context.Context) { |
There was a problem hiding this comment.
Drive-by note: In principle, these new "data" handling types feel appropriate to centralize the data logic of the Config/Plan/State handling. However, I'm not sure that we should necessarily get away from having Config, Plan, and State types. This RemoveResource method is a good example of something that should only be implemented with State and not Plan.
I'm wondering if Config, Plan, and State should still exist with encapsulating the underlying data types, e.g. briefly
type Config struct {
ReadOnlyData
}
type Plan struct {
*Data
}
type State struct {
*Data
}
func (s State) RemoveResource(ctx context.Context) {/* ... */}I'm also wondering if this particular change could be done outside the larger refactoring.
| // parseConfig turns a *tfprotov6.DynamicValue and a Schema into a Config. | ||
| func parseConfig(ctx context.Context, dv *tfprotov6.DynamicValue, schema Schema) (ReadOnlyData, diag.Diagnostics) { |
There was a problem hiding this comment.
Drive-by nit: I'd be tempted to name this newReadOnlyData for clarity, unless its updated to return a Config (which I'd still opt for the new/New prefix).
| "github.com/hashicorp/terraform-plugin-framework/diag" | ||
| ) | ||
|
|
||
| func serveGetResourceType(ctx context.Context, provider Provider, typ string) (ResourceType, diag.Diagnostics) { |
There was a problem hiding this comment.
Drive-by question: Is there a particular reason these aren't unexported methods on Provider? e.g.
func (p Provider) getResourceType(ctx context.Context, typ string) (ResourceType, diag.Diagnostics) {/* ... */}| ) | ||
|
|
||
| // IsCreate returns true if the request is creating a resource. | ||
| func serveResourceApplyIsCreate(ctx context.Context, state *Data) bool { |
There was a problem hiding this comment.
Drive-by question: Should this be functionality in terraform-plugin-go?
|
|
|
There's been dozens of pull requests to accomplish even part of this. 😛 This has really served as a valuable reference for various ideas though, so thank you. |
|
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. |
A massive refactoring effort to:
PlanandStateSetAttributemethods do not support setting an untyped nil value #66, Optionally relax requirements of Config.Get, Plan.Get, and State.Get #84)tftypes.Valuetoattr.Value(Reexamine where we convert from Terraform values intoattr.Values. #172)and clean up a few other things along the way. It turns out when you start poking at one of these, the whole ball of yarn starts unraveling. Especially since this is an offshoot of the work I was doing in #52.