Create, update, move and delete a GCP folders. #416

rosbo · 2017-09-13T00:36:04Z

Fixes #412

… delete operations.

selmanj

Looks pretty good; I like the thorough documentation!

selmanj · 2017-09-13T23:32:44Z

google/resource_google_folder.go

+	// Since we waited above, the operation is guaranteed to have been successful by this point.
+	waitOp, err := config.clientResourceManager.Operations.Get(op.Name).Do()
+	if err != nil {
+		return fmt.Errorf("The folder '%s' has been created but we could not retrieve its id. Delete the folder manually and retry or use 'terraform import'.", displayName)


The err is eaten here; should probably include it in the error message

selmanj · 2017-09-13T23:33:49Z

google/resource_google_folder.go

+
+		Schema: map[string]*schema.Schema{
+			// Format is 'folders/{folder_id}.
+			// The folder id holds the same value.


I think you mean 'id' or 'folder_id' as in terraform id, right? This comment was confusing to me because I don't see folder_id anywhere (this is exacerbated by the fact that other resources have fields like instance_id (which I am responsible for sorry) that are distinct from id).

Replaced 'folder id' by 'terraform id'

selmanj · 2017-09-13T23:39:06Z

google/resource_google_folder.go

+	if err != nil {
+		return fmt.Errorf("The folder '%s' has been created but we could not retrieve its id. Delete the folder manually and retry or use 'terraform import'.", displayName)
+	}
+	response := waitOp.Response.(map[string]interface{})


I'm surprised this works, the documentation seems to indicate that there should be some sort of struct here.

Given we don't control the code that is in Response and the type assertion will cause a big nasty error if it fails, how about using the checked type assertion here? eg:

response, ok := waitOp.Response.(map[string]interface{}) if !ok { // ... }

Good idea. Done

selmanj · 2017-09-13T23:41:41Z

google/resource_google_folder.go

+
+	d.Partial(true)
+	if d.HasChange("display_name") {
+		_, err := config.clientResourceManagerV2Beta1.Folders.Patch(d.Id(), &resourceManagerV2Beta1.Folder{


Any way you could combine the patch code for display_name and parent into one api call? This would result in less code and be less likely to fail.

No we can't. The display_name must be updated with Patch and the parent must be updated with Move

I missed that, thanks.

selmanj · 2017-09-13T23:42:53Z

google/resource_google_folder.go

+	if err != nil {
+		return fmt.Errorf("Error deleting folder %s", displayName)
+	}
+


Missing a d.SetId("") call to indicate successful deletion

Although apparently that isn't needed since the tests passed? I'm curious if we should be doing that or not.

In resource.go in terraform core:

// Destroy the resource since it is created if err := r.Delete(data, meta); err != nil { return r.recordCurrentSchemaVersion(data.State()), err } // Make sure the ID is gone. data.SetId("")

It does it for us. Calling it again won't cause harm. Should I add it for consistency?

I'm 👍 on not setting it, except for in non-Delete methods (Read when not found, etc.)

Good to know, will omit that on delete in the future.

selmanj · 2017-09-13T23:44:08Z

google/resource_google_folder_test.go

+		CheckDestroy: testAccCheckGoogleFolderDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccGoogleFolder_basis(folderDisplayName, parent),


minor nit
basis -> basic (unless this config is really the 'basis' for the test?)

I meant the basis because I use it as a basic for two tests but I guess the intention wasn't clear. I renamed it basic.

selmanj · 2017-09-13T23:47:50Z

google/resource_google_folder_test.go

+	newFolderDisplayName := "tf-test-renamed-" + acctest.RandString(10)
+	org := os.Getenv("GOOGLE_ORG")
+	parent := "organizations/" + org
+	folder := &resourceManagerV2Beta1.Folder{}


Another nit; I've seen other tests declare the resource variable as the struct type, not the pointer type (e.g. resourceManagerV2Beta1.Folder rather than *resourceManagerV2Beta1.Folder. The pointer allocation here feels a bit less explicit. But if this was intentional go ahead and leave it in as this is mainly stylistic.

selmanj · 2017-09-13T23:49:31Z

google/resource_google_folder_test.go

+	})
+}
+
+func TestAccGoogleFolder_move(t *testing.T) {


I got this initially confused with rename (move in hierarchies is often used to rename the element, like in filesystems). Consider adding the suffix _moveParent to the test name?

renamed to _moveParent

selmanj · 2017-09-13T23:50:25Z

google/resource_google_folder_test.go

+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckGoogleFolderExists("google_folder.folder1", folder),
+					testAccCheckGoogleFolderDisplayName(folder, folder1DisplayName),
+					testAccCheckGoogleFolderExists("google_folder.folder2", folder),


Can you use a separate variable here for folder 2? Overwriting folder with the contents of folder 2 and then making similar assertions got really hard to read for me

selmanj · 2017-09-13T23:52:24Z

website/docs/r/google_folder.html.markdown

+
+* `name` - The resource name of the Folder. Its format is folders/{folder_id}.
+* `lifecycle_state` - The lifecycle state of the folder such as `ACTIVE` or `DELETE_REQUESTED`.
+* `name` - Timestamp when the Folder was created. Assigned by the server.


name -> create_time

rosbo

TF_ACC=1 go test ./google -v -run TestAccGoogleFolder_ -timeout 120m
=== RUN   TestAccGoogleFolder_import
--- PASS: TestAccGoogleFolder_import (13.79s)
=== RUN   TestAccGoogleFolder_rename
--- PASS: TestAccGoogleFolder_rename (14.80s)
=== RUN   TestAccGoogleFolder_moveParent
--- PASS: TestAccGoogleFolder_moveParent (37.93s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	66.645s

rosbo · 2017-09-14T00:07:59Z

google/resource_google_folder.go

+
+		Schema: map[string]*schema.Schema{
+			// Format is 'folders/{folder_id}.
+			// The folder id holds the same value.


Replaced 'folder id' by 'terraform id'

rosbo · 2017-09-14T00:08:34Z

google/resource_google_folder.go

+	// Since we waited above, the operation is guaranteed to have been successful by this point.
+	waitOp, err := config.clientResourceManager.Operations.Get(op.Name).Do()
+	if err != nil {
+		return fmt.Errorf("The folder '%s' has been created but we could not retrieve its id. Delete the folder manually and retry or use 'terraform import'.", displayName)


rosbo · 2017-09-14T00:13:00Z

google/resource_google_folder.go

+	if err != nil {
+		return fmt.Errorf("The folder '%s' has been created but we could not retrieve its id. Delete the folder manually and retry or use 'terraform import'.", displayName)
+	}
+	response := waitOp.Response.(map[string]interface{})


Good idea. Done

rosbo · 2017-09-14T00:14:56Z

google/resource_google_folder.go

+
+	d.Partial(true)
+	if d.HasChange("display_name") {
+		_, err := config.clientResourceManagerV2Beta1.Folders.Patch(d.Id(), &resourceManagerV2Beta1.Folder{


No we can't. The display_name must be updated with Patch and the parent must be updated with Move

rosbo · 2017-09-14T00:16:41Z

google/resource_google_folder.go

+	if err != nil {
+		return fmt.Errorf("Error deleting folder %s", displayName)
+	}
+


In resource.go in terraform core:

// Destroy the resource since it is created if err := r.Delete(data, meta); err != nil { return r.recordCurrentSchemaVersion(data.State()), err } // Make sure the ID is gone. data.SetId("")

It does it for us. Calling it again won't cause harm. Should I add it for consistency?

rosbo · 2017-09-14T00:18:48Z

google/resource_google_folder_test.go

+	newFolderDisplayName := "tf-test-renamed-" + acctest.RandString(10)
+	org := os.Getenv("GOOGLE_ORG")
+	parent := "organizations/" + org
+	folder := &resourceManagerV2Beta1.Folder{}


rosbo · 2017-09-14T00:19:35Z

google/resource_google_folder_test.go

+		CheckDestroy: testAccCheckGoogleFolderDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccGoogleFolder_basis(folderDisplayName, parent),


I meant the basis because I use it as a basic for two tests but I guess the intention wasn't clear. I renamed it basic.

rosbo · 2017-09-14T00:19:55Z

google/resource_google_folder_test.go

+	})
+}
+
+func TestAccGoogleFolder_move(t *testing.T) {


renamed to _moveParent

rosbo · 2017-09-14T00:20:02Z

google/resource_google_folder_test.go

+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckGoogleFolderExists("google_folder.folder1", folder),
+					testAccCheckGoogleFolderDisplayName(folder, folder1DisplayName),
+					testAccCheckGoogleFolderExists("google_folder.folder2", folder),


rosbo · 2017-09-14T00:20:25Z

website/docs/r/google_folder.html.markdown

+
+* `name` - The resource name of the Folder. Its format is folders/{folder_id}.
+* `lifecycle_state` - The lifecycle state of the folder such as `ACTIVE` or `DELETE_REQUESTED`.
+* `name` - Timestamp when the Folder was created. Assigned by the server.


danawillow · 2017-09-14T04:33:45Z

google/resource_google_folder.go

+		Schema: map[string]*schema.Schema{
+			// Format is 'folders/{folder_id}.
+			// The terraform id holds the same value.
+			"name": &schema.Schema{


The convention we've mostly been falling into is to list all Required fields first, then Optional, then Computed. I think that would make it more clear that display_name is what the user sets, rather than name (since for so many other resources it's name)

danawillow · 2017-09-14T04:38:29Z

google/resource_google_folder.go

+	if err != nil {
+		return fmt.Errorf("The folder '%s' has been created but we could not retrieve its id. Delete the folder manually and retry or use 'terraform import': %s", displayName, err)
+	}
+	response, ok := waitOp.Response.(map[string]interface{})


nit: if you felt like saving a line of code, you could also do
if response, ok := waitOp.Response.(map[string]interface{}); ok {

I'd prefer if the ok was negated altogether as it reduces nesting. In general if one of the paths leads to an error (or other early return) that's the one that I nest inside if.

It's also a bit more explicit (it requires a bit of careful reading to realize that when val, ok := response["name"]; ok evaluates to false, it falls to the error at the bottom.

Discussed offline. Added a comment.

danawillow · 2017-09-14T04:47:21Z

website/docs/r/google_folder.html.markdown

+
+A folder can contain projects, other folders, or a combination of both. You can use folders to group projects under an organization in a hierarchy. For example, your organization might contain multiple departments, each with its own set of Cloud Platform resources. Folders allows you to group these resources on a per-department basis. Folders are used to group resources that share common IAM policies.
+
+Folders created lives inside an Organization. See the [Organization documentation](https://cloud.google.com/resource-manager/docs/quickstarts) for more details.


nit: s/lives/live

danawillow · 2017-09-14T04:49:09Z

google/resource_google_folder.go

+			// Format is either folders/{folder_id} or organizations/{org_id}.
+			"parent": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,


Should this be required?

paddycarver · 2017-09-14T05:47:30Z

google/resource_google_folder.go

+	if err != nil {
+		return fmt.Errorf("Error deleting folder %s", displayName)
+	}
+


I'm 👍 on not setting it, except for in non-Delete methods (Read when not found, etc.)

paddycarver · 2017-09-14T07:39:52Z

website/docs/r/google_folder.html.markdown

+
+A folder can contain projects, other folders, or a combination of both. You can use folders to group projects under an organization in a hierarchy. For example, your organization might contain multiple departments, each with its own set of Cloud Platform resources. Folders allows you to group these resources on a per-department basis. Folders are used to group resources that share common IAM policies.
+
+Folders created lives inside an Organization. See the [Organization documentation](https://cloud.google.com/resource-manager/docs/quickstarts) for more details.


"Created folders live", or some other variation?

selmanj · 2017-09-14T16:29:12Z

Sorry, meant to change review status to just "comments" without explicit approval but apparently I can only switch from request changes -> approve.

This is good to merge with me as soon as everyone else's comments are addressed.

rosbo · 2017-09-14T17:17:41Z

TF_ACC=1 go test ./google -v -run TestAccGoogleFolder_ -timeout 120m
=== RUN   TestAccGoogleFolder_import
--- PASS: TestAccGoogleFolder_import (13.73s)
=== RUN   TestAccGoogleFolder_rename
--- PASS: TestAccGoogleFolder_rename (14.40s)
=== RUN   TestAccGoogleFolder_moveParent
--- PASS: TestAccGoogleFolder_moveParent (37.29s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	65.553s

* Initialize resourcemanager v2beta1 client * Create new google_folder resource supporting create, move, update and delete operations. * Add documentation for folders

@rileykarson

…ashicorp#416)  /cc @rileykarson

ghost · 2020-03-31T13:52:26Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

rosbo added 3 commits September 12, 2017 15:34

Initialize resourcemanager v2beta1 client

134116a

Create new google_folder resource supporting create, move, update and…

ccd8855

… delete operations.

Add documentation for folders

883c252

rosbo requested review from paddycarver and selmanj September 13, 2017 00:36

fix formatting small formatting issues

9646380

selmanj suggested changes Sep 13, 2017

View reviewed changes

Addressed selmanj's comments

71d1edd

rosbo commented Sep 14, 2017

View reviewed changes

danawillow reviewed Sep 14, 2017

View reviewed changes

paddycarver reviewed Sep 14, 2017

View reviewed changes

selmanj approved these changes Sep 14, 2017

View reviewed changes

Addressed comments

600e890

rosbo merged commit 167abea into hashicorp:master Sep 14, 2017

rosbo deleted the crud-folders branch September 14, 2017 18:20

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

Add docs for the bigquery dataset delete_contents_on_destroy field (h…

1df0bc1

…ashicorp#416)  /cc @rileykarson

ghost locked and limited conversation to collaborators Mar 31, 2020


		A folder can contain projects, other folders, or a combination of both. You can use folders to group projects under an organization in a hierarchy. For example, your organization might contain multiple departments, each with its own set of Cloud Platform resources. Folders allows you to group these resources on a per-department basis. Folders are used to group resources that share common IAM policies.

		Folders created lives inside an Organization. See the [Organization documentation](https://cloud.google.com/resource-manager/docs/quickstarts) for more details.

Create, update, move and delete a GCP folders. #416

Create, update, move and delete a GCP folders. #416

Conversation

rosbo commented Sep 13, 2017

selmanj 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

rosbo 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

selmanj commented Sep 14, 2017

rosbo commented Sep 14, 2017

ghost commented Mar 31, 2020