remove: Removes mongodbatlas_advanced_cluster deprecated params #3635
Conversation
| secrets: inherit | ||
| uses: ./.github/workflows/acceptance-tests.yml | ||
| with: | ||
| reduced_tests: true | ||
| reduced_tests: false |
There was a problem hiding this comment.
will revert before merging
There was a problem hiding this comment.
nit: my recommendation is these cases is to add a comment so it's clear and harder to forget, e.g.:
reduced_tests: false // TODO: CHANGE TO true BEFORE MERGING
|
APIx bot: a message has been sent to Docs Slack channel |
There was a problem hiding this comment.
Pull Request Overview
This PR removes deprecated parameters from the mongodbatlas_advanced_cluster resource and data sources, cleaning up the API surface by eliminating legacy schema attributes that were marked for deprecation.
Key changes include:
- Removal of deprecated attributes:
use_replication_spec_per_shard,num_shards,id,disk_size_gb,default_read_concern, andfail_index_key_too_long - Elimination of legacy API compatibility layers and dual-schema support
- Simplification of test configurations and removal of old schema test cases
Reviewed Changes
Copilot reviewed 36 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/service/advancedclustertpf/schema.go | Removes deprecated schema attributes and related plan modifiers |
| internal/service/advancedclustertpf/resource.go | Simplifies resource logic by removing legacy API handling |
| internal/service/advancedclustertpf/resource_test.go | Updates test cases to use new schema and removes old schema tests |
| internal/service/advancedclustertpf/resource_compatiblity.go | Removes legacy compatibility functions for old sharding config |
| internal/testutil/acc/advanced_cluster.go | Updates test utilities to remove deprecated parameters |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Tests timing out in cloud-dev for now but were run previously https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/17350913203/job/49257053501 on the draft branch UPDATE: tests are now passing except TestAccAdvancedCluster_updateDeleteTimeoutFlex due to ATLAS_GENERAL_ERROR. This succeeds locally. |
| WithAnalyticsSpecs: false, // other update made after removed analytics block, computed value is expected to be the same | ||
| }, isSDKv2), | ||
| Check: checkReplicaSetAWSProvider(isTPF, projectID, clusterName, 50, 5, true, true), | ||
| WithAnalyticsSpecs: true, // other update made after removed analytics block, computed value is expected to be the same |
There was a problem hiding this comment.
I believe this comment is not accurate now as analytics specs is never removed. Lets double check what is the expected behaviour we want to capture in the test and from there adjust.
There was a problem hiding this comment.
the test now simply checks update of nodes & disk_size_gb, while these values are explicitly configured for both analytics & electable specs, will remove the comment.
the expected behavior now is that if we expect analytics specs disk_size_gb to be updated as well, then user needs to explicitly specify it in the config, otherwise previous state is preserved (similar to TestAccAdvancedCluster_removeBlocksFromConfig).
On the contrary, previously id root disk_size_gb (now removed) was updated , then even if analytics specs had been removed from config, it would still be updated.
There was a problem hiding this comment.
@AgustinBettati can you confirm if any doc changes need to be made for this?
There was a problem hiding this comment.
Okay, I am fine with adjusting this test as we still capture the behaviour of having removal of blocks being a no-op in TestAccAdvancedCluster_removeBlocksFromConfig. As a side note I see that WithAnalyticsSpecs is now always equal to true, we can remove that logic to simplify.
In terms of docs I believe the best practice section covers this aspect, only consideration is we could make it more visible (potentially linking from top of the page or from specific relevant attributes).
There was a problem hiding this comment.
I'm not sure if the top of the page is the best idea because we already have a plethora of notes there. I'll add a mention in replication specs attribute though
There was a problem hiding this comment.
adding this for region_configs & replication_specs
| func configAWSProvider(t *testing.T, configInfo ReplicaSetAWSConfig, isTPF bool) string { | ||
| t.Helper() | ||
| analyticsSpecs := "" | ||
|
|
||
| if isOptionalTrue(useSDKv2...) { | ||
| if !isTPF { |
There was a problem hiding this comment.
nit: Given we are preserving ConvertAdvancedClusterToTPF, can we leverage it here to only have one config definition.
There was a problem hiding this comment.
I'd prefer to keep it as is, this config also uses root level disk_size_gb which is different from the one used for TPF
| { | ||
| ProtoV6ProviderFactories: acc.TestAccProviderV6Factories, | ||
| Config: configShardedTransitionOldToNewSchema(t, true, projectID, clusterName, true, true, false), | ||
| Check: acc.CheckIndependentShardScalingMode(resourceName, clusterName, "CLUSTER"), |
There was a problem hiding this comment.
great that we are preserving these checks of the autoscaling mode
| @@ -0,0 +1,382 @@ | |||
| package advancedclustertpf_test | |||
There was a problem hiding this comment.
like the approach of having separate file for specific 1.x migration paths
internal/service/advancedclustertpf/resource_compatibility_reuse.go
Outdated
Show resolved
Hide resolved
| keepUnknownsCalls = schemafunc.KeepUnknownFuncOr(keepUnkownFuncWithNodeCount, keepUnkownFuncWithNonEmptyAutoScaling) | ||
| keepUnknownsCalls = schemafunc.KeepUnknownFuncOr(keepUnkownFuncWithNonEmptyAutoScaling) | ||
| ) | ||
|
|
||
| func keepUnkownFuncWithNodeCount(name string, replacement attr.Value) bool { | ||
| return name == "node_count" && !replacement.Equal(types.Int64Value(0)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Do you recall why this change was needed? Would not expect change to the plan modifier logic associated to node_count
There was a problem hiding this comment.
can double check with @EspenAlbert #3592 (comment)
but I believe this was actually not being used.
There was a problem hiding this comment.
Sorry, I think Agustin is right. This function should not be removed.
I think I read num_shards instead of node_count.
This ensures that a value in the state that is not 0 remains unknown.
There was a problem hiding this comment.
@EspenAlbert is there an existing test for this that should have failed?
There was a problem hiding this comment.
@EspenAlbert @AgustinBettati fyi I have also added a list of some of the tests that have been removed in this PR.
There was a problem hiding this comment.
Behavior node_count in the plan:
- Copied from state if block removed (Leo's logic in AdjustRegionConfigsChildren)
- if node_count > 0 in the state: KeepUnknown. No longer used because of (1)
- if node_count == 0: UseState value. Can happen since the API returns spec values even when not set in the config.
Having investigated this. We could remove the function and always keep it unknown.
To do test case (2), we would need to add a plan modifier test, similar to the TestPlanChecksClusterTwoRepSpecsWithAutoScalingAndSpecs.
I added a commit here to show how the behavior of (1) is used.
There was a problem hiding this comment.
Just to make sure I understand, the current function is redundant because if node_count > 0 in the state (of a removed block) AdjustRegionConfigsChildren will ensure it has a known value. Essentially there would never be a case where node_count is left unknown right?
Agree we can add a test to verify this is correct.
There was a problem hiding this comment.
added test in this commit which checks node_count is known
internal/service/advancedclustertpf/resource_migration_v1x_test.go
Outdated
Show resolved
Hide resolved
internal/service/advancedclustertpf/model_ClusterDescriptionProcessArgs20240805.go
Show resolved
Hide resolved
|
only |
Description
Removes mongodbatlas_advanced_cluster deprecated params.
TPF preview to V2 state diff: https://www.diffchecker.com/T3369rEg/
SDKv2 to V2 state diff: https://www.diffchecker.com/4VjSiR7x/
Some of the tests that have been removed:
Related documentation changes: #3634
Link to any related issue(s): CLOUDP-333683
Type of change:
Required Checklist:
Further comments