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

Add billing_project configuration to the provider #3886

Merged

Conversation

upodroid
Copy link
Contributor

@upodroid upodroid commented Aug 19, 2020

Fixes: hashicorp/terraform-provider-google#6747

This PR introduces a new field in the provider billing_project and amends ACM Products to support User ADCs.

I took a few ideas from how the cloud_feed

I'm stuck on testing it properly.

If this PR is for Terraform, I acknowledge that I have:

  • Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
  • Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

provider: added a new field `billing_project` to the provider that's associated as a billing/quota project with most requests when `user_project_override` true

@google-cla google-cla bot added the cla: yes label Aug 19, 2020
@modular-magician
Copy link
Collaborator

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.

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

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

@upodroid
Copy link
Contributor Author

The env variable is set in both provider.go and provider-test.go

REDACTED  MCW0CDP3YY  ~  go  …  github.com  hashicorp  terraform-provider-google-beta   master  41⬇  23✎  $    make testacc TEST=./google-beta TESTARGS='-run=TestAccAccessContextManager'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccAccessContextManager -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccAccessContextManager
=== RUN   TestAccAccessContextManager/access_level_full
    TestAccAccessContextManager/access_level_full: testing.go:674: Step 0 error: errors during apply:
        
        Error: billing_project: required field is not set
        
          on /var/folders/b1/dthn83bs2qbcrg38qszm22440000gn/T/tf-test046903338/main.tf line 2:
          (source code not available)
        
        
=== RUN   TestAccAccessContextManager/access_level_custom
    TestAccAccessContextManager/access_level_custom: testing.go:674: Step 0 error: errors during apply:
        
        Error: billing_project: required field is not set
        
          on /var/folders/b1/dthn83bs2qbcrg38qszm22440000gn/T/tf-test231287532/main.tf line 2:
          (source code not available)
        
        
=== RUN   TestAccAccessContextManager/access_policy
    TestAccAccessContextManager/access_policy: testing.go:674: Step 0 error: errors during apply:
        
        Error: billing_project: required field is not set
        
          on /var/folders/b1/dthn83bs2qbcrg38qszm22440000gn/T/tf-test844553982/main.tf line 2:
          (source code not available)
        
        
=== RUN   TestAccAccessContextManager/service_perimeter
    TestAccAccessContextManager/service_perimeter: testing.go:674: Step 0 error: errors during apply:
        
        Error: billing_project: required field is not set
        
          on /var/folders/b1/dthn83bs2qbcrg38qszm22440000gn/T/tf-test116288736/main.tf line 2:
          (source code not available)
        
        
=== RUN   TestAccAccessContextManager/service_perimeter_update
    TestAccAccessContextManager/service_perimeter_update: testing.go:674: Step 0 error: errors during apply:
        
        Error: billing_project: required field is not set
        
          on /var/folders/b1/dthn83bs2qbcrg38qszm22440000gn/T/tf-test462741266/main.tf line 2:
          (source code not available)
        
        
=== RUN   TestAccAccessContextManager/service_perimeter_resource
    TestAccAccessContextManager/service_perimeter_resource: bootstrap_utils_test.go:334: Bootstrapping failed. Unable to load test config: unable to parse credentials from 'true': json: cannot unmarshal bool into Go value of type google.credentialsFile
=== RUN   TestAccAccessContextManager/access_level
    TestAccAccessContextManager/access_level: testing.go:674: Step 0 error: errors during apply:
        
        Error: billing_project: required field is not set
        
          on /var/folders/b1/dthn83bs2qbcrg38qszm22440000gn/T/tf-test825725972/main.tf line 2:
          (source code not available)
        
        
--- FAIL: TestAccAccessContextManager (1.39s)
    --- FAIL: TestAccAccessContextManager/access_level_full (0.25s)
    --- FAIL: TestAccAccessContextManager/access_level_custom (0.25s)
    --- FAIL: TestAccAccessContextManager/access_policy (0.22s)
    --- FAIL: TestAccAccessContextManager/service_perimeter (0.22s)
    --- FAIL: TestAccAccessContextManager/service_perimeter_update (0.22s)
    --- FAIL: TestAccAccessContextManager/service_perimeter_resource (0.00s)
    --- FAIL: TestAccAccessContextManager/access_level (0.22s)
FAIL
FAIL    github.com/hashicorp/terraform-provider-google-beta/google-beta 2.473s
FAIL
make: *** [testacc] Error 1
 maha4472  MCW0CDP3YY  ~  go  …  github.com  hashicorp  terraform-provider-google-beta   master  41⬇  23✎  USAGE  $    env | grep GOOGLE
GOOGLE_REGION=us-central1
GOOGLE_BILLING_PROJECT=REDACTED
GOOGLE_ORG=684510004702
GOOGLE_ZONE=us-central1-a
GOOGLE_PROJECT=REDACTED
GOOGLE_USE_DEFAULT_CREDENTIALS=true
 REDACTED  MCW0CDP3YY  ~  go  …  github.com  hashicorp  terraform-provider-google-beta   master  41⬇  23✎  $    

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 18 files changed, 458 insertions(+), 15 deletions(-))
Terraform Beta: Diff ( 18 files changed, 458 insertions(+), 15 deletions(-))
TF Conversion: Diff ( 3 files changed, 19 insertions(+))

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable approach! Right now it's presented as an alternative sourcing mechanism for supports_indirect_user_project_override, but I think we can use the provider-level billing_project resource across all resources regardless of whether they support a direct/indirect project or no project (like ACM).

I'll provide some more specific comments inline, but broadly:

  • This should take priority over direct/indirect projects, since it's a dedicated field. If a user specified it, we should prefer it.
  • All MM-generated resources should be able to use it. We don't need to lock it behind supports_indirect_user_project_override.

products/accesscontextmanager/terraform.yaml Outdated Show resolved Hide resolved
templates/terraform/resource.erb Outdated Show resolved Hide resolved
third_party/terraform/utils/utils.go.erb Outdated Show resolved Hide resolved
third_party/terraform/utils/provider.go.erb Show resolved Hide resolved
templates/terraform/resource.erb Outdated Show resolved Hide resolved
templates/terraform/resource.erb Outdated Show resolved Hide resolved
third_party/terraform/utils/field_helpers.go Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 160 files changed, 4622 insertions(+), 665 deletions(-))
Terraform Beta: Diff ( 191 files changed, 5530 insertions(+), 784 deletions(-))
TF Conversion: Diff ( 3 files changed, 19 insertions(+))

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Broadly looks good in resource.erb! Left a couple more minor comments.

templates/terraform/resource.erb Outdated Show resolved Hide resolved
templates/terraform/resource.erb Outdated Show resolved Hide resolved
Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Oh, whoops, I didn't reply to the VCR comment. Unfortunately we're unable to run those locally. Since they contain secrets, we're also unable to run them on community contributions or to share logs. The way you ran the tests in #3886 (comment) should be sufficient.

You're passing our Cloud Build CI right now (this one doesn't run integration tests, though) other than some of the testing-related code that isn't being used anywhere triggering a linter error. We can get to that on my next review pass- I need to double check with a teammate how well our CI can test this stuff. It generally runs as a service account, but there may be a way to run an interactive account too that I'm not aware of.

@upodroid
Copy link
Contributor Author

I was looking for the debug logs but I figured how to get them by prepending TF_LOG=debug before the make command.

It is not setting the header. I'll investigate and update the PR if I can't figure it out.

@upodroid
Copy link
Contributor Author

I think we are good to go.

 REDACTED  MCW0CDP3YY  ~  go  …  github.com  hashicorp  terraform-provider-google-beta   master  49⬇  188✎  $    make testacc TEST=./google-beta TESTARGS='-run=TestAccAccessContextManager'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccAccessContextManager -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccAccessContextManager
=== RUN   TestAccAccessContextManager/access_level
=== RUN   TestAccAccessContextManager/access_level_full
=== RUN   TestAccAccessContextManager/access_level_custom
=== RUN   TestAccAccessContextManager/access_policy
=== RUN   TestAccAccessContextManager/service_perimeter
=== RUN   TestAccAccessContextManager/service_perimeter_update
=== RUN   TestAccAccessContextManager/service_perimeter_resource
    TestAccAccessContextManager/service_perimeter_resource: provider_test.go:911: VCR enabled, skipping test: TestAccAccessContextManager/service_perimeter_resource
--- PASS: TestAccAccessContextManager (170.16s)
    --- PASS: TestAccAccessContextManager/access_level (30.09s)
    --- PASS: TestAccAccessContextManager/access_level_full (19.22s)
    --- PASS: TestAccAccessContextManager/access_level_custom (23.58s)
    --- PASS: TestAccAccessContextManager/access_policy (16.08s)
    --- PASS: TestAccAccessContextManager/service_perimeter (23.51s)
    --- PASS: TestAccAccessContextManager/service_perimeter_update (57.68s)
    --- SKIP: TestAccAccessContextManager/service_perimeter_resource (0.00s)
PASS
ok      github.com/hashicorp/terraform-provider-google-beta/google-beta 171.102s
 REDACTED  MCW0CDP3YY  ~  go  …  github.com  hashicorp  terraform-provider-google-beta   master  49⬇  188✎  USAGE  $    make testacc TEST=./google-beta TESTARGS='-run=TestAccCloudAssetFolderFeed_cloudAssetFolderFeedExample'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccCloudAssetFolderFeed_cloudAssetFolderFeedExample -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccCloudAssetFolderFeed_cloudAssetFolderFeedExample
=== PAUSE TestAccCloudAssetFolderFeed_cloudAssetFolderFeedExample
=== CONT  TestAccCloudAssetFolderFeed_cloudAssetFolderFeedExample
--- PASS: TestAccCloudAssetFolderFeed_cloudAssetFolderFeedExample (30.85s)
PASS
ok      github.com/hashicorp/terraform-provider-google-beta/google-beta 31.807s

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 161 files changed, 4634 insertions(+), 665 deletions(-))
Terraform Beta: Diff ( 192 files changed, 5542 insertions(+), 784 deletions(-))
TF Conversion: Diff ( 3 files changed, 19 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 161 files changed, 3751 insertions(+), 665 deletions(-))
Terraform Beta: Diff ( 192 files changed, 4460 insertions(+), 784 deletions(-))
TF Conversion: Diff ( 3 files changed, 19 insertions(+))

templates/terraform/resource.erb Outdated Show resolved Hide resolved
api/resource.rb Outdated Show resolved Hide resolved
third_party/terraform/utils/provider_test.go.erb Outdated Show resolved Hide resolved
third_party/terraform/utils/provider_test.go.erb Outdated Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 160 files changed, 3872 insertions(+), 673 deletions(-))
Terraform Beta: Diff ( 191 files changed, 4619 insertions(+), 792 deletions(-))
TF Conversion: Diff ( 3 files changed, 18 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 160 files changed, 3868 insertions(+), 666 deletions(-))
Terraform Beta: Diff ( 191 files changed, 4615 insertions(+), 785 deletions(-))
TF Conversion: Diff ( 3 files changed, 18 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 164 files changed, 3892 insertions(+), 666 deletions(-))
Terraform Beta: Diff ( 195 files changed, 4639 insertions(+), 785 deletions(-))
TF Conversion: Diff ( 3 files changed, 18 insertions(+))

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in getting back, I had a busy end of last week! Just one last thing about how we're writing the billingProject value, I think.

templates/terraform/resource.erb Outdated Show resolved Hide resolved
@upodroid
Copy link
Contributor Author

Lint and acceptance tests are passing 😃

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 164 files changed, 5670 insertions(+), 663 deletions(-))
Terraform Beta: Diff ( 195 files changed, 6775 insertions(+), 782 deletions(-))
TF Conversion: Diff ( 3 files changed, 18 insertions(+))

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your collaboration when making changes to how this PR ended up- the billing_project field works consistently across the majority of the provider's resources, and simplified the custom logic in google_cloud_asset_feed!

@upodroid
Copy link
Contributor Author

Yeah, we solved it for all future APIs that don't play nice with User ADCs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants