-
Notifications
You must be signed in to change notification settings - Fork 101
list: add types for ListResource
support
#1150
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
Conversation
|
||
package schema | ||
|
||
type Schema struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 Clearly, this is a (very) partial implementation of a schema type 😃 In the interest of small pull requests, I'd like to save the details for the next PR.
// Metadata can satisfy both interfaces. | ||
Metadata(context.Context, resource.MetadataRequest, *resource.MetadataResponse) | ||
|
||
// ListResourceConfigSchema should return the schema for list blocks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ConfigValidators
, we have the precedent of a comment about method naming, such as:
// This method name is separate from ConfigValidators in resource and other packages in
// order to allow generic validators.
This method is not named Schema
for a similar reason. To comment or not to comment?
ListResource | ||
|
||
// ListResourceConfigValidators returns a list of functions which will all be performed during validation. | ||
ListResourceConfigValidators(context.Context) []ConfigValidator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ConfigValidators
, we have the precedent of a comment about method naming, such as:
// This method name is separate from ConfigValidators in resource and other packages in
// order to allow generic validators.
This method is not named ConfigValidators
for a similar reason. To comment or not to comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it might be worth being a little more specific in what these validators are used for at some point (i.e. list
config validation when used with terraform query
), then point to the other method name for resource
config validation.
Same for the other methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 – will revisit the comments in a follow-up PR.
ListResource | ||
|
||
// ValidateListResourceConfig performs the validation. | ||
ValidateListResourceConfig(context.Context, ValidateConfigRequest, *ValidateConfigResponse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ConfigValidators
, we have the precedent of a comment about method naming, such as:
// This method name is separate from ConfigValidators in resource and other packages in
// order to allow generic validators.
This method is not named ValidateConfig
for a similar reason. To comment or not to comment?
// The method signature is intended to be compatible with the Metadata | ||
// method signature in the Resource interface. One implementation of | ||
// Metadata can satisfy both interfaces. | ||
Metadata(context.Context, resource.MetadataRequest, *resource.MetadataResponse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A trade-off of compatibility: a bit of overloading of resource.MetadataRequest
and resource.MetadataResponse
. I think it's ok. I'm also open to improving on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense, i briefly mentioned that you could share the interfaces internally if you wanted in our 1/1, but that would mean you couldn't write a descriptive package comment for the method, so not really a big deal to duplicate imo.
// signature is intended to be compatible with the Configure method | ||
// signature in the Resource interface. One implementation of Configure can | ||
// satisfy both interfaces. | ||
Configure(context.Context, resource.ConfigureRequest, *resource.ConfigureResponse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A trade-off of compatibility: a bit of overloading of resource.ConfigureRequest
and resource.ConfigureResponse
. I think it's ok. I'm also open to improving on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am halfway between ListResource
and List
😆 , left some thoughts. I do like the interface design in general 👍🏻
// Copyright (c) HashiCorp, Inc. | ||
// SPDX-License-Identifier: MPL-2.0 | ||
|
||
package list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A thought about the package location. Since list is coupled to managed resource (i.e. can't list without some managed resource existing in the provider), should we move all of these packages under resource
?
resource/list
resource/list/schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😃 I started from resource
and ended up in a top-level list
package.
I took a cue from: an instance of a "list resource" corresponds to a "list block" in Terraform.
So from a config perspective, list
feels okay at the level of resource
and ephemeral
. And the import paths feel simpler.
And we leave the door open for our understanding of list
to evolve.
Schema: c.Schema, | ||
TerraformValue: c.Raw, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note for later, eventually we'll need some form of Set*
methods here. Typically providers are always supplementing data, i.e., we already created the object, populated some config data in, the provider "sets" the other attributes in the state, then we return it to Terraform.
Here, it might be a little different, we are creating the objects from scratch, so maybe different helpers can assist with that.
// The method signature is intended to be compatible with the Metadata | ||
// method signature in the Resource interface. One implementation of | ||
// Metadata can satisfy both interfaces. | ||
Metadata(context.Context, resource.MetadataRequest, *resource.MetadataResponse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense, i briefly mentioned that you could share the interfaces internally if you wanted in our 1/1, but that would mean you couldn't write a descriptive package comment for the method, so not really a big deal to duplicate imo.
✅ I've removed the TODO comment (012fb27). |
I'm 👍 on this. Shipping it.
I think: [1] yes, they are two names for the same "underlying type," [2] I'd like to de-duplicate if possible, [3] maybe they are not aliases – one should not be assignable to the other ( 💭 What if we extract an unexported type?
|
For consistency within the |
ListResource | ||
|
||
// ListResourceConfigValidators returns a list of functions which will all be performed during validation. | ||
ListResourceConfigValidators(context.Context) []ConfigValidator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it might be worth being a little more specific in what these validators are used for at some point (i.e. list
config validation when used with terraform query
), then point to the other method name for resource
config validation.
Same for the other methods
No changelog entry for the moment. We'll write a feature-level changelog entry 🔜. |
Description
This pull request adds a top-level
list
package.list.ListResource
is the Framework concept that defines alist
block type, just asephemeral.EphemeralResource
defines anephemeral
block type andresource.Resource
defines aresource
block type.A list resource always returns instances of a single managed resource type. For example, a
random_pet
list resource can only return instances of arandom_pet
managed resource type.A developer can choose to implement the
list.ListResource
interface with an existingresource.Resource
. This is intended to allow for cohesion between list resource logic and with managed resource CRUD logic.A developer can instead choose to implement the
list.ListResource
interface separately from an existingresource.Resource
. This is intended to allow for decoupling list resource logic from managed resource CRUD logic.To allow for response data that exceeds the client's or server's maximum gRPC message size, the Plugin Protocol specifies a
stream
response for theListResource
RPC. Accordingly, the handler's result type is namedlist.ListResourceStream
and it includes an iterator function of typeiter.Seq[ListResourceEvent]
.Rollback Plan
Changes to Security Controls
None.