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

Escape the display name in active folder data source (in case of spaces, etc) #1261

Merged
merged 7 commits into from
Apr 9, 2018

Conversation

ts-tek
Copy link
Contributor

@ts-tek ts-tek commented Mar 27, 2018

Folders can have spaces in them. This change makes it so that the active_folder data source can correctly query for these.

@emilymye
Copy link
Contributor

Hi @ts-tek, thanks for contributing! Could you add a test to data_source_google_active_folder_test.go?

@ts-tek
Copy link
Contributor Author

ts-tek commented Mar 28, 2018

Sure will do.

ts-tek added 2 commits March 28, 2018 08:41

Verified

This commit was signed with the committer’s verified signature.
N0taN3rd John Berlin

Verified

This commit was signed with the committer’s verified signature.
N0taN3rd John Berlin
@ts-tek
Copy link
Contributor Author

ts-tek commented Mar 28, 2018

I've added a test, but not sure how to run the acceptance tests since I don't know what creds to use.

@danawillow
Copy link
Contributor

I went ahead and ran the tests for you:


------- Stdout: -------
=== RUN   TestAccDataSourceGoogleActiveFolder
--- FAIL: TestAccDataSourceGoogleActiveFolder (12.14s)
    testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
        
        * google_folder.foobar_space: 1 error(s) occurred:
        
        * google_folder.foobar_space: Error creating folder 'Space terraform-test-m9ybqwf0vm' in 'organizations/*******': googleapi: Error 400: field [Folder.display_name] has issue [invalid format], badRequest
    testing.go:573: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: Error refreshing: 1 error(s) occurred:
        
        * data.google_active_folder.my_folder_space: 1 error(s) occurred:
        
        * data.google_active_folder.my_folder_space: data.google_active_folder.my_folder_space: More than one folder found
        
        State: <nil>
FAIL

@ts-tek
Copy link
Contributor Author

ts-tek commented Mar 28, 2018

Thank you, this should fix that.

@danawillow
Copy link
Contributor

=== RUN   TestAccDataSourceGoogleActiveFolder
--- FAIL: TestAccDataSourceGoogleActiveFolder (15.12s)
    testing.go:513: Step 1 error: Check failed: Check 1/1 error: display_name is terraform-test-4hin4soucw; want terraform test 4hin4soucw
FAIL

(also psst @emilymye I totally forgot about this yesterday but you should be able to run the tests in teamcity as well- just go to branches and then click the ... next to "Run" for the branch you want and change the value of TEST_PATTERN to the test pattern you want to run)

@ts-tek
Copy link
Contributor Author

ts-tek commented Mar 28, 2018

Got caught out, one more shot now please?

@paddycarver
Copy link
Contributor

New results:

--- FAIL: TestAccDataSourceGoogleActiveFolder (25.07s)
    testing.go:513: Step 1 error: Check failed: Check 1/1 error: name is folders/591018781897; want folders/663644256839
FAIL

@ts-tek
Copy link
Contributor Author

ts-tek commented Mar 30, 2018

Frustrating, so it looks like the Google REST API is returning an unexpected result. If you create two folders that differ by a space character, say: 'Test Folder' and 'Test-Folder', you get the following results in a search request when setting displayName to the following values in the search:

  • Test Folder: empty result set
  • Test+Folder: both folders returned
  • Test-Folder: both folders returned
  • Test%20Folder: empty result set
  • \"Test Folder\": both folders returned

I think this is a bug in the API unless I'm missing something.

I think I'll need to workaround by just assuming there could be multiple results and iterating through them until finding a folder that matches the exact display name given. Will knock something up to that effect.

@ts-tek
Copy link
Contributor Author

ts-tek commented Mar 30, 2018

OK, ready for test again.

@danawillow
Copy link
Contributor

Looks like @paddycarver re-ran the test. Here's the output:


------- Stdout: -------
=== RUN   TestAccDataSourceGoogleActiveFolder
--- FAIL: TestAccDataSourceGoogleActiveFolder (12.43s)
    testing.go:513: Step 1 error: Error refreshing: 1 error(s) occurred:
        
        * data.google_active_folder.space: 1 error(s) occurred:
        
        * data.google_active_folder.space: data.google_active_folder.space: Folder not found
FAIL

Also, I'd love to help you be able to run the tests on your own, if you have the right org permissions to do so. Take a look at the instructions at https://github.com/terraform-providers/terraform-provider-google/blob/master/.github/CONTRIBUTING.md#tests and let me know if you run into any issues!

@ts-tek
Copy link
Contributor Author

ts-tek commented Apr 6, 2018

Thank you for the pointer. I've made some more changes and verified the acceptance tests work now as well.

Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Looks good, thanks so much @ts-tek!

@danawillow danawillow merged commit 024e0b7 into hashicorp:master Apr 9, 2018
@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 👉 hashibot-feedback@hashicorp.com. Thanks!

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants