From 53de46328c5a72eb7dafbccc6d91a858a9c0d674 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Tue, 25 Feb 2025 09:55:06 +0000 Subject: [PATCH 01/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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 From c8abacd13efeb4b9250bbe669ffd07bb9e5e8cf6 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Fri, 11 Jul 2025 09:10:07 +0100 Subject: [PATCH 14/20] chore: revert old changes for advancedclustertpf --- .../advancedclustertpf/plan_modifier.go | 51 ++++++------------- .../project/resource_project_schema.go | 1 + 2 files changed, 16 insertions(+), 36 deletions(-) diff --git a/internal/service/advancedclustertpf/plan_modifier.go b/internal/service/advancedclustertpf/plan_modifier.go index f0a31cd97b..d430624d3f 100644 --- a/internal/service/advancedclustertpf/plan_modifier.go +++ b/internal/service/advancedclustertpf/plan_modifier.go @@ -9,7 +9,6 @@ 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" ) @@ -18,6 +17,7 @@ var ( attributeRootChangeMapping = map[string][]string{ "disk_size_gb": {}, // disk_size_gb can be change at any level/spec "replication_specs": {}, + "mongo_db_major_version": {"mongo_db_version"}, "tls_cipher_config_mode": {"custom_openssl_cipher_config_tls12"}, "cluster_type": {"config_server_management_mode", "config_server_type"}, // computed values of config server change when REPLICA_SET changes to SHARDED } @@ -62,7 +62,7 @@ func useStateForUnknowns(ctx context.Context, diags *diag.Diagnostics, state, pl return } attributeChanges := schemafunc.NewAttributeChanges(ctx, state, plan) - keepUnknown := []string{"connection_strings", "state_name", "mongo_db_version"} // Volatile attributes, should not be copied from state + keepUnknown := []string{"connection_strings", "state_name"} // Volatile attributes, should not be copied from state keepUnknown = append(keepUnknown, attributeChanges.KeepUnknown(attributeRootChangeMapping)...) keepUnknown = append(keepUnknown, determineKeepUnknownsAutoScaling(ctx, diags, state, plan)...) schemafunc.CopyUnknowns(ctx, state, plan, keepUnknown, nil) @@ -114,39 +114,23 @@ func AdjustRegionConfigsChildren(ctx context.Context, diags *diag.Diagnostics, s for i := range minLen(planRepSpecsTF, stateRepSpecsTF) { stateRegionConfigsTF := TFModelList[TFRegionConfigsModel](ctx, diags, stateRepSpecsTF[i].RegionConfigs) planRegionConfigsTF := TFModelList[TFRegionConfigsModel](ctx, diags, planRepSpecsTF[i].RegionConfigs) - planElectableSpecInReplicationSpec := findDefinedElectableSpecInReplicationSpec(ctx, planRegionConfigsTF) if diags.HasError() { return } for j := range minLen(planRegionConfigsTF, stateRegionConfigsTF) { - stateElectableSpecs := TFModelObject[TFSpecsModel](ctx, stateRegionConfigsTF[j].ElectableSpecs) - planElectableSpecs := TFModelObject[TFSpecsModel](ctx, planRegionConfigsTF[j].ElectableSpecs) - if planElectableSpecs == nil && stateElectableSpecs != nil && stateElectableSpecs.NodeCount.ValueInt64() > 0 { - planRegionConfigsTF[j].ElectableSpecs = stateRegionConfigsTF[j].ElectableSpecs - planElectableSpecs = stateElectableSpecs - } stateReadOnlySpecs := TFModelObject[TFSpecsModel](ctx, stateRegionConfigsTF[j].ReadOnlySpecs) planReadOnlySpecs := TFModelObject[TFSpecsModel](ctx, planRegionConfigsTF[j].ReadOnlySpecs) - if stateReadOnlySpecs != nil { // read_only_specs is present in state - // logic below ensures that if read only specs is present in state but not in the plan, plan will be populated so that read only spec configuration is not removed on update operations + planElectableSpecs := TFModelObject[TFSpecsModel](ctx, planRegionConfigsTF[j].ElectableSpecs) + if stateReadOnlySpecs != nil && planElectableSpecs != nil { // read_only_specs is present in state and electable_specs in the plan newPlanReadOnlySpecs := planReadOnlySpecs if newPlanReadOnlySpecs == nil { newPlanReadOnlySpecs = new(TFSpecsModel) // start with null attributes if not present plan } - baseReadOnlySpecs := stateReadOnlySpecs // using values directly from state if no electable specs are present in plan - if planElectableSpecInReplicationSpec != nil { // ensures values are taken from a defined electable spec if not present in current region config - baseReadOnlySpecs = planElectableSpecInReplicationSpec - } - if planElectableSpecs != nil { - // we favor plan electable spec defined in same region config over one defined in replication spec - // with current API this is redudant but is more future proof in case scaling between regions becomes independent in the future - baseReadOnlySpecs = planElectableSpecs - } - copyAttrIfDestNotKnown(&baseReadOnlySpecs.DiskSizeGb, &newPlanReadOnlySpecs.DiskSizeGb) - copyAttrIfDestNotKnown(&baseReadOnlySpecs.EbsVolumeType, &newPlanReadOnlySpecs.EbsVolumeType) - copyAttrIfDestNotKnown(&baseReadOnlySpecs.InstanceSize, &newPlanReadOnlySpecs.InstanceSize) - copyAttrIfDestNotKnown(&baseReadOnlySpecs.DiskIops, &newPlanReadOnlySpecs.DiskIops) - // unknown node_count is always taken from state as it not dependent on electable_specs changes + // unknown node_count is got from state, all other unknowns are got from electable_specs plan + copyAttrIfDestNotKnown(&planElectableSpecs.DiskSizeGb, &newPlanReadOnlySpecs.DiskSizeGb) + copyAttrIfDestNotKnown(&planElectableSpecs.EbsVolumeType, &newPlanReadOnlySpecs.EbsVolumeType) + copyAttrIfDestNotKnown(&planElectableSpecs.InstanceSize, &newPlanReadOnlySpecs.InstanceSize) + copyAttrIfDestNotKnown(&planElectableSpecs.DiskIops, &newPlanReadOnlySpecs.DiskIops) copyAttrIfDestNotKnown(&stateReadOnlySpecs.NodeCount, &newPlanReadOnlySpecs.NodeCount) objType, diagsLocal := types.ObjectValueFrom(ctx, SpecsObjType.AttrTypes, newPlanReadOnlySpecs) diags.Append(diagsLocal...) @@ -201,16 +185,6 @@ func AdjustRegionConfigsChildren(ctx context.Context, diags *diag.Diagnostics, s plan.ReplicationSpecs = listRepSpecs } -func findDefinedElectableSpecInReplicationSpec(ctx context.Context, regionConfigs []TFRegionConfigsModel) *TFSpecsModel { - for i := range regionConfigs { - electableSpecs := TFModelObject[TFSpecsModel](ctx, regionConfigs[i].ElectableSpecs) - if electableSpecs != nil { - return electableSpecs - } - } - return nil -} - // determineKeepUnknownsChangedReplicationSpec: These fields must be kept unknown in the replication_specs[index_of_changes] func determineKeepUnknownsChangedReplicationSpec(keepUnknownsAlways []string, attributeChanges *schemafunc.AttributeChanges, parentPath string) []string { var keepUnknowns = slices.Clone(keepUnknownsAlways) @@ -292,11 +266,16 @@ func TFModelObject[T any](ctx context.Context, input types.Object) *T { } func copyAttrIfDestNotKnown[T attr.Value](src, dest *T) { - if !customplanmodifier.IsKnown(*dest) { + if !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 258a4ae819..4aa52f9b43 100644 --- a/internal/service/project/resource_project_schema.go +++ b/internal/service/project/resource_project_schema.go @@ -14,6 +14,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/v20250312005/admin" ) From 078b07cc25cea172e4fe36ab55c9e88fb8821848 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Fri, 11 Jul 2025 09:18:21 +0100 Subject: [PATCH 15/20] test: remove migration test util in favor of acc test step for empty plan check --- internal/service/project/resource_project_test.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/internal/service/project/resource_project_test.go b/internal/service/project/resource_project_test.go index e6e949ad68..041e21d212 100644 --- a/internal/service/project/resource_project_test.go +++ b/internal/service/project/resource_project_test.go @@ -22,7 +22,6 @@ 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 ( @@ -688,14 +687,7 @@ func TestAccProject_withFalseDefaultSettings(t *testing.T) { 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 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 - ConfigPlanChecks: mig.TestStepConfigPlanChecksEmptyPlan(), - }, + acc.TestStepCheckEmptyPlan(alertSettingsAbsent), }, }) } From fae76d431feb7c7b031bc00089bd128c3340719d Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Fri, 11 Jul 2025 09:22:00 +0100 Subject: [PATCH 16/20] revert old changes --- .../advancedclustertpf/plan_modifier.go | 43 +++++++++++++++---- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/internal/service/advancedclustertpf/plan_modifier.go b/internal/service/advancedclustertpf/plan_modifier.go index d430624d3f..183ac46efa 100644 --- a/internal/service/advancedclustertpf/plan_modifier.go +++ b/internal/service/advancedclustertpf/plan_modifier.go @@ -17,7 +17,6 @@ var ( attributeRootChangeMapping = map[string][]string{ "disk_size_gb": {}, // disk_size_gb can be change at any level/spec "replication_specs": {}, - "mongo_db_major_version": {"mongo_db_version"}, "tls_cipher_config_mode": {"custom_openssl_cipher_config_tls12"}, "cluster_type": {"config_server_management_mode", "config_server_type"}, // computed values of config server change when REPLICA_SET changes to SHARDED } @@ -62,7 +61,7 @@ func useStateForUnknowns(ctx context.Context, diags *diag.Diagnostics, state, pl return } attributeChanges := schemafunc.NewAttributeChanges(ctx, state, plan) - keepUnknown := []string{"connection_strings", "state_name"} // Volatile attributes, should not be copied from state + keepUnknown := []string{"connection_strings", "state_name", "mongo_db_version"} // Volatile attributes, should not be copied from state keepUnknown = append(keepUnknown, attributeChanges.KeepUnknown(attributeRootChangeMapping)...) keepUnknown = append(keepUnknown, determineKeepUnknownsAutoScaling(ctx, diags, state, plan)...) schemafunc.CopyUnknowns(ctx, state, plan, keepUnknown, nil) @@ -114,23 +113,39 @@ func AdjustRegionConfigsChildren(ctx context.Context, diags *diag.Diagnostics, s for i := range minLen(planRepSpecsTF, stateRepSpecsTF) { stateRegionConfigsTF := TFModelList[TFRegionConfigsModel](ctx, diags, stateRepSpecsTF[i].RegionConfigs) planRegionConfigsTF := TFModelList[TFRegionConfigsModel](ctx, diags, planRepSpecsTF[i].RegionConfigs) + planElectableSpecInReplicationSpec := findDefinedElectableSpecInReplicationSpec(ctx, planRegionConfigsTF) if diags.HasError() { return } for j := range minLen(planRegionConfigsTF, stateRegionConfigsTF) { + stateElectableSpecs := TFModelObject[TFSpecsModel](ctx, stateRegionConfigsTF[j].ElectableSpecs) + planElectableSpecs := TFModelObject[TFSpecsModel](ctx, planRegionConfigsTF[j].ElectableSpecs) + if planElectableSpecs == nil && stateElectableSpecs != nil && stateElectableSpecs.NodeCount.ValueInt64() > 0 { + planRegionConfigsTF[j].ElectableSpecs = stateRegionConfigsTF[j].ElectableSpecs + planElectableSpecs = stateElectableSpecs + } stateReadOnlySpecs := TFModelObject[TFSpecsModel](ctx, stateRegionConfigsTF[j].ReadOnlySpecs) planReadOnlySpecs := TFModelObject[TFSpecsModel](ctx, planRegionConfigsTF[j].ReadOnlySpecs) - planElectableSpecs := TFModelObject[TFSpecsModel](ctx, planRegionConfigsTF[j].ElectableSpecs) - if stateReadOnlySpecs != nil && planElectableSpecs != nil { // read_only_specs is present in state and electable_specs in the plan + if stateReadOnlySpecs != nil { // read_only_specs is present in state + // logic below ensures that if read only specs is present in state but not in the plan, plan will be populated so that read only spec configuration is not removed on update operations newPlanReadOnlySpecs := planReadOnlySpecs if newPlanReadOnlySpecs == nil { newPlanReadOnlySpecs = new(TFSpecsModel) // start with null attributes if not present plan } - // unknown node_count is got from state, all other unknowns are got from electable_specs plan - copyAttrIfDestNotKnown(&planElectableSpecs.DiskSizeGb, &newPlanReadOnlySpecs.DiskSizeGb) - copyAttrIfDestNotKnown(&planElectableSpecs.EbsVolumeType, &newPlanReadOnlySpecs.EbsVolumeType) - copyAttrIfDestNotKnown(&planElectableSpecs.InstanceSize, &newPlanReadOnlySpecs.InstanceSize) - copyAttrIfDestNotKnown(&planElectableSpecs.DiskIops, &newPlanReadOnlySpecs.DiskIops) + baseReadOnlySpecs := stateReadOnlySpecs // using values directly from state if no electable specs are present in plan + if planElectableSpecInReplicationSpec != nil { // ensures values are taken from a defined electable spec if not present in current region config + baseReadOnlySpecs = planElectableSpecInReplicationSpec + } + if planElectableSpecs != nil { + // we favor plan electable spec defined in same region config over one defined in replication spec + // with current API this is redudant but is more future proof in case scaling between regions becomes independent in the future + baseReadOnlySpecs = planElectableSpecs + } + copyAttrIfDestNotKnown(&baseReadOnlySpecs.DiskSizeGb, &newPlanReadOnlySpecs.DiskSizeGb) + copyAttrIfDestNotKnown(&baseReadOnlySpecs.EbsVolumeType, &newPlanReadOnlySpecs.EbsVolumeType) + copyAttrIfDestNotKnown(&baseReadOnlySpecs.InstanceSize, &newPlanReadOnlySpecs.InstanceSize) + copyAttrIfDestNotKnown(&baseReadOnlySpecs.DiskIops, &newPlanReadOnlySpecs.DiskIops) + // unknown node_count is always taken from state as it not dependent on electable_specs changes copyAttrIfDestNotKnown(&stateReadOnlySpecs.NodeCount, &newPlanReadOnlySpecs.NodeCount) objType, diagsLocal := types.ObjectValueFrom(ctx, SpecsObjType.AttrTypes, newPlanReadOnlySpecs) diags.Append(diagsLocal...) @@ -185,6 +200,16 @@ func AdjustRegionConfigsChildren(ctx context.Context, diags *diag.Diagnostics, s plan.ReplicationSpecs = listRepSpecs } +func findDefinedElectableSpecInReplicationSpec(ctx context.Context, regionConfigs []TFRegionConfigsModel) *TFSpecsModel { + for i := range regionConfigs { + electableSpecs := TFModelObject[TFSpecsModel](ctx, regionConfigs[i].ElectableSpecs) + if electableSpecs != nil { + return electableSpecs + } + } + return nil +} + // determineKeepUnknownsChangedReplicationSpec: These fields must be kept unknown in the replication_specs[index_of_changes] func determineKeepUnknownsChangedReplicationSpec(keepUnknownsAlways []string, attributeChanges *schemafunc.AttributeChanges, parentPath string) []string { var keepUnknowns = slices.Clone(keepUnknownsAlways) From 9bb557b3fc665c40caabe32eb6e51482b4d71731 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Fri, 11 Jul 2025 09:39:27 +0100 Subject: [PATCH 17/20] doc: Updates docs with latest API docs --- .changelog/3105.txt | 3 --- docs/resources/project.md | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) delete mode 100644 .changelog/3105.txt diff --git a/.changelog/3105.txt b/.changelog/3105.txt deleted file mode 100644 index 67a00812f4..0000000000 --- a/.changelog/3105.txt +++ /dev/null @@ -1,3 +0,0 @@ -```release-note:bug -resource/mongodbatlas_project: Fixes import when `with_default_alerts_settings` is set -``` diff --git a/docs/resources/project.md b/docs/resources/project.md index fcec8a465c..69bc2ec872 100644 --- a/docs/resources/project.md +++ b/docs/resources/project.md @@ -51,7 +51,7 @@ resource "mongodbatlas_project" "test" { * `org_id` - (Required) The ID of the organization you want to create the project within. * `project_owner_id` - (Optional) Unique 24-hexadecimal digit string that identifies the Atlas user account to be granted the [Project Owner](https://docs.atlas.mongodb.com/reference/user-roles/#mongodb-authrole-Project-Owner) role on the specified project. If you set this parameter, it overrides the default value of the oldest [Organization Owner](https://docs.atlas.mongodb.com/reference/user-roles/#mongodb-authrole-Organization-Owner). * `tags` - (Optional) Map that contains key-value pairs between 1 to 255 characters in length for tagging and categorizing the project. See [below](#tags). -* `with_default_alerts_settings` - (Optional) It allows users to disable the creation of the default alert settings. By default, this flag is set to true. +* `with_default_alerts_settings` - (Optional) Flag that indicates whether to create the project with default alert settings. This setting cannot be updated after project creation. * `is_collect_database_specifics_statistics_enabled` - (Optional) Flag that indicates whether to enable statistics in [cluster metrics](https://www.mongodb.com/docs/atlas/monitor-cluster-metrics/) collection for the project. By default, this flag is set to true. * `is_data_explorer_enabled` - (Optional) Flag that indicates whether to enable Data Explorer for the project. If enabled, you can query your database with an easy to use interface. When Data Explorer is disabled, you cannot terminate slow operations from the [Real-Time Performance Panel](https://www.mongodb.com/docs/atlas/real-time-performance-panel/#std-label-real-time-metrics-status-tab) or create indexes from the [Performance Advisor](https://www.mongodb.com/docs/atlas/performance-advisor/#std-label-performance-advisor). You can still view Performance Advisor recommendations, but you must create those indexes from [mongosh](https://www.mongodb.com/docs/mongodb-shell/#mongodb-binary-bin.mongosh). By default, this flag is set to true. * `is_extended_storage_sizes_enabled` - (Optional) Flag that indicates whether to enable extended storage sizes for the specified project. Clusters with extended storage sizes must be on AWS or GCP, and cannot span multiple regions. When extending storage size, initial syncs and cross-project snapshot restores will be slow. This setting should only be used as a measure of temporary relief; consider sharding if more storage is required. From 6a551b7e22827199c9eeb93f94cc5bf1fc577bbf Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Fri, 11 Jul 2025 09:40:30 +0100 Subject: [PATCH 18/20] refactor: remove the withDefaultAlertSettings override for plan --- internal/service/project/resource_project.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/service/project/resource_project.go b/internal/service/project/resource_project.go index f54db63baf..46357ab6c5 100644 --- a/internal/service/project/resource_project.go +++ b/internal/service/project/resource_project.go @@ -335,7 +335,6 @@ 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 projectPlanNewPtr.ProjectOwnerID = projectPlan.ProjectOwnerID if projectPlan.Tags.IsNull() && len(projectPlanNewPtr.Tags.Elements()) == 0 { projectPlanNewPtr.Tags = types.MapNull(types.StringType) From 954229ac7b863e2c83440398454ac8107892de98 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Fri, 11 Jul 2025 09:40:57 +0100 Subject: [PATCH 19/20] refactor: Add create only attribute plan modifier for project_owner_id --- internal/service/project/resource_project_schema.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/service/project/resource_project_schema.go b/internal/service/project/resource_project_schema.go index 4aa52f9b43..83601426ab 100644 --- a/internal/service/project/resource_project_schema.go +++ b/internal/service/project/resource_project_schema.go @@ -50,6 +50,9 @@ func ResourceSchema(ctx context.Context) schema.Schema { }, "project_owner_id": schema.StringAttribute{ Optional: true, + PlanModifiers: []planmodifier.String{ + customplanmodifier.CreateOnlyAttributePlanModifier(), + }, }, "with_default_alerts_settings": schema.BoolAttribute{ // Default values also must be Computed otherwise Terraform throws error: From 8a032488d353c1cd9e88d5d04a136c6bac4d8b91 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Fri, 11 Jul 2025 17:23:15 +0100 Subject: [PATCH 20/20] chore: update related docs --- internal/common/customplanmodifier/create_only.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/common/customplanmodifier/create_only.go b/internal/common/customplanmodifier/create_only.go index 65e664b306..b368c7f89d 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 set after import, 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 the state value (see below).", attrPath) detail := fmt.Sprintf("The current state value is %s", stateValue) diags.AddError(message, detail) }