Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add folder data source #738

Merged
merged 4 commits into from
Nov 15, 2017
Merged

Conversation

nlopes
Copy link
Contributor

@nlopes nlopes commented Nov 14, 2017

It is common to split infrastructure in GCP by environments, where all are under the same
folder. You can think of it this way: you have a folder named "EnvA" that contains two
different projects, each project managed by a filesystem folder in the terraform
directory.

Currently, one would have to hardcode the folder name in the variables file (as an option)
after it had been created. With this data source it becomes possible to reference the
folder with a given display name (this has to been unique within the same parent).

It is common to split infrastructure in GCP by environments, where all are under the same
folder. You can think of it this way: you have a folder named "EnvA" that contains two
different projects, each project managed by a filesystem folder in the terraform
directory.

Currently, one would have to hardcode the folder name in the variables file (as an option)
after it had been created. With this data source it becomes possible to reference the
folder with a given display name (this has to been unique within the same parent).
@nlopes
Copy link
Contributor Author

nlopes commented Nov 14, 2017

» make testacc TEST=./google TESTARGS='-run=TestAccDataSourceGoogleFolder' GOOGLE_USE_DEFAULT_CREDENTIALS=true

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccDataSourceGoogleFolder -timeout 120m
=== RUN   TestAccDataSourceGoogleFolder
--- PASS: TestAccDataSourceGoogleFolder (14.82s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	14.845s

@nlopes
Copy link
Contributor Author

nlopes commented Nov 14, 2017

Part of better support for folders (#132)

@nlopes
Copy link
Contributor Author

nlopes commented Nov 14, 2017

Will add documentation in a coming commit.

@rosbo
Copy link
Contributor

rosbo commented Nov 14, 2017

Hi,

I am hesitant about adding this data source as it would lead to unexpected behavior if two folders have the same display name. In a large organization, it is easy for someone to create a new folder without knowing about the other and it would break all your config. I had the same hesitation about adding a google_project data source that would also search for display name. See discussion in #699 for more details.

In the end, I think what you want to achieve would be better and safer using terraform_remote_state (assuming you are storing your state file remotely).

  1. You have a central project creating the folders.
  2. Each projects under an environment referenced to the folder via the remote state:

Central project TF config:

resource "google_folder" "envA" {
  display_name = "Environment A"
  parent     = "organizations/1234567"
}

output "folder_name" {
  value = "${google_folder.envA.name}"
}

Project 1 in envA TF config:

data "terraform_remote_state" "central" {
   ... (add the config to connect to the remote state here)
}

resource "google_project" "project1" {
 name = "Project 1
 project-id = "project-1"
 folder_id = "${data.terraform_remote_state.central.folder_name}
}

@rosbo
Copy link
Contributor

rosbo commented Nov 14, 2017

After looking more closely, the folder display name has to be unique for a given parent. So, I am less hesistant to add. However, the question is whether it is useful given you can achieve something similar with the terraform_remote_state. Let me know if the approach I described above works for you.

The main benefit of the terraform_remote_state is that you can use it to retrieve arbitrary name, id and so on from any other terraform project.

@nlopes
Copy link
Contributor Author

nlopes commented Nov 15, 2017

That's a good shout and I understand the reasoning. I'm not too fussed about dropping this in favour of the remote state if you feel that's the right approach. I'll still raise that I feel a bit weird about going across state files for something that could be an explicit data source referencing the component I want.

I don't think it increases security scope but that data remote state will include way more than I'd like just to reach for a folder id which makes me a bit queasy.

@rosbo rosbo self-requested a review November 15, 2017 17:31
@rosbo
Copy link
Contributor

rosbo commented Nov 15, 2017

Given that the name is guaranteed to be unique, our users won't accidentally mess up their setup by creating a second folder with the same name. For this reason, I think it is reasonable to add this data source.

Reviewing...

Copy link
Contributor

@rosbo rosbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments/suggestions. Thanks for your contribution

parent := d.Get("parent").(string)
displayName := d.Get("display_name").(string)

queryString := fmt.Sprintf("lifecycleState=ACTIVE AND parent=%s AND displayName=%s", parent, displayName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you rename the data source to google_active_folder to be explicit about the fact that you can only fetch active folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do, that makes a lot of sense.

}...,
)

parent := fmt.Sprintf("organizations/%s", os.Getenv("GOOGLE_ORG"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use the global test variable org for this.

Copy link
Contributor Author

@nlopes nlopes Nov 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point me to the right place? Where does that live? I don't seem to be able to find it.

Update: no need, just found it.

)

func TestAccDataSourceGoogleFolder(t *testing.T) {
skipIfEnvNotSet(t,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be simplified to skipIfEnvNotSet(t, "GOOGLE_ORG)


# google\_folder

Get a folder within GCP by display_name and parent.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add tick around display_name and parent

"google.golang.org/api/googleapi"
)

func dataSourceGoogleFolder() *schema.Resource {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this method to match the new resource name along with the read method

"github.com/hashicorp/terraform/terraform"
)

func TestAccDataSourceGoogleFolder(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And all these methods here too

@rosbo rosbo self-assigned this Nov 15, 2017
@rosbo
Copy link
Contributor

rosbo commented Nov 15, 2017

--- PASS: TestAccDataSourceGoogleActiveFolder (13.58s)

@rosbo rosbo merged commit 5a261e0 into hashicorp:master Nov 15, 2017
@rosbo
Copy link
Contributor

rosbo commented Nov 15, 2017

Merged! Thank you Norberto for your contribution :)

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
@ghost
Copy link

ghost commented Mar 30, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants