refactor: Uses the new create-only plan modifier in flex cluster #3658
Conversation
| // Handle timeout with cleanup logic | ||
| deleteOnCreateTimeout := cleanup.ResolveDeleteOnCreateTimeout(tfModel.DeleteOnCreateTimeout) | ||
| err = cleanup.HandleCreateTimeout(deleteOnCreateTimeout, err, func(ctxCleanup context.Context) error { | ||
| err = cleanup.HandleCreateTimeout(tfModel.DeleteOnCreateTimeout.ValueBool(), err, func(ctxCleanup context.Context) error { |
| Computed: true, | ||
| PlanModifiers: []planmodifier.Bool{ | ||
| customplanmodifier.CreateOnlyBoolPlanModifier(), | ||
| customplanmodifier.CreateOnlyBoolWithDefault(true), |
There was a problem hiding this comment.
This change will be applied to the other resources in a follow up PR:
- encryptionatrestprivateendpoint
- pushbasedlogexport
- streamprocessor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the plan modifier system to use new create-only plan modifiers with improved organization and functionality. The change splits the plan modifier functionality into separate type-specific structs and enables default value support for boolean attributes.
- Replaces old unified plan modifier functions with new type-specific ones (CreateOnlyBool/CreateOnlyString)
- Adds default value support for boolean plan modifiers through CreateOnlyBoolWithDefault
- Updates flex cluster resource to use the new plan modifier with default value functionality
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/common/customplanmodifier/create_only.go | Removes old unified plan modifier implementation |
| internal/common/customplanmodifier/create_only_bool.go | Adds new boolean-specific plan modifier with default value support |
| internal/common/customplanmodifier/create_only_string.go | Adds new string-specific plan modifier implementation |
| internal/service/flexcluster/resource_schema.go | Updates to use new CreateOnlyBoolWithDefault modifier and adds Computed attribute |
| internal/service/flexcluster/resource.go | Simplifies timeout handling by using boolean value directly |
| internal/service/flexcluster/resource_test.go | Adds ImportStateVerifyIgnore for delete_on_create_timeout |
| internal/service/project/resource_project_schema.go | Updates function calls to use new plan modifier names |
| internal/service/streamprocessor/resource_schema.go | Updates function calls to use new plan modifier names |
| internal/service/pushbasedlogexport/resource_schema.go | Updates function calls to use new plan modifier names |
| internal/service/encryptionatrestprivateendpoint/resource_schema.go | Updates function calls to use new plan modifier names |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // It shows a helpful error message helping the user to update their config to match the state. | ||
| // Never use a schema.Default for create only attributes, instead use `WithDefault`, the default will lead to plan changes that are not expected after import. | ||
| // No default value implemented for string until we have a use case. | ||
| // Implement CopyFromPlan if the attribute is not in the API Response. |
There was a problem hiding this comment.
What is this exactly referring to? Not sure how relevant this guidance is, it applies for any attribute which is not present in API response.
| @@ -0,0 +1,95 @@ | |||
| package customplanmodifier | |||
There was a problem hiding this comment.
shouldn't we unit test these plan modifiers?
There was a problem hiding this comment.
These are tested by the acceptance tests for project & flex resources:
From project:
- TestAccProject_withFalseDefaultSettings
- TestMigProject_withFalseDefaultAlertSettings
- TestMigProject_withTrueDefaultAlertSettings
It is not straightforward to unit test them as they are called in a specific context.
| Optional: true, | ||
| PlanModifiers: []planmodifier.Bool{ | ||
| customplanmodifier.CreateOnlyBoolPlanModifier(), | ||
| customplanmodifier.CreateOnlyBool(), |
There was a problem hiding this comment.
Double checking here: will this also be usable for SDKv2? just remembered of this comment.
Feel free to correct me if I am saying something incorrect - I guess it won't, which means that I am wondering if the "import" use case needs to be handled also for the SDKv2 resources? cc @oarbusi
There was a problem hiding this comment.
This is a good point.
https://developer.hashicorp.com/terraform/plugin/framework/migrating/resources/plan-modification#migration-notes
We could investigate using DiffSuppress function for this, will have to do another investigation.
There was a problem hiding this comment.
The DiffSuppress is on the resource level:
https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema#CustomizeDiffFunc
So, I don't think it is worth changing the logic for the SDKv2 resources.
The behavior of a non-empty plan after import for these resources when an explicit config value is set, I think, is ok to keep for now; otherwise, we would need to add the check of Null to defined-value in all Resources.Update methods.
There was a problem hiding this comment.
Problem I see is not being consistent between SDKv2 and TPF resources. If we do this only in the TPF resources and not in SDKv2, we are not consistent and we might cause more confusion than clarity
There was a problem hiding this comment.
I'll try to do it in the follow-up PR to see how much work it is:
https://jira.mongodb.org/browse/CLOUDP-343190?focusedCommentId=7658580&focusedId=7658580&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-7658580
| Update: true, | ||
| Delete: true, | ||
| }), | ||
| "delete_on_create_timeout": schema.BoolAttribute{ |
There was a problem hiding this comment.
@EspenAlbert @AgustinBettati chatting more with @oarbusi , I reflected on one detail I missed during the explanation. The plan change during the import would only occur if delete_on_create_timeout is explicitly set in the configuration used for the import. If not, there is no issue with the existing logic.
That means: if in the import this value is not specified by the practitioner (like it really should be, as there's no reason why it should be set), the current code doesn't have any limitation. Considered that, I can still figure there's a value in improving the overall experience, but I see less urgency in making this change.
Is there any other consideration I missed?
There was a problem hiding this comment.
Correct. Nothing else apart from the ones documented here; https://docs.google.com/document/d/1iu18eHeECR2ofy51uuYOyP8dxS2Q5m8joTnEQu4Q4lQ/edit?tab=t.0#bookmark=id.qliuhcp0nzu1
There was a problem hiding this comment.
ok, I was of the impression that it was something that was "always" going to happen when an import was made. My take then is that this is not worth it a refactoring now that we are very close to the release.
As it comes down to best practices, I still see the value but I would postpone this change to perhaps a minor release. This will help us focusing in getting 2.0.0 out with less moving pieces.
@AgustinBettati @EspenAlbert @lantoli @maastha @marcabreracast @oarbusi looking for feedback here, I'd like to make a call together.
There was a problem hiding this comment.
Thinking on this more, I think that if we can't provide the same experience in SDKv2 and TPF, I would maybe hold this off until we can, because not having the same UX across resources can be confusing for customers
There was a problem hiding this comment.
Agree on postponing this and including it in a minor release instead of 2.0.0
There was a problem hiding this comment.
I am leaning towards merging. The plan behavior is more explicit, and we are following hashicorp best practices with this refactor (using computed for an attribute with a default value)
Since this plan modifier is already released now in 1.41 we would need to keep both implementations which adds tech debt.
There was a problem hiding this comment.
I'm ok to merge this PR if it's ready
There was a problem hiding this comment.
It is ready, but I think the team is not convinced. @marcosuma @AgustinBettati @oarbusi
There was a problem hiding this comment.
@lantoli @EspenAlbert maybe I was misunderstood - I am not saying we shouldn't merge. I am saying we shouldn't link this to 2.0.0 as there's really no strong reason to do so.
if we merge just this PR, we'll have:
- this resource (TPF) using this version of plan modifier
- other resources (TPF) using the other approach but they can be updated
- other resources (SDKv2) which would stay with a different behavior.
What is the plan to align all of those? Why do you want to link this to 2.0.0?
There was a problem hiding this comment.
Ok. Allthough no blocker for 2.0.0 I think we should merge it.
Some clarifications If we merge:
- flex cluster resource (TPF) using this version of plan modifier for delete_on_create_timeout with default=true
- other resources (TPF) using the plan modifier for delete_on_create_timeout but without a default (set in create handler instead) --> I expect to follow up with using default instead after merging.
- other resources (SDKv2) with delete_on_create_timeout which would stay with an allowed plan update after import (when the attribute is changed). --> I can look into updating this today.
EspenAlbert
force-pushed
the
CLOUDP-343190_flex_delete_on_create_timeout
branch
from
75e97a7 to
674b795
Compare
|
we're good to go |
Description
Uses the new create-only plan modifier in flex cluster.
Also splits the plan modifier into two different structs:
Additional changes:
customplanmodifierpackageCreateOnlyBoolWithDefaultfor the other resourcesLink to any related issue(s): CLOUDP-343190
Type of change:
Required Checklist:
Further comments