Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 0 additions & 135 deletions internal/common/customplanmodifier/create_only.go

This file was deleted.

84 changes: 84 additions & 0 deletions internal/common/customplanmodifier/create_only_bool.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package customplanmodifier
Copy link
Collaborator

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?

Copy link
Collaborator Author

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:

  • TestAccProject_withFalseDefaultSettings
  • TestMigProject_withFalseDefaultAlertSettings
  • TestMigProject_withTrueDefaultAlertSettings

It is not straightforward to unit test them as they are called in a specific context.


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.
// 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()
}

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)
}

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)
}
49 changes: 49 additions & 0 deletions internal/common/customplanmodifier/create_only_string.go
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
Expand Up @@ -48,7 +48,7 @@ func ResourceSchema(ctx context.Context) schema.Schema {
"delete_on_create_timeout": schema.BoolAttribute{
Optional: true,
PlanModifiers: []planmodifier.Bool{
customplanmodifier.CreateOnlyBoolPlanModifier(),
customplanmodifier.CreateOnlyBool(),
},
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`.",
},
Expand Down
3 changes: 1 addition & 2 deletions internal/service/flexcluster/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Expand Down
11 changes: 6 additions & 5 deletions internal/service/flexcluster/resource_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ func ResourceSchema(ctx context.Context) schema.Schema {
"project_id": schema.StringAttribute{
Required: true,
PlanModifiers: []planmodifier.String{
customplanmodifier.CreateOnlyStringPlanModifier(),
customplanmodifier.CreateOnlyString(),
},
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.",
},
Expand All @@ -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.",
},
Expand All @@ -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/).",
},
Expand Down Expand Up @@ -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),
Copy link
Collaborator Author

@EspenAlbert EspenAlbert Sep 8, 2025

Choose a reason for hiding this comment

The 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:

  • encryptionatrestprivateendpoint
  • pushbasedlogexport
  • streamprocessor

},
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`.",
},
Expand Down
9 changes: 5 additions & 4 deletions internal/service/flexcluster/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,11 @@ func basicTestCase(t *testing.T) *resource.TestCase {
Check: checksFlexCluster(projectID, clusterName, false, true),
},
{
ResourceName: resourceName,
ImportStateIdFunc: acc.ImportStateIDFuncProjectIDClusterName(resourceName, "project_id", "name"),
ImportState: true,
ImportStateVerify: true,
ResourceName: resourceName,
ImportStateIdFunc: acc.ImportStateIDFuncProjectIDClusterName(resourceName, "project_id", "name"),
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"delete_on_create_timeout"},
},
},
}
Expand Down
4 changes: 2 additions & 2 deletions internal/service/project/resource_project_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,15 @@ func ResourceSchema(ctx context.Context) schema.Schema {
"project_owner_id": schema.StringAttribute{
Optional: true,
PlanModifiers: []planmodifier.String{
customplanmodifier.CreateOnlyStringPlanModifier(),
customplanmodifier.CreateOnlyString(),
},
},
"with_default_alerts_settings": schema.BoolAttribute{
// Default values also must be Computed otherwise Terraform throws error:
// Provider produced invalid plan: planned an invalid value for a non-computed attribute.
Optional: true,
Computed: true,
PlanModifiers: []planmodifier.Bool{customplanmodifier.CreateOnlyAttributePlanModifierWithBoolDefault(true)},
PlanModifiers: []planmodifier.Bool{customplanmodifier.CreateOnlyBoolWithDefault(true)},
},
"is_collect_database_specifics_statistics_enabled": schema.BoolAttribute{
Computed: true,
Expand Down
2 changes: 1 addition & 1 deletion internal/service/pushbasedlogexport/resource_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func ResourceSchema(ctx context.Context) schema.Schema {
"delete_on_create_timeout": schema.BoolAttribute{
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

@oarbusi oarbusi Sep 8, 2025

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok to merge this PR if it's ready

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

  • 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.
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.

Optional: true,
PlanModifiers: []planmodifier.Bool{
customplanmodifier.CreateOnlyBoolPlanModifier(),
customplanmodifier.CreateOnlyBool(),
Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

@EspenAlbert EspenAlbert Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

},
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`.",
},
Expand Down
2 changes: 1 addition & 1 deletion internal/service/streamprocessor/resource_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func ResourceSchema(ctx context.Context) schema.Schema {
"delete_on_create_timeout": schema.BoolAttribute{
Optional: true,
PlanModifiers: []planmodifier.Bool{
customplanmodifier.CreateOnlyBoolPlanModifier(),
customplanmodifier.CreateOnlyBool(),
},
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`.",
},
Expand Down
Loading