feat: Don't use num_shards and root disk_size_gb in conversion #65

lantoli · 2025-08-20T08:32:45Z

Description

Don't use num_shards and root disk_size_gb in conversion.

Other conversions such as with dynamic blocks will be done in follow-up PRs
Documentation will be updated in follow-up PRs

Note: Same limitation applies to num_shards as in clu2adv command:

num_shards in replication_specs must be a numeric literal expression, e.g. var.num_shards is not supported. This is to allow creating a replication_specs element per shard in mongodbatlas_advanced_cluster. This limitation doesn't apply if you're using dynamic blocks in regions_config or replication_specs.

Link to any related issue(s): CLOUDP-339203

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR. A migration guide must be created or updated if the new feature will go in a major version.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR. A migration guide must be created or updated.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contributing guides
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals I have added appropriate changelog entries.
If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

Copilot

Pull Request Overview

This PR modifies the Terraform advanced cluster to v2 conversion logic to exclude num_shards and root-level disk_size_gb attributes from the conversion process. The change addresses limitations in handling non-numerical num_shards values and improves disk size handling by distributing root-level disk_size_gb values to individual specs.

Key changes:

Remove num_shards from replication specs during conversion and expand specs based on shard count
Extract and distribute root-level disk_size_gb to individual electable, read-only, and analytics specs
Add error handling for non-numerical num_shards values

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

File	Description
internal/convert/adv2v2.go	Core conversion logic updated to handle `num_shards` expansion and `disk_size_gb` distribution
internal/convert/const.go	Added new error constant for root attribute handling
internal/convert/testdata/adv2v2/*.tf	Test files demonstrating conversion behavior for various scenarios
internal/convert/testdata/adv2v2/errors.json	Added error message for non-numerical `num_shards` handling

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

internal/convert/testdata/adv2v2/num_shards.out.tf

marcosuma

ty

EspenAlbert · 2025-08-20T11:44:51Z

internal/convert/testdata/adv2v2/errors.json

 	"replication_specs_missing_region_configs": "replication_specs must have at least one region_configs",
-	"missing_replication_specs": "must have at least one replication_specs"
+	"missing_replication_specs": "must have at least one replication_specs",
+	"num_shards_not_numerical": "setting num_shards: failed to evaluate number"


I'm not sure the full context of this error, but not sure how I would fix it 😅

as in the other command we distinguish two main use cases:

advanced_cluster used directly without a module, we expect a numerical value in num_shards

advanced_cluster in a module, in this case we support variables in num_shards and dynamic blocks (PR will follow with this implementation).

see PR description note for more info

Thank you for the explanation!
FFTR or leave open for others

internal/convert/adv2v2.go

EspenAlbert · 2025-08-20T11:55:12Z

internal/convert/testdata/adv2v2/num_shards.in.tf

@@ -0,0 +1,60 @@
+resource "mongodbatlas_advanced_cluster" "geo" {


[nit] prefer one resource per file

I think it's a tradeoff between having a lot of small files and one big file, in this case I think it makes sense to have resources together when they test something very similar with slight variations.

Sounds good

EspenAlbert

A few nit comments, but looks good!

AgustinBettati · 2025-08-20T13:21:57Z

internal/convert/testdata/adv2v2/num_shards_not_numerical.in.tf

+  cluster_type = "GEOSHARDED"
+  replication_specs {
+    zone_name  = "Zone 1"
+    num_shards = var.num_shards # unresolved num_shards


q: Could var.num_shards be used to generate a foreach of replication_specs? Ok if we have this as a known limitation but was curious if this approach could work.

@AgustinBettati hi, I think this is related to this comment as well.
We have it as a limitation when a plain cluster (without module) is being used as. But the loop approach is done when dynamic blocks are used, e.g. here.

I see. But if a customer has a resource without a module and using var.num_shards, converting to using a foreach and leveraging the var.num_shards value could be possible for reaching an equivalent config right?

@AgustinBettati yes, I'm working on it in a follow-up PR but it's not simple. For example if there are multiple replication_specs, at least one of them with variable, then concat must be used

lantoli added 2 commits August 20, 2025 10:26

failing test for disk_size_gb

e6484d1

implemen disk_size_gb

2b9facc

github-actions bot added the enhancement New feature or request label Aug 20, 2025

lantoli added 5 commits August 20, 2025 12:58

failing num_shards test

8f5149b

implement num_shards

f5d5ae3

test with both num_shards and disk_size_gb

03bd5e5

comment about don't using error when getting diskSizeGB

88289ce

simplify for

29c0a50

lantoli marked this pull request as ready for review August 20, 2025 11:10

Copilot AI review requested due to automatic review settings August 20, 2025 11:10

lantoli requested a review from a team as a code owner August 20, 2025 11:10

Copilot AI reviewed Aug 20, 2025

View reviewed changes

internal/convert/testdata/adv2v2/num_shards.out.tf Show resolved Hide resolved

marcosuma approved these changes Aug 20, 2025

View reviewed changes

EspenAlbert reviewed Aug 20, 2025

View reviewed changes

internal/convert/adv2v2.go Show resolved Hide resolved

EspenAlbert reviewed Aug 20, 2025

View reviewed changes

EspenAlbert approved these changes Aug 20, 2025

View reviewed changes

lantoli merged commit 36d404e into main Aug 20, 2025
9 checks passed

lantoli deleted the CLOUDP-339203_deprecated branch August 20, 2025 12:07

AgustinBettati reviewed Aug 20, 2025

View reviewed changes

		@@ -0,0 +1,60 @@
		resource "mongodbatlas_advanced_cluster" "geo" {

feat: Don't use num_shards and root disk_size_gb in conversion #65

feat: Don't use num_shards and root disk_size_gb in conversion #65

Uh oh!

Conversation

lantoli commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change:

Required Checklist:

Further comments

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

marcosuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EspenAlbert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lantoli commented Aug 20, 2025 •

edited

Loading