feat: Supports dynamic blocks in regions_config #33

lantoli · 2025-03-17T13:58:22Z

Description

Supports dynamic blocks in regions_config. In follow-up support for replication_specs will be done.

Additionally in this PR, limitations for num_shards, electable_specs and priority have been removed. regions_config priority reorder is now done if possible (priorities are numerical literals), else regions_config original order is kept.

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

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

…den file

internal/convert/convert.go

* main: chore: Bump github.com/spf13/afero from 1.12.0 to 1.14.0 (#36) chore: Bump golangci/golangci-lint-action from 6.5.0 to 6.5.2 (#34) chore: Bump docker/login-action from 3.3.0 to 3.4.0 (#35)

marcosuma

Great job, it's also nice to see the amount of test changes

marcosuma · 2025-03-19T12:42:33Z

README.md

+#### Dynamic blocks in regions_config
+
+You can use `dynamic` blocks for `regions_config`. The plugin assumes that `for_each` has an expression which is evaluated to a `list` or `set` of objects.
+Dynamic block and individual blocks for `regions_config` are not supported at the same time in a `replication_specs`. This is an example of how to use dynamic blocks in `regions_config`:


should we move what's not supported in the limitations?

is there an "easy" why we don't support dynamic and individual blocks?

would a workaround be to execute the command twice, one with dynamic block only and one with individual?

As we have specific sections for the different dynamic blocks where we explain them in more details, I think it's better to keep the limitation for each dynamic block in their specific section, but I've added a note here: 32ca654

The main reasons are:

Customers won't probably have definitions file like that.

Effort and complexity to support it is not trivial.

The result output will be quite complicated.
I've added a comment in the previous commit regarding this so they can give feedback if some customer needs it. For example we support it in tags and label as some customers are using it, e.g:

atlas-cli-plugin-terraform/internal/convert/testdata/clu2adv/dynamic_tags_labels.out.tf

Line 142 in e9c2134

tags = merge(

That workaround might help, at the end I think the solution would be to use the merge function like in tags.

The point I was trying to get is: customers will come at us with questions. For limitations we are aware, it's useful to have clear instructions on "why" the limitation and "what is the alternative". (this internal thread should teach us).

Said that, it looks like the "why" is more on "we are not investing on it", which is fine. Can we at least have a "what's the alternative" clearly written in the docs?

added here: 2bec1f9

marcosuma · 2025-03-19T12:43:23Z

README.md

- [`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`.
- [`num_shards`](https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/cluster#num_shards-2) in `replication_specs` must be a numeric [literal expression](https://developer.hashicorp.com/nomad/docs/job-specification/hcl2/expressions#literal-expressions), e.g. `var.num_shards` is not supported. This is to allow creating a `replication_specs` element per shard in `mongodbatlas_advanced_cluster`.
- `dynamic` blocks are currently supported only for `tags` and `labels`. **Coming soon**: support for `replication_specs` and `regions_config`.
+- [`num_shards`](https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/cluster#num_shards-2) in `replication_specs` must be a numeric [literal expression](https://developer.hashicorp.com/nomad/docs/job-specification/hcl2/expressions#literal-expressions), 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`.


so great to see our limitations going away. Great stuff @lantoli

marcosuma · 2025-03-19T12:57:39Z

internal/convert/testdata/clu2adv/dynamic_regions_config_auto_scaling.out.tf

+  replication_specs = [
+    for i in range(var.replication_specs.num_shards) : {
+      region_configs = flatten([
+        for priority in range(7, 0, -1) : [


nit: @lantoli is it possible to add a comment in the output that explains why we do that?

thanks, added here: 4ab2d3a

lantoli · 2025-03-19T13:21:00Z

internal/convert/convert.go

+	if priorityStr == "" {
+		return nil, fmt.Errorf("%s: %s not found", errRepSpecs, nPriority)
+	}
+	region, err := getRegionConfig(d.content, root, true)


@AgustinBettati FYI region config object creation is reused in individual and dynamic block

internal/convert/testdata/clu2adv/dynamic_regions_config_auto_scaling.out.tf

internal/convert/convert.go

marcosuma · 2025-03-19T17:36:25Z

README.md

- [`num_shards`](https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/cluster#num_shards-2) in `replication_specs` must be a numeric [literal expression](https://developer.hashicorp.com/nomad/docs/job-specification/hcl2/expressions#literal-expressions), e.g. `var.num_shards` is not supported. This is to allow creating a `replication_specs` element per shard in `mongodbatlas_advanced_cluster`.
- `dynamic` blocks are currently supported only for `tags` and `labels`. **Coming soon**: support for `replication_specs` and `regions_config`.
+- [`num_shards`](https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/cluster#num_shards-2) in `replication_specs` must be a numeric [literal expression](https://developer.hashicorp.com/nomad/docs/job-specification/hcl2/expressions#literal-expressions), 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`.
+- `dynamic` blocks are currently supported only for `tags`, `labels` and `regions_config`. See limitations in their corresponding dynamic block sections above. **Coming soon**: support for `replication_specs`.


can you link "See limitations" to "#### Dynamic blocks in regions_config"?

added here: f90d5a2

README.md

EspenAlbert

Great job 👏

Co-authored-by: Marco Suma <[email protected]>

AgustinBettati

leaving one doubt for my understanding

AgustinBettati · 2025-03-20T09:19:54Z

README.md

- [`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`.
- [`num_shards`](https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/cluster#num_shards-2) in `replication_specs` must be a numeric [literal expression](https://developer.hashicorp.com/nomad/docs/job-specification/hcl2/expressions#literal-expressions), e.g. `var.num_shards` is not supported. This is to allow creating a `replication_specs` element per shard in `mongodbatlas_advanced_cluster`.
- `dynamic` blocks are currently supported only for `tags` and `labels`. **Coming soon**: support for `replication_specs` and `regions_config`.
+- [`num_shards`](https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/cluster#num_shards-2) in `replication_specs` must be a numeric [literal expression](https://developer.hashicorp.com/nomad/docs/job-specification/hcl2/expressions#literal-expressions), 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`.


This limitation doesn't apply if you're using dynamic blocks in regions_config or replication_specs

I am not fully clear why we can support this case but not when is a regular literal replication_specs block.

because in the literal case we physically replicate the block num_shard times so we avoid to introduce for loops and the adv_cluster is straighthforward. However in the module case as we need to create the loops and introduce some complexity, it's ok to also iterate through the priorities. We could potentially support that case if some customers are interested

AgustinBettati · 2025-03-20T09:28:10Z

internal/convert/testdata/clu2adv/dynamic_regions_config_auto_scaling.out.tf

+
+  backup_enabled = var.backup_enabled
+  replication_specs = [
+    for i in range(var.replication_specs.num_shards) : {


like the simple way of supporting num_shards

@AgustinBettati yeah exactly, we could do it using the for loop, right now we copy the block shard times

doc assumption for dynamic blocks in tags and labels

c7857b0

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

lantoli mentioned this pull request Mar 17, 2025

feat: Supports dynamic blocks in tags and labels #31

Merged

13 tasks

lantoli added 6 commits March 17, 2025 17:49

dynamic_regions_config example

72a2fbc

range for priorities

26d2183

allow dynamic block in regions_config

cf5e3f7

doc

771845c

update comment

5dfbede

minimum implementation to have test failing because difference in gol…

17a6444

…den file

oarbusi reviewed Mar 18, 2025

View reviewed changes

internal/convert/convert.go Outdated Show resolved Hide resolved

lantoli added 8 commits March 18, 2025 12:29

export enclose funcs

0f10bb9

create EncloseNewLines and remove SetAttrExpr

44a4bc7

root replication_specs

130009b

remove priority checks about numerical literal

b1dc050

reuse getRegionConfig from dynamic block logic

c5e6d5f

only sort by priority if all priorities are numerical literals

ae8471b

remove limitations for priority and electable_nodes

3f93627

use config in dynamic blocks from individual

ff3fc22

lantoli changed the title ~~feat: Supports dynamic blocks in regions_config and replication_specs~~ feat: Supports dynamic blocks in regions_config Mar 18, 2025

lantoli added 11 commits March 19, 2025 06:05

passing test

87bf0fc

add auto_scaling example

b4db307

fix region_configs name replacement

969da96

refactor isDynamicBlock

55b93fd

go back to unexported tokenNewLine

6186dcd

add analytics specs

1883ff0

Merge branch 'main' into CLOUDP-303941_regions_config

032c072

* main: chore: Bump github.com/spf13/afero from 1.12.0 to 1.14.0 (#36) chore: Bump golangci/golangci-lint-action from 6.5.0 to 6.5.2 (#34) chore: Bump docker/login-action from 3.3.0 to 3.4.0 (#35)

example in readme

4684c7c

clarify num_shards limitation

861a6c4

feedback section

165256e

getDynamicBlockRegionConfigsRegionArray

11314b5

lantoli added 3 commits March 19, 2025 13:15

refactor fillRegionConfigsDynamicBlock

b2c3161

EncloseBracketsNewLines

8f6e967

fillRegionConfigsDynamicBlock doc

0f2dfeb

lantoli marked this pull request as ready for review March 19, 2025 12:29

lantoli requested a review from a team as a code owner March 19, 2025 12:29

move shards closer to where it's used

876b6eb

marcosuma approved these changes Mar 19, 2025

View reviewed changes

lantoli commented Mar 19, 2025

View reviewed changes

lantoli added 3 commits March 19, 2025 18:02

add comment for priority loop

4ab2d3a

add dynamic block doc

32ca654

small doc adjustment

0d61fe7

EspenAlbert reviewed Mar 19, 2025

View reviewed changes

internal/convert/testdata/clu2adv/dynamic_regions_config_auto_scaling.out.tf Show resolved Hide resolved

EspenAlbert reviewed Mar 19, 2025

View reviewed changes

internal/convert/convert.go Show resolved Hide resolved

EspenAlbert reviewed Mar 19, 2025

View reviewed changes

internal/convert/convert.go Show resolved Hide resolved

marcosuma reviewed Mar 19, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

rename to fillReplicationSpecsWithDynamicRegionConfigs

5fa1cb7

EspenAlbert approved these changes Mar 19, 2025

View reviewed changes

lantoli and others added 3 commits March 19, 2025 19:03

Update README.md

b8258ae

Co-authored-by: Marco Suma <[email protected]>

link to limitations

f90d5a2

how to handle limitation

2bec1f9

lantoli merged commit 1bb46dd into main Mar 19, 2025
7 checks passed

lantoli deleted the CLOUDP-303941_regions_config branch March 19, 2025 18:37

AgustinBettati reviewed Mar 20, 2025

View reviewed changes

feat: Supports dynamic blocks in regions_config #33

feat: Supports dynamic blocks in regions_config #33

Uh oh!

Conversation

lantoli commented Mar 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!

Uh oh!

marcosuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lantoli Mar 19, 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EspenAlbert left a comment

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!

lantoli Mar 20, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lantoli commented Mar 17, 2025 •

edited

Loading

lantoli Mar 19, 2025 •

edited

Loading

lantoli Mar 20, 2025 •

edited

Loading