providers/google: google_project supports billing account #11653

evandbrown · 2017-02-03T03:43:55Z

This change allows a Terraform user to set and update the billing account associated with their project.

evandbrown · 2017-02-03T03:47:24Z

This one is really tricky to write an integration test for. We'd need to create two dummy Billing Accounts in the testing Organization, and give the service account that runs the integration test the proper permission in the Organization. Then introduce the two Billing Account IDs as env vars to the tests. Not impossible, but really raises the bar for someone running the integration tests locally.

For now, I'm testing manually to verify create and update of a project with a billing account. Thoughts, @danawillow and @paddyforan?

Thanks!

danawillow

LGTM, just a few nitpicks.

As for testing, I agree that it'll be tricky, especially for someone running locally (and I don't want another set of environment variables that have to be set for someone running it locally). If we had a way of using mocks (and then running non-mocks only via CI) that would probably be the ideal solution, but I don't see that happening anytime soon. For now my opinion is to just get the change in- I don't think that's too reckless (famous last words).

danawillow · 2017-02-06T21:36:59Z

builtin/providers/google/resource_google_project.go

+		ba := cloudbilling.ProjectBillingInfo{
+			BillingAccountName: "billingAccounts/" + v.(string),
+		}
+		_, err = config.clientBilling.Projects.UpdateBillingInfo("projects/"+pid, &ba).Do()


nit: even though it's a teeny tiny thing, since you do "projects/" + pid a few times I wouldn't mind seeing that in a function

Not a nit at all! Done.

danawillow · 2017-02-06T21:39:49Z

builtin/providers/google/resource_google_project.go

+		return fmt.Errorf("Error reading billing account for project %q: %v", "projects/"+pid, err)
+	}
+	if ba.BillingAccountName != "" {
+		_ba := strings.Split(ba.BillingAccountName, "billingAccounts/")


TrimPrefix is probably cleaner than split

The way this is written, I don't actually know what the string is expected to be... it could be any one of these:

projects/{PROJECT}/billingAccounts/{account name}

billingAccounts/{account name}

https://www.googlespis.com/projects/{PROJECT}/billingAccounts/{account name}

Honestly, would love some clarification (esp as a code comment) around what ba.BillingAccountName is expected to be at this point. And agree strings.Split may not be the most semantic function to use.

TIL TrimPrefix. Done.

paddycarver · 2017-02-07T05:10:55Z

Have a few thoughts on this, let me circle around tomorrow. :)

paddycarver

This looks great, with just a few nitpicks here and there. For my part, though, I'd love to see some sort of tests here.

If I understand your explanation right, the problem is we would need to do some special setup on the project before we could run the tests, namely adding billing accounts, giving the service account permissions, and passing the billing accounts in as an environment variable.

My gut on this is that we've kind of crossed that bridge already:

XPN requires our project to be whitelisted before the tests can be run, anyways.
Projects requires the Org ID to be passed in as an environment variable.
Projects requires the service account to have specific permissions.

On my end, I think "untested features" is more frightening than "painful to test features", and at least with the painful to test features, we have good examples to look at when rethinking our testing framework, and can potentially make them simpler to run. There are some ways I can think of to automate the stuff we've mentioned so Terraform could bootstrap testing environments, and having "here's why that's a good idea" is always good.

That said, I'm open to being convinced that increasing the difficulty of running tests on non-official projects is not worth it in this case. My general stance is that making testing on non-official projects harder is bad, and we should avoid it where possible, but if it's necessary for us to test functionality, let's do it.

paddycarver · 2017-02-07T17:34:44Z

builtin/providers/google/resource_google_project.go

+		_, err = config.clientBilling.Projects.UpdateBillingInfo("projects/"+pid, &ba).Do()
+		if err != nil {
+			d.Set("billing_account", "")
+			if _err, ok := err.(*googleapi.Error); ok {


Is there a difference between how *googleapi.Error and error get printed when using %v?

There is. The former will unpack and print any nested errors.

Fair enough!

paddycarver · 2017-02-07T17:37:56Z

builtin/providers/google/resource_google_project.go

+		return fmt.Errorf("Error reading billing account for project %q: %v", "projects/"+pid, err)
+	}
+	if ba.BillingAccountName != "" {
+		_ba := strings.Split(ba.BillingAccountName, "billingAccounts/")


The way this is written, I don't actually know what the string is expected to be... it could be any one of these:

projects/{PROJECT}/billingAccounts/{account name}

billingAccounts/{account name}

https://www.googlespis.com/projects/{PROJECT}/billingAccounts/{account name}

Honestly, would love some clarification (esp as a code comment) around what ba.BillingAccountName is expected to be at this point. And agree strings.Split may not be the most semantic function to use.

sethvargo · 2017-02-13T22:22:37Z

builtin/providers/google/resource_google_project.go

@@ -210,6 +231,18 @@ func resourceGoogleProjectRead(d *schema.ResourceData, meta interface{}) error {
 	d.Set("policy_etag", pol.Etag)
 	d.Set("policy_data", string(polBytes))

+	// Read the billing account
+	ba, err := config.clientBilling.Projects.GetBillingInfo("projects/" + pid).Do()


It looks like it's possible to create projects without a billing account (now), and this line of code is causing any projects which do not have a billing account to return a 403 on read.

Hmm, I'm not seeing this. I've created a few projects (and just added tests that do the same) that don't have billing accounts but make it through read. GetBillingInfo should return an empty billing account if none is set. Possible to see your config?

@evandbrown it's sadder than that 😦 . Steps to repro:

Compile Terraform without this PR

Create project

Compile Terraform with this PR

Change a project attr

Get error

This change allows a Terraform user to set and update the billing account associated with their project.

This change adds optional acceptance tests for project billing accounts. GOOGLE_PROJECT_BILLING_ACCOUNT and GOOGLE_PROJECT_BILLING_ACCOUNT_2 must be set in the environment for the tests to run; otherwise, they will be skipped. Also includes a few code cleanups per review.

evandbrown · 2017-02-15T00:57:51Z

@paddyforan totally reasonable; tests are added.

sethvargo · 2017-02-17T19:10:33Z

builtin/providers/google/resource_google_project_iam_policy_test.go

+func testAccGoogleProject_createBilling(pid, name, org, billing string) string {
+	return fmt.Sprintf(`
+resource "google_project" "acceptance" {
+    project_id = "%s"


Looks like tabs vs spaces happened here

paddycarver · 2017-02-17T22:07:57Z

Thanks for adding those tests, @evandbrown! This looks great. Looks like @sethvargo had a nit about tabs/spaces, don't know if you want to update that or not. Either is fine by me. But this has a 👍 from me.

* Vendor google.golang.org/api/cloudbilling/v1 * providers/google: Add cloudbilling client * providers/google: google_project supports billing account This change allows a Terraform user to set and update the billing account associated with their project. * providers/google: Testing project billing account This change adds optional acceptance tests for project billing accounts. GOOGLE_PROJECT_BILLING_ACCOUNT and GOOGLE_PROJECT_BILLING_ACCOUNT_2 must be set in the environment for the tests to run; otherwise, they will be skipped. Also includes a few code cleanups per review. * providers/google: Improve project billing error message

ghost · 2020-04-16T02:19:49Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

evandbrown added enhancement provider/google-cloud labels Feb 3, 2017

evandbrown requested a review from danawillow February 3, 2017 03:43

danawillow approved these changes Feb 6, 2017

View reviewed changes

paddycarver suggested changes Feb 7, 2017

View reviewed changes

sethvargo suggested changes Feb 13, 2017

View reviewed changes

evandbrown added 4 commits February 14, 2017 14:40

Vendor google.golang.org/api/cloudbilling/v1

33b84b2

providers/google: Add cloudbilling client

9d34608

providers/google: google_project supports billing account

438caaa

This change allows a Terraform user to set and update the billing account associated with their project.

evandbrown force-pushed the google-project-billing branch from 9f3b76e to d49ab62 Compare February 15, 2017 00:55

providers/google: Improve project billing error message

e69e4d5

sethvargo reviewed Feb 17, 2017

View reviewed changes

sethvargo approved these changes Feb 17, 2017

View reviewed changes

paddycarver approved these changes Feb 17, 2017

View reviewed changes

stack72 merged commit facc50d into hashicorp:master Feb 20, 2017

ghost locked and limited conversation to collaborators Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

providers/google: google_project supports billing account #11653

providers/google: google_project supports billing account #11653

evandbrown commented Feb 3, 2017

evandbrown commented Feb 3, 2017

danawillow left a comment

danawillow Feb 6, 2017

evandbrown Feb 15, 2017

danawillow Feb 6, 2017

paddycarver Feb 7, 2017

evandbrown Feb 15, 2017

paddycarver commented Feb 7, 2017

paddycarver left a comment

paddycarver Feb 7, 2017

evandbrown Feb 14, 2017

paddycarver Feb 15, 2017

paddycarver Feb 7, 2017

sethvargo Feb 13, 2017

evandbrown Feb 15, 2017

sethvargo Feb 15, 2017

evandbrown commented Feb 15, 2017

sethvargo Feb 17, 2017

paddycarver commented Feb 17, 2017

ghost commented Apr 16, 2020

providers/google: google_project supports billing account #11653

providers/google: google_project supports billing account #11653

Conversation

evandbrown commented Feb 3, 2017

evandbrown commented Feb 3, 2017

danawillow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddycarver commented Feb 7, 2017

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evandbrown commented Feb 15, 2017

Choose a reason for hiding this comment

paddycarver commented Feb 17, 2017

ghost commented Apr 16, 2020