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

Folder support: Assign/Reassign a google project to a folder. #438

Merged
merged 4 commits into from
Sep 22, 2017

Conversation

srivasta
Copy link
Contributor

This branch addresses terraform-provider-google issue #411

@srivasta srivasta changed the title Folder support Folder support: Assign/Reassign a google project to a folder. Sep 15, 2017
@srivasta
Copy link
Contributor Author

srivasta commented Sep 15, 2017

@rosbo I don't have enough rights to actually assign reviewers. I re-rebased, and resolved all new conflicts

Note: Pull request authors can't request reviews unless they are either a repository owner or collaborator with write access to the repository.

Copy link
Contributor

@rosbo rosbo left a comment

Choose a reason for hiding this comment

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

Very good PR. Just a few comments that should be easy enough to address. Thanks!

"folder_id": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you specify a folder, you should not be able to specify an org. To make sure it fails at
the terraform plan step, add ConflictsWith: []string{"org_id"} in the schema definition for the folder_id field. And vice-versa

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.

d.Set("org_id", p.Parent.Id)
switch p.Parent.Type {
case "organization":
d.Set("org_id", p.Parent.Id)
Copy link
Contributor

Choose a reason for hiding this comment

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

add d.Set("folder_id", "") here. and the same for the `org_id field in the folder case below. The reason is that if you move a project from a folder to an org outside of Terraform, we want Terraform to clear the other field when reading the state.

Copy link
Contributor Author

@srivasta srivasta Sep 19, 2017

Choose a reason for hiding this comment

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

Done.


// Project parent has changed
if d.HasChange("org_id") || d.HasChange("folder_id") {
if v, ok := d.GetOk("org_id"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extracting line 239-253 in a getParentResourceId (or something along these lines) and reuse it for update and create.

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.

Changing this forces a new project to be created.
Changing this forces a new project to be created. If the `org_id`
is specified, but not the `folder_id`, then the project is created
at the top level. If both are specified, `org_id` is ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add the ConflictsWith I suggested earlier, you can update this comment to say that folder_id or org_id can be specified but not both.

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

@srivasta
Copy link
Contributor Author

srivasta commented Sep 19, 2017

Done. Made all the changes requested. I also squashed the commits in this branch, since intermediate points were not interesting, and will give a cleaner history on the mainline.

+ Make the org_id optional when creating a project. Closes hashicorp#131
+ Mark org_id as computed to allow for GCP automatically assigning the org.
+ Add an acceptance test for project creation without an organization.
+ Skip TestAccGoogleProject_createWithoutOrg if GOOGLE_ORG is set.
+ Add a folder_id to the google_project resource, optionally
  specifying the ID of the GCP folder in which the GCP project should
  live.
+ Document how one can provision a project into a folder, and added a
  sample configuration to create a project into an existing folder.

Based on work by ubschmidt2 on branch
 https://github.com/ubschmidt2/terraform-provider-google/commits/folder

Signed-off-by: Manoj Srivastava <[email protected]>
@@ -24,15 +24,42 @@ var (
originalPolicy *cloudresourcemanager.Policy
)

// Test that a Project resource can be created without an organization
func TestAccGoogleProject_createWithoutOrg(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We run our acceptance tests using a service account (not user credentials). Unfortunately, a service account belongs to an org and therefor, cannot create a project without a parent organization.

One solution: we could add a skip if a service account is used. This means we could run it locally but they wouldn't be ran by our CI server.

@paddycarver Any other solutions for this? Running user credentials for the CI server? Not sure it is safe/worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added logic to skip the test if a service account is used. @paddycarver if you have a better suggestion, we can fix it in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm late to the party here--sorry, I was out of office on Thursday--but IIRC we were talking about the possibility of getting the service account our CI runs with whitelisted for creating a project without a parent organisation, though I've lost track of where that discussion landed or what its current status is. That's my preferred solution, honestly, but we can follow up on this with a more permanent fix in a separate PR.

},
})
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you can create folders, you can add acceptance tests for assigning a project to a folder and moving a project from a folder to an org and vice versa. Since you will be on vacation, I can also take care of writing the new tests. LMK what you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds great. I won't have time for this till middle of next month, and so it would be great if you could add the tests with folder creation and movement between folders. I'll circle back in case you don't have time in the next couple of weeks

Copy link
Contributor

Choose a reason for hiding this comment

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

Oki. I will write these tests today. Thanks!

@rosbo rosbo merged commit c1d0e71 into hashicorp:master Sep 22, 2017
negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017
…orp#438)

+ Make the org_id optional when creating a project. Closes hashicorp#131
+ Mark org_id as computed to allow for GCP automatically assigning the org.
+ Add an acceptance test for project creation without an organization.
+ Skip TestAccGoogleProject_createWithoutOrg if GOOGLE_ORG is set.
+ Add a folder_id to the google_project resource, optionally
  specifying the ID of the GCP folder in which the GCP project should
  live.
+ Document how one can provision a project into a folder, and added a
  sample configuration to create a project into an existing folder.
* Skip test without org if service account is used
* Support folders/* or id only for the folder id field
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
…orp#438)

+ Make the org_id optional when creating a project. Closes hashicorp#131
+ Mark org_id as computed to allow for GCP automatically assigning the org.
+ Add an acceptance test for project creation without an organization.
+ Skip TestAccGoogleProject_createWithoutOrg if GOOGLE_ORG is set.
+ Add a folder_id to the google_project resource, optionally
  specifying the ID of the GCP folder in which the GCP project should
  live.
+ Document how one can provision a project into a folder, and added a
  sample configuration to create a project into an existing folder.
* Skip test without org if service account is used
* Support folders/* or id only for the folder id field
@ghost
Copy link

ghost commented Mar 30, 2020

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 locked and limited conversation to collaborators Mar 30, 2020
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