Refactor base package for more reusability #2116
Conversation
fairclothjm
force-pushed
the
mux-provider/configure-and-base-schema-updates
branch
from
ad8652e to
ddf65df
Compare
| @@ -89,7 +89,7 @@ func (r *PasswordPolicyResource) Create(ctx context.Context, req resource.Create | |||
| return | |||
| } | |||
|
|
|||
| client, err := client.GetClient(ctx, r.meta, plan.Namespace.ValueString()) | |||
| client, err := client.GetClient(ctx, r.Meta(), plan.Namespace.ValueString()) | |||
There was a problem hiding this comment.
I am considering changing the 3rd argument to GetClient to be of type any and require it to not be nil. My concern is that users may incorrectly use GetClient and not pass plan.Namespace.ValueString() which is necessary for properly setting the namespace. Any thoughts?
There was a problem hiding this comment.
Oh, I remember I thought about this previously but couldn't come up with a solution since each resource will have it's own Terraform resource data model and GetClient won't know the type. Maybe we could use reflection but this might be ok for now?
There was a problem hiding this comment.
I looked into it a bit and I don't think reflection will do it but it should be possible with generics. But at that point I am inclined to leave this as-is since generics would also require some boilerplate/setup for consumers of GetClient.
There was a problem hiding this comment.
Hmm, I see what you're saying. And these models can't embed iirc from one of your previous PRs. Thinking on this a bit.
There was a problem hiding this comment.
I see the problem you are describing, and I am inclined to say that it is okay as-is for the moment. In the future we can perhaps have different methods like GetClient and GetNSClient to separate and help further ensure that practitioners don't use the methods wrong, and use either function depending on what the value of plan.Namespace.ValueString() is.
GetNSClient could perhaps enforce the behavior you suggested above, where the namespace passed into that method has to be non-nil. For GetClient we could have more leeway and if the provided namespace is empty, we can log to the console that we received an empty namespace and we will be returning the client in the root namespace "" (as long as they have permissions for it).
There was a problem hiding this comment.
I'm also okay with leaving it as is for the moment. Only thing I could think of would be to require each resource data model to implement an interface we define, say ResourceDataModel, with a Namespace() method. The interface would be the type of the third argument like GetClient(ctx context.Context, meta interface{}, model model.ResourceDataModel) and implementation would call its Namespace() method.
One benefit is that it would make developers more aware of the need to include a Namespace field on these data models. One drawback is that it's more boilerplate since we can't embed. I'm not convinced it's the best solution but wanted to note it anyway as I was thinking about this.
There was a problem hiding this comment.
@austingebauer Yeah, I had a similar idea but wanted to avoid more boilerplate for FW resources.
| base.ResourceWithConfigure | ||
| base.WithImportByID |
There was a problem hiding this comment.
Cool, can see how this removes the boilerplate 👍
internal/framework/model/model.go
Outdated
|
|
||
| // ToAPIModel is helper to translate Vault response data to its respective | ||
| // Vault API data model | ||
| func ToAPIModel(data, model any, diagnostics diag.Diagnostics) error { |
fairclothjm
deleted the
mux-provider/configure-and-base-schema-updates
branch
Description
This PR modifies the
basepackage to include the helpersResourceWithConfigureandWithImportByIDwhich can be embedded in Framework Resources so that there is less boilerplate.