INTMDB-227:Create new Resource and Datasource for Serverless Instance #585

abner-dou · 2021-10-14T00:06:41Z

Description

Added new resource mongodbatlas_serverless_instance and datasources mongodbatlas_serverless_instances and mongodbatlas_serverless_instances.

New documentation was added

Link to any related issue(s):INTMDB-227

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

themantissa · 2021-10-14T00:32:05Z

@abner-dou not sure what happened here with the tests?

abner-dou · 2021-10-14T00:39:13Z

@themantissa I cancelled the acctest because I'm running the same test on my other PR, I will resume the acctest for this when the other finishes

andreaangiolillo

LGTM

andreaangiolillo · 2021-10-14T08:40:24Z

mongodbatlas/data_source_mongodbatlas_serverless_instances.go

+	options := &matlas.ListOptions{
+		/*	PageNum:      d.Get("page_num").(int),
+			ItemsPerPage: d.Get("items_per_page").(int),
+
+		*/
+	}


[q] I am not sure why these lines are commented out

I forgot to delete it

andreaangiolillo · 2021-10-14T08:41:17Z

mongodbatlas/data_source_mongodbatlas_serverless_instances.go

+	projectID, projectIDOK := d.GetOk("project_id")
+
+	if !(projectIDOK) {
+		return diag.Errorf("project_id must be configured")
+	}


Suggested change

projectID, projectIDOK := d.GetOk("project_id")

if !(projectIDOK) {

return diag.Errorf("project_id must be configured")

}

projectID, err := d.GetOk("project_id")

if !(err) {

return diag.Errorf("project_id must be configured")

}

I was just following the naming convention used in the provider for this cases, but I can change it this way

I guess projectIDOK it's fine since it's a boolean, and not an error

My only concern here is that variable is only used on that single if, don't know if you can actually reduce it to a one liner and assign on true, return error if false

the thing is, I need the other asigned variable projectID in order to be use on the rest of the function, if I use a single if line with the declaration, I will need to call again to the d.GetOk to obtain the value

thetonymaster · 2021-10-14T20:15:20Z

mongodbatlas/data_source_mongodbatlas_serverless_instances.go

+		}
+	}
+
+	return serverlessInstancesMap, nil


You can rewrite this as len(serverlessInstances) > 0 and return nil, nil

this condition is already in line 65

if len(serverlessInstances) == 0 { return nil, nil }

Yes, but there's more flow to this

if len(serverlessInstances) > 0 { var serverlessInstancesMap []map[string]interface{} serverlessInstancesMap = make([]map[string]interface{}, len(serverlessInstances)) for i := range serverlessInstances { serverlessInstancesMap[i] = map[string]interface{}{ "connection_strings_standard_srv": serverlessInstances[i].ConnectionStrings.StandardSrv, "create_date": serverlessInstances[i].CreateDate, "id": serverlessInstances[i].ID, "links": flattenLinks(serverlessInstances[i].Links), "mongo_db_version": serverlessInstances[i].MongoDBVersion, "name": serverlessInstances[i].Name, "provider_settings_backing_provider_name": serverlessInstances[i].ProviderSettings.BackingProviderName, "provider_settings_region_name": serverlessInstances[i].ProviderSettings.RegionName, "provider_settings_provider_name": serverlessInstances[i].ProviderSettings.ProviderName, "state_name": serverlessInstances[i].StateName, } return serverlessInstancesMap , nil } return nil, nil

Also why are we returning an error where is none?

thetonymaster · 2021-10-14T20:19:18Z

mongodbatlas/provider.go

 	}
+
+	if enableBetaFeatures := os.Getenv("MONGODB_ATLAS_ENABLE_BETA"); enableBetaFeatures == "true" {
+		resourcesMap["mongodbatlas_serverless_instance"] = resourceMongoDBAtlasServerlessInstance()


can we add a function to add all beta features?

a new function to set only beta features already added

thetonymaster · 2021-10-14T20:20:11Z

mongodbatlas/provider.go

-			"mongodbatlas_cloud_backup_schedule":                 resourceMongoDBAtlasCloudBackupSchedule(),
-		},
+	if enableBetaFeatures := os.Getenv("MONGODB_ATLAS_ENABLE_BETA"); enableBetaFeatures == "true" {
+		dataSourcesMap["mongodbatlas_serverless_instance"] = dataSourceMongoDBAtlasServerlessInstance()


same here, maybe we can just add a single function to add both, and receives dataSourcesMap and resourcesMap

nvm disregard this comment, just add a variable for MONGODB_ATLAS_ENABLE_BETA in the init of the package, set it to true if it's set to true

thetonymaster · 2021-10-14T20:43:37Z

mongodbatlas/resource_mongodbatlas_serverless_instance.go

+	}
+}
+
+func resourceMongoDBAtlasServerlessInstanceUpdate(ctx context.Context, data *schema.ResourceData, i interface{}) diag.Diagnostics {


@themantissa will there be a update for this? since we don't have a forcenew on the requiered arguments (or any at all) any chance on this will force on a hard delete (remove then add the resource).

Eventually yes, but for now there is not. This is so tricky in how Terraform handles these! If someone does not realize what will happen if they, for example, change the name and, due to a ForceNew this removes and recreates the resource, it will delete all that person's data. If that's the only option that will make sense we need a very very clear warning in the docs that any change of the required parameters will result in a new serverless instance and data loss. fyi @shum the lack of a clean update on this is really painful for finding a solid way to protect users.

FWIW names are immutable and will probably remain so

So let's leave it like this now, name might have to be force new and have a big fat warning in the docs, along other potential arguments. Since now it's in beta don't see an issue leaving it as is until it's implemented. Unless someone complains about the case I mentioned.

👍 fyi @shum

thetonymaster · 2021-10-14T20:46:15Z

website/docs/d/search_index.html.markdown

@@ -1,7 +1,7 @@
 ---
 layout: "mongodbatlas"
 page_title: "MongoDB Atlas: search index"
-sidebar_current: "docs-mongodbatlas-search-index"
+sidebar_current: "docs-mongodbatlas-datasource-search-index"


is this for another resource?

yes! I realize that I made a previous mistake for this resource in other PR (the change is so little and don't have an impact in provider)

Any change that's not in the scope of the ticket should be another PR. This is okay this time but work should match the ticket.

gssbzn · 2021-10-20T15:43:03Z

mongodbatlas/data_source_mongodbatlas_serverless_instances.go

+	options := &matlas.ListOptions{}
+
+	serverlessInstances, _, err := conn.ServerlessInstances.List(ctx, projectID.(string), options)


[q] why not nil?

Suggested change

options := &matlas.ListOptions{}

serverlessInstances, _, err := conn.ServerlessInstances.List(ctx, projectID.(string), options)

serverlessInstances, _, err := conn.ServerlessInstances.List(ctx, projectID.(string), nil)

also are we worry about users with more instances than the default pagination limit?

I already managed the pagination with a recursive function to get all the instances

themantissa

Docs edits, thanks!

themantissa · 2021-10-27T00:09:18Z

website/docs/d/search_index.html.markdown

@@ -1,7 +1,7 @@
 ---
 layout: "mongodbatlas"
 page_title: "MongoDB Atlas: search index"
-sidebar_current: "docs-mongodbatlas-search-index"
+sidebar_current: "docs-mongodbatlas-datasource-search-index"


Any change that's not in the scope of the ticket should be another PR. This is okay this time but work should match the ticket.

website/docs/r/serverless_instance.html.markdown

themantissa · 2021-10-27T00:16:59Z

@abner-dou is this ready for a re-review from Gustavo? If so please click the button to re-request. Though I'd fix the tests first.

themantissa

LGTM, thank you!

themantissa · 2021-10-27T23:33:46Z

@abner-dou again, is this ready for a re-review from Gustavo? If so please click the button to re-request. I don't want to do it if it's not ready, i.e. his comments aren't addressed. @thetonymaster can you do the DoU review here?

gssbzn

LGTM thanks for the pagination

thetonymaster

LGTM

abner-dou added 3 commits October 12, 2021 16:13

added new serverless resource and datasource

2243a38

added serverless instance documentation

39e3db6

added MONGODB_ATLAS_ENABLE_BETA env to run serverless instane

437888a

abner-dou added the run-testacc To run acceptance tests label Oct 14, 2021

abner-dou requested review from themantissa, thetonymaster and coderGo93 October 14, 2021 00:06

abner-dou self-assigned this Oct 14, 2021

abner-dou requested a review from andreaangiolillo as a code owner October 14, 2021 00:06

andreaangiolillo approved these changes Oct 14, 2021

View reviewed changes

thetonymaster reviewed Oct 14, 2021

View reviewed changes

fix

217a970

abner-dou added run-testacc To run acceptance tests and removed run-testacc To run acceptance tests labels Oct 19, 2021

added MONGODB_ATLAS_ENABLE_BETA to acctest yaml

157e357

abner-dou added run-testacc To run acceptance tests and removed run-testacc To run acceptance tests labels Oct 19, 2021

abner-dou requested a review from thetonymaster October 20, 2021 15:36

gssbzn reviewed Oct 20, 2021

View reviewed changes

added pagination get instances

d0a62b0

abner-dou added run-testacc To run acceptance tests and removed run-testacc To run acceptance tests labels Oct 25, 2021

fix

c4d15c5

abner-dou removed the run-testacc To run acceptance tests label Oct 26, 2021

abner-dou added the run-testacc To run acceptance tests label Oct 26, 2021

themantissa requested changes Oct 27, 2021

View reviewed changes

fix lint

432a5bb

abner-dou added run-testacc To run acceptance tests and removed run-testacc To run acceptance tests labels Oct 27, 2021

added documentation for beta feature

70e1eab

abner-dou requested a review from themantissa October 27, 2021 19:00

abner-dou added run-testacc To run acceptance tests and removed run-testacc To run acceptance tests labels Oct 27, 2021

abner-dou requested a review from andreaangiolillo October 27, 2021 19:00

themantissa approved these changes Oct 27, 2021

View reviewed changes

abner-dou requested a review from gssbzn October 27, 2021 23:36

gssbzn approved these changes Oct 28, 2021

View reviewed changes

thetonymaster approved these changes Oct 28, 2021

View reviewed changes

abner-dou and others added 2 commits October 28, 2021 13:29

Merge branch 'master' into INTMDB-227-serverless

9b53b2e

fix lint

448ce52

abner-dou merged commit 99bb60e into master Oct 28, 2021

abner-dou deleted the INTMDB-227-serverless branch October 28, 2021 19:47

		options := &matlas.ListOptions{}

		serverlessInstances, _, err := conn.ServerlessInstances.List(ctx, projectID.(string), options)

INTMDB-227:Create new Resource and Datasource for Serverless Instance #585

INTMDB-227:Create new Resource and Datasource for Serverless Instance #585

Conversation

abner-dou commented Oct 14, 2021 • edited Loading

Description

Type of change:

Required Checklist:

Further comments

themantissa commented Oct 14, 2021

abner-dou commented Oct 14, 2021 • edited Loading

andreaangiolillo 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

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

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

themantissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themantissa commented Oct 27, 2021 • edited Loading

themantissa left a comment

Choose a reason for hiding this comment

themantissa commented Oct 27, 2021

gssbzn left a comment

Choose a reason for hiding this comment

thetonymaster left a comment

Choose a reason for hiding this comment

abner-dou commented Oct 14, 2021 •

edited

Loading

abner-dou commented Oct 14, 2021 •

edited

Loading

themantissa commented Oct 27, 2021 •

edited

Loading