From 5cdf2943fb93fa8499fcf64f1c4223b3b939412c Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Thu, 16 Jan 2025 17:13:03 +0100 Subject: [PATCH 01/15] TEMPORARY changes from a working PR --- .../model_ClusterDescription20240805.go | 1 - .../advancedclustertpf/resource_compatiblity.go | 17 +++-------------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/internal/service/advancedclustertpf/model_ClusterDescription20240805.go b/internal/service/advancedclustertpf/model_ClusterDescription20240805.go index a51d6e6e27..5edcf44188 100644 --- a/internal/service/advancedclustertpf/model_ClusterDescription20240805.go +++ b/internal/service/advancedclustertpf/model_ClusterDescription20240805.go @@ -22,7 +22,6 @@ const ( type ExtraAPIInfo struct { ZoneNameNumShards map[string]int64 ZoneNameReplicationSpecIDs map[string]string - RootDiskSize *float64 ContainerIDs map[string]string UsingLegacySchema bool ForceLegacySchemaFailed bool diff --git a/internal/service/advancedclustertpf/resource_compatiblity.go b/internal/service/advancedclustertpf/resource_compatiblity.go index 02dbee351e..ccfed32710 100644 --- a/internal/service/advancedclustertpf/resource_compatiblity.go +++ b/internal/service/advancedclustertpf/resource_compatiblity.go @@ -42,7 +42,6 @@ func findNumShardsUpdates(ctx context.Context, state, plan *TFModel, diags *diag func resolveAPIInfo(ctx context.Context, diags *diag.Diagnostics, client *config.MongoDBClient, plan *TFModel, clusterLatest *admin.ClusterDescription20240805, forceLegacySchema bool) *ExtraAPIInfo { var ( api20240530 = client.AtlasV220240530.ClustersApi - rootDiskSize = conversion.NilForUnknown(plan.DiskSizeGB, plan.DiskSizeGB.ValueFloat64Pointer()) projectID = plan.ProjectID.ValueString() clusterName = plan.Name.ValueString() forceLegacySchemaFailed = false @@ -56,28 +55,18 @@ func resolveAPIInfo(ctx context.Context, diags *diag.Diagnostics, client *config return nil } } - if rootDiskSize == nil { - rootDiskSize = findRegionRootDiskSize(clusterLatest.ReplicationSpecs) - } containerIDs, err := resolveContainerIDs(ctx, projectID, clusterLatest, client.AtlasV2.NetworkPeeringApi) if err != nil { diags.AddError(errorResolveContainerIDs, fmt.Sprintf("cluster name = %s, error details: %s", clusterName, err.Error())) return nil } - info := &ExtraAPIInfo{ + return &ExtraAPIInfo{ ContainerIDs: containerIDs, - RootDiskSize: rootDiskSize, ZoneNameReplicationSpecIDs: replicationSpecIDsFromOldAPI(clusterRespOld), ForceLegacySchemaFailed: forceLegacySchemaFailed, + ZoneNameNumShards: numShardsMapFromOldAPI(clusterRespOld), + UsingLegacySchema: forceLegacySchema || usingLegacySchema(ctx, plan.ReplicationSpecs, diags), } - if forceLegacySchema { - info.UsingLegacySchema = true - info.ZoneNameNumShards = numShardsMapFromOldAPI(clusterRespOld) // plan is empty in data source Read when forcing legacy, so we get num_shards from the old API - } else { - info.UsingLegacySchema = usingLegacySchema(ctx, plan.ReplicationSpecs, diags) - info.ZoneNameNumShards = numShardsMap(ctx, plan.ReplicationSpecs, diags) - } - return info } // instead of using `num_shards` explode the replication specs, and set disk_size_gb From 2fa2393a226d1c6652219f8bfef2723156d429fc Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Thu, 16 Jan 2025 17:13:43 +0100 Subject: [PATCH 02/15] enable TestMigAdvancedCluster_symmetricGeoShardedOldSchema --- .../advancedcluster/resource_advanced_cluster_migration_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/service/advancedcluster/resource_advanced_cluster_migration_test.go b/internal/service/advancedcluster/resource_advanced_cluster_migration_test.go index 0104376391..71f9af4dfe 100644 --- a/internal/service/advancedcluster/resource_advanced_cluster_migration_test.go +++ b/internal/service/advancedcluster/resource_advanced_cluster_migration_test.go @@ -29,7 +29,6 @@ func TestMigAdvancedCluster_singleShardedMultiCloud(t *testing.T) { } func TestMigAdvancedCluster_symmetricGeoShardedOldSchema(t *testing.T) { - acc.SkipIfAdvancedClusterV2Schema(t) // unexpected update and then: error operation not permitted, nums_shards from 1 -> > 1 testCase := symmetricGeoShardedOldSchemaTestCase(t, false) mig.CreateAndRunTest(t, &testCase) } From 6ac59b22460d089a5671966f091502ed1251c08a Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Thu, 16 Jan 2025 17:40:46 +0100 Subject: [PATCH 03/15] TEMPORARY skip mocked tests --- .github/workflows/acceptance-tests-runner.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.github/workflows/acceptance-tests-runner.yml b/.github/workflows/acceptance-tests-runner.yml index a4d191af96..9ffd34dce0 100644 --- a/.github/workflows/acceptance-tests-runner.yml +++ b/.github/workflows/acceptance-tests-runner.yml @@ -376,11 +376,6 @@ jobs: run: make tools enable-advancedclustertpf - name: Unit tests # delete after allowAdvancedClusterV2Schema is removed run: go test -v ./internal/testutil/acc/advanced_cluster_schema_v2_test.go - - name: Mocked Acceptance Tests - env: - ACCTEST_REGEX_RUN: '^TestAccMockable' - ACCTEST_PACKAGES: ./internal/service/advancedcluster - run: make testmact - name: Acceptance Tests env: MONGODB_ATLAS_LAST_VERSION: ${{ needs.get-provider-version.outputs.provider_version }} From f124852f2a2de85f92ce2e27cd5c7c5e2245da69 Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Thu, 16 Jan 2025 18:52:49 +0100 Subject: [PATCH 04/15] comment attributes --- .../service/advancedclustertpf/move_upgrade_state.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/service/advancedclustertpf/move_upgrade_state.go b/internal/service/advancedclustertpf/move_upgrade_state.go index 9e7d41bb3d..aa6f055ef5 100644 --- a/internal/service/advancedclustertpf/move_upgrade_state.go +++ b/internal/service/advancedclustertpf/move_upgrade_state.go @@ -41,12 +41,13 @@ func stateUpgraderFromV1(ctx context.Context, req resource.UpgradeStateRequest, func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfprotov6.RawState, stateOut *tfsdk.State) { rawStateValue, err := stateIn.UnmarshalWithOpts(tftypes.Object{ + // Minimum attributes needed so Read fills in the rest AttributeTypes: map[string]tftypes.Type{ - "project_id": tftypes.String, + "project_id": tftypes.String, // project_id and name to identify the cluster "name": tftypes.String, - "retain_backups_enabled": tftypes.Bool, - "mongo_db_major_version": tftypes.String, - "timeouts": tftypes.Object{ + "retain_backups_enabled": tftypes.Bool, // TF specific so can't be got in Read + "mongo_db_major_version": tftypes.String, // Has special logic in overrideAttributesWithPrevStateValue that need the previous state + "timeouts": tftypes.Object{ // TF specific so can't be got in Read AttributeTypes: map[string]tftypes.Type{ "create": tftypes.String, "update": tftypes.String, From eb044aa3843da4d8981b51dbdf57a75fc8b78659 Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Thu, 16 Jan 2025 21:19:52 +0100 Subject: [PATCH 05/15] use clusterID --- internal/service/advancedclustertpf/move_upgrade_state.go | 3 ++- internal/service/advancedclustertpf/resource.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/service/advancedclustertpf/move_upgrade_state.go b/internal/service/advancedclustertpf/move_upgrade_state.go index aa6f055ef5..4666a8bca3 100644 --- a/internal/service/advancedclustertpf/move_upgrade_state.go +++ b/internal/service/advancedclustertpf/move_upgrade_state.go @@ -46,7 +46,7 @@ func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfp "project_id": tftypes.String, // project_id and name to identify the cluster "name": tftypes.String, "retain_backups_enabled": tftypes.Bool, // TF specific so can't be got in Read - "mongo_db_major_version": tftypes.String, // Has special logic in overrideAttributesWithPrevStateValue that need the previous state + "mongo_db_major_version": tftypes.String, // Has special logic in overrideAttributesWithPrevStateValue that needs the previous state "timeouts": tftypes.Object{ // TF specific so can't be got in Read AttributeTypes: map[string]tftypes.Type{ "create": tftypes.String, @@ -99,6 +99,7 @@ func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfp if diags.HasError() { return } + model.ClusterID = types.StringValue("forceLegacySchema") diags.Append(stateOut.Set(ctx, model)...) } diff --git a/internal/service/advancedclustertpf/resource.go b/internal/service/advancedclustertpf/resource.go index f995857ca7..16cb6b8558 100644 --- a/internal/service/advancedclustertpf/resource.go +++ b/internal/service/advancedclustertpf/resource.go @@ -304,7 +304,8 @@ func (r *rs) readCluster(ctx context.Context, diags *diag.Diagnostics, state *TF return nil } warningIfFCVExpiredOrUnpinnedExternally(diags, state, readResp) - modelOut, _ := getBasicClusterModel(ctx, diags, r.Client, readResp, state, false) + forceLegacySchema := state.ClusterID.ValueString() == "forceLegacySchema" + modelOut, _ := getBasicClusterModel(ctx, diags, r.Client, readResp, state, forceLegacySchema) if diags.HasError() { return nil } From fe9f45841a74368985bf9b86885705667ac09870 Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Fri, 17 Jan 2025 10:17:42 +0100 Subject: [PATCH 06/15] check num_shards --- .../advancedclustertpf/move_upgrade_state.go | 68 ++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/internal/service/advancedclustertpf/move_upgrade_state.go b/internal/service/advancedclustertpf/move_upgrade_state.go index 4666a8bca3..8576c7924f 100644 --- a/internal/service/advancedclustertpf/move_upgrade_state.go +++ b/internal/service/advancedclustertpf/move_upgrade_state.go @@ -3,6 +3,7 @@ package advancedclustertpf import ( "context" "fmt" + "math/big" "strings" "github.com/hashicorp/terraform-plugin-framework-timeouts/resource/timeouts" @@ -99,7 +100,72 @@ func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfp if diags.HasError() { return } - model.ClusterID = types.StringValue("forceLegacySchema") + + rawStateValue2, err := stateIn.UnmarshalWithOpts(tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "replication_specs": tftypes.List{ + ElementType: tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "num_shards": tftypes.Number, + }, + }, + }, + }, + }, tfprotov6.UnmarshalOpts{ValueFromJSONOpts: tftypes.ValueFromJSONOpts{IgnoreUndefinedAttributes: true}}) + if err != nil { + diags.AddError("Unable to Unmarshal state", err.Error()) + return + } + + forceLegacySchema := false + var rawState2 map[string]tftypes.Value + if err := rawStateValue2.As(&rawState2); err != nil { + diags.AddError("Unable to Parse state", err.Error()) + return + } + + var rawState3 []tftypes.Value + if err := rawState2["replication_specs"].As(&rawState3); err != nil { + diags.AddError("Unable to Parse state", err.Error()) + return + } + /* + for _, rawStateValue := range rawState2 { + var numShards int + if err := rawStateValue + numShards := getAttrFromRawState[int](diags, rawStateValue, "num_shards") + } + */ + + for _, rawStateValue := range rawState3 { + var rawState4 map[string]tftypes.Value + if err := rawStateValue.As(&rawState4); err != nil { + diags.AddError("Unable to Parse state", err.Error()) + return + } + + var objectData map[string]tftypes.Value + if err := rawStateValue.As(&objectData); err != nil { + fmt.Printf("Error: %s\n", err) + return + } + numShardsData := objectData["num_shards"] + var numShards *big.Float + if err := numShardsData.As(&numShards); err != nil { + fmt.Printf("Error: %s\n", err) + return + } + + one := big.NewFloat(1.0) + if numShards != nil && numShards.Cmp(one) > 0 { + forceLegacySchema = true + break + } + } + + if forceLegacySchema { + model.ClusterID = types.StringValue("forceLegacySchema") + } diags.Append(stateOut.Set(ctx, model)...) } From c9e6e8a310bd770ae9537063c71c75b8b45b74f9 Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Fri, 17 Jan 2025 11:10:45 +0100 Subject: [PATCH 07/15] Revert "TEMPORARY changes from a working PR" This reverts commit 5cdf2943fb93fa8499fcf64f1c4223b3b939412c. --- .../model_ClusterDescription20240805.go | 1 + .../advancedclustertpf/resource_compatiblity.go | 17 ++++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/internal/service/advancedclustertpf/model_ClusterDescription20240805.go b/internal/service/advancedclustertpf/model_ClusterDescription20240805.go index 268b9e4fd6..31e59425e4 100644 --- a/internal/service/advancedclustertpf/model_ClusterDescription20240805.go +++ b/internal/service/advancedclustertpf/model_ClusterDescription20240805.go @@ -22,6 +22,7 @@ const ( type ExtraAPIInfo struct { ZoneNameNumShards map[string]int64 ZoneNameReplicationSpecIDs map[string]string + RootDiskSize *float64 ContainerIDs map[string]string UsingLegacySchema bool ForceLegacySchemaFailed bool diff --git a/internal/service/advancedclustertpf/resource_compatiblity.go b/internal/service/advancedclustertpf/resource_compatiblity.go index ccfed32710..02dbee351e 100644 --- a/internal/service/advancedclustertpf/resource_compatiblity.go +++ b/internal/service/advancedclustertpf/resource_compatiblity.go @@ -42,6 +42,7 @@ func findNumShardsUpdates(ctx context.Context, state, plan *TFModel, diags *diag func resolveAPIInfo(ctx context.Context, diags *diag.Diagnostics, client *config.MongoDBClient, plan *TFModel, clusterLatest *admin.ClusterDescription20240805, forceLegacySchema bool) *ExtraAPIInfo { var ( api20240530 = client.AtlasV220240530.ClustersApi + rootDiskSize = conversion.NilForUnknown(plan.DiskSizeGB, plan.DiskSizeGB.ValueFloat64Pointer()) projectID = plan.ProjectID.ValueString() clusterName = plan.Name.ValueString() forceLegacySchemaFailed = false @@ -55,18 +56,28 @@ func resolveAPIInfo(ctx context.Context, diags *diag.Diagnostics, client *config return nil } } + if rootDiskSize == nil { + rootDiskSize = findRegionRootDiskSize(clusterLatest.ReplicationSpecs) + } containerIDs, err := resolveContainerIDs(ctx, projectID, clusterLatest, client.AtlasV2.NetworkPeeringApi) if err != nil { diags.AddError(errorResolveContainerIDs, fmt.Sprintf("cluster name = %s, error details: %s", clusterName, err.Error())) return nil } - return &ExtraAPIInfo{ + info := &ExtraAPIInfo{ ContainerIDs: containerIDs, + RootDiskSize: rootDiskSize, ZoneNameReplicationSpecIDs: replicationSpecIDsFromOldAPI(clusterRespOld), ForceLegacySchemaFailed: forceLegacySchemaFailed, - ZoneNameNumShards: numShardsMapFromOldAPI(clusterRespOld), - UsingLegacySchema: forceLegacySchema || usingLegacySchema(ctx, plan.ReplicationSpecs, diags), } + if forceLegacySchema { + info.UsingLegacySchema = true + info.ZoneNameNumShards = numShardsMapFromOldAPI(clusterRespOld) // plan is empty in data source Read when forcing legacy, so we get num_shards from the old API + } else { + info.UsingLegacySchema = usingLegacySchema(ctx, plan.ReplicationSpecs, diags) + info.ZoneNameNumShards = numShardsMap(ctx, plan.ReplicationSpecs, diags) + } + return info } // instead of using `num_shards` explode the replication specs, and set disk_size_gb From 5b925e5a9e4b6b7c4979984ff7f7b77971367066 Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Fri, 17 Jan 2025 11:10:53 +0100 Subject: [PATCH 08/15] Revert "TEMPORARY skip mocked tests" This reverts commit 6ac59b22460d089a5671966f091502ed1251c08a. --- .github/workflows/acceptance-tests-runner.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/acceptance-tests-runner.yml b/.github/workflows/acceptance-tests-runner.yml index 9ffd34dce0..a4d191af96 100644 --- a/.github/workflows/acceptance-tests-runner.yml +++ b/.github/workflows/acceptance-tests-runner.yml @@ -376,6 +376,11 @@ jobs: run: make tools enable-advancedclustertpf - name: Unit tests # delete after allowAdvancedClusterV2Schema is removed run: go test -v ./internal/testutil/acc/advanced_cluster_schema_v2_test.go + - name: Mocked Acceptance Tests + env: + ACCTEST_REGEX_RUN: '^TestAccMockable' + ACCTEST_PACKAGES: ./internal/service/advancedcluster + run: make testmact - name: Acceptance Tests env: MONGODB_ATLAS_LAST_VERSION: ${{ needs.get-provider-version.outputs.provider_version }} From 82c87ff5f51d1b1238a335587208f0b9c6dd53b1 Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Fri, 17 Jan 2025 11:14:27 +0100 Subject: [PATCH 09/15] comment entry points --- internal/service/advancedclustertpf/move_upgrade_state.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/service/advancedclustertpf/move_upgrade_state.go b/internal/service/advancedclustertpf/move_upgrade_state.go index 8576c7924f..ea4ee3290b 100644 --- a/internal/service/advancedclustertpf/move_upgrade_state.go +++ b/internal/service/advancedclustertpf/move_upgrade_state.go @@ -19,10 +19,12 @@ import ( "go.mongodb.org/atlas-sdk/v20241113004/admin" ) +// MoveState is used with moved block to upgrade from cluster to adv_cluster func (r *rs) MoveState(context.Context) []resource.StateMover { return []resource.StateMover{{StateMover: stateMover}} } +// UpgradeState is used to upgrade from adv_cluster schema v1 (SDKv2) to v2 (TPF) func (r *rs) UpgradeState(ctx context.Context) map[int64]resource.StateUpgrader { return map[int64]resource.StateUpgrader{ 1: {StateUpgrader: stateUpgraderFromV1}, From 197e0fcb07046c2c12c44e4193285c69724ecde8 Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Fri, 17 Jan 2025 12:18:17 +0100 Subject: [PATCH 10/15] refactor state move and upgrader logic --- .../advancedclustertpf/move_upgrade_state.go | 202 ++++++++---------- .../service/advancedclustertpf/resource.go | 2 +- 2 files changed, 96 insertions(+), 108 deletions(-) diff --git a/internal/service/advancedclustertpf/move_upgrade_state.go b/internal/service/advancedclustertpf/move_upgrade_state.go index ea4ee3290b..5bd24602e7 100644 --- a/internal/service/advancedclustertpf/move_upgrade_state.go +++ b/internal/service/advancedclustertpf/move_upgrade_state.go @@ -42,158 +42,97 @@ func stateUpgraderFromV1(ctx context.Context, req resource.UpgradeStateRequest, setStateResponse(ctx, &resp.Diagnostics, req.RawState, &resp.State) } -func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfprotov6.RawState, stateOut *tfsdk.State) { - rawStateValue, err := stateIn.UnmarshalWithOpts(tftypes.Object{ - // Minimum attributes needed so Read fills in the rest +// Minimum attributes needed from source schema. Read will fill in the rest +var stateAttrs = map[string]tftypes.Type{ + "project_id": tftypes.String, // project_id and name to identify the cluster + "name": tftypes.String, + "retain_backups_enabled": tftypes.Bool, // TF specific so can't be got in Read + "mongo_db_major_version": tftypes.String, // Has special logic in overrideAttributesWithPrevStateValue that needs the previous state + "timeouts": tftypes.Object{ // TF specific so can't be got in Read AttributeTypes: map[string]tftypes.Type{ - "project_id": tftypes.String, // project_id and name to identify the cluster - "name": tftypes.String, - "retain_backups_enabled": tftypes.Bool, // TF specific so can't be got in Read - "mongo_db_major_version": tftypes.String, // Has special logic in overrideAttributesWithPrevStateValue that needs the previous state - "timeouts": tftypes.Object{ // TF specific so can't be got in Read - AttributeTypes: map[string]tftypes.Type{ - "create": tftypes.String, - "update": tftypes.String, - "delete": tftypes.String, - }, + "create": tftypes.String, + "update": tftypes.String, + "delete": tftypes.String, + }, + }, + "replication_specs": tftypes.List{ // Needed to check if some num_shards are > 1 so we need to force legacy schema + ElementType: tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "num_shards": tftypes.Number, }, }, + }, +} + +func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfprotov6.RawState, stateOut *tfsdk.State) { + rawStateValue, err := stateIn.UnmarshalWithOpts(tftypes.Object{ + AttributeTypes: stateAttrs, }, tfprotov6.UnmarshalOpts{ValueFromJSONOpts: tftypes.ValueFromJSONOpts{IgnoreUndefinedAttributes: true}}) if err != nil { diags.AddError("Unable to Unmarshal state", err.Error()) return } - var rawState map[string]tftypes.Value - if err := rawStateValue.As(&rawState); err != nil { + var stateObj map[string]tftypes.Value + if err := rawStateValue.As(&stateObj); err != nil { diags.AddError("Unable to Parse state", err.Error()) return } - - projectID := getAttrFromRawState[string](diags, rawState, "project_id") - name := getAttrFromRawState[string](diags, rawState, "name") + projectID, name := getProjectIDNameFromStateObj(diags, stateObj) if diags.HasError() { return } - if !conversion.IsStringPresent(projectID) || !conversion.IsStringPresent(name) { - diags.AddError("Unable to read project_id or name from state", fmt.Sprintf("project_id: %s, name: %s", - conversion.SafeString(projectID), conversion.SafeString(name))) - return - } - model := NewTFModel(ctx, &admin.ClusterDescription20240805{ GroupId: projectID, Name: name, - }, getAttrTimeout(diags, rawState), diags, ExtraAPIInfo{}) - if diags.HasError() { - return - } - - if retainBackupsEnabled := getAttrFromRawState[bool](diags, rawState, "retain_backups_enabled"); retainBackupsEnabled != nil { - model.RetainBackupsEnabled = types.BoolPointerValue(retainBackupsEnabled) - } - if mongoDBMajorVersion := getAttrFromRawState[string](diags, rawState, "mongo_db_major_version"); mongoDBMajorVersion != nil { - model.MongoDBMajorVersion = types.StringPointerValue(mongoDBMajorVersion) - } + }, getTimeoutFromStateObj(diags, stateObj), diags, ExtraAPIInfo{}) if diags.HasError() { return } - AddAdvancedConfig(ctx, model, nil, nil, diags) if diags.HasError() { return } - - rawStateValue2, err := stateIn.UnmarshalWithOpts(tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "replication_specs": tftypes.List{ - ElementType: tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "num_shards": tftypes.Number, - }, - }, - }, - }, - }, tfprotov6.UnmarshalOpts{ValueFromJSONOpts: tftypes.ValueFromJSONOpts{IgnoreUndefinedAttributes: true}}) - if err != nil { - diags.AddError("Unable to Unmarshal state", err.Error()) - return - } - - forceLegacySchema := false - var rawState2 map[string]tftypes.Value - if err := rawStateValue2.As(&rawState2); err != nil { - diags.AddError("Unable to Parse state", err.Error()) - return - } - - var rawState3 []tftypes.Value - if err := rawState2["replication_specs"].As(&rawState3); err != nil { - diags.AddError("Unable to Parse state", err.Error()) - return - } - /* - for _, rawStateValue := range rawState2 { - var numShards int - if err := rawStateValue - numShards := getAttrFromRawState[int](diags, rawStateValue, "num_shards") - } - */ - - for _, rawStateValue := range rawState3 { - var rawState4 map[string]tftypes.Value - if err := rawStateValue.As(&rawState4); err != nil { - diags.AddError("Unable to Parse state", err.Error()) - return - } - - var objectData map[string]tftypes.Value - if err := rawStateValue.As(&objectData); err != nil { - fmt.Printf("Error: %s\n", err) - return - } - numShardsData := objectData["num_shards"] - var numShards *big.Float - if err := numShardsData.As(&numShards); err != nil { - fmt.Printf("Error: %s\n", err) - return - } - - one := big.NewFloat(1.0) - if numShards != nil && numShards.Cmp(one) > 0 { - forceLegacySchema = true - break - } - } - - if forceLegacySchema { - model.ClusterID = types.StringValue("forceLegacySchema") - } + setOptionalModelAttrs(stateObj, model) diags.Append(stateOut.Set(ctx, model)...) } -func getAttrFromRawState[T any](diags *diag.Diagnostics, rawState map[string]tftypes.Value, attrName string) *T { +func getAttrFromStateObj[T any](diags *diag.Diagnostics, rawState map[string]tftypes.Value, attrName string) *T { var ret *T if err := rawState[attrName].As(&ret); err != nil { - diags.AddAttributeError(path.Root(attrName), fmt.Sprintf("Unable to read cluster %s", attrName), err.Error()) + diags.AddAttributeError(path.Root(attrName), fmt.Sprintf("Unable to read cluster attribute %s", attrName), err.Error()) return nil } return ret } -func getAttrTimeout(diags *diag.Diagnostics, rawState map[string]tftypes.Value) timeouts.Value { +func getProjectIDNameFromStateObj(diags *diag.Diagnostics, stateObj map[string]tftypes.Value) (projectID, name *string) { + projectID = getAttrFromStateObj[string](diags, stateObj, "project_id") + name = getAttrFromStateObj[string](diags, stateObj, "name") + if diags.HasError() { + return + } + if !conversion.IsStringPresent(projectID) || !conversion.IsStringPresent(name) { + diags.AddError("Unable to read project_id or name from state", fmt.Sprintf("project_id: %s, name: %s", + conversion.SafeString(projectID), conversion.SafeString(name))) + return + } + return projectID, name +} + +func getTimeoutFromStateObj(diags *diag.Diagnostics, stateObj map[string]tftypes.Value) timeouts.Value { attrTypes := map[string]attr.Type{ "create": types.StringType, "update": types.StringType, "delete": types.StringType, } nullObj := timeouts.Value{Object: types.ObjectNull(attrTypes)} - timeoutState := getAttrFromRawState[map[string]tftypes.Value](diags, rawState, "timeouts") + timeoutState := getAttrFromStateObj[map[string]tftypes.Value](diags, stateObj, "timeouts") if diags.HasError() || timeoutState == nil { return nullObj } timeoutMap := make(map[string]attr.Value) for action := range attrTypes { - actionTimeout := getAttrFromRawState[string](diags, *timeoutState, action) + actionTimeout := getAttrFromStateObj[string](diags, *timeoutState, action) if actionTimeout == nil { timeoutMap[action] = types.StringNull() } else { @@ -207,3 +146,52 @@ func getAttrTimeout(diags *diag.Diagnostics, rawState map[string]tftypes.Value) } return timeouts.Value{Object: obj} } + +func setOptionalModelAttrs(stateObj map[string]tftypes.Value, model *TFModel) { + var diags *diag.Diagnostics // discard errors as these are optional attributes + if retainBackupsEnabled := getAttrFromStateObj[bool](diags, stateObj, "retain_backups_enabled"); retainBackupsEnabled != nil { + model.RetainBackupsEnabled = types.BoolPointerValue(retainBackupsEnabled) + } + if mongoDBMajorVersion := getAttrFromStateObj[string](diags, stateObj, "mongo_db_major_version"); mongoDBMajorVersion != nil { + model.MongoDBMajorVersion = types.StringPointerValue(mongoDBMajorVersion) + } + if isLegacySchemaState(stateObj) { + sendLegacySchemaRequestToRead(model) + } +} + +func isLegacySchemaState(stateObj map[string]tftypes.Value) bool { + var diags *diag.Diagnostics // discard errors and assume not legacy if there are any errors + one := big.NewFloat(1.0) + specsVal := getAttrFromStateObj[[]tftypes.Value](diags, stateObj, "replication_specs") + if specsVal == nil { + return false + } + for _, specVal := range *specsVal { + var specObj map[string]tftypes.Value + if err := specVal.As(&specObj); err != nil { + return false + } + numShardsVal := specObj["num_shards"] + var numShards *big.Float + if err := numShardsVal.As(&numShards); err != nil || numShards == nil { + return false + } + if numShards.Cmp(one) > 0 { // legacy schema if numShards > 1 + return true + } + } + return false +} + +// sendLegacySchemaRequestToRead sets ClusterID to a special value so Read can know whether it must use legacy schema. +// private state can't be used here because it's not available in Move Upgrader. +// ClusterID is computed (not optional) so the value will be overridden in Read and the special value won't ever appear in the state file. +func sendLegacySchemaRequestToRead(model *TFModel) { + model.ClusterID = types.StringValue("forceLegacySchema") +} + +// receivedLegacySchemaRequestInRead checks if Read has to use the legacy schema because a State Move or Upgrader happened just before. +func receivedLegacySchemaRequestInRead(model *TFModel) bool { + return model.ClusterID.ValueString() == "forceLegacySchema" +} diff --git a/internal/service/advancedclustertpf/resource.go b/internal/service/advancedclustertpf/resource.go index 16cb6b8558..e8c029b9d0 100644 --- a/internal/service/advancedclustertpf/resource.go +++ b/internal/service/advancedclustertpf/resource.go @@ -304,7 +304,7 @@ func (r *rs) readCluster(ctx context.Context, diags *diag.Diagnostics, state *TF return nil } warningIfFCVExpiredOrUnpinnedExternally(diags, state, readResp) - forceLegacySchema := state.ClusterID.ValueString() == "forceLegacySchema" + forceLegacySchema := receivedLegacySchemaRequestInRead(state) modelOut, _ := getBasicClusterModel(ctx, diags, r.Client, readResp, state, forceLegacySchema) if diags.HasError() { return nil From fd373c6abf6caf30527f8607934dca4555191a00 Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Fri, 17 Jan 2025 12:41:12 +0100 Subject: [PATCH 11/15] simplify error logic --- .../advancedclustertpf/move_upgrade_state.go | 32 +++++++------------ 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/internal/service/advancedclustertpf/move_upgrade_state.go b/internal/service/advancedclustertpf/move_upgrade_state.go index 5bd24602e7..5ee440651d 100644 --- a/internal/service/advancedclustertpf/move_upgrade_state.go +++ b/internal/service/advancedclustertpf/move_upgrade_state.go @@ -9,7 +9,6 @@ import ( "github.com/hashicorp/terraform-plugin-framework-timeouts/resource/timeouts" "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/diag" - "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/tfsdk" "github.com/hashicorp/terraform-plugin-framework/types" @@ -84,7 +83,7 @@ func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfp model := NewTFModel(ctx, &admin.ClusterDescription20240805{ GroupId: projectID, Name: name, - }, getTimeoutFromStateObj(diags, stateObj), diags, ExtraAPIInfo{}) + }, getTimeoutFromStateObj(stateObj), diags, ExtraAPIInfo{}) if diags.HasError() { return } @@ -96,21 +95,17 @@ func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfp diags.Append(stateOut.Set(ctx, model)...) } -func getAttrFromStateObj[T any](diags *diag.Diagnostics, rawState map[string]tftypes.Value, attrName string) *T { +func getAttrFromStateObj[T any](rawState map[string]tftypes.Value, attrName string) *T { var ret *T if err := rawState[attrName].As(&ret); err != nil { - diags.AddAttributeError(path.Root(attrName), fmt.Sprintf("Unable to read cluster attribute %s", attrName), err.Error()) return nil } return ret } func getProjectIDNameFromStateObj(diags *diag.Diagnostics, stateObj map[string]tftypes.Value) (projectID, name *string) { - projectID = getAttrFromStateObj[string](diags, stateObj, "project_id") - name = getAttrFromStateObj[string](diags, stateObj, "name") - if diags.HasError() { - return - } + projectID = getAttrFromStateObj[string](stateObj, "project_id") + name = getAttrFromStateObj[string](stateObj, "name") if !conversion.IsStringPresent(projectID) || !conversion.IsStringPresent(name) { diags.AddError("Unable to read project_id or name from state", fmt.Sprintf("project_id: %s, name: %s", conversion.SafeString(projectID), conversion.SafeString(name))) @@ -119,20 +114,20 @@ func getProjectIDNameFromStateObj(diags *diag.Diagnostics, stateObj map[string]t return projectID, name } -func getTimeoutFromStateObj(diags *diag.Diagnostics, stateObj map[string]tftypes.Value) timeouts.Value { +func getTimeoutFromStateObj(stateObj map[string]tftypes.Value) timeouts.Value { attrTypes := map[string]attr.Type{ "create": types.StringType, "update": types.StringType, "delete": types.StringType, } nullObj := timeouts.Value{Object: types.ObjectNull(attrTypes)} - timeoutState := getAttrFromStateObj[map[string]tftypes.Value](diags, stateObj, "timeouts") - if diags.HasError() || timeoutState == nil { + timeoutState := getAttrFromStateObj[map[string]tftypes.Value](stateObj, "timeouts") + if timeoutState == nil { return nullObj } timeoutMap := make(map[string]attr.Value) for action := range attrTypes { - actionTimeout := getAttrFromStateObj[string](diags, *timeoutState, action) + actionTimeout := getAttrFromStateObj[string](*timeoutState, action) if actionTimeout == nil { timeoutMap[action] = types.StringNull() } else { @@ -140,19 +135,17 @@ func getTimeoutFromStateObj(diags *diag.Diagnostics, stateObj map[string]tftypes } } obj, d := types.ObjectValue(attrTypes, timeoutMap) - diags.Append(d...) - if diags.HasError() { + if d.HasError() { return nullObj } return timeouts.Value{Object: obj} } func setOptionalModelAttrs(stateObj map[string]tftypes.Value, model *TFModel) { - var diags *diag.Diagnostics // discard errors as these are optional attributes - if retainBackupsEnabled := getAttrFromStateObj[bool](diags, stateObj, "retain_backups_enabled"); retainBackupsEnabled != nil { + if retainBackupsEnabled := getAttrFromStateObj[bool](stateObj, "retain_backups_enabled"); retainBackupsEnabled != nil { model.RetainBackupsEnabled = types.BoolPointerValue(retainBackupsEnabled) } - if mongoDBMajorVersion := getAttrFromStateObj[string](diags, stateObj, "mongo_db_major_version"); mongoDBMajorVersion != nil { + if mongoDBMajorVersion := getAttrFromStateObj[string](stateObj, "mongo_db_major_version"); mongoDBMajorVersion != nil { model.MongoDBMajorVersion = types.StringPointerValue(mongoDBMajorVersion) } if isLegacySchemaState(stateObj) { @@ -161,9 +154,8 @@ func setOptionalModelAttrs(stateObj map[string]tftypes.Value, model *TFModel) { } func isLegacySchemaState(stateObj map[string]tftypes.Value) bool { - var diags *diag.Diagnostics // discard errors and assume not legacy if there are any errors one := big.NewFloat(1.0) - specsVal := getAttrFromStateObj[[]tftypes.Value](diags, stateObj, "replication_specs") + specsVal := getAttrFromStateObj[[]tftypes.Value](stateObj, "replication_specs") if specsVal == nil { return false } From fbdd98d3f691e4d2e332fc92d56e1f6968e18e4c Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Fri, 17 Jan 2025 14:50:34 +0100 Subject: [PATCH 12/15] pass replication_specs with num_shards so ClusterID is not needed --- .../advancedclustertpf/move_upgrade_state.go | 72 ++++++++++--------- .../service/advancedclustertpf/resource.go | 3 +- 2 files changed, 41 insertions(+), 34 deletions(-) diff --git a/internal/service/advancedclustertpf/move_upgrade_state.go b/internal/service/advancedclustertpf/move_upgrade_state.go index 5ee440651d..d74d4b918f 100644 --- a/internal/service/advancedclustertpf/move_upgrade_state.go +++ b/internal/service/advancedclustertpf/move_upgrade_state.go @@ -91,7 +91,7 @@ func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfp if diags.HasError() { return } - setOptionalModelAttrs(stateObj, model) + setOptionalModelAttrs(ctx, stateObj, model) diags.Append(stateOut.Set(ctx, model)...) } @@ -141,49 +141,57 @@ func getTimeoutFromStateObj(stateObj map[string]tftypes.Value) timeouts.Value { return timeouts.Value{Object: obj} } -func setOptionalModelAttrs(stateObj map[string]tftypes.Value, model *TFModel) { +func setOptionalModelAttrs(ctx context.Context, stateObj map[string]tftypes.Value, model *TFModel) { if retainBackupsEnabled := getAttrFromStateObj[bool](stateObj, "retain_backups_enabled"); retainBackupsEnabled != nil { model.RetainBackupsEnabled = types.BoolPointerValue(retainBackupsEnabled) } if mongoDBMajorVersion := getAttrFromStateObj[string](stateObj, "mongo_db_major_version"); mongoDBMajorVersion != nil { model.MongoDBMajorVersion = types.StringPointerValue(mongoDBMajorVersion) } - if isLegacySchemaState(stateObj) { - sendLegacySchemaRequestToRead(model) + if specsVal := getAttrFromStateObj[[]tftypes.Value](stateObj, "replication_specs"); specsVal != nil { + var specModels []TFReplicationSpecsModel + for _, specVal := range *specsVal { + var specObj map[string]tftypes.Value + if err := specVal.As(&specObj); err != nil { + continue + } + if specModel := replicationSpecModelWithNumShards(specObj["num_shards"]); specModel != nil { + specModels = append(specModels, *specModel) + } + } + if len(specModels) > 0 { + model.ReplicationSpecs, _ = types.ListValueFrom(ctx, ReplicationSpecsObjType, specModels) + } } } -func isLegacySchemaState(stateObj map[string]tftypes.Value) bool { - one := big.NewFloat(1.0) - specsVal := getAttrFromStateObj[[]tftypes.Value](stateObj, "replication_specs") - if specsVal == nil { - return false +func replicationSpecModelWithNumShards(numShardsVal tftypes.Value) *TFReplicationSpecsModel { + var numShardsFloat *big.Float + if err := numShardsVal.As(&numShardsFloat); err != nil || numShardsFloat == nil { + return nil } - for _, specVal := range *specsVal { - var specObj map[string]tftypes.Value - if err := specVal.As(&specObj); err != nil { - return false - } - numShardsVal := specObj["num_shards"] - var numShards *big.Float - if err := numShardsVal.As(&numShards); err != nil || numShards == nil { - return false - } - if numShards.Cmp(one) > 0 { // legacy schema if numShards > 1 - return true - } + numShards, _ := numShardsFloat.Int64() + return &TFReplicationSpecsModel{ + NumShards: types.Int64Value(numShards), + RegionConfigs: types.ListNull(RegionConfigsObjType), + ContainerId: types.MapNull(types.StringType), + Id: types.StringNull(), + ExternalId: types.StringNull(), + ZoneId: types.StringNull(), + ZoneName: types.StringNull(), } - return false } -// sendLegacySchemaRequestToRead sets ClusterID to a special value so Read can know whether it must use legacy schema. -// private state can't be used here because it's not available in Move Upgrader. -// ClusterID is computed (not optional) so the value will be overridden in Read and the special value won't ever appear in the state file. -func sendLegacySchemaRequestToRead(model *TFModel) { - model.ClusterID = types.StringValue("forceLegacySchema") -} +/* -// receivedLegacySchemaRequestInRead checks if Read has to use the legacy schema because a State Move or Upgrader happened just before. -func receivedLegacySchemaRequestInRead(model *TFModel) bool { - return model.ClusterID.ValueString() == "forceLegacySchema" +type TFReplicationSpecsModel struct { + RegionConfigs types.List `tfsdk:"region_configs"` + ContainerId types.Map `tfsdk:"container_id"` + Id types.String `tfsdk:"id"` + ExternalId types.String `tfsdk:"external_id"` + ZoneId types.String `tfsdk:"zone_id"` + ZoneName types.String `tfsdk:"zone_name"` + NumShards types.Int64 `tfsdk:"num_shards"` } + +*/ diff --git a/internal/service/advancedclustertpf/resource.go b/internal/service/advancedclustertpf/resource.go index e8c029b9d0..f995857ca7 100644 --- a/internal/service/advancedclustertpf/resource.go +++ b/internal/service/advancedclustertpf/resource.go @@ -304,8 +304,7 @@ func (r *rs) readCluster(ctx context.Context, diags *diag.Diagnostics, state *TF return nil } warningIfFCVExpiredOrUnpinnedExternally(diags, state, readResp) - forceLegacySchema := receivedLegacySchemaRequestInRead(state) - modelOut, _ := getBasicClusterModel(ctx, diags, r.Client, readResp, state, forceLegacySchema) + modelOut, _ := getBasicClusterModel(ctx, diags, r.Client, readResp, state, false) if diags.HasError() { return nil } From 936ee2b03ced00412747f4e29edfa13d35aec70d Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Fri, 17 Jan 2025 14:52:02 +0100 Subject: [PATCH 13/15] leftovers --- .../advancedclustertpf/move_upgrade_state.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/internal/service/advancedclustertpf/move_upgrade_state.go b/internal/service/advancedclustertpf/move_upgrade_state.go index d74d4b918f..ddb287a0f9 100644 --- a/internal/service/advancedclustertpf/move_upgrade_state.go +++ b/internal/service/advancedclustertpf/move_upgrade_state.go @@ -181,17 +181,3 @@ func replicationSpecModelWithNumShards(numShardsVal tftypes.Value) *TFReplicatio ZoneName: types.StringNull(), } } - -/* - -type TFReplicationSpecsModel struct { - RegionConfigs types.List `tfsdk:"region_configs"` - ContainerId types.Map `tfsdk:"container_id"` - Id types.String `tfsdk:"id"` - ExternalId types.String `tfsdk:"external_id"` - ZoneId types.String `tfsdk:"zone_id"` - ZoneName types.String `tfsdk:"zone_name"` - NumShards types.Int64 `tfsdk:"num_shards"` -} - -*/ From 8fe7cd68035b5904a77ebf373590a7520c7711e0 Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Fri, 17 Jan 2025 14:54:13 +0100 Subject: [PATCH 14/15] adjust doc --- internal/service/advancedclustertpf/move_upgrade_state.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/advancedclustertpf/move_upgrade_state.go b/internal/service/advancedclustertpf/move_upgrade_state.go index ddb287a0f9..201dbbf972 100644 --- a/internal/service/advancedclustertpf/move_upgrade_state.go +++ b/internal/service/advancedclustertpf/move_upgrade_state.go @@ -41,7 +41,7 @@ func stateUpgraderFromV1(ctx context.Context, req resource.UpgradeStateRequest, setStateResponse(ctx, &resp.Diagnostics, req.RawState, &resp.State) } -// Minimum attributes needed from source schema. Read will fill in the rest +// Attributes needed from source schema. Read will fill in the rest var stateAttrs = map[string]tftypes.Type{ "project_id": tftypes.String, // project_id and name to identify the cluster "name": tftypes.String, @@ -54,7 +54,7 @@ var stateAttrs = map[string]tftypes.Type{ "delete": tftypes.String, }, }, - "replication_specs": tftypes.List{ // Needed to check if some num_shards are > 1 so we need to force legacy schema + "replication_specs": tftypes.List{ // Needed to send num_shards to Read so it can decide if it's using the legacy schema. ElementType: tftypes.Object{ AttributeTypes: map[string]tftypes.Type{ "num_shards": tftypes.Number, From b67243fab155f0f51f69b614bcded7e74c293a94 Mon Sep 17 00:00:00 2001 From: Leo Antoli <430982+lantoli@users.noreply.github.com> Date: Fri, 17 Jan 2025 14:59:04 +0100 Subject: [PATCH 15/15] clarify doc --- internal/service/advancedclustertpf/move_upgrade_state.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/service/advancedclustertpf/move_upgrade_state.go b/internal/service/advancedclustertpf/move_upgrade_state.go index 201dbbf972..2154d7b5e2 100644 --- a/internal/service/advancedclustertpf/move_upgrade_state.go +++ b/internal/service/advancedclustertpf/move_upgrade_state.go @@ -41,7 +41,9 @@ func stateUpgraderFromV1(ctx context.Context, req resource.UpgradeStateRequest, setStateResponse(ctx, &resp.Diagnostics, req.RawState, &resp.State) } -// Attributes needed from source schema. Read will fill in the rest +// stateAttrs has the attributes needed from source schema. +// Filling these attributes in the destination will prevent plan changes when moving/upgrading state. +// Read will fill in the rest. var stateAttrs = map[string]tftypes.Type{ "project_id": tftypes.String, // project_id and name to identify the cluster "name": tftypes.String,