Fix custom timeout handling under PlanResourceChange #2390
Conversation
Under PlanResourceChange the way custom timeout option is handled is not properly communicating the value to the underlying TF provider. This is now fixed and the existing test made to pass. See also: https://www.pulumi.com/docs/concepts/options/customtimeouts/
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2390 +/- ##
==========================================
+ Coverage 57.08% 57.10% +0.02%
==========================================
Files 368 369 +1
Lines 50535 50593 +58
==========================================
+ Hits 28848 28893 +45
- Misses 20114 20123 +9
- Partials 1573 1577 +4 ☔ View full report in Codecov by Sentry. |
|
https://github.com/pulumi/terraform-plugin-sdk/blob/upstream-v2.33.0/helper/schema/core_schema.go#L320 is the interesting bit that defines the schema for timeouts piecemeal, unless the user already defined "timeouts" attribute or block. I've not verified how this works in TF but would be interesting:
|
This adds cross tests for custom timeouts, comparing our handling of custom timeouts with terraform's. To do that we've exposed the `plan` error in tfcheck. Looks like the TF rules are the following: - If the schema specifies a timeout for the operation, then the user is allowed to override that. - If the schema does not specify a timeout, then the resource is said not to support timeouts. That makes it a runtime error if the user then tries to customise the timeout on the resource. That makes sense conceptually, as not all resources would be implemented in a way which supports timeouts. Related to #2386
| // Send logs or status logs to the user. | ||
| // | ||
| // Logged messages are pre-associated with the resource they are called from. | ||
| type Logger interface { |
There was a problem hiding this comment.
This needs its own package so that it's usable from tfshim/* modules without imposing a circular pacakge dependency build error.
|
The current CI failure is due to TestStacktraceDisplayed being flaky: #2388 |
| impliedType cty.Type, | ||
| ) map[string]any { | ||
| config := map[string]any{} | ||
| for k, v := range configWithoutTimeouts { |
There was a problem hiding this comment.
What is configWithoutTimeouts? Is it the TF config translation of the Pulumi config? A comment explaining that would be helpful.
It would be amazing if configWithoutTimeouts was it's own type, but that's a painful pattern in Go.
There was a problem hiding this comment.
Yeah. I can use ResourceConfig which is what's needed, but I always forget how to construct that. All the helper types aren't quite there yet to be truly usable. Added some commits.
Generally I agree with your sentiment, just qualifying it with this idea that I sweat the signatures a bit less for functions that are private file-local single-use functions, e.g. the smaller the scope the less I worry about this. But I'm quite open to efforts to raise the bar on code quality in this codebase so if you have plans here happy to play along or follow a style guide.
pkg/tfshim/sdk-v2/provider2.go
Outdated
| cfg, err := recoverAndCoerceCtyValueWithSchema(res.CoreConfigSchema(), | ||
| p.configWithTimeouts(ctx, config.Config, opts.TimeoutOptions, ty)) |
There was a problem hiding this comment.
It looks like this is only called during Diff. Doesn't this need to be called during update as well?
There was a problem hiding this comment.
Yeah so Update calls Diff again to redo the planning. Part of the test we're recovering here is the gRPC protocol layer test for the Update Pulumi RPC method and that now passes, so I'm pretty confident it's working. I had to fix that test to emulate an actual news!=olds change as not doing that bypasses the timeouts in the upstream impl.
|
This PR has been shipped in release v3.90.0. |
Under PlanResourceChange the way custom timeout option is handled is not properly communicating the value to the underlying TF provider. This is now fixed and the existing test made to pass.
See also: https://www.pulumi.com/docs/concepts/options/customtimeouts/
Fixes #2386
New warnings are introduced for the case when a user tries to set a customTimeouts option on a resource that does not support it. The customTiemeouts in this case are a no-op. I believe this should be a warning to guide the user but we might also consider downgrading severity to an INFO or debug level message.