Skip to content

google folder data source #1280

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

Merged
merged 3 commits into from
Apr 5, 2018
Merged

google folder data source #1280

merged 3 commits into from
Apr 5, 2018

Conversation

lnesci
Copy link
Contributor

@lnesci lnesci commented Apr 1, 2018

Created to solve #1269 .
The code is based on current organization and active_folder data source but to unify it looks like organization data source.
It support get by id, search by fields (for example ACTIVE + parent + display name) and lookup organization (it searches up the tree in case there are sub folders, not just the parent).
This is my first contribution, I hope is welcome and please let me know if I need to modify something.
Thanks!

@rosbo rosbo self-assigned this Apr 2, 2018
@rosbo
Copy link
Contributor

rosbo commented Apr 4, 2018

Hi @Inesci,

The reason why we called the data source to search by display name google_active_folder is because you can have multiple folders with the same display name under the same parent but only one ACTIVE folder with the same display name under that parent. To make sure we get a consistent result from the API and a result consistent with the user expectation, this is why we are filtering on ACTIVE state and make it explicit in the data source name.

For that reason, I would keep two datasources. One to retrieve the folder by id and another one to retrieve it by name.

I am curious about your use case to retrieve the folder by id. If you have the id, what do you use the display_name for in your config?

Thank you

@lnesci
Copy link
Contributor Author

lnesci commented Apr 4, 2018

Hi @rosbo,

It is for a module that has only the folder id, from it we need to retrieve the organization and from the organization infer the domain to then integrate with gsuite.

Thanks

@morgante
Copy link

morgante commented Apr 4, 2018

I think it's sufficient for our use case to simply add a new data source to retrieve folder by ID.

@lnesci
Copy link
Contributor Author

lnesci commented Apr 4, 2018

@rosbo just to double check, the only pending task is to remove the query part of this new data source?
Thanks

@rosbo
Copy link
Contributor

rosbo commented Apr 4, 2018

@lnesci your use case makes sense. Thanks for the clarification. Yes, removing the query part of the new data source is the only thing left before I give my 👍

@lnesci
Copy link
Contributor Author

lnesci commented Apr 4, 2018

$ make testacc TEST=./google TESTARGS='-run=TestAccDataSourceGoogleFolder_' GOOGLE_USE_DEFAULT_CREDENTIALS=true GOOGLE_FOLDER=886025180534 GOOGLE_PROJECT=base-project-196723 GOOGLE_REGION=us-central1 GOOGLE_ORG=65779779009
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccDataSourceGoogleFolder_
-timeout 120m
=== RUN TestAccDataSourceGoogleFolder_byFullName
--- PASS: TestAccDataSourceGoogleFolder_byFullName (2.72s)
=== RUN TestAccDataSourceGoogleFolder_byShortName
--- PASS: TestAccDataSourceGoogleFolder_byShortName (1.58s)
=== RUN TestAccDataSourceGoogleFolder_lookupOrganization
--- PASS: TestAccDataSourceGoogleFolder_lookupOrganization (1.20s)
=== RUN TestAccDataSourceGoogleFolder_byFullNameNotFound
--- PASS: TestAccDataSourceGoogleFolder_byFullNameNotFound (0.25s)
=== RUN TestAccDataSourceGoogleFolder_attributesCheck
--- PASS: TestAccDataSourceGoogleFolder_attributesCheck (15.26s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google (cached)

@lnesci
Copy link
Contributor Author

lnesci commented Apr 4, 2018

@rosbo code updated, please let me know if something needs to be modified.
Thanks!

The following arguments are supported:

* `folder` (Required) - The name of the Folder in the form `{folder_id}` or `folders/{folder_id}`.
* `lookup_organization` (Optional) - `true` to find the organization that the folder belongs, `false` to avoid the lookup. It searches up the tree.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document whether it is set to true or false by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in new commit


func testAccCheckGoogleFolder_attributesCheckConfig(parent string, displayName string) string {
return fmt.Sprintf(`
resource "google_folder" "foobar" {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should create a new folder in the other tests above too instead of using GOOGLE_FOLDER.

To make test run faster, and reduce the number of new folders our tests generate, using GOOGLE_FOLDER is a good idea. However, I suggest we do this in a separate PR since we will need to update our CI server config and GCP account to create and reuse that folder. We should also update the active_folder_data source test.

I filed an issue #1295 to track this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in new commit

@lnesci
Copy link
Contributor Author

lnesci commented Apr 5, 2018

@rosbo requested changes done in commit 432a9de.

$ make testacc TEST=./google TESTARGS='-run=TestAccDataSourceGoogleFolder_' GOOGLE_USE_DEFAULT_CREDENTIALS=true GOOGLE_PROJECT=base-project-196723 GOOGLE_REGION=us-central1 GOOGLE_ORG=65779779009
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccDataSourceGoogleFolder_ -timeout 120m
=== RUN TestAccDataSourceGoogleFolder_byFullName
--- PASS: TestAccDataSourceGoogleFolder_byFullName (17.28s)
=== RUN TestAccDataSourceGoogleFolder_byShortName
--- PASS: TestAccDataSourceGoogleFolder_byShortName (14.73s)
=== RUN TestAccDataSourceGoogleFolder_lookupOrganization
--- PASS: TestAccDataSourceGoogleFolder_lookupOrganization (15.46s)
=== RUN TestAccDataSourceGoogleFolder_byFullNameNotFound
--- PASS: TestAccDataSourceGoogleFolder_byFullNameNotFound (0.34s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 48.000s

Please let me know if something else needs to be modified.

Thanks!

@rosbo rosbo merged commit b02686b into hashicorp:master Apr 5, 2018
@rosbo
Copy link
Contributor

rosbo commented Apr 6, 2018

Thanks for your contribution! Merging.

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
* adding google folder data source with get by id, search by fields and lookup organization functionality

* removing search functionality

* creating folders for each test and updating documentation with default values
@ghost
Copy link

ghost commented Nov 19, 2018

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 unassigned rosbo Nov 19, 2018
@ghost ghost locked and limited conversation to collaborators Nov 19, 2018
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.

4 participants