feat: Converts clusters with multiple replications_specs, region_configs and shards #22

lantoli · 2025-02-17T10:26:10Z

Description

Converts clusters with multiple replications_specs, region_configs and shards

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

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

lantoli · 2025-02-17T10:29:13Z

internal/convert/testdata/clu2adv/adv_config_bi_connector_pinned_fcv.in.tf

@@ -16,7 +16,6 @@ resource "mongodbatlas_cluster" "this" {
  pinned_fcv {
    # comments in pinned_fcv are kept
    expiration_date = var.fcv_date
-    version         = var.fcv_date


@AgustinBettati removed version following this comment, thanks: #21 (comment)

lantoli · 2025-02-18T07:12:27Z

internal/convert/convert.go

+		if err := fillRegionConfigs(specb, specbSrc, root); err != nil {
+			return err
+		}
+		for range shardsVal {


@AgustinBettati FYI use num_shards to duplicate the replication_specs

marcosuma · 2025-02-18T07:32:53Z

README.md

@@ -36,6 +36,7 @@ If you want to overwrite the output file if it exists, or even use the same outp

 - The plugin doesn't support `regions_config` without `electable_nodes` as there can be some issues with `priority` when they only have `analytics_nodes` and/or `electable_nodes`.
 - `priority` is required in `regions_config` and must be a resolved number between 7 and 1, e.g. `var.priority` is not supported. This is to allow reordering them by descending priority as this is expected in `mongodbatlas_advanced_cluster`.
+- `num_shards` is required in `replication_specs` and must be a resolved number, e.g. `var.num_shards` is not supported. This is to allow creating a `replication_specs` element per shard in `mongodbatlas_advanced_cluster`.


will this limitation go away? considering the field is optional, can't we assume that absence of it means = 1?

num_shards is required in replication_specs: https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/cluster#num_shards-2

your link is to num_shards at root level

ah my bad - so I am not following this limitation..

num_shards is required ...

gives me the feeling that the field must be set as a requirement for the tool, not because it's already required. Does it make sense to just remove the "is required"? And could you perhaps add the link to the field from the official doc?

Suggested change

- `num_shards` is required in `replication_specs` and must be a resolved number, e.g. `var.num_shards` is not supported. This is to allow creating a `replication_specs` element per shard in `mongodbatlas_advanced_cluster`.

- `num_shards` in `replication_specs` must be a resolved number, e.g. `var.num_shards` is not supported. This is to allow creating a `replication_specs` element per shard in `mongodbatlas_advanced_cluster`.

changed here: 8de0488

marcosuma · 2025-02-18T07:35:15Z

README.md

@@ -36,6 +36,7 @@ If you want to overwrite the output file if it exists, or even use the same outp



extending my question here: is your plan to remove all these limitations?

sorry, what's the limitation you're referring to here?

all these limitations

I don't understand if these entries in the limitations section are temporary or will go away with more implementation

all current limitations in README are hard to remove so they'll probably stay as limitations. is there any specific limitation that you think we should try to remove?

none in particular. I only recall there were limitations that were removed, so if there are some that you think will go away, just call it out in the doc

ah, ok, got it

internal/convert/testdata/clu2adv/multi_replicaction_specs.in.tf

AgustinBettati

LGTM

AgustinBettati · 2025-02-18T10:34:52Z

README.md

 - `priority` is required in `regions_config` and must be a resolved number between 7 and 1, e.g. `var.priority` is not supported. This is to allow reordering them by descending priority as this is expected in `mongodbatlas_advanced_cluster`.
+- `num_shards` is required in `replication_specs` and must be a resolved number, e.g. `var.num_shards` is not supported. This is to allow creating a `replication_specs` element per shard in `mongodbatlas_advanced_cluster`.


small nit on phrasing, would replace "resolved" with Literal expression

thanks, changed here: 8de0488

AgustinBettati · 2025-02-18T10:40:18Z

internal/convert/testdata/clu2adv/replication_specs_missing_num_shards.in.tf

+  provider_instance_size_name = "M10"
+  replication_specs {
+    zone_name = "Zone 1"
+    # missing num_shards


I would not be concerned about testing invalid mongodbatlas_cluster configurations but no concern in leaving this

you're right, we don't normally check for cluster validation, e.g. if you have a "hello" attribute at root level, I will be kept in adv_cluster, which is invalid.

In this case I wanted to add the test to make sure that we don't panic if num_shards is not present.

EspenAlbert · 2025-02-18T10:44:40Z

internal/convert/convert.go

+		for range shardsVal {
+			specbs = append(specbs, specb)
+		}
+		resourceb.RemoveBlock(specSrc)


Nice approach of using a while loop. Didn't know about this RemoveBlock.
Looks like we are also able to maintain all extra info, such as comments since we are using specSrc.Body()?
I guess if num_shards > 1, then we will duplicate all extra comments?

No, unfortunately we're not keeping the comments here as we just move the attributes we need, and then we remove the original blocks.

We only copy comments (and everything in the blocks) in a block when we don't need to do any transformation, i.e. when using fillBlockOpt with timeouts, adv_configudation, bi_connector, labels and tags and. In this case we only need to wrap the body in an attribute, but not need to treat the internal attributes.

EspenAlbert · 2025-02-18T10:48:14Z

internal/convert/testdata/clu2adv/multi_replicaction_specs.in.tf

@@ -0,0 +1,59 @@
+resource "mongodbatlas_cluster" "multirep" {


why not use different files?
One is multi_replication_specs_with_num_shards?

I'm trying to find a balance as we're starting to have many files, so I'm trying to group some clusters with similar tests in the same file, similar to what was done here: https://github.com/mongodb-labs/atlas-cli-plugin-terraform/blob/main/internal/convert/testdata/clu2adv/tags_labels_timeouts.in.tf

EspenAlbert

Nice

marcosuma

LGTM! only a minor comment

marcosuma · 2025-02-18T14:14:18Z

README.md

@@ -35,7 +35,8 @@ If you want to overwrite the output file if it exists, or even use the same outp
 ### Limitations

 - The plugin doesn't support `regions_config` without `electable_nodes` as there can be some issues with `priority` when they only have `analytics_nodes` and/or `electable_nodes`.
- `priority` is required in `regions_config` and must be a resolved number between 7 and 1, e.g. `var.priority` is not supported. This is to allow reordering them by descending priority as this is expected in `mongodbatlas_advanced_cluster`.
+- [`priority`](https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/cluster#priority-1) is required in `regions_config` and must be a numeric [literal expression](https://developer.hashicorp.com/nomad/docs/job-specification/hcl2/expressions#literal-expressions) between 7 and 1, e.g. `var.priority` is not supported. This is to allow reordering them by descending priority as this is expected in `mongodbatlas_advanced_cluster`.


@lantoli similar question here, in what context the "is required" is true? is it for the plugin to work? and it's because of the re-ordering?

yes for the plugin to work, right now the plugin will raise an error if it's not provided. It's because the reordering, and also because it's required in Atlas when electable_specs are specified, so in reality a valid (old) c luster should almost always have priority, except if there are only analytics or read_only nodes, that we're not supporting in the plugin as in the first limitation.

added more info in the doc tab "Plugin additional features"

remove version from pinned_fcv in example

1665c65

github-actions bot added the enhancement New feature or request label Feb 17, 2025

lantoli mentioned this pull request Feb 17, 2025

feat: Convert tags, labels, timeouts, advanced_configuration, bi_connector_config and pinned_fcv #21

Merged

13 tasks

lantoli commented Feb 17, 2025

View reviewed changes

lantoli added 6 commits February 17, 2025 13:02

failing test

57f284b

multi repls

af906f1

num_shards

d677568

error conditions

45a1207

fillTagsLabelsOpt

ddcc273

fillRegionConfigs

3766427

lantoli commented Feb 18, 2025

View reviewed changes

lantoli marked this pull request as ready for review February 18, 2025 07:12

lantoli requested a review from a team as a code owner February 18, 2025 07:12

marcosuma reviewed Feb 18, 2025

View reviewed changes

internal/convert/testdata/clu2adv/multi_replicaction_specs.in.tf Show resolved Hide resolved

AgustinBettati approved these changes Feb 18, 2025

View reviewed changes

EspenAlbert reviewed Feb 18, 2025

View reviewed changes

EspenAlbert approved these changes Feb 18, 2025

View reviewed changes

marcosuma approved these changes Feb 18, 2025

View reviewed changes

feedback about literal expressions

8de0488

marcosuma reviewed Feb 18, 2025

View reviewed changes

marcosuma approved these changes Feb 18, 2025

View reviewed changes

change preview provider v2 doc

8ad9282

lantoli merged commit c4722c9 into main Feb 18, 2025
6 checks passed

lantoli deleted the CLOUDP-299292_shards branch February 18, 2025 15:02

	- `num_shards` is required in `replication_specs` and must be a resolved number, e.g. `var.num_shards` is not supported. This is to allow creating a `replication_specs` element per shard in `mongodbatlas_advanced_cluster`.
	- `num_shards` in `replication_specs` must be a resolved number, e.g. `var.num_shards` is not supported. This is to allow creating a `replication_specs` element per shard in `mongodbatlas_advanced_cluster`.

		@@ -36,6 +36,7 @@ If you want to overwrite the output file if it exists, or even use the same outp

		- `priority` is required in `regions_config` and must be a resolved number between 7 and 1, e.g. `var.priority` is not supported. This is to allow reordering them by descending priority as this is expected in `mongodbatlas_advanced_cluster`.
		- `num_shards` is required in `replication_specs` and must be a resolved number, e.g. `var.num_shards` is not supported. This is to allow creating a `replication_specs` element per shard in `mongodbatlas_advanced_cluster`.

		@@ -0,0 +1,59 @@
		resource "mongodbatlas_cluster" "multirep" {

feat: Converts clusters with multiple replications_specs, region_configs and shards #22

feat: Converts clusters with multiple replications_specs, region_configs and shards #22

Uh oh!

Conversation

lantoli commented Feb 17, 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!

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!

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AgustinBettati 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!

Choose a reason for hiding this comment

Uh oh!

EspenAlbert Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

Uh oh!

lantoli commented Feb 17, 2025 •

edited

Loading

lantoli Feb 18, 2025 •

edited

Loading

EspenAlbert Feb 18, 2025 •

edited

Loading

lantoli Feb 18, 2025 •

edited

Loading