-
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
google_project_organization_policy #1226
google_project_organization_policy #1226
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
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.
found a folder :)
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is only used in the test, should it live in that file?
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.
done!
"github.com/hashicorp/terraform/terraform" | ||
"google.golang.org/api/cloudresourcemanager/v1" | ||
) | ||
|
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.
Mind adding a comment here explaining why the tests don't run in parallel? (that way nobody tries to re-parallelize them)
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.
done
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.
Thanks @lawrenae! Looks good!
* 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 👉 [email protected]. Thanks! |
this to implement #1193
Feedback most welcome -- this is my 1st PR for this project.