chore: Implements `tags` and `labels` as MapAttribute in `advanced_cluster` schema v2 #2996

lantoli · 2025-01-23T06:55:26Z

Description

Implements tags and labels as MapAttribute in advanced_cluster schema v2.

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

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

* master: chore: Updates repository to use supported Terraform versions (#2997) chore: Updates CHANGELOG.md header for v1.26.0 release chore: Updates examples link in index.md for v1.26.0 release doc: Upgrade guide for v1.26.0 (#2992)

This reverts commit 3653edc.

* master: chore: Extracts out SDK logic from TPF to facilitate reuse in SDKv2 for advanced_cluster (#2982) ci: Adds clean-atlas-org action and related tests for project cleanup (#2960) chore: Adds MacT documentation (#2993)

lantoli · 2025-01-24T16:29:56Z

internal/service/advancedcluster/resource_advanced_cluster_test.go

+		value := block["value"]
+		checks = append(checks,
+			acc.TestCheckResourceAttrSchemaV2(isAcc, resourceName, key, value),
+		)


thanks @EspenAlbert for the tips for checking maps

EspenAlbert · 2025-01-24T17:06:58Z

internal/service/advancedclustertpf/model_ClusterDescription20240805.go

+	elms := make(map[string]string)
+	if input != nil {
+		for _, item := range *input {
+			key := conversion.SafeValue(item.Key)


Any reason why we are not using item.GetKey()?

thanks changed here: f24f8dc

EspenAlbert · 2025-01-24T17:07:49Z

internal/service/advancedclustertpf/model_ClusterDescription20240805.go

-			Value: types.StringValue(item.Value),
+func NewTagsObjType(ctx context.Context, diags *diag.Diagnostics, input *[]admin.ResourceTag) types.Map {
+	elms := make(map[string]string)
+	if input != nil {


nice refactor!

EspenAlbert · 2025-01-24T17:09:29Z

internal/service/advancedclustertpf/model_to_ClusterDescription20240805.go

@@ -113,25 +115,26 @@ func resolveZoneNameOrUseDefault(item *TFReplicationSpecsModel) string {
 	return *zoneName
 }

-func newResourceTag(ctx context.Context, input types.Set, diags *diag.Diagnostics) *[]admin.ResourceTag {
+func newResourceTag(ctx context.Context, diags *diag.Diagnostics, input types.Map) *[]admin.ResourceTag {
 	if input.IsUnknown() {


input.IsUnknown() can be removed now that it is optional only

thanks changed here: f24f8dc

EspenAlbert · 2025-01-24T17:10:38Z

internal/service/advancedclustertpf/move_upgrade_state.go

@@ -94,6 +94,11 @@ func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfp
 		return
 	}
 	setOptionalModelAttrs(ctx, stateObj, model)
+
+	// Set tags and labels to null instead of empty so there is no plan change if there are no tags or labels when Read is called.


I guess you had to learn this the hard way? If you do nothing they are set to empty map? {}?

ha ha, exactly

EspenAlbert · 2025-01-24T17:12:06Z

internal/service/advancedclustertpf/resource_compatiblity.go

+	if mapIn.IsNull() {
+		*mapOut = types.MapNull(types.StringType)
+	} else {
+		*mapOut = types.MapValueMust(types.StringType, nil)


nice usage of Must

EspenAlbert · 2025-01-24T17:12:56Z

internal/testutil/acc/advanced_cluster_schema_v2_test.go

+				tags = {
+					"Key Tag 1" = "Value Tag 1"
+					"Key Tag 2" = "Value Tag 2"
+					tag = "tagvalue"


cool how it only uses " when necessary

yeah, i love it

EspenAlbert

A few minor comments. But looks good!

* master: chore: Adds cipher config & default_max_time_ms to advanced_cluster_tpf (#2972)

AgustinBettati · 2025-01-27T10:47:05Z

internal/service/advancedclustertpf/resource_compatiblity.go

+	overrideMapStringWithPrevStateValue(&modelIn.Labels, &modelOut.Labels)
+	overrideMapStringWithPrevStateValue(&modelIn.Tags, &modelOut.Tags)
+}
+func overrideMapStringWithPrevStateValue(mapIn, mapOut *types.Map) {


I see for cases of the empty lists/map we sometimes go for setting nil directly (example) or in cases like this one support defining the empty map in the config. I would assess what is the drawback UX wise if we dont have this logic to avoid relying on previous state is not ideal.

discussed offline

…uster` schema v2 (#2996) * schema change * model change * create TF models * create Admin models * labels & tags conversion * TEMPORARY skip mocked tests * handle null vs [] * Revert "TEMPORARY skip mocked tests" This reverts commit 3653edc. * temporary don't test check label and tags * improve update and override * fix plan changes in state upgrade * test checks for labels and tags * apply feedback

* master: chore: Adds nil check before retrieving httpResponse data from API calls (#3015) chore: Run advanced_cluster_tpf tests group only on workflow dispatch (#3016) test: Adds a case for finding `DISK_SIZE_GB_INCONSISTENT` error from API and update TPF implementation in advanced_cluster (#3014) chore: Bump github.com/hashicorp/terraform-exec from 0.21.0 to 0.22.0 (#3007) chore: Bump github.com/hashicorp/terraform-plugin-framework-timeouts (#3009) chore: Bump github.com/evanphx/json-patch/v5 from 5.9.0 to 5.9.11 (#3010) chore: Bump github.com/zclconf/go-cty from 1.16.1 to 1.16.2 (#3011) chore: Uses always new schema when moving from cluster (#3004) chore: Implements `tags` and `labels` as MapAttribute in `advanced_cluster` schema v2 (#2996)

lantoli added 5 commits January 22, 2025 18:59

schema change

623161a

model change

214906b

create TF models

8fd6649

create Admin models

99cde29

labels & tags conversion

fdc1bf0

lantoli changed the title ~~chore: Cloudp 295198 tpf tags labels~~ chore: Implements tags and labels as MapAttribute in advanced_cluster schema v2 Jan 23, 2025

lantoli added 9 commits January 23, 2025 08:26

TEMPORARY skip mocked tests

3653edc

handle null vs []

360c86e

Revert "TEMPORARY skip mocked tests"

e150067

This reverts commit 3653edc.

Merge branch 'master' into CLOUDP-295198_tpf_tags_labels

86314a9

* master: chore: Extracts out SDK logic from TPF to facilitate reuse in SDKv2 for advanced_cluster (#2982) ci: Adds clean-atlas-org action and related tests for project cleanup (#2960) chore: Adds MacT documentation (#2993)

temporary don't test check label and tags

bf19829

improve update and override

c3aceeb

fix plan changes in state upgrade

1198109

test checks for labels and tags

0067508

lantoli marked this pull request as ready for review January 24, 2025 16:24

lantoli requested a review from a team as a code owner January 24, 2025 16:24

lantoli commented Jan 24, 2025

View reviewed changes

EspenAlbert reviewed Jan 24, 2025

View reviewed changes

EspenAlbert approved these changes Jan 24, 2025

View reviewed changes

lantoli added 2 commits January 24, 2025 20:13

Merge branch 'master' into CLOUDP-295198_tpf_tags_labels

35900d4

* master: chore: Adds cipher config & default_max_time_ms to advanced_cluster_tpf (#2972)

apply feedback

f24f8dc

lantoli merged commit 3057211 into master Jan 24, 2025
67 of 68 checks passed

lantoli deleted the CLOUDP-295198_tpf_tags_labels branch January 24, 2025 21:07

AgustinBettati reviewed Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Implements `tags` and `labels` as MapAttribute in `advanced_cluster` schema v2 #2996

chore: Implements `tags` and `labels` as MapAttribute in `advanced_cluster` schema v2 #2996

lantoli commented Jan 23, 2025 •

edited

Loading

lantoli Jan 24, 2025

EspenAlbert Jan 24, 2025

lantoli Jan 24, 2025

EspenAlbert Jan 24, 2025

EspenAlbert Jan 24, 2025

lantoli Jan 24, 2025

EspenAlbert Jan 24, 2025

lantoli Jan 24, 2025

EspenAlbert Jan 24, 2025

EspenAlbert Jan 24, 2025

lantoli Jan 24, 2025

EspenAlbert left a comment

AgustinBettati Jan 27, 2025

lantoli Jan 27, 2025

chore: Implements tags and labels as MapAttribute in advanced_cluster schema v2 #2996

chore: Implements tags and labels as MapAttribute in advanced_cluster schema v2 #2996

Conversation

lantoli commented Jan 23, 2025 • edited Loading

Description

Type of change:

Required Checklist:

Further comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EspenAlbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore: Implements `tags` and `labels` as MapAttribute in `advanced_cluster` schema v2 #2996

chore: Implements `tags` and `labels` as MapAttribute in `advanced_cluster` schema v2 #2996

lantoli commented Jan 23, 2025 •

edited

Loading