Implement Using Custom Types for Timeouts #18

bendbennett · 2022-12-07T12:42:07Z

Closes: #17

Bumps [github.com/hashicorp/terraform-plugin-framework](https://github.com/hashicorp/terraform-plugin-framework) from 0.16.0 to 0.17.0. - [Release notes](https://github.com/hashicorp/terraform-plugin-framework/releases) - [Changelog](https://github.com/hashicorp/terraform-plugin-framework/blob/main/CHANGELOG.md) - [Commits](hashicorp/terraform-plugin-framework@v0.16.0...v0.17.0) --- updated-dependencies: - dependency-name: github.com/hashicorp/terraform-plugin-framework dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>

…e use of type-specific schema and attributes

…chema-specific functions to add a nested blocks or nested attributes into a schema (#17)

bflad

We should likely drop the existing tfsdk implementations in the same version too. 😄

bflad · 2022-12-12T18:11:43Z

timeouts/type/timeouts.go

+	return t.getTimeout(ctx, attributeNameDelete)
+}
+
+func (t TimeoutsValue) getTimeout(_ context.Context, timeoutName string) (time.Duration, error) {


I would recommend switching this to diag.Diagnostics over error so its more friendly to use in provider code. 👍

bflad · 2022-12-12T18:53:01Z

timeouts/datasource/schema.go

+	if opts.Create {
+		attrTypes[attributeNameCreate] = types.StringType
+	}


Not due to this pull request, but data source schemas should only support a read timeout.

bflad · 2022-12-12T18:55:53Z

timeouts/type/timeouts.go

@@ -0,0 +1,120 @@
+package timeouts


I wonder if this should be promoted to the top level timeouts package so provider developers do not have an awkward type import if they do not import alias it?

I did this initially and then thought that it would be useful to have timeouts value that only has the relevant functions for the schema type that it is being used with. For instance, data source should only have access to the timeouts Read function.

I think we need some different naming from Type and Value though. Be interested to hear your thoughts.

…ts functions (#17)

…to resource and data source specific implementations (#17)

bendbennett · 2022-12-14T07:34:15Z

We should likely drop the existing tfsdk implementations in the same version too. 😄

Have dropped all usages of tfsdk. It might be worth noting that there is no prior released version that contains deprecation notices for the tfsdk implementations.

bflad

😅 Apparently I had this comment queued up but didn't submit it.

bflad · 2022-12-14T16:58:10Z

README.md

+```go
+import (
+    /* ... */
+    "github.com/hashicorp/terraform-plugin-framework-timeouts/timeouts/resource"


I think it'd be good to avoid the naming collision with the github.com/hashicorp/terraform-plugin-framework/resource package for these and in Go its generally good practice to have the underlying package name match the end of the import path to prevent confusion -- to that end, I'd suggest either github.com/hashicorp/terraform-plugin-framework-timeouts/resource/timeouts or github.com/hashicorp/terraform-plugin-framework-timeouts/resourcetimeouts

bflad

A diagnostics usability question, but otherwise optimistically approving this. ✅

.changelog/18.txt

bflad · 2022-12-19T21:09:16Z

datasource/timeouts/timeouts.go

+	value, ok := t.Object.Attributes()[timeoutName]
+	if !ok {
+		diags.Append(diag.NewErrorDiagnostic(
+			"Timeout Does Not Exist",
+			fmt.Sprintf("timeout for %q does not exist", timeoutName),
+		))
+
+		return defaultTimeout, diags
+	}


How do you feel about the prior implementation that accepted a default timeout duration inline? I think returning an error diagnostic here could be problematic for provider implementations which elsewhere in the framework do not need to conditionally choose when to append diagnostics to their response diagnostics. I'd expect something like the following to "always work" (configured or not):

timeout, diags := data.Timeouts.Read(ctx) // or Read(ctx, 20*time.Minute) resp.Diagnostics(diags...)

If we wanted to make this nicer for troubleshooting, I'd suggest passing context.Context through and logging using tflog.Info(ctx, timeoutName + " timeout configuration not found, using provided default")

I think the super cool thing here is that once we have a custom type for "Go time durations", we can likely remove the diag.Diagnostics return completely. 😄

bflad · 2022-12-19T21:12:15Z

datasource/timeouts/timeouts_test.go

+
+// equateErrorMessage reports errors to be equal if both are nil
+// or both have the same message.
+var equateErrorMessage = cmp.Comparer(func(x, y error) bool {


Is this necessary? I haven't come across needing anything special necessary with go-cmp comparing diag.Diagnostics, but if there is something special it'd be good to figure it out.

Hmmmm..........have just checked and following the most recent code changes this no longer seems to be required.

bflad · 2022-12-19T21:16:23Z

README.md

+The following illustrates nested block syntax for defining timeouts on a resource and a data source.

-```terraform
+```hcl


Using terraform for code blocks should be valid in GitHub: https://github.com/github/linguist/blob/c1c34e5260797b4d598f5ec76f19723bfc5a1894/lib/linguist/languages.yml#L2529-L2544 (and especially helpful should the language ever be split off from generic hcl formatting)

Co-authored-by: Brian Flad <bflad417@gmail.com>

…t is not in the Terraform configuration (#17)

bflad

Looks good to me 🚀

github-actions · 2023-01-20T02:20:21Z

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.

dependabot bot and others added 6 commits November 30, 2022 19:40

Add new packages for datasourcetimeouts and resourcetimeouts that mak…

aa0bcd6

…e use of type-specific schema and attributes

Add custom type and value for timeouts. Add resource and datasource s…

60c7d37

…chema-specific functions to add a nested blocks or nested attributes into a schema (#17)

Adding Type function to TimeoutsValue (#17)

810210d

Adding changelog entry (#17)

5bc9230

Update README (#17)

f9ecfa2

bendbennett requested a review from a team as a code owner December 7, 2022 12:42

bendbennett changed the base branch from main to dependabot/go_modules/github.com/hashicorp/terraform-plugin-framework-0.17.0 December 7, 2022 12:42

Fix linting (#17)

943ba4c

bflad reviewed Dec 12, 2022

View reviewed changes

bendbennett added 5 commits December 13, 2022 12:33

Making Read the attribute for timeouts on data sources (#17)

97bff15

Moving timeouts Type to top-level and returning diags from get timeou…

7e640ef

…ts functions (#17)

Linting (#17)

7c65b9a

Removing usage of tfsdk.Block and tfsdk.Attribute (#17)

5f87fdb

Removing all usages of tfsdk package and separating type and value in…

60a4f84

…to resource and data source specific implementations (#17)

Bump terraform-plugin-framework to v1.0.0 (#17)

1459b16

bendbennett requested a review from bflad December 15, 2022 16:57

bflad reviewed Dec 15, 2022

View reviewed changes

Restructuring packages (#17)

1dc5f00

bendbennett added the enhancement New feature or request label Dec 16, 2022

bendbennett added this to the v0.3.0 milestone Dec 16, 2022

bendbennett changed the base branch from dependabot/go_modules/github.com/hashicorp/terraform-plugin-framework-0.17.0 to main December 16, 2022 12:14

bookshelfdave added the tf-devex-triage label Dec 19, 2022

bflad approved these changes Dec 19, 2022

View reviewed changes

bendbennett and others added 3 commits December 20, 2022 16:39

Apply suggestions from code review

ea45484

Co-authored-by: Brian Flad <bflad417@gmail.com>

Refactoring to use a supplied default timeout if the requested timeou…

61c354d

…t is not in the Terraform configuration (#17)

Bumping terraform-plugin-framework to v1.0.1 (#17)

6058ceb

bflad approved these changes Dec 20, 2022

View reviewed changes

bendbennett merged commit 638afa4 into main Dec 20, 2022

bendbennett deleted the bendbennett/issues-17 branch December 20, 2022 17:04

github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2023

Implement Using Custom Types for Timeouts #18

Implement Using Custom Types for Timeouts #18

Uh oh!

Conversation

bendbennett commented Dec 7, 2022

Uh oh!

bflad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendbennett Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendbennett commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bflad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bflad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bflad left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bendbennett Dec 14, 2022 •

edited

Loading

bendbennett commented Dec 14, 2022 •

edited

Loading