-
Notifications
You must be signed in to change notification settings - Fork 208
refactor: Uses the new create-only plan modifier in flex cluster #3658
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
base: master
Are you sure you want to change the base?
Changes from all commits
6909929
6e7ffbb
a0ad4a7
674b795
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
package customplanmodifier | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
|
||
"github.com/hashicorp/terraform-plugin-framework/attr" | ||
"github.com/hashicorp/terraform-plugin-framework/diag" | ||
"github.com/hashicorp/terraform-plugin-framework/path" | ||
planmodifier "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" | ||
"github.com/hashicorp/terraform-plugin-framework/tfsdk" | ||
"github.com/hashicorp/terraform-plugin-framework/types" | ||
) | ||
|
||
// CreateOnlyBool creates a plan modifier that prevents updates to boolean attributes. | ||
// This is useful for attributes only supported in create and not in update. | ||
// 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. | ||
// If the attribute is not in the API Response implement CopyFromPlan behavior when converting API Model to TF Model. | ||
func CreateOnlyBool() planmodifier.Bool { | ||
return &createOnlyBoolPlanModifier{} | ||
} | ||
|
||
// CreateOnlyBoolWithDefault sets a default value on create operation that will show in the plan. | ||
// This avoids any custom logic in the resource "Create" handler. | ||
AgustinBettati marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// On update the default has no impact and the UseStateForUnknown behavior is observed instead. | ||
// Always use Optional+Computed when using a default value. | ||
// If the attribute is not in the API Response implement CopyFromPlan behavior when converting API Model to TF Model. | ||
func CreateOnlyBoolWithDefault(b bool) planmodifier.Bool { | ||
return &createOnlyBoolPlanModifier{defaultBool: &b} | ||
} | ||
|
||
type createOnlyBoolPlanModifier struct { | ||
defaultBool *bool | ||
} | ||
|
||
func (d *createOnlyBoolPlanModifier) Description(ctx context.Context) string { | ||
return d.MarkdownDescription(ctx) | ||
} | ||
|
||
func (d *createOnlyBoolPlanModifier) MarkdownDescription(ctx context.Context) string { | ||
return "Ensures the update operation fails when updating an attribute. If the read after import doesn't equal the configuration value it will also raise an error." | ||
} | ||
|
||
// isCreate uses the full state to check if this is a create operation | ||
func isCreate(t *tfsdk.State) bool { | ||
return t.Raw.IsNull() | ||
marcosuma marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
EspenAlbert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
func (d *createOnlyBoolPlanModifier) UseDefault() bool { | ||
return d.defaultBool != nil | ||
} | ||
|
||
func (d *createOnlyBoolPlanModifier) PlanModifyBool(ctx context.Context, req planmodifier.BoolRequest, resp *planmodifier.BoolResponse) { | ||
if isCreate(&req.State) { | ||
if !IsKnown(req.PlanValue) && d.UseDefault() { | ||
resp.PlanValue = types.BoolPointerValue(d.defaultBool) | ||
} | ||
return | ||
} | ||
if isUpdated(req.StateValue, req.PlanValue) { | ||
d.addDiags(&resp.Diagnostics, req.Path, req.StateValue) | ||
} | ||
if !IsKnown(req.PlanValue) { | ||
resp.PlanValue = req.StateValue | ||
} | ||
} | ||
|
||
// isUpdated checks if the attribute was updated. | ||
// Special case when the attribute is removed/set to null in the plan: | ||
// Computed Attribute: returns false (unknown in the plan) | ||
// Optional Attribute: returns true if the state has a value | ||
func isUpdated(state, plan attr.Value) bool { | ||
if !IsKnown(plan) { | ||
return false | ||
} | ||
return !state.Equal(plan) | ||
} | ||
EspenAlbert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
func (d *createOnlyBoolPlanModifier) addDiags(diags *diag.Diagnostics, attrPath path.Path, stateValue attr.Value) { | ||
message := fmt.Sprintf("%s cannot be updated or set after import, remove it from the configuration or use the state value (see below).", attrPath) | ||
detail := fmt.Sprintf("The current state value is %s", stateValue) | ||
diags.AddError(message, detail) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package customplanmodifier | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
|
||
"github.com/hashicorp/terraform-plugin-framework/attr" | ||
"github.com/hashicorp/terraform-plugin-framework/diag" | ||
"github.com/hashicorp/terraform-plugin-framework/path" | ||
planmodifier "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" | ||
) | ||
|
||
// CreateOnlyString creates a plan modifier that prevents updates to string attributes. | ||
// This is useful for attributes only supported in create and not in update. | ||
// 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. | ||
// If the attribute is not in the API Response implement CopyFromPlan behavior when converting API Model to TF Model. | ||
func CreateOnlyString() planmodifier.String { | ||
return &createOnlyStringPlanModifier{} | ||
} | ||
|
||
type createOnlyStringPlanModifier struct{} | ||
|
||
func (d *createOnlyStringPlanModifier) Description(ctx context.Context) string { | ||
return d.MarkdownDescription(ctx) | ||
} | ||
|
||
func (d *createOnlyStringPlanModifier) MarkdownDescription(ctx context.Context) string { | ||
return "Ensures the update operation fails when updating an attribute. If the read after import don't equal the configuration value it will also raise an error." | ||
} | ||
|
||
func (d *createOnlyStringPlanModifier) PlanModifyString(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) { | ||
if isCreate(&req.State) { | ||
return | ||
} | ||
if isUpdated(req.StateValue, req.PlanValue) { | ||
d.addDiags(&resp.Diagnostics, req.Path, req.StateValue) | ||
} | ||
if !IsKnown(req.PlanValue) { | ||
resp.PlanValue = req.StateValue | ||
} | ||
} | ||
|
||
func (d *createOnlyStringPlanModifier) addDiags(diags *diag.Diagnostics, attrPath path.Path, stateValue attr.Value) { | ||
message := fmt.Sprintf("%s cannot be updated or set after import, remove it from the configuration or use the state value (see below).", attrPath) | ||
detail := fmt.Sprintf("The current state value is %s", stateValue) | ||
diags.AddError(message, detail) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,8 +80,7 @@ func (r *rs) Create(ctx context.Context, req resource.CreateRequest, resp *resou | |
flexClusterResp, err := CreateFlexCluster(ctx, projectID, clusterName, flexClusterReq, connV2.FlexClustersApi, &createTimeout) | ||
|
||
// 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed logic |
||
cleanResp, cleanErr := r.Client.AtlasV2.FlexClustersApi.DeleteFlexCluster(ctxCleanup, projectID, clusterName).Execute() | ||
if validate.StatusNotFound(cleanResp) { | ||
return nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,14 +22,14 @@ func ResourceSchema(ctx context.Context) schema.Schema { | |
"project_id": schema.StringAttribute{ | ||
Required: true, | ||
PlanModifiers: []planmodifier.String{ | ||
customplanmodifier.CreateOnlyStringPlanModifier(), | ||
customplanmodifier.CreateOnlyString(), | ||
marcosuma marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
MarkdownDescription: "Unique 24-hexadecimal character string that identifies the project.", | ||
}, | ||
"name": schema.StringAttribute{ | ||
Required: true, | ||
PlanModifiers: []planmodifier.String{ | ||
customplanmodifier.CreateOnlyStringPlanModifier(), | ||
customplanmodifier.CreateOnlyString(), | ||
}, | ||
MarkdownDescription: "Human-readable label that identifies the instance.", | ||
}, | ||
|
@@ -38,7 +38,7 @@ func ResourceSchema(ctx context.Context) schema.Schema { | |
"backing_provider_name": schema.StringAttribute{ | ||
Required: true, | ||
PlanModifiers: []planmodifier.String{ | ||
customplanmodifier.CreateOnlyStringPlanModifier(), | ||
customplanmodifier.CreateOnlyString(), | ||
}, | ||
MarkdownDescription: "Cloud service provider on which MongoDB Cloud provisioned the flex cluster.", | ||
}, | ||
|
@@ -59,7 +59,7 @@ func ResourceSchema(ctx context.Context) schema.Schema { | |
"region_name": schema.StringAttribute{ | ||
Required: true, | ||
PlanModifiers: []planmodifier.String{ | ||
customplanmodifier.CreateOnlyStringPlanModifier(), | ||
customplanmodifier.CreateOnlyString(), | ||
}, | ||
MarkdownDescription: "Human-readable label that identifies the geographic location of your MongoDB flex cluster. The region you choose can affect network latency for clients accessing your databases. For a complete list of region names, see [AWS](https://docs.atlas.mongodb.com/reference/amazon-aws/#std-label-amazon-aws), [GCP](https://docs.atlas.mongodb.com/reference/google-gcp/), and [Azure](https://docs.atlas.mongodb.com/reference/microsoft-azure/).", | ||
}, | ||
|
@@ -148,8 +148,9 @@ func ResourceSchema(ctx context.Context) schema.Schema { | |
}, | ||
"delete_on_create_timeout": schema.BoolAttribute{ | ||
Optional: true, | ||
Computed: true, | ||
PlanModifiers: []planmodifier.Bool{ | ||
customplanmodifier.CreateOnlyBoolPlanModifier(), | ||
customplanmodifier.CreateOnlyBoolWithDefault(true), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change will be applied to the other resources in a follow up PR:
|
||
}, | ||
MarkdownDescription: "Indicates whether to delete the resource being created if a timeout is reached when waiting for completion. When set to `true` and timeout occurs, it triggers the deletion and returns immediately without waiting for deletion to complete. When set to `false`, the timeout will not trigger resource deletion. If you suspect a transient error when the value is `true`, wait before retrying to allow resource deletion to finish. Default is `true`.", | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,7 +59,7 @@ func ResourceSchema(ctx context.Context) schema.Schema { | |
"delete_on_create_timeout": schema.BoolAttribute{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree on postponing this and including it in a minor release instead of 2.0.0 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok to merge this PR if it's ready There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is ready, but I think the team is not convinced. @marcosuma @AgustinBettati @oarbusi There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Allthough no blocker for 2.0.0 I think we should merge it.
|
||
Optional: true, | ||
PlanModifiers: []planmodifier.Bool{ | ||
customplanmodifier.CreateOnlyBoolPlanModifier(), | ||
customplanmodifier.CreateOnlyBool(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The DiffSuppress is on the resource level: 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll try to do it in the follow-up PR to see how much work it is: |
||
}, | ||
MarkdownDescription: "Indicates whether to delete the resource being created if a timeout is reached when waiting for completion. When set to `true` and timeout occurs, it triggers the deletion and returns immediately without waiting for deletion to complete. When set to `false`, the timeout will not trigger resource deletion. If you suspect a transient error when the value is `true`, wait before retrying to allow resource deletion to finish. Default is `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.
shouldn't we unit test these plan modifiers?
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.
These are tested by the acceptance tests for project & flex resources:
From project:
It is not straightforward to unit test them as they are called in a specific context.