-
Notifications
You must be signed in to change notification settings - Fork 3
Add Description to Timeout Attributes #52
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
bflad
left a 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.
Adding attribute descriptions would definitely be great, but a few things to potentially consider while we are doing this. 😄
- The Go documentation is tailored for Go developers, which may differ slightly from the audience of folks reading/writing Terraform configuration:
- When talking about units, it might be good to write out that the "s" unit means "seconds", etc., especially for non-English speakers.
- In general, Terraform resource operations are "long-running" operations over a network where even milliseconds is very unlikely in practice. It may be good to pare down the list to discussing only longer duration units.
- Folks may want to know when each of the timeouts actually apply, such as
readbeing any refresh or planning (with refresh enabled) operation and thedeletevalue only being applied if it is saved into state before the destroy operation occurs. - Is it worth looking into #37 as well? (The answer there may be separately switching the resource attribute value handling to
Defaultif it makes sense)
|
Thank you for PR. I appreciate quick code change. Please, can somebody merge this into main branch? It seems it is "just" waiting for a review. I'm waiting for merge into the main branch. Thx |
|
Please, can you approve the PR? Thx |
|
Hi @jbinko 👋🏻, this PR is still a WIP re: the above comments, but it's not currently in our team's prioritized work (no timeline). If you want to use any of that work before we review/release this PR, you could always install the module from one of the commits on the PR, for example eccc121: # go get github.com/hashicorp/terraform-plugin-framework-timeouts@<commit>
go get github.com/hashicorp/terraform-plugin-framework-timeouts@eccc1210acfdd389ae2bc4027caa54f0a7d46f9d |
|
|
bflad
left a 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.
Overall this looks good but I'm wondering about the unit testing not picking up the Description equality correctly. I'll check that upstream.
| "read": schema.StringAttribute{ | ||
| Optional: true, | ||
| Validators: []validator.String{ | ||
| validators.TimeDuration(), | ||
| }, | ||
| }, |
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.
Hm, wouldn't this also have the default description or am I thinking about this wrong?
| "read": schema.StringAttribute{ | ||
| Optional: true, | ||
| Validators: []validator.String{ | ||
| validators.TimeDuration(), | ||
| }, | ||
| }, |
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.
Similarly here regarding the default description. I wonder if I need to double check the Equal() implementation.
|
It looks like the equality handlers for attributes doesn't handle nested attributes and blocks doesn't handle nested attributes and blocks. Will create an issue upstream and get it patched up. |
Co-authored-by: Brian Flad <bflad417@gmail.com>
Thanks for getting this patched up. Have updated to terraform-plugin-framework |
bflad
left a 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.
Looks good to me 🚀
|
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. |
Closes: #51
Closes: #37