google_project_organization_policy #1226
Conversation
There was a problem hiding this comment.
Thanks @lawrenae! The code looks good, just had a few nitpicks with the docs. The tests need a small bit of work but nothing too terrible.
| func TestAccProjectOrganizationPolicy_boolean(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| folder := acctest.RandomWithPrefix("tf-test") |
There was a problem hiding this comment.
Looks like a lot of this test was copied directly from the "folder" test and a fair number of "folder" references stuck behind. Mind cleaning those up?
|
|
||
| ```hcl | ||
| resource "google_project_organization_policy" "serial_port_policy" { | ||
| project = "123456789" |
There was a problem hiding this comment.
I think the resource wants the project id right, not the number? If so, mind doing something with text in it so people don't think they have to figure out what their project number is?
|
|
||
| ```hcl | ||
| resource "google_project_organization_policy" "services_policy" { | ||
| project = "folders/123456789" |
|
|
||
| The following arguments are supported: | ||
|
|
||
| * `project` - (Required) The project id of the project to set the policy for. Its format is simply {project_id}. |
There was a problem hiding this comment.
I'm not sure the second sentence is really necessary here since we use project all over the google terraform provider to mean the exact same thing. Up to you though, I won't block on it.
|
Hi @danawillow -- great feedback. I've updated accordingly! |
| } | ||
|
|
||
| config := testAccProvider.Meta().(*Config) | ||
| //TODO: fix this to return an ACTUAL projectID from state |
There was a problem hiding this comment.
Can you fix this? (There's also a similar issue in the destroy() function)
Also, I'd highly recommend running the tests on your own if you can- this will make the review go a bit faster so we don't need a full back-and-forth if the tests are failing. There are some instructions at https://github.com/terraform-providers/terraform-provider-google/blob/master/.github/CONTRIBUTING.md#tests, let me know if you have any questions!
There was a problem hiding this comment.
should be fixed - All the tests (for google_project_organization_policy_test) now pass. My apologies for not getting them going sooner!
google/iam_project.go
Outdated
| @@ -71,3 +72,10 @@ func (u *ProjectIamUpdater) GetMutexKey() string { | |||
| func (u *ProjectIamUpdater) DescribeResource() string { | |||
| return fmt.Sprintf("project %q", u.resourceId) | |||
| } | |||
|
|
|||
| func canonicalProjectId(project string) string { | |||
There was a problem hiding this comment.
Since this is only used in the test, should it live in that file?
| "github.com/hashicorp/terraform/terraform" | ||
| "google.golang.org/api/cloudresourcemanager/v1" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Mind adding a comment here explaining why the tests don't run in parallel? (that way nobody tries to re-parallelize them)
* support google_project_organization_policy * add documentation for google_project_organization_policy * remove "folder" references and cleanup docs * fix tests * un-parallelize tests * add comment about non-parralel tests * moving canonicalProjectId() to test
|
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
locked and limited conversation to collaborators
this to implement #1193
Feedback most welcome -- this is my 1st PR for this project.