From 53de46328c5a72eb7dafbccc6d91a858a9c0d674 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Tue, 25 Feb 2025 09:55:06 +0000 Subject: [PATCH 01/13] fix: Sets default value for WithDefaultAlertsSettings during state import --- internal/service/project/resource_project.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/service/project/resource_project.go b/internal/service/project/resource_project.go index 83fb0409b4..26411fc7b5 100644 --- a/internal/service/project/resource_project.go +++ b/internal/service/project/resource_project.go @@ -333,9 +333,13 @@ func (r *projectRS) ImportState(ctx context.Context, req resource.ImportStateReq } func updatePlanFromConfig(projectPlanNewPtr, projectPlan *TFProjectRSModel) { - // we need to reset defaults from what was previously in the state: - // https://discuss.hashicorp.com/t/boolean-optional-default-value-migration-to-framework/55932 - projectPlanNewPtr.WithDefaultAlertsSettings = projectPlan.WithDefaultAlertsSettings + // After import the state&plan will be null so we set to true (default value) + if projectPlanNewPtr.WithDefaultAlertsSettings.IsNull() && projectPlan.WithDefaultAlertsSettings.IsNull() { + projectPlanNewPtr.WithDefaultAlertsSettings = types.BoolValue(true) + } else { + // Force value from plan as this is not returned from the API to avoid inconsistent result errors + projectPlanNewPtr.WithDefaultAlertsSettings = projectPlan.WithDefaultAlertsSettings + } projectPlanNewPtr.ProjectOwnerID = projectPlan.ProjectOwnerID if projectPlan.Tags.IsNull() && len(projectPlanNewPtr.Tags.Elements()) == 0 { projectPlanNewPtr.Tags = types.MapNull(types.StringType) From 26f73534024a5a464b27d16cacaf75a9242bd89f Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Tue, 25 Feb 2025 09:58:58 +0000 Subject: [PATCH 02/13] chore: Adds release note --- .changelog/3105.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/3105.txt diff --git a/.changelog/3105.txt b/.changelog/3105.txt new file mode 100644 index 0000000000..2e65c9318f --- /dev/null +++ b/.changelog/3105.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/mongodbatlas_project: Sets default value for `with_default_alerts_settings` during state import +``` From 5e67e149912ecfc6a40e71124d6abe61ddbec26c Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Tue, 25 Feb 2025 10:27:22 +0000 Subject: [PATCH 03/13] fix: Removes 'with_default_alerts_settings' from ImportStateVerifyIgnore in project tests when it is not set to `false` in earlier steps --- internal/service/project/resource_project_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/internal/service/project/resource_project_test.go b/internal/service/project/resource_project_test.go index e4854c81ef..3972005f07 100644 --- a/internal/service/project/resource_project_test.go +++ b/internal/service/project/resource_project_test.go @@ -613,7 +613,7 @@ func TestAccProject_basic(t *testing.T) { ImportStateIdFunc: acc.ImportStateProjectIDFunc(resourceName), ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"with_default_alerts_settings", "project_owner_id"}, + ImportStateVerifyIgnore: []string{"project_owner_id"}, }, }, }) @@ -1055,11 +1055,10 @@ func TestAccProject_withTags(t *testing.T) { Check: tagChecks(tagsOnlyIgnored, "Environment", "NewKey"), }, { - ResourceName: resourceName, - ImportStateIdFunc: acc.ImportStateProjectIDFunc(resourceName), - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"with_default_alerts_settings"}, + ResourceName: resourceName, + ImportStateIdFunc: acc.ImportStateProjectIDFunc(resourceName), + ImportState: true, + ImportStateVerify: true, }, }, }) From 57ddae6009bcba5fef6d37ddede76f2c111ea2d3 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Tue, 25 Feb 2025 15:10:45 +0000 Subject: [PATCH 04/13] doc: update changelog message --- .changelog/3105.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/3105.txt b/.changelog/3105.txt index 2e65c9318f..67a00812f4 100644 --- a/.changelog/3105.txt +++ b/.changelog/3105.txt @@ -1,3 +1,3 @@ ```release-note:bug -resource/mongodbatlas_project: Sets default value for `with_default_alerts_settings` during state import +resource/mongodbatlas_project: Fixes import when `with_default_alerts_settings` is set ``` From 7bc763ed6481b25a311faa31eae7befc3708260d Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Wed, 26 Feb 2025 15:06:26 +0000 Subject: [PATCH 05/13] doc: Add back reference based on comments --- internal/service/project/resource_project.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/service/project/resource_project.go b/internal/service/project/resource_project.go index 26411fc7b5..cc0527c585 100644 --- a/internal/service/project/resource_project.go +++ b/internal/service/project/resource_project.go @@ -338,6 +338,7 @@ func updatePlanFromConfig(projectPlanNewPtr, projectPlan *TFProjectRSModel) { projectPlanNewPtr.WithDefaultAlertsSettings = types.BoolValue(true) } else { // Force value from plan as this is not returned from the API to avoid inconsistent result errors + // https://discuss.hashicorp.com/t/boolean-optional-default-value-migration-to-framework/55932 projectPlanNewPtr.WithDefaultAlertsSettings = projectPlan.WithDefaultAlertsSettings } projectPlanNewPtr.ProjectOwnerID = projectPlan.ProjectOwnerID From de2589d03e2640705a3f960572f714dda978d5f7 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Mon, 3 Mar 2025 16:07:49 +0000 Subject: [PATCH 06/13] revert changes of old implementation --- internal/service/project/resource_project.go | 11 +++-------- internal/service/project/resource_project_test.go | 11 ++++++----- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/internal/service/project/resource_project.go b/internal/service/project/resource_project.go index cc0527c585..83fb0409b4 100644 --- a/internal/service/project/resource_project.go +++ b/internal/service/project/resource_project.go @@ -333,14 +333,9 @@ func (r *projectRS) ImportState(ctx context.Context, req resource.ImportStateReq } func updatePlanFromConfig(projectPlanNewPtr, projectPlan *TFProjectRSModel) { - // After import the state&plan will be null so we set to true (default value) - if projectPlanNewPtr.WithDefaultAlertsSettings.IsNull() && projectPlan.WithDefaultAlertsSettings.IsNull() { - projectPlanNewPtr.WithDefaultAlertsSettings = types.BoolValue(true) - } else { - // Force value from plan as this is not returned from the API to avoid inconsistent result errors - // https://discuss.hashicorp.com/t/boolean-optional-default-value-migration-to-framework/55932 - projectPlanNewPtr.WithDefaultAlertsSettings = projectPlan.WithDefaultAlertsSettings - } + // we need to reset defaults from what was previously in the state: + // https://discuss.hashicorp.com/t/boolean-optional-default-value-migration-to-framework/55932 + projectPlanNewPtr.WithDefaultAlertsSettings = projectPlan.WithDefaultAlertsSettings projectPlanNewPtr.ProjectOwnerID = projectPlan.ProjectOwnerID if projectPlan.Tags.IsNull() && len(projectPlanNewPtr.Tags.Elements()) == 0 { projectPlanNewPtr.Tags = types.MapNull(types.StringType) diff --git a/internal/service/project/resource_project_test.go b/internal/service/project/resource_project_test.go index 3972005f07..e4854c81ef 100644 --- a/internal/service/project/resource_project_test.go +++ b/internal/service/project/resource_project_test.go @@ -613,7 +613,7 @@ func TestAccProject_basic(t *testing.T) { ImportStateIdFunc: acc.ImportStateProjectIDFunc(resourceName), ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"project_owner_id"}, + ImportStateVerifyIgnore: []string{"with_default_alerts_settings", "project_owner_id"}, }, }, }) @@ -1055,10 +1055,11 @@ func TestAccProject_withTags(t *testing.T) { Check: tagChecks(tagsOnlyIgnored, "Environment", "NewKey"), }, { - ResourceName: resourceName, - ImportStateIdFunc: acc.ImportStateProjectIDFunc(resourceName), - ImportState: true, - ImportStateVerify: true, + ResourceName: resourceName, + ImportStateIdFunc: acc.ImportStateProjectIDFunc(resourceName), + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"with_default_alerts_settings"}, }, }, }) From 7934ad03207607cd19bf0b0809c009f38469d32e Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Mon, 3 Mar 2025 16:08:28 +0000 Subject: [PATCH 07/13] refactor: Introduce Modifier interface and enhance non-updatable attribute handling to support multiple planmodifiers --- .../customplanmodifier/non_updatable.go | 44 +++++++++++++------ .../service/flexcluster/resource_schema.go | 8 ++-- 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/internal/common/customplanmodifier/non_updatable.go b/internal/common/customplanmodifier/non_updatable.go index 7f47282bb1..875da27698 100644 --- a/internal/common/customplanmodifier/non_updatable.go +++ b/internal/common/customplanmodifier/non_updatable.go @@ -4,33 +4,49 @@ 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" ) -func NonUpdatableStringAttributePlanModifier() planmodifier.String { - return &nonUpdatableStringAttributePlanModifier{} +type Modifier interface { + planmodifier.String + planmodifier.Bool } -type nonUpdatableStringAttributePlanModifier struct { +func NonUpdatableAttributePlanModifier() Modifier { + return &nonUpdatableAttributePlanModifier{} } -func (d *nonUpdatableStringAttributePlanModifier) Description(ctx context.Context) string { +type nonUpdatableAttributePlanModifier struct { +} + +func (d *nonUpdatableAttributePlanModifier) Description(ctx context.Context) string { return d.MarkdownDescription(ctx) } -func (d *nonUpdatableStringAttributePlanModifier) MarkdownDescription(ctx context.Context) string { +func (d *nonUpdatableAttributePlanModifier) MarkdownDescription(ctx context.Context) string { return "Ensures that update operations fails when updating an attribute." } -func (d *nonUpdatableStringAttributePlanModifier) PlanModifyString(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) { - planAttributeValue := req.PlanValue - stateAttributeValue := req.StateValue +func (d *nonUpdatableAttributePlanModifier) PlanModifyBool(ctx context.Context, req planmodifier.BoolRequest, resp *planmodifier.BoolResponse) { + if d.isUpdated(req.PlanValue, req.StateValue) { + d.addDiags(&resp.Diagnostics, req.Path) + } +} - if !stateAttributeValue.IsNull() && stateAttributeValue.ValueString() != planAttributeValue.ValueString() { - resp.Diagnostics.AddError( - fmt.Sprintf("%s cannot be updated", req.Path), - fmt.Sprintf("%s cannot be updated", req.Path), - ) - return +func (d *nonUpdatableAttributePlanModifier) PlanModifyString(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) { + if d.isUpdated(req.PlanValue, req.StateValue) { + d.addDiags(&resp.Diagnostics, req.Path) } } + +func (d *nonUpdatableAttributePlanModifier) isUpdated(planValue, stateValue attr.Value) bool { + return !stateValue.IsNull() && !planValue.Equal(stateValue) +} + +func (d *nonUpdatableAttributePlanModifier) addDiags(diags *diag.Diagnostics, attrPath path.Path) { + message := fmt.Sprintf("%s cannot be updated", attrPath) + diags.AddError(message, message) +} diff --git a/internal/service/flexcluster/resource_schema.go b/internal/service/flexcluster/resource_schema.go index d703b24b30..fd9d4e5982 100644 --- a/internal/service/flexcluster/resource_schema.go +++ b/internal/service/flexcluster/resource_schema.go @@ -21,14 +21,14 @@ func ResourceSchema(ctx context.Context) schema.Schema { "project_id": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableStringAttributePlanModifier(), + customplanmodifier.NonUpdatableAttributePlanModifier(), }, MarkdownDescription: "Unique 24-hexadecimal character string that identifies the project.", }, "name": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableStringAttributePlanModifier(), + customplanmodifier.NonUpdatableAttributePlanModifier(), }, MarkdownDescription: "Human-readable label that identifies the instance.", }, @@ -37,7 +37,7 @@ func ResourceSchema(ctx context.Context) schema.Schema { "backing_provider_name": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableStringAttributePlanModifier(), + customplanmodifier.NonUpdatableAttributePlanModifier(), }, MarkdownDescription: "Cloud service provider on which MongoDB Cloud provisioned the flex cluster.", }, @@ -58,7 +58,7 @@ func ResourceSchema(ctx context.Context) schema.Schema { "region_name": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableStringAttributePlanModifier(), + customplanmodifier.NonUpdatableAttributePlanModifier(), }, 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/).", }, From af84e3856ad9acd1d05ec0978c3e9676a6c93f90 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Mon, 3 Mar 2025 16:10:15 +0000 Subject: [PATCH 08/13] refactor: Mark with_default_alerts_settings as NonUpdateable --- internal/service/project/resource_project_schema.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/service/project/resource_project_schema.go b/internal/service/project/resource_project_schema.go index d49d12366e..8cd3c69323 100644 --- a/internal/service/project/resource_project_schema.go +++ b/internal/service/project/resource_project_schema.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/constant" + "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/customplanmodifier" "go.mongodb.org/atlas-sdk/v20241113005/admin" ) @@ -54,9 +55,10 @@ func ResourceSchema(ctx context.Context) schema.Schema { "with_default_alerts_settings": schema.BoolAttribute{ // Default values also must be Computed otherwise Terraform throws error: // Schema Using Attribute Default For Non-Computed Attribute - Optional: true, - Computed: true, - Default: booldefault.StaticBool(true), + Optional: true, + Computed: true, + Default: booldefault.StaticBool(true), + PlanModifiers: []planmodifier.Bool{customplanmodifier.NonUpdatableAttributePlanModifier()}, }, "is_collect_database_specifics_statistics_enabled": schema.BoolAttribute{ Computed: true, From 66e55e90e7815ba52deb48559bd0e7c0465fa19e Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Mon, 3 Mar 2025 16:11:34 +0000 Subject: [PATCH 09/13] refactor:Rename NonUpdatableAttributePlanModifier with CreateOnlyAttributePlanModifier in resource schemas --- .../{non_updatable.go => create_only.go} | 18 +++++++++--------- .../service/flexcluster/resource_schema.go | 8 ++++---- .../service/project/resource_project_schema.go | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) rename internal/common/customplanmodifier/{non_updatable.go => create_only.go} (51%) diff --git a/internal/common/customplanmodifier/non_updatable.go b/internal/common/customplanmodifier/create_only.go similarity index 51% rename from internal/common/customplanmodifier/non_updatable.go rename to internal/common/customplanmodifier/create_only.go index 875da27698..d9ca89994b 100644 --- a/internal/common/customplanmodifier/non_updatable.go +++ b/internal/common/customplanmodifier/create_only.go @@ -15,38 +15,38 @@ type Modifier interface { planmodifier.Bool } -func NonUpdatableAttributePlanModifier() Modifier { - return &nonUpdatableAttributePlanModifier{} +func CreateOnlyAttributePlanModifier() Modifier { + return &createOnlyAttributePlanModifier{} } -type nonUpdatableAttributePlanModifier struct { +type createOnlyAttributePlanModifier struct { } -func (d *nonUpdatableAttributePlanModifier) Description(ctx context.Context) string { +func (d *createOnlyAttributePlanModifier) Description(ctx context.Context) string { return d.MarkdownDescription(ctx) } -func (d *nonUpdatableAttributePlanModifier) MarkdownDescription(ctx context.Context) string { +func (d *createOnlyAttributePlanModifier) MarkdownDescription(ctx context.Context) string { return "Ensures that update operations fails when updating an attribute." } -func (d *nonUpdatableAttributePlanModifier) PlanModifyBool(ctx context.Context, req planmodifier.BoolRequest, resp *planmodifier.BoolResponse) { +func (d *createOnlyAttributePlanModifier) PlanModifyBool(ctx context.Context, req planmodifier.BoolRequest, resp *planmodifier.BoolResponse) { if d.isUpdated(req.PlanValue, req.StateValue) { d.addDiags(&resp.Diagnostics, req.Path) } } -func (d *nonUpdatableAttributePlanModifier) PlanModifyString(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) { +func (d *createOnlyAttributePlanModifier) PlanModifyString(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) { if d.isUpdated(req.PlanValue, req.StateValue) { d.addDiags(&resp.Diagnostics, req.Path) } } -func (d *nonUpdatableAttributePlanModifier) isUpdated(planValue, stateValue attr.Value) bool { +func (d *createOnlyAttributePlanModifier) isUpdated(planValue, stateValue attr.Value) bool { return !stateValue.IsNull() && !planValue.Equal(stateValue) } -func (d *nonUpdatableAttributePlanModifier) addDiags(diags *diag.Diagnostics, attrPath path.Path) { +func (d *createOnlyAttributePlanModifier) addDiags(diags *diag.Diagnostics, attrPath path.Path) { message := fmt.Sprintf("%s cannot be updated", attrPath) diags.AddError(message, message) } diff --git a/internal/service/flexcluster/resource_schema.go b/internal/service/flexcluster/resource_schema.go index fd9d4e5982..353a0afc6f 100644 --- a/internal/service/flexcluster/resource_schema.go +++ b/internal/service/flexcluster/resource_schema.go @@ -21,14 +21,14 @@ func ResourceSchema(ctx context.Context) schema.Schema { "project_id": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableAttributePlanModifier(), + customplanmodifier.CreateOnlyAttributePlanModifier(), }, MarkdownDescription: "Unique 24-hexadecimal character string that identifies the project.", }, "name": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableAttributePlanModifier(), + customplanmodifier.CreateOnlyAttributePlanModifier(), }, MarkdownDescription: "Human-readable label that identifies the instance.", }, @@ -37,7 +37,7 @@ func ResourceSchema(ctx context.Context) schema.Schema { "backing_provider_name": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableAttributePlanModifier(), + customplanmodifier.CreateOnlyAttributePlanModifier(), }, MarkdownDescription: "Cloud service provider on which MongoDB Cloud provisioned the flex cluster.", }, @@ -58,7 +58,7 @@ func ResourceSchema(ctx context.Context) schema.Schema { "region_name": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableAttributePlanModifier(), + customplanmodifier.CreateOnlyAttributePlanModifier(), }, 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/).", }, diff --git a/internal/service/project/resource_project_schema.go b/internal/service/project/resource_project_schema.go index 8cd3c69323..c40802ecc8 100644 --- a/internal/service/project/resource_project_schema.go +++ b/internal/service/project/resource_project_schema.go @@ -58,7 +58,7 @@ func ResourceSchema(ctx context.Context) schema.Schema { Optional: true, Computed: true, Default: booldefault.StaticBool(true), - PlanModifiers: []planmodifier.Bool{customplanmodifier.NonUpdatableAttributePlanModifier()}, + PlanModifiers: []planmodifier.Bool{customplanmodifier.CreateOnlyAttributePlanModifier()}, }, "is_collect_database_specifics_statistics_enabled": schema.BoolAttribute{ Computed: true, From bd76a9304308062eea5a69634cb6cbc1ddcde4dc Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Wed, 12 Mar 2025 14:46:48 +0000 Subject: [PATCH 10/13] feat: Implement CreateOnlyAttributePlanModifier with default boolean support and refactor IsKnown utility function --- .../common/customplanmodifier/create_only.go | 45 +++++++++++++++---- .../common/customplanmodifier/is_known.go | 8 ++++ .../advancedclustertpf/plan_modifier.go | 8 +--- .../project/resource_project_schema.go | 6 +-- 4 files changed, 48 insertions(+), 19 deletions(-) create mode 100644 internal/common/customplanmodifier/is_known.go diff --git a/internal/common/customplanmodifier/create_only.go b/internal/common/customplanmodifier/create_only.go index d9ca89994b..347437d12d 100644 --- a/internal/common/customplanmodifier/create_only.go +++ b/internal/common/customplanmodifier/create_only.go @@ -8,6 +8,8 @@ import ( "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" ) type Modifier interface { @@ -15,11 +17,20 @@ type Modifier interface { planmodifier.Bool } +// CreateOnlyAttributePlanModifier returns a plan modifier that ensures that update operations fails when the attribute is changed. +// This is useful for attributes only supported in create and not in update. +// Never use a schema.Default for create only attributes, instead use WithXXXDefault, the default will lead to plan changes that are not expected after import. +// Implement CopyFromPlan if the attribute is not in the API Response. func CreateOnlyAttributePlanModifier() Modifier { return &createOnlyAttributePlanModifier{} } +func CreateOnlyAttributePlanModifierWithBoolDefault(b bool) Modifier { + return &createOnlyAttributePlanModifier{defaultBool: &b} +} + type createOnlyAttributePlanModifier struct { + defaultBool *bool } func (d *createOnlyAttributePlanModifier) Description(ctx context.Context) string { @@ -27,26 +38,42 @@ func (d *createOnlyAttributePlanModifier) Description(ctx context.Context) strin } func (d *createOnlyAttributePlanModifier) MarkdownDescription(ctx context.Context) string { - return "Ensures that update operations fails when updating an attribute." + 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 isCreate(t *tfsdk.State) bool { + return t.Raw.IsNull() } func (d *createOnlyAttributePlanModifier) PlanModifyBool(ctx context.Context, req planmodifier.BoolRequest, resp *planmodifier.BoolResponse) { - if d.isUpdated(req.PlanValue, req.StateValue) { - d.addDiags(&resp.Diagnostics, req.Path) + if isCreate(&req.State) { + if !IsKnown(req.PlanValue) && d.defaultBool != nil { + resp.PlanValue = types.BoolPointerValue(d.defaultBool) + } + return + } + if isUpdated(req.StateValue, req.PlanValue) { + d.addDiags(&resp.Diagnostics, req.Path, req.StateValue) } } func (d *createOnlyAttributePlanModifier) PlanModifyString(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) { - if d.isUpdated(req.PlanValue, req.StateValue) { - d.addDiags(&resp.Diagnostics, req.Path) + if isCreate(&req.State) { + return + } + if isUpdated(req.StateValue, req.PlanValue) { + d.addDiags(&resp.Diagnostics, req.Path, req.StateValue) } } -func (d *createOnlyAttributePlanModifier) isUpdated(planValue, stateValue attr.Value) bool { - return !stateValue.IsNull() && !planValue.Equal(stateValue) +func isUpdated(state, plan attr.Value) bool { + if !IsKnown(plan) { + return false + } + return !state.Equal(plan) } -func (d *createOnlyAttributePlanModifier) addDiags(diags *diag.Diagnostics, attrPath path.Path) { - message := fmt.Sprintf("%s cannot be updated", attrPath) +func (d *createOnlyAttributePlanModifier) addDiags(diags *diag.Diagnostics, attrPath path.Path, stateValue attr.Value) { + message := fmt.Sprintf("%s cannot be updated or imported, remove it from the configuration or use state value=%v", attrPath, stateValue) diags.AddError(message, message) } diff --git a/internal/common/customplanmodifier/is_known.go b/internal/common/customplanmodifier/is_known.go new file mode 100644 index 0000000000..8eedd62889 --- /dev/null +++ b/internal/common/customplanmodifier/is_known.go @@ -0,0 +1,8 @@ +package customplanmodifier + +import "github.com/hashicorp/terraform-plugin-framework/attr" + +// IsKnown returns true if the attribute is known (not null or unknown). Note that !IsKnown is not the same as IsUnknown because null is !IsKnown but not IsUnknown. +func IsKnown(attribute attr.Value) bool { + return !attribute.IsNull() && !attribute.IsUnknown() +} diff --git a/internal/service/advancedclustertpf/plan_modifier.go b/internal/service/advancedclustertpf/plan_modifier.go index d430624d3f..f214d11dd4 100644 --- a/internal/service/advancedclustertpf/plan_modifier.go +++ b/internal/service/advancedclustertpf/plan_modifier.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" + "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/customplanmodifier" "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/schemafunc" ) @@ -266,16 +267,11 @@ func TFModelObject[T any](ctx context.Context, input types.Object) *T { } func copyAttrIfDestNotKnown[T attr.Value](src, dest *T) { - if !isKnown(*dest) { + if !customplanmodifier.IsKnown(*dest) { *dest = *src } } -// isKnown returns true if the attribute is known (not null or unknown). Note that !isKnown is not the same as IsUnknown because null is !isKnown but not IsUnknown. -func isKnown(attribute attr.Value) bool { - return !attribute.IsNull() && !attribute.IsUnknown() -} - func minLen[T any](a, b []T) int { la, lb := len(a), len(b) if la < lb { diff --git a/internal/service/project/resource_project_schema.go b/internal/service/project/resource_project_schema.go index 91af5ad358..bd17dc691c 100644 --- a/internal/service/project/resource_project_schema.go +++ b/internal/service/project/resource_project_schema.go @@ -6,7 +6,6 @@ import ( "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/resource/schema" - "github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault" "github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64planmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/objectplanmodifier" @@ -54,11 +53,10 @@ func ResourceSchema(ctx context.Context) schema.Schema { }, "with_default_alerts_settings": schema.BoolAttribute{ // Default values also must be Computed otherwise Terraform throws error: - // Schema Using Attribute Default For Non-Computed Attribute + // Provider produced invalid plan: planned an invalid value for a non-computed attribute. Optional: true, Computed: true, - Default: booldefault.StaticBool(true), - PlanModifiers: []planmodifier.Bool{customplanmodifier.CreateOnlyAttributePlanModifier()}, + PlanModifiers: []planmodifier.Bool{customplanmodifier.CreateOnlyAttributePlanModifierWithBoolDefault(true)}, }, "is_collect_database_specifics_statistics_enabled": schema.BoolAttribute{ Computed: true, From d0cb7726342a942babee4356b2b7f0707a955a43 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Wed, 12 Mar 2025 15:21:02 +0000 Subject: [PATCH 11/13] chore: small fix to planModifier using state value when Unknown in the plan --- .../common/customplanmodifier/create_only.go | 17 ++++++++++++++--- .../project/resource_project_migration_test.go | 2 +- .../service/project/resource_project_test.go | 18 +++++++++++------- internal/testutil/acc/project.go | 1 - 4 files changed, 26 insertions(+), 12 deletions(-) diff --git a/internal/common/customplanmodifier/create_only.go b/internal/common/customplanmodifier/create_only.go index 347437d12d..cc410764f1 100644 --- a/internal/common/customplanmodifier/create_only.go +++ b/internal/common/customplanmodifier/create_only.go @@ -45,9 +45,13 @@ func isCreate(t *tfsdk.State) bool { return t.Raw.IsNull() } +func (d *createOnlyAttributePlanModifier) UseDefault() bool { + return d.defaultBool != nil +} + func (d *createOnlyAttributePlanModifier) PlanModifyBool(ctx context.Context, req planmodifier.BoolRequest, resp *planmodifier.BoolResponse) { if isCreate(&req.State) { - if !IsKnown(req.PlanValue) && d.defaultBool != nil { + if !IsKnown(req.PlanValue) && d.UseDefault() { resp.PlanValue = types.BoolPointerValue(d.defaultBool) } return @@ -55,6 +59,9 @@ func (d *createOnlyAttributePlanModifier) PlanModifyBool(ctx context.Context, re if isUpdated(req.StateValue, req.PlanValue) { d.addDiags(&resp.Diagnostics, req.Path, req.StateValue) } + if !IsKnown(req.PlanValue) { + resp.PlanValue = req.StateValue + } } func (d *createOnlyAttributePlanModifier) PlanModifyString(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) { @@ -64,6 +71,9 @@ func (d *createOnlyAttributePlanModifier) PlanModifyString(ctx context.Context, if isUpdated(req.StateValue, req.PlanValue) { d.addDiags(&resp.Diagnostics, req.Path, req.StateValue) } + if !IsKnown(req.PlanValue) { + resp.PlanValue = req.StateValue + } } func isUpdated(state, plan attr.Value) bool { @@ -74,6 +84,7 @@ func isUpdated(state, plan attr.Value) bool { } func (d *createOnlyAttributePlanModifier) addDiags(diags *diag.Diagnostics, attrPath path.Path, stateValue attr.Value) { - message := fmt.Sprintf("%s cannot be updated or imported, remove it from the configuration or use state value=%v", attrPath, stateValue) - diags.AddError(message, message) + message := fmt.Sprintf("%s cannot be updated or imported, remove it from the configuration or use state value.", attrPath) + detail := fmt.Sprintf("The current state value is %s", stateValue) + diags.AddError(message, detail) } diff --git a/internal/service/project/resource_project_migration_test.go b/internal/service/project/resource_project_migration_test.go index 6e7d033f92..3d8100000b 100644 --- a/internal/service/project/resource_project_migration_test.go +++ b/internal/service/project/resource_project_migration_test.go @@ -84,7 +84,7 @@ func TestMigProject_withFalseDefaultSettings(t *testing.T) { orgID = os.Getenv("MONGODB_ATLAS_ORG_ID") projectOwnerID = os.Getenv("MONGODB_ATLAS_PROJECT_OWNER_ID") projectName = acc.RandomProjectName() - config = configWithFalseDefaultSettings(orgID, projectName, projectOwnerID) + config = configWithDefaultAlertSettings(orgID, projectName, projectOwnerID, false) ) resource.Test(t, resource.TestCase{ diff --git a/internal/service/project/resource_project_test.go b/internal/service/project/resource_project_test.go index 45ba6109aa..b7b9c83ef7 100644 --- a/internal/service/project/resource_project_test.go +++ b/internal/service/project/resource_project_test.go @@ -658,13 +658,17 @@ func TestAccProject_withFalseDefaultSettings(t *testing.T) { CheckDestroy: acc.CheckDestroyProject, Steps: []resource.TestStep{ { - Config: configWithFalseDefaultSettings(orgID, projectName, projectOwnerID), + Config: configWithDefaultAlertSettings(orgID, projectName, projectOwnerID, false), Check: resource.ComposeAggregateTestCheckFunc( checkExists(resourceName), resource.TestCheckResourceAttr(resourceName, "name", projectName), resource.TestCheckResourceAttr(resourceName, "org_id", orgID), ), }, + { + Config: configWithDefaultAlertSettings(projectName, orgID, projectOwnerID, true), + ExpectError: regexp.MustCompile("with_default_alerts_settings cannot be updated or imported, remove it from the configuration or use state value"), + }, }, }) } @@ -688,7 +692,7 @@ func TestAccProject_withUpdatedSettings(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "name", projectName), resource.TestCheckResourceAttr(resourceName, "org_id", orgID), resource.TestCheckResourceAttr(resourceName, "project_owner_id", projectOwnerID), - resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "false"), + resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "true"), // uses default value resource.TestCheckResourceAttr(resourceName, "is_collect_database_specifics_statistics_enabled", "false"), resource.TestCheckResourceAttr(resourceName, "is_data_explorer_enabled", "false"), resource.TestCheckResourceAttr(resourceName, "is_extended_storage_sizes_enabled", "false"), @@ -701,7 +705,7 @@ func TestAccProject_withUpdatedSettings(t *testing.T) { Config: acc.ConfigProjectWithSettings(projectName, orgID, projectOwnerID, true), Check: resource.ComposeAggregateTestCheckFunc( checkExists(resourceName), - resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "true"), + resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "true"), // uses default value resource.TestCheckResourceAttr(resourceName, "is_collect_database_specifics_statistics_enabled", "true"), resource.TestCheckResourceAttr(resourceName, "is_data_explorer_enabled", "true"), resource.TestCheckResourceAttr(resourceName, "is_extended_storage_sizes_enabled", "true"), @@ -714,7 +718,7 @@ func TestAccProject_withUpdatedSettings(t *testing.T) { Config: acc.ConfigProjectWithSettings(projectName, orgID, projectOwnerID, false), Check: resource.ComposeAggregateTestCheckFunc( checkExists(resourceName), - resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "false"), + resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "true"), // uses default value resource.TestCheckResourceAttr(resourceName, "is_collect_database_specifics_statistics_enabled", "false"), resource.TestCheckResourceAttr(resourceName, "is_data_explorer_enabled", "false"), resource.TestCheckResourceAttr(resourceName, "is_extended_storage_sizes_enabled", "false"), @@ -1233,15 +1237,15 @@ func configGovWithOwner(orgID, projectName, projectOwnerID string) string { `, orgID, projectName, projectOwnerID) } -func configWithFalseDefaultSettings(orgID, projectName, projectOwnerID string) string { +func configWithDefaultAlertSettings(orgID, projectName, projectOwnerID string, withDefaultAlertsSettings bool) string { return fmt.Sprintf(` resource "mongodbatlas_project" "test" { org_id = %[1]q name = %[2]q project_owner_id = %[3]q - with_default_alerts_settings = false + with_default_alerts_settings = %[4]t } - `, orgID, projectName, projectOwnerID) + `, orgID, projectName, projectOwnerID, withDefaultAlertsSettings) } func configWithLimits(orgID, projectName string, limits []*admin.DataFederationLimit) string { diff --git a/internal/testutil/acc/project.go b/internal/testutil/acc/project.go index 8885fb7253..c1d902ed4c 100644 --- a/internal/testutil/acc/project.go +++ b/internal/testutil/acc/project.go @@ -36,7 +36,6 @@ func ConfigProjectWithSettings(projectName, orgID, projectOwnerID string, value name = %[1]q org_id = %[2]q project_owner_id = %[3]q - with_default_alerts_settings = %[4]t is_collect_database_specifics_statistics_enabled = %[4]t is_data_explorer_enabled = %[4]t is_extended_storage_sizes_enabled = %[4]t From 57f8e64c595c0d7417a88563724e0c7ff4999e30 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Thu, 13 Mar 2025 09:43:56 +0000 Subject: [PATCH 12/13] test: Support testing plan error after importing --- .../service/project/resource_project_test.go | 37 ++++++++++++++++--- internal/testutil/mig/test_step.go | 14 ++++--- 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/internal/service/project/resource_project_test.go b/internal/service/project/resource_project_test.go index b7b9c83ef7..ca091490e4 100644 --- a/internal/service/project/resource_project_test.go +++ b/internal/service/project/resource_project_test.go @@ -22,6 +22,7 @@ import ( "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/conversion" "github.com/mongodb/terraform-provider-mongodbatlas/internal/service/project" "github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/acc" + "github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/mig" ) var ( @@ -647,9 +648,20 @@ func TestAccGovProject_withProjectOwner(t *testing.T) { func TestAccProject_withFalseDefaultSettings(t *testing.T) { var ( - orgID = os.Getenv("MONGODB_ATLAS_ORG_ID") - projectOwnerID = os.Getenv("MONGODB_ATLAS_PROJECT_OWNER_ID") - projectName = acc.RandomProjectName() + orgID = os.Getenv("MONGODB_ATLAS_ORG_ID") + projectOwnerID = os.Getenv("MONGODB_ATLAS_PROJECT_OWNER_ID") + projectName = acc.RandomProjectName() + importResourceName = resourceName + "2" + alertSettingsFalse = configWithDefaultAlertSettings(orgID, projectName, projectOwnerID, false) + alertSettingsTrue = configWithDefaultAlertSettings(orgID, projectName, projectOwnerID, true) + // To test plan behavior after import it is necessary to use a different resource name, otherwise we get: + // Terraform is already managing a remote object for mongodbatlas_project.test. To import to this address you must first remove the existing object from the state. + // This happens because `ImportStatePersist` uses the previous WorkingDirectory where the state from previous steps are saved + // resource "mongodbatlas_project" "test" --> resource "mongodbatlas_project" "test2" + alertSettingsFalseImport = strings.Replace(alertSettingsFalse, "test", "test2", 1) + // Need BOTH mongodbatlas_project.test and mongodbatlas_project.test2, otherwise we get: + // expected empty plan, but mongodbatlas_project.test has planned action(s): [delete] + alertSettingsAbsent = alertSettingsFalse + strings.Replace(configBasic(orgID, projectName, "", false, nil, nil), "test", "test2", 1) ) resource.ParallelTest(t, resource.TestCase{ @@ -658,7 +670,7 @@ func TestAccProject_withFalseDefaultSettings(t *testing.T) { CheckDestroy: acc.CheckDestroyProject, Steps: []resource.TestStep{ { - Config: configWithDefaultAlertSettings(orgID, projectName, projectOwnerID, false), + Config: alertSettingsFalse, Check: resource.ComposeAggregateTestCheckFunc( checkExists(resourceName), resource.TestCheckResourceAttr(resourceName, "name", projectName), @@ -666,9 +678,24 @@ func TestAccProject_withFalseDefaultSettings(t *testing.T) { ), }, { - Config: configWithDefaultAlertSettings(projectName, orgID, projectOwnerID, true), + Config: alertSettingsTrue, + ExpectError: regexp.MustCompile("with_default_alerts_settings cannot be updated or imported, remove it from the configuration or use state value"), + }, + { + Config: alertSettingsFalseImport, + ResourceName: importResourceName, + ImportStateIdFunc: acc.ImportStateProjectIDFunc(resourceName), + ImportState: true, + ImportStatePersist: true, // save the state to use it in the next plan + }, + { + Config: alertSettingsFalseImport, // when the value is set after import, the first plan will fail since the value cannot be read from API and the plan modifier will detect the change from null --> false ExpectError: regexp.MustCompile("with_default_alerts_settings cannot be updated or imported, remove it from the configuration or use state value"), }, + { + Config: alertSettingsAbsent, // removing `with_default_alerts_settings` from the configuration should have no plan changes + ConfigPlanChecks: mig.TestStepConfigPlanChecksEmptyPlan(), + }, }, }) } diff --git a/internal/testutil/mig/test_step.go b/internal/testutil/mig/test_step.go index 4de74c2f24..b116b5f1f6 100644 --- a/internal/testutil/mig/test_step.go +++ b/internal/testutil/mig/test_step.go @@ -10,11 +10,15 @@ func TestStepCheckEmptyPlan(config string) resource.TestStep { return resource.TestStep{ ProtoV6ProviderFactories: acc.TestAccProviderV6Factories, Config: config, - ConfigPlanChecks: resource.ConfigPlanChecks{ - PreApply: []plancheck.PlanCheck{ - acc.DebugPlan(), - plancheck.ExpectEmptyPlan(), - }, + ConfigPlanChecks: TestStepConfigPlanChecksEmptyPlan(), + } +} + +func TestStepConfigPlanChecksEmptyPlan() resource.ConfigPlanChecks { + return resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + acc.DebugPlan(), + plancheck.ExpectEmptyPlan(), }, } } From 4ae2cccf71e62392eded33c2cddd7e35b04ae793 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Thu, 13 Mar 2025 10:02:46 +0000 Subject: [PATCH 13/13] doc: Update error message in addDiags to clarify import restrictions --- internal/common/customplanmodifier/create_only.go | 2 +- internal/service/project/resource_project_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/common/customplanmodifier/create_only.go b/internal/common/customplanmodifier/create_only.go index cc410764f1..65e664b306 100644 --- a/internal/common/customplanmodifier/create_only.go +++ b/internal/common/customplanmodifier/create_only.go @@ -84,7 +84,7 @@ func isUpdated(state, plan attr.Value) bool { } func (d *createOnlyAttributePlanModifier) addDiags(diags *diag.Diagnostics, attrPath path.Path, stateValue attr.Value) { - message := fmt.Sprintf("%s cannot be updated or imported, remove it from the configuration or use state value.", attrPath) + message := fmt.Sprintf("%s cannot be updated or set after import, remove it from the configuration or use state value.", attrPath) detail := fmt.Sprintf("The current state value is %s", stateValue) diags.AddError(message, detail) } diff --git a/internal/service/project/resource_project_test.go b/internal/service/project/resource_project_test.go index ca091490e4..d423f45355 100644 --- a/internal/service/project/resource_project_test.go +++ b/internal/service/project/resource_project_test.go @@ -679,7 +679,7 @@ func TestAccProject_withFalseDefaultSettings(t *testing.T) { }, { Config: alertSettingsTrue, - ExpectError: regexp.MustCompile("with_default_alerts_settings cannot be updated or imported, remove it from the configuration or use state value"), + ExpectError: regexp.MustCompile("with_default_alerts_settings cannot be updated or set after import, remove it from the configuration or use state value"), }, { Config: alertSettingsFalseImport, @@ -690,7 +690,7 @@ func TestAccProject_withFalseDefaultSettings(t *testing.T) { }, { Config: alertSettingsFalseImport, // when the value is set after import, the first plan will fail since the value cannot be read from API and the plan modifier will detect the change from null --> false - ExpectError: regexp.MustCompile("with_default_alerts_settings cannot be updated or imported, remove it from the configuration or use state value"), + ExpectError: regexp.MustCompile("with_default_alerts_settings cannot be updated or set after import, remove it from the configuration or use state value"), }, { Config: alertSettingsAbsent, // removing `with_default_alerts_settings` from the configuration should have no plan changes