INTMDB-290: Added advanced configuration for datasource/resource of advanced cluster #658

coderGo93 · 2022-01-14T23:09:17Z

Description

Added parameter advanced_configuration for resource and datasource of advanced_cluster
Added docs and tests

Results of tests

Resource
Advanced cluster

make testacc TESTARGS='-run=TestAccResourceMongoDBAtlasAdvancedCluster_advancedConf'   [15:28:46]
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... | grep -v /integrationtesting) -v -parallel 20 -run=TestAccResourceMongoDBAtlasAdvancedCluster_advancedConf -timeout 300m -cover -ldflags="-s -w -X 'github.com/mongodb/terraform-provider-mongodbatlas/version.ProviderVersion=acc'"
?       github.com/mongodb/terraform-provider-mongodbatlas      [no test files]
=== RUN   TestAccResourceMongoDBAtlasAdvancedCluster_advancedConf
=== PAUSE TestAccResourceMongoDBAtlasAdvancedCluster_advancedConf
=== CONT  TestAccResourceMongoDBAtlasAdvancedCluster_advancedConf
--- PASS: TestAccResourceMongoDBAtlasAdvancedCluster_advancedConf (1401.44s)
PASS
coverage: 5.6% of statements
ok      github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas 1404.113s       coverage: 5.6% of statements
?       github.com/mongodb/terraform-provider-mongodbatlas/version      [no test files]

Datasource
Advanced cluster

make testacc TESTARGS='-run=TestAccDataSourceMongoDBAtlasAdvancedCluster_advancedConf'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... | grep -v /integrationtesting) -v -parallel 20 -run=TestAccDataSourceMongoDBAtlasAdvancedCluster_advancedConf -timeout 300m -cover -ldflags="-s -w -X 'github.com/mongodb/terraform-provider-mongodbatlas/version.ProviderVersion=acc'"
?       github.com/mongodb/terraform-provider-mongodbatlas      [no test files]
=== RUN   TestAccDataSourceMongoDBAtlasAdvancedCluster_advancedConf
=== PAUSE TestAccDataSourceMongoDBAtlasAdvancedCluster_advancedConf
=== CONT  TestAccDataSourceMongoDBAtlasAdvancedCluster_advancedConf
--- PASS: TestAccDataSourceMongoDBAtlasAdvancedCluster_advancedConf (746.11s)
PASS
coverage: 6.3% of statements
ok      github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas 750.576s        coverage: 6.3% of statements
?       github.com/mongodb/terraform-provider-mongodbatlas/version      [no test files]

Advanced clusters

make testacc TESTARGS='-run=TestAccDataSourceMongoDBAtlasAdvancedClusters_advancedConf'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... | grep -v /integrationtesting) -v -parallel 20 -run=TestAccDataSourceMongoDBAtlasAdvancedClusters_advancedConf -timeout 300m -cover -ldflags="-s -w -X 'github.com/mongodb/terraform-provider-mongodbatlas/version.ProviderVersion=acc'"
?       github.com/mongodb/terraform-provider-mongodbatlas      [no test files]
=== RUN   TestAccDataSourceMongoDBAtlasAdvancedClusters_advancedConf
=== PAUSE TestAccDataSourceMongoDBAtlasAdvancedClusters_advancedConf
=== CONT  TestAccDataSourceMongoDBAtlasAdvancedClusters_advancedConf
--- PASS: TestAccDataSourceMongoDBAtlasAdvancedClusters_advancedConf (682.70s)
PASS
coverage: 6.2% of statements
ok      github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas 689.151s        coverage: 6.2% of statements
?       github.com/mongodb/terraform-provider-mongodbatlas/version      [no test files]

Link to any related issue(s):

Type of change:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the Terraform contribution guidelines
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

Further comments

…ced cluster

thetonymaster

LGTM

thetonymaster · 2022-01-18T18:44:04Z

mongodbatlas/resource_mongodbatlas_advanced_cluster.go

+		advancedConfReq := expandProcessArgs(d, aclist[0].(map[string]interface{}))
+
+		if ok {
+			_, _, err := conn.Clusters.UpdateProcessArgs(ctx, projectID, cluster.Name, advancedConfReq)


Remove this NL

themantissa

One questions about tests coverage difference between cluster and advanced cluster but that's it. Thank you!

themantissa · 2022-01-19T00:59:29Z

mongodbatlas/resource_mongodbatlas_advanced_cluster_test.go

@@ -247,6 +248,70 @@ func TestAccResourceMongoDBAtlasAdvancedCluster_Paused(t *testing.T) {
 	})
 }



When we added the new read/write concern options to advanced configuration we added another test:
https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/mongodbatlas/resource_mongodbatlas_cluster_test.go#L160
Should we add that here? Or at least add the additional parameters to these tests? DefaultReadConcern
& DefaultWriteConcern ?

Good point, will add those parameters in the same test and see whether it passes or not

andreaangiolillo · 2022-01-19T10:33:51Z

mongodbatlas/data_source_mongodbatlas_advanced_clusters.go

@@ -24,6 +25,7 @@ func dataSourceMongoDBAtlasAdvancedClusters() *schema.Resource {
 				Computed: true,
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
+						"advanced_configuration": clusterAdvancedConfigurationSchemaComputed(),
 						"backup_enabled": {


This is not related to this change but I see that this schema has

"name": { Type: schema.TypeString, Computed: true, },

where we are using Required: true for the name in data_source_mongodbatlas_advanced_cluster.go.
Is this correct or both should be Computed: true?

In the singular datasource is required to get a specific advanced cluster along the project ID, in the plural datasource, it will get all the advanced clusters from the project ID provided and then list it with all computed parameters. Also, you cannot use Computed if you are using Required at the same time.

andreaangiolillo · 2022-01-19T10:36:14Z

mongodbatlas/data_source_mongodbatlas_advanced_clusters.go

+		log.Printf("[WARN] Error setting `advanced_configuration` for the cluster(%s): %s", clusters[i].ID, err)
+


[q] Shouldn’t we check that err != nil before printing the log message?

You're right, will add the validation, thank you

andreaangiolillo · 2022-01-19T10:41:02Z

mongodbatlas/data_source_mongodbatlas_advanced_clusters.go

@@ -221,7 +223,11 @@ func flattenAdvancedClusters(ctx context.Context, d *schema.ResourceData, conn *
 	results := make([]map[string]interface{}, 0)



[q] why d *schema.ResourceData is an argument of this function if it is not used?

Good eye, will remove the argument since is not necessary this time

andreaangiolillo · 2022-01-19T10:54:21Z

mongodbatlas/resource_mongodbatlas_advanced_cluster.go

+	*/
+	if d.HasChange("advanced_configuration") {
+		ac := d.Get("advanced_configuration")
+		if aclist, ok1 := ac.([]interface{}); ok1 && len(aclist) > 0 {


[q] any reason why you are defining ok1 instead ok?

No reason at all and good eye, will update the naming, thank you.

andreaangiolillo · 2022-01-19T10:56:03Z

mongodbatlas/resource_mongodbatlas_advanced_cluster.go

@@ -540,6 +579,22 @@ func resourceMongoDBAtlasAdvancedClusterUpdate(ctx context.Context, d *schema.Re
 		}
 	}

+	/*
+		Check if advanced configuration option has a changes to update it


I think this comment is not clear. Maybe something like "update advanced configuration options if needed" would be better

Understood, will update the comment, thank you

andreaangiolillo

LGTM. Thanks for all the changes

themantissa

LGTM! Thank you!

coderGo93 added 2 commits January 13, 2022 15:59

added advanced configuration for datasource(s) and resource of adavan…

237aacb

…ced cluster

fixes doc typo

8c6fa1c

coderGo93 requested review from abner-dou, andreaangiolillo and themantissa as code owners January 14, 2022 23:09

coderGo93 requested a review from thetonymaster January 14, 2022 23:09

coderGo93 added 2 commits January 14, 2022 16:21

fixes linter

5d74fb7

fixes doc typo

473af7a

coderGo93 changed the title ~~added advanced configuration for datasource(s) and resource of adavan…~~ INTMDB-290: Added advanced configuration for datasource/resource of advanced cluster Jan 14, 2022

thetonymaster approved these changes Jan 18, 2022

View reviewed changes

themantissa requested changes Jan 19, 2022

View reviewed changes

andreaangiolillo reviewed Jan 19, 2022

View reviewed changes

added testacc and refactor

bafb355

coderGo93 requested review from themantissa and andreaangiolillo January 19, 2022 21:17

andreaangiolillo approved these changes Jan 19, 2022

View reviewed changes

themantissa approved these changes Jan 19, 2022

View reviewed changes

coderGo93 added the run-testacc To run acceptance tests label Jan 19, 2022

coderGo93 merged commit eac7d92 into master Jan 20, 2022

coderGo93 deleted the INTMDB-290 branch January 20, 2022 03:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTMDB-290: Added advanced configuration for datasource/resource of advanced cluster #658

INTMDB-290: Added advanced configuration for datasource/resource of advanced cluster #658

coderGo93 commented Jan 14, 2022

thetonymaster left a comment

thetonymaster Jan 18, 2022

themantissa left a comment

themantissa Jan 19, 2022

coderGo93 Jan 19, 2022

andreaangiolillo Jan 19, 2022

coderGo93 Jan 19, 2022 •

edited

Loading

andreaangiolillo Jan 19, 2022

coderGo93 Jan 19, 2022

andreaangiolillo Jan 19, 2022

coderGo93 Jan 19, 2022

andreaangiolillo Jan 19, 2022

coderGo93 Jan 19, 2022

andreaangiolillo Jan 19, 2022

coderGo93 Jan 19, 2022

andreaangiolillo left a comment

themantissa left a comment

		@@ -247,6 +248,70 @@ func TestAccResourceMongoDBAtlasAdvancedCluster_Paused(t *testing.T) {
		})
		}

		log.Printf("[WARN] Error setting `advanced_configuration` for the cluster(%s): %s", clusters[i].ID, err)

		@@ -221,7 +223,11 @@ func flattenAdvancedClusters(ctx context.Context, d schema.ResourceData, conn
		results := make([]map[string]interface{}, 0)

INTMDB-290: Added advanced configuration for datasource/resource of advanced cluster #658

INTMDB-290: Added advanced configuration for datasource/resource of advanced cluster #658

Conversation

coderGo93 commented Jan 14, 2022

Description

Type of change:

Required Checklist:

Further comments

thetonymaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themantissa left a 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

coderGo93 Jan 19, 2022 • edited Loading

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

andreaangiolillo left a comment

Choose a reason for hiding this comment

themantissa left a comment

Choose a reason for hiding this comment

coderGo93 Jan 19, 2022 •

edited

Loading