-
Couldn't load subscription status.
- Fork 208
refactor: Uses delete_on_create_timeout with default=true support across TPF resources #3809
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
refactor: Uses delete_on_create_timeout with default=true support across TPF resources #3809
Conversation
| Config: updateStepNoWait, | ||
| Check: updateStep.Check, | ||
| }, | ||
| // Changes: skip_wait_on_update true -> null |
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.
This summarizes the change in behavior well.
The old way allowed updating to null while the new way keeps the state value if removed (UseStateForUnknown behavior).
This is why we need the ImportStateVerifyIgnore: []string{"delete_on_create_timeout"} in import as it will not be present in the state after import.
Because once the value has been used in create it remains unchanged.
Note: A fresh import and setting delete_on_create_timeout in the config will raise an update error saying it cannot be updated.
| Config: configWithTimeout(timeoutsStrLongFalse), | ||
| Check: resource.TestCheckResourceAttr(resourceID, "delete_on_create_timeout", "false"), | ||
| Config: configWithTimeout(timeoutsStrLongFalse), | ||
| ExpectError: regexp.MustCompile("delete_on_create_timeout cannot be updated or set after import .*"), |
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.
Example of error message when trying to update
…and fix extra space in error message for delete_on_create_timeout
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.
Pull Request Overview
This PR refactors the delete_on_create_timeout attribute across multiple TPF (Terraform Plugin Framework) resources to use a new plan modifier CreateOnlyBoolWithDefault(true) that provides a default value of true, making the attribute computed. This standardizes the behavior across resources including stream processors, search deployments, push-based log exports, encryption-at-rest private endpoints, and advanced clusters.
Key Changes:
- Replaced
customplanmodifier.CreateOnly()withcustomplanmodifier.CreateOnlyBoolWithDefault(true)for thedelete_on_create_timeoutattribute - Added
Computed: trueto thedelete_on_create_timeoutschema attribute - Removed the
ResolveDeleteOnCreateTimeouthelper function, directly usingValueBool()instead
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/service/streamprocessor/resource_schema.go | Updated schema to make delete_on_create_timeout computed with default true |
| internal/service/streamprocessor/resource.go | Changed to use ValueBool() directly instead of ResolveDeleteOnCreateTimeout |
| internal/service/streamprocessor/resource_test.go | Added delete_on_create_timeout to import verify ignore list and refactored import steps |
| internal/service/searchdeployment/resource_schema.go | Updated schema to make delete_on_create_timeout computed with default true |
| internal/service/searchdeployment/resource.go | Changed to use ValueBool() directly instead of ResolveDeleteOnCreateTimeout |
| internal/service/searchdeployment/resource_test.go | Updated tests to handle delete_on_create_timeout as computed attribute |
| internal/service/pushbasedlogexport/resource_schema.go | Updated schema to make delete_on_create_timeout computed with default true |
| internal/service/pushbasedlogexport/resource.go | Changed to use ValueBool() directly instead of ResolveDeleteOnCreateTimeout |
| internal/service/pushbasedlogexport/resource_test.go | Added delete_on_create_timeout to import verify ignore list |
| internal/service/encryptionatrestprivateendpoint/resource_schema.go | Updated schema to make delete_on_create_timeout computed with default true |
| internal/service/encryptionatrestprivateendpoint/resource.go | Changed to use ValueBool() directly instead of ResolveDeleteOnCreateTimeout |
| internal/service/encryptionatrestprivateendpoint/resource_test.go | Added delete_on_create_timeout to import verify ignore list and refactored import steps |
| internal/service/advancedcluster/schema.go | Updated schema to make delete_on_create_timeout computed with default true |
| internal/service/advancedcluster/resource.go | Changed to use ValueBool() directly instead of ResolveDeleteOnCreateTimeout |
| internal/common/cleanup/handle_timeout.go | Removed ResolveDeleteOnCreateTimeout helper function |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| projectID, clusterName := waitParams.ProjectID, waitParams.ClusterName | ||
| clusterDetailStr := fmt.Sprintf("Cluster name %s (project_id=%s).", clusterName, projectID) | ||
| if cleanup.ResolveDeleteOnCreateTimeout(plan.DeleteOnCreateTimeout) { | ||
| if plan.DeleteOnCreateTimeout.ValueBool() { |
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.
can plan.DeleteOnCreateTimeout be null?
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.
No, it has a default value of true
| }, | ||
| "delete_on_create_timeout": schema.BoolAttribute{ | ||
| Optional: true, | ||
| Computed: true, |
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 see we're adding computed but we're also adding the plan modifier. I would like certainty that we're not adding one more field to the output plan that says known after apply. We're getting a few GH issues lately talking about the plan verbosity
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.
Actually this PR improves that behavior by showing the planned true value (see doc)
| testLogDestConfig = connectionConfig{connectionType: connTypeTestLog, pipelineStepIsSource: false} | ||
| ) | ||
|
|
||
| func importStep() resource.TestStep { |
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.
Can be a var as well. Same as in resource_database_user_test.go
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 normally prefer functions to global vars, which is safer in case the var is modified later and might affect other tests. (some exceptions apply, e.g. for an array of strings, if we're sure they're not modified)
Description
Uses delete_on_create_timeout with default=true support across TPF resources
Link to any related issue(s): CLOUDP-343190
Type of change:
Required Checklist:
Further comments