-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Promote Billing Budgets to GA #4273
Promote Billing Budgets to GA #4273
Conversation
Fork Update
Co-authored-by: upodroid <cy@borg.dev>
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. |
FYI, the API is bugged. Weidly, it is close to this example https://cloud.google.com/billing/docs/how-to/budget-api#create Beta API:
GA API:
I opened a case with Google for answers. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1485 insertions(+), 8 deletions(-)) |
Weird! Post here when you've gotten an answer. |
Co-authored-by: upodroid <cy@borg.dev>
Co-authored-by: upodroid <cy@borg.dev>
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1438 insertions(+), 11 deletions(-)) |
I figured out the bug. Basically in v1, the budget isn't nested anymore. The example was out of date. |
Co-authored-by: upodroid <cy@borg.dev>
I'm getting a weird crash. My diffs haven't changed the resourceBillingBudgetImport function so i'm not sure where the project is being pulled. This is also another API that mangles project_ids and converts them to project numbers :(((((((
|
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 133 files changed, 2395 insertions(+), 193 deletions(-)) |
I think we'll want to preserve the structure of the resource as it was, and convert the message we send to the API using an encoder. Same as before- breaking changes to resources are something we'd really like to avoid (even though admittedly, this API is making it pretty hard for us) |
For the crash, it appears that import never worked for the resource. It's been a restriction that some beta tests can't test import, and I guess we never actually ran import on the resource. It tries to set a value on the You can disable import. |
We are good to go now. All tests are passing except the basic one.
That was happening when Computed was set on budget_filter_* and when I removed(currently commented out) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 134 files changed, 2349 insertions(+), 220 deletions(-)) |
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.
Ah, I see that we'd used flatten_object
here so the change is actually fairly safe to bring the budget
fields up to the top level.
primary_resource_id: 'budget' | ||
vars: | ||
display_name: 'Example Billing Budget' | ||
test_env_vars: | ||
billing_acct: :BILLING_ACCT | ||
skip_test: true |
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.
Why are all these tests skipped and converted to being handwritten? Nothing seems to have changed significantly in the handwritten ones.
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.
Import tests were failing.
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.
Oh, huh, I guess we never actually added code to skip the import step if the resource doesn't support import. Huh! Mind commenting something to that effect?
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 introduced a new field that excludes import step test from generated tests.
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.
Re: TestAccBillingBudget_billingBudgetBasicExample
That diff was happening when Computed
was set? Making the field Optional
+ Computed
should resolve it, in theory.
I did that and it didn't solve it. run gcbrun against this branch now and you should see the same error i'm seeing. |
/gcbrun |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=161501" |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 137 files changed, 2375 insertions(+), 223 deletions(-)) |
1 similar comment
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 137 files changed, 2375 insertions(+), 223 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceComposerEnvironment_basic|TestAccDataSourceComputeResourcePolicy|TestAccDataSourceSpannerInstance_basic|TestAccNotebooksInstanceIamMemberGenerated|TestAccNotebooksInstanceIamPolicyGenerated|TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample|TestAccBillingBudget_billingBudgetLastperiodExample|TestAccBillingBudget_billingBudgetFilterExample|TestAccBillingBudget_billingBudgetBasicExample|TestAccBigqueryDataTransferConfig|TestAccBillingBudget_billingBudgetNotifyExample|TestAccCloudSchedulerJob_schedulerJobHttpExample|TestAccComposerEnvironment_withUpdateOnCreate|TestAccComputeBackendService_backendServiceCacheExample|TestAccComputeGlobalForwardingRule_globalForwardingRuleHttpExample|TestAccComputeGlobalForwardingRule_globalForwardingRuleInternalExample|TestAccComputeManagedSslCertificate_managedSslCertificateBasicExample|TestAccComputeRegionBackendService_regionBackendServiceCacheExample|TestAccComputeTargetGrpcProxy_targetGrpcProxyBasicExample|TestAccComputeTargetGrpcProxy_update|TestAccComputeTargetHttpProxy_targetHttpProxyBasicExample|TestAccComputeTargetHttpsProxy_targetHttpsProxyBasicExample|TestAccComputeUrlMap_urlMapBasicExample|TestAccComputeUrlMap_urlMapTrafficDirectorRouteExample|TestAccComputeUrlMap_urlMapTrafficDirectorPathExample|TestAccComputeUrlMap_urlMapTrafficDirectorPathPartialExample|TestAccComputeUrlMap_urlMapTrafficDirectorRoutePartialExample|TestAccComputeUrlMap_urlMapHeaderBasedRoutingExample|TestAccComputeUrlMap_urlMapParameterBasedRoutingExample|TestAccComputeUrlMap_defaultRouteActionTrafficDirectorPathUpdate|TestAccComputeUrlMap_defaultRouteActionTrafficDirectorUpdate|TestAccComputeUrlMap_trafficDirectorPathUpdate|TestAccFirestoreDocument_firestoreDocumentBasicExample|TestAccFirestoreDocument_firestoreDocumentNestedDocumentExample|TestAccFirestoreDocument_update|TestAccFilestoreInstance_filestoreInstanceBasicExample|TestAccFilestoreInstance_update|TestAccNotebooksEnvironment_notebookEnvironmentBasicExample|TestAccNotebooksInstance_notebookInstanceBasicExample|TestAccNotebooksInstance_notebookInstanceBasicContainerExample|TestAccNotebooksInstance_notebookInstanceBasicGpuExample|TestAccNotebooksInstance_notebookInstanceFullExample|TestAccOSLoginSSHPublicKey_osLoginSshKeyExpiry|TestAccStorageBucket_lifecycleRuleStateLive|TestAccStorageBucket_lifecycleRuleStateAny You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=161503" |
Co-authored-by: upodroid <cy@borg.dev>
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 139 files changed, 2405 insertions(+), 205 deletions(-)) |
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.
Tests pass and changes LGTM. Thanks @upodroid!
@slevenick: Mind doing a second review pass and merging when good? I sometimes miss things in large PRs after a lengthy review. Most notably import is dropped- that's explicit though, by my read it never could have worked.
Yeah, the ruby logic was counter intuitive. I wonder how many handwritten tests do we have because MM didn't support importless testing? I remember handwriting one test a while back because you couldn't specify external providers(random), which is trivial to implement in MM. |
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.
LGTM overall!
Fixes: hashicorp/terraform-provider-google#7877
Fixes: hashicorp/terraform-provider-google#7890
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)