-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
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).
» 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 |
Part of better support for folders (#132) |
Will add documentation in a coming commit. |
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).
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}
} |
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 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 |
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. |
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... |
There was a problem hiding this 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
google/data_source_google_folder.go
Outdated
parent := d.Get("parent").(string) | ||
displayName := d.Get("display_name").(string) | ||
|
||
queryString := fmt.Sprintf("lifecycleState=ACTIVE AND parent=%s AND displayName=%s", parent, displayName) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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/data_source_google_folder.go
Outdated
"google.golang.org/api/googleapi" | ||
) | ||
|
||
func dataSourceGoogleFolder() *schema.Resource { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
--- PASS: TestAccDataSourceGoogleActiveFolder (13.58s) |
Merged! Thank you Norberto for your contribution :) |
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! |
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).