chore: Upgrades `advanced_cluster` resource to auto-generated SDK #1947

lantoli · 2024-02-14T15:57:39Z

Description

Upgrades advanced_cluster resource to auto-generated SDK

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

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.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contribution guidelines
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 defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.
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

…eckMongoDBAtlasAdvancedClusterAnalyticsScaling

This reverts commit d6818e6.

* master: fix: Fixes timeout and removes deletion logic on update failure for mongodbatlas_search_index resource (#1950) task: remove dependency on pointy (#1953) fix: Converts snapshot_id field from required to optional. (#1924) # Conflicts: # internal/service/cluster/resource_cluster.go

oarbusi · 2024-02-20T08:13:10Z

internal/service/cluster/model_cluster.go

+
+	if d.HasChange("provider_disk_type_name") {
+		_, newdiskTypeName := d.GetChange("provider_disk_type_name")
+		diskTypeName := cast.ToString(newdiskTypeName)


more of a general comment: I see cast.ToString/Bool/etc being used a lot and also see .(bool)/.(string)/etc being used. I think the latter is the more idiomatic way of casting in Go and in my opinion more clear. WDYT? No need to change in this PR but I think we should probably only use one of the ways moving forward

you're totally right, it's something i also discussed with @gssbzn that we're using both styles all across the code and we might want to unify

oarbusi

LGTM overall

AgustinBettati

main comment to double check is the one related to accept_data_risks_and_force_replica_set_reconfig

internal/service/advancedcluster/resource_advanced_cluster.go

AgustinBettati · 2024-02-20T12:26:01Z

internal/service/advancedcluster/data_source_advanced_cluster.go

-	if err := d.Set("labels", FlattenLabels(RemoveLabel(cluster.Labels, DefaultLabel))); err != nil {
+	if err := d.Set("labels", flattenLabels(cluster.GetLabels())); err != nil {


When removing the logic of handling the default label, how does this impact existing terraform state/clusters? I see the risk that now that the read does not filter out the default label, users could encounter a non empty plan.

I see that the expander and flattener have the logic of removing/validating this default label, so the only change is that now in the create we do not add this default label. Would this be correct?

correct, the logic has been simplified but we're keeping the logic to filter out the special label so no unexpected plans will happen

@lantoli Wanted to call out that we currently mention in the documentation for cluster and advanced cluster that users should not set Infrastructure Tool label, should we update this in documentation as well?

terraform-provider-mongodbatlas/website/docs/d/advanced_cluster.html.markdown

Line 91 in d7b15fc

Key-value pairs that categorize the cluster. Each key and value has a maximum length of 255 characters. You cannot set the key `Infrastructure Tool`, it is used for internal purposes to track aggregate usage.

Or can users still not set it based on the logic?

i've kept that behavior so the doc is correct, user will get an error if trying to set that label

internal/service/advancedcluster/model_advanced_cluster.go

internal/service/advancedcluster/resource_advanced_cluster_migration_test.go

…returns a value

AgustinBettati

LGTM, thanks for the follow ups

…st groups that must be executed before merging

* master: chore: Bump dorny/paths-filter from 3.0.0 to 3.0.1 (#1964) chore: Bump github.com/aws/aws-sdk-go from 1.50.17 to 1.50.22 (#1965) chore: Upgrades `advanced_cluster` resource to auto-generated SDK (#1947) fix: mongodbatlas_encryption_at_rest resource with google_cloud_kms_config (#1962) chore: Restores atlas streams guide in examples section (#1958) chore: Signs created tag during release process (#1960)

lantoli added 5 commits February 14, 2024 16:29

rename

b198e50

mig.TestStep

1f96b82

SkipInUnitTest

9464d48

TEMPORARY skip most acc tests

508d117

CheckDestroyTeamAdvancedCluster with new SDK

cee2552

lantoli changed the title ~~chore: Upgrades advanced_cluster resource to auto-generated SDK~~ chore: Upgrades advanced_cluster resource to auto-generated SDK Feb 14, 2024

lantoli added 24 commits February 15, 2024 08:12

more renames

9ed4f9d

unify CheckDestroy

c8a6991

acc.SkipInUnitTest in atlas_user ds

5f09abf

remove unneeded testAccCheckMongoDBAtlasAdvancedClusterAttributes

387ce06

refactor testAccCheckMongoDBAtlasAdvancedClusterScaling and testAccCh…

1c0099d

…eckMongoDBAtlasAdvancedClusterAnalyticsScaling

refactor checkExists

b558570

import

0de9b7e

TEMPORARY only run advanced cluster tests

8c0dd50

add new mig test

bb2c153

extract resource and ds names in tests

7940d80

read & ds in new sdk still using some old sdk

f62a76d

fix mig test

924312b

flattenProcessArgs

07a8f83

TEMPORARY don't run network tests

edd5127

split cluster and advanced cluster common methods that will diverge

b45ae6b

run cluster tests

d6818e6

TEMPORARY don't run network tests (fix)

2410003

Revert "run cluster tests"

8746120

This reverts commit d6818e6.

FlattenTags

60c171a

flattenLabels

b1d811d

cluster and project random names

5b8a27f

flattenBiConnectorConfig

5412441

flattenConnectionStrings

1ab9b2f

lantoli added 13 commits February 17, 2024 18:38

revert long tests

8eb9ebd

most of create

e1136c4

TEMPORARY: don't run serverless tests

6a1f37e

delete

3cb23bc

expandProcessArgs in create

ead12e5

expandProcessArgs

575fcd6

split ResourceClusterRefreshFunc

687b62c

resourceRefreshFunc

5f37936

UpgradeCluster

6be0170

getUpgradeRequest

d881453

revert test skips

4938549

remove old sdk from tests

741c71c

blanks

2804312

lantoli marked this pull request as ready for review February 20, 2024 06:48

lantoli requested a review from a team as a code owner February 20, 2024 06:48

oarbusi reviewed Feb 20, 2024

View reviewed changes

oarbusi approved these changes Feb 20, 2024

View reviewed changes

AgustinBettati requested changes Feb 20, 2024

View reviewed changes

lantoli added 4 commits February 20, 2024 16:12

keep behavior for accept_data_risks_and_force_replica_set_reconfig

72f10e1

move out UpgradeCluster from model

c371a9a

remove pointer from expandProcessArgs return so it's clear it always …

5df8b25

…returns a value

rename mig.TestStep to TestStepCheckEmptyPlan

1ab91d8

lantoli requested a review from AgustinBettati February 20, 2024 15:36

AgustinBettati approved these changes Feb 20, 2024

View reviewed changes

lantoli added 3 commits February 20, 2024 18:16

random names functions and mig.TestStepCheckEmptyPlan for tests in te…

2d9fd0e

…st groups that must be executed before merging

revert acc and mig config

6732903

fix test

5db5eee

lantoli merged commit 3c58898 into master Feb 20, 2024
44 checks passed

lantoli deleted the CLOUDP-229554_advanced_cluster branch February 20, 2024 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Upgrades `advanced_cluster` resource to auto-generated SDK #1947

chore: Upgrades `advanced_cluster` resource to auto-generated SDK #1947

lantoli commented Feb 14, 2024 •

edited

oarbusi Feb 20, 2024 •

edited

lantoli Feb 20, 2024

oarbusi left a comment

AgustinBettati left a comment

AgustinBettati Feb 20, 2024

AgustinBettati Feb 20, 2024 •

edited

lantoli Feb 20, 2024 •

edited

maastha Feb 21, 2024

lantoli Feb 21, 2024

AgustinBettati left a comment

		if err := d.Set("labels", FlattenLabels(RemoveLabel(cluster.Labels, DefaultLabel))); err != nil {
		if err := d.Set("labels", flattenLabels(cluster.GetLabels())); err != nil {

chore: Upgrades advanced_cluster resource to auto-generated SDK #1947

chore: Upgrades advanced_cluster resource to auto-generated SDK #1947

Conversation

lantoli commented Feb 14, 2024 • edited

Description

Type of change:

Required Checklist:

Further comments

oarbusi Feb 20, 2024 • edited

Choose a reason for hiding this comment

lantoli Feb 20, 2024

Choose a reason for hiding this comment

oarbusi left a comment

Choose a reason for hiding this comment

AgustinBettati left a comment

Choose a reason for hiding this comment

AgustinBettati Feb 20, 2024

Choose a reason for hiding this comment

AgustinBettati Feb 20, 2024 • edited

Choose a reason for hiding this comment

lantoli Feb 20, 2024 • edited

Choose a reason for hiding this comment

maastha Feb 21, 2024

Choose a reason for hiding this comment

lantoli Feb 21, 2024

Choose a reason for hiding this comment

AgustinBettati left a comment

Choose a reason for hiding this comment

chore: Upgrades `advanced_cluster` resource to auto-generated SDK #1947

chore: Upgrades `advanced_cluster` resource to auto-generated SDK #1947

lantoli commented Feb 14, 2024 •

edited

oarbusi Feb 20, 2024 •

edited

AgustinBettati Feb 20, 2024 •

edited

lantoli Feb 20, 2024 •

edited