Add Schema Mutation and Timeout Retrieval Functions #5
Conversation
…ate and delete timeouts (#4)
bendbennett
changed the title
bflad
left a comment
bflad
left a comment
There was a problem hiding this comment.
Overall I think this is on the right track -- I left some comments along the way here to hopefully clarify the public API and logic a little. Please reach out with any questions.
| - uses: actions/setup-go@v3 | ||
| with: | ||
| go-version: ${{ steps.go-version.outputs.version }} | ||
| go-version-file: 'go.mod' |
There was a problem hiding this comment.
Over in #3 this also dropped the step above, as it's no longer necessary.
| with: | ||
| go-version: ${{ steps.go-version.outputs.version }} | ||
| - uses: goreleaser/goreleaser-action@v2 | ||
| go-version-file: 'go.mod' |
There was a problem hiding this comment.
Similarly the above step can be dropped
| - uses: actions/setup-go@v3 | ||
| with: | ||
| go-version: ${{ steps.go-version.outputs.version }} | ||
| go-version-file: 'go.mod' |
There was a problem hiding this comment.
Similarly the above step can be dropped
go.mod
Outdated
|
|
||
| require ( | ||
| github.com/google/go-cmp v0.5.9 | ||
| github.com/hashicorp/terraform-plugin-framework v0.12.1-0.20220913180647-04ff77076480 |
internal/validators/timeduration.go
Outdated
|
|
||
| // Description describes the validation in plain text formatting. | ||
| func (validator timeDurationValidator) Description(_ context.Context) string { | ||
| return "string must be parseable as time.Duration" |
There was a problem hiding this comment.
Nit: Practitioners may not know what a time.Duration is or that provider code is typically Go-based to know to lookup the time package documentation, so I would suggest offering some examples in the description or the returned error diagnostic.
timeouts/schema.go
Outdated
| create = "create" | ||
| read = "read" | ||
| update = "update" | ||
| del = "delete" |
There was a problem hiding this comment.
I'm sorry for this OCD, but everything else is written out. 😭
delete = "delete"Maybe this would be better to create these constants with a little more explanation like:
// Attribute name constants
const (
attributeNameCreate = "create"
attributeNameDelete = "delete"
attributeNameRead = "read"
attributeNameUpdate = "update"
)There was a problem hiding this comment.
I used delete to avoid "Constant 'delete' collides with the 'builtin' function" notification in my IDE.
Have rename all using attributeName....
README.md
Outdated
| func (r exampleResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) { | ||
| var data exampleResourceData | ||
|
|
||
| diags := req.Config.Get(ctx, &data) |
There was a problem hiding this comment.
Nit: I'd always recommend showing fetching from the req.Plan to prevent other provider developer confusion. 😄
README.md
Outdated
| func (r exampleResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) { | ||
| var data exampleResourceData | ||
|
|
||
| diags := req.Config.Get(ctx, &data) |
There was a problem hiding this comment.
Similarly regarding req.Plan
|
|
||
| // Although the schema validation guarantees that the type for create timeout | ||
| // is parseable as a time.Duration, this function accepts any types.Object. | ||
| duration, err := time.ParseDuration(createTimeout.(types.String).Value) |
There was a problem hiding this comment.
Should these return early without error if the types.String is null?
There was a problem hiding this comment.
Have added checks and return default if types.String is null.
bflad
left a comment
bflad
left a comment
There was a problem hiding this comment.
This looks good to me 🚀 Let's ship a v0.1.0 😄
|
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. |
2 participants
Closes: #4