Add billing subaccount #2640

adarobin · 2019-11-11T14:45:23Z

This implements a new google_billing_subaccount resource and would close hashicorp/terraform-provider-google#4033

Release Note Template for Downstream PRs (will be copied)

`google_billing_subaccount`

googlebot · 2019-11-11T14:45:29Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

adarobin · 2019-11-11T14:46:06Z

@googlebot I signed it!

modular-magician · 2019-11-11T14:47:22Z

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs.

Thanks for your contribution! A human will be with you soon.

@rambleraptor, please review this PR or find an appropriate assignee.

rambleraptor · 2019-11-11T18:37:04Z

Hello! Thanks for the PR. I've added @rileykarson who can better help get this merged in.

Before we begin, we have to make sure that @googlebot recognizes that you've signed the CLA. I'm hoping that tagging it will cause it to kick off again.

adarobin · 2019-11-11T21:13:07Z

@googlebot I signed it!

rileykarson

I made a first pass, leaning fairly nitpicky for consistency with other provider resource files.

Are you able to add an acceptance test? https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_compute_address_generated_test.go is a good reference for what most acceptance tests look like, while https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_google_billing_account_iam_test.go is a good example of how to work with billing accounts.

third_party/terraform/utils/provider.go.erb

third_party/terraform/resources/resource_google_billing_subaccount.go

third_party/terraform/website/docs/r/google_billing_subaccount.html.markdown

adarobin · 2019-11-11T21:53:44Z

@rileykarson I'll take a try at adding an acceptance test and go through your comments.

I'm not sure what the deal with the CLA is, a Corporate CLA was signed last week and I am a member of the group.

rileykarson

Can you confirm that the email listed on the second line at https://github.com/GoogleCloudPlatform/magic-modules/commit/f865406be26aa27c14f1fb7558d406f1ffeea162.patch is the email covered by your corporate CLA?

Since these changes are to a third_party directory the CLA doesn't actually apply. I'd slightly prefer that the bot is happy, but it's not strictly necessary.

adarobin · 2019-11-12T18:36:18Z

@rileykarson I have made changes based on your comments and wrote some tests.

As far as the CLA, we had an authorized signer fill out the form late last week, but it looks like it hasn't been approved fully yet.

sokoow · 2020-04-10T08:23:21Z

@rileykarson @adarobin are ye still haggling on the CLA yet ? Any plans to progress this ?

adarobin · 2020-04-16T12:30:14Z

@sokoow I am still pushing for the CLA on my end. I believe it was close, but then all of this COVID-19 stuff happened.

sokoow · 2020-04-16T12:50:43Z

respect, keep us posted pls

third_party/terraform/website/docs/r/google_billing_subaccount.html.markdown

modular-magician · 2020-11-17T16:29:38Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 8 files changed, 343 insertions(+))
Terraform Beta: Diff ( 6 files changed, 337 insertions(+))

modular-magician · 2020-11-17T17:03:45Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 337 insertions(+))
Terraform Beta: Diff ( 6 files changed, 337 insertions(+))

adarobin · 2020-12-03T15:30:55Z

@googlebot I signed it!

google-cla · 2020-12-03T15:31:09Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

adarobin · 2020-12-03T15:33:55Z

@rileykarson Finally have a CLA, what are my next steps?

adarobin · 2020-12-03T16:04:47Z

@samos123 Just realized I didn't ping you too, I have the CLA now!

rileykarson

Thanks for picking this back up! I've made a review pass. Sorry if any of it conflicts with previous comments of mine, it's been a while 🙂

third_party/terraform/website/docs/r/google_billing_subaccount.html.markdown

third_party/terraform/resources/resource_google_billing_subaccount.go

rileykarson · 2020-12-10T21:32:49Z

third_party/terraform/resources/resource_google_billing_subaccount.go

+				Type:     schema.TypeString,
+				Computed: true,
+			},
+			"rename_on_destroy": {


Instead of {{action}}_on_destroy, can you call this field deletion_policy and allow "" and "RENAME_ON_DESTROY" to be specified? https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_user#deletion_policy is a good model, where we've done the same.

I looked at that resource and I think I have implemented it as you have requested

google-cla · 2020-12-11T17:39:59Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

modular-magician · 2020-12-11T17:48:42Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 352 insertions(+))
Terraform Beta: Diff ( 6 files changed, 352 insertions(+))

rileykarson

I'm working on getting a test master billing account, I'll get back to this once I've done so. I've submitted a ticket- that hopefully won't be long!

rileykarson

Alright, environment set up. _basic succeeded, _renameOnDestroy failed- I think it's just because we're still using the old rename_on_destroy field (inline).

third_party/terraform/tests/resource_google_billing_subaccount_test.go

google-cla · 2020-12-15T16:40:13Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

modular-magician · 2020-12-15T16:48:03Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 352 insertions(+))
Terraform Beta: Diff ( 6 files changed, 352 insertions(+))

rileykarson

Alright, just a couple of lint errors left. Sorry for not catching these in my prior pass! I lost track when digging into registering a parent billing account.

==> Checking source code against linters...
google/resource_google_billing_subaccount.go:96:7: Error return value of `d.Set` is not checked (errcheck)
	d.Set("name", billingAccount.Name)
	     ^
google/resource_google_billing_subaccount.go:97:7: Error return value of `d.Set` is not checked (errcheck)
	d.Set("display_name", billingAccount.DisplayName)
	     ^
google/resource_google_billing_subaccount.go:98:7: Error return value of `d.Set` is not checked (errcheck)
	d.Set("open", billingAccount.Open)
	     ^
google/resource_google_billing_subaccount.go:99:7: Error return value of `d.Set` is not checked (errcheck)
	d.Set("master_billing_account", billingAccount.MasterBillingAccount)
	     ^
google/resource_google_billing_subaccount.go:100:7: Error return value of `d.Set` is not checked (errcheck)
	d.Set("billing_account_id", strings.TrimPrefix(d.Get("name").(string), "billingAccounts/"))
	     ^
google/resource_google_billing_subaccount_test.go:92:6: `testAccBillingSubccount_updateRenameOnDestroy` is unused (deadcode)
func testAccBillingSubccount_updateRenameOnDestroy(masterBillingAccountId string) string {
     ^
make: *** [lint] Error 1

rileykarson · 2020-12-15T17:26:54Z

third_party/terraform/tests/resource_google_billing_subaccount_test.go

+`, masterBillingAccountId)
+}
+
+func testAccBillingSubccount_updateRenameOnDestroy(masterBillingAccountId string) string {


This is unused, and can be removed.

removed testAccBillingSubccount_updateRenameOnDestroy

rileykarson · 2020-12-15T17:29:02Z

third_party/terraform/resources/resource_google_billing_subaccount.go

+		return handleNotFoundError(err, d, fmt.Sprintf("Billing Subaccount Not Found : %s", id))
+	}
+
+	d.Set("name", billingAccount.Name)


Mind checking the error returned from these d.Set calls, and returning a message including the field name if there's a problem? Like in https://github.com/hashicorp/terraform-provider-google-beta/blob/master/google-beta/resource_container_cluster.go#L1584.

added error checking for d.Set calls

google-cla · 2020-12-15T18:57:12Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

modular-magician · 2020-12-15T19:05:26Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 352 insertions(+))
Terraform Beta: Diff ( 7 files changed, 353 insertions(+), 1 deletion(-))

rileykarson

LGTM. Thanks @adarobin!

upodroid · 2021-01-11T20:02:58Z

Hi

~~I saw this in the new release of the provider and I was wondering why a new resource was made instead of adding master_billing_account field to the google_billing_account resource?~~

nvm

https://cloud.google.com/billing/docs/reference/rest/v1/billingAccounts#BillingAccount

googlebot added the cla: no label Nov 11, 2019

modular-magician added the community label Nov 11, 2019

rambleraptor requested a review from rileykarson November 11, 2019 18:36

rambleraptor removed the cla: no label Nov 11, 2019

googlebot added the cla: no label Nov 11, 2019

rileykarson reviewed Nov 11, 2019

View reviewed changes

samos123 suggested changes Apr 17, 2020

View reviewed changes

third_party/terraform/website/docs/r/google_billing_subaccount.html.markdown Outdated Show resolved Hide resolved

google-cla bot added cla: yes and removed cla: no labels Dec 3, 2020

rileykarson reviewed Dec 10, 2020

View reviewed changes

adarobin added 3 commits December 11, 2020 11:33

Add google_billing_subaccount resource

2815d55

add documentation for google_billing_subaccount

023f911

incorporate code review suggestions

856b63e

adarobin added 9 commits December 11, 2020 11:33

add tests for google_billing_subaccount

9fe205a

fix documentation

d2c2552

Up Terraform SDK version

9ef4f3d

sort provider schema

54307bb

do a read after creation

72e8b02

remove unneeded variable

5d40637

rework rename on destroy

4202c2d

update documentation

91e0453

fix errors resulting from rebase

44a0d9a

rileykarson reviewed Dec 11, 2020

View reviewed changes

rileykarson reviewed Dec 14, 2020

View reviewed changes

third_party/terraform/tests/resource_google_billing_subaccount_test.go Outdated Show resolved Hide resolved

fix billing subaccount tests

db90b33

rileykarson reviewed Dec 15, 2020

View reviewed changes

adarobin added 2 commits December 15, 2020 13:50

remove unused function

f686ad5

check return value of d.Set

ce3f9dd

rileykarson approved these changes Dec 15, 2020

View reviewed changes

rileykarson merged commit 509eb9e into GoogleCloudPlatform:master Dec 15, 2020

This was referenced Dec 15, 2020

Add billing subaccount hashicorp/terraform-provider-google-beta#2788

Merged

Add billing subaccount hashicorp/terraform-provider-google#8022

Merged

Add billing subaccount #2640

Add billing subaccount #2640

Conversation

adarobin commented Nov 11, 2019 • edited by rileykarson Loading

googlebot commented Nov 11, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

adarobin commented Nov 11, 2019

modular-magician commented Nov 11, 2019

rambleraptor commented Nov 11, 2019

adarobin commented Nov 11, 2019

rileykarson left a comment

Choose a reason for hiding this comment

adarobin commented Nov 11, 2019

rileykarson left a comment

Choose a reason for hiding this comment

adarobin commented Nov 12, 2019

sokoow commented Apr 10, 2020

adarobin commented Apr 16, 2020

sokoow commented Apr 16, 2020

modular-magician commented Nov 17, 2020

Diff report:

modular-magician commented Nov 17, 2020

Diff report:

adarobin commented Dec 3, 2020

google-cla bot commented Dec 3, 2020

adarobin commented Dec 3, 2020

adarobin commented Dec 3, 2020

rileykarson left a comment

Choose a reason for hiding this comment

rileykarson Dec 10, 2020

Choose a reason for hiding this comment

adarobin Dec 11, 2020

Choose a reason for hiding this comment

google-cla bot commented Dec 11, 2020

modular-magician commented Dec 11, 2020

Diff report:

rileykarson left a comment • edited Loading

Choose a reason for hiding this comment

rileykarson left a comment

Choose a reason for hiding this comment

google-cla bot commented Dec 15, 2020

modular-magician commented Dec 15, 2020

Diff report:

rileykarson left a comment • edited Loading

Choose a reason for hiding this comment

rileykarson Dec 15, 2020

Choose a reason for hiding this comment

adarobin Dec 15, 2020

Choose a reason for hiding this comment

rileykarson Dec 15, 2020

Choose a reason for hiding this comment

adarobin Dec 15, 2020

Choose a reason for hiding this comment

google-cla bot commented Dec 15, 2020

modular-magician commented Dec 15, 2020

Diff report:

rileykarson left a comment

Choose a reason for hiding this comment

upodroid commented Jan 11, 2021 • edited Loading

adarobin commented Nov 11, 2019 •

edited by rileykarson

Loading

rileykarson left a comment •

edited

Loading

rileykarson left a comment •

edited

Loading

upodroid commented Jan 11, 2021 •

edited

Loading