Add support for Analytics Hub Subscriptions
This CL adds support for Analytics Hub Subscriptions as a resource, which allows users to subscribe to Analytics Hub Listings.
Release Note Template for Downstream PRs (will be copied)
`google_bigquery_analytics_hub_listing_subscription`
Hello! I am a robot. It looks like you are a: ~Community Contributor~ Googler ~Core Contributor~. Tests will run automatically.
@NickElliot, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.
You can help make sure that review is quick by doing a self-review and by running impacted tests locally.
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
Terraform GA: Diff ( 4 files changed, 996 insertions(+), 2 deletions(-)) Terraform Beta: Diff ( 4 files changed, 996 insertions(+), 2 deletions(-)) TF Conversion: Diff ( 1 file changed, 169 insertions(+)) TF OiCS: Diff ( 4 files changed, 139 insertions(+))
Missing test report
Your PR includes resource fields which are not covered by any test.
Resource: google_bigquery_analytics_hub_subscription (1 total tests)
Please add an acceptance test which includes these fields. The test should include the following:
resource "google_bigquery_analytics_hub_subscription" "primary" {
destination_dataset {
description = # value needed
friendly_name = # value needed
labels = # value needed
}
}
Tests analytics
Total tests: 3266
Passed tests 2928
Skipped tests: 334
Affected tests: 4
Action taken
Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccBigqueryAnalyticsHubSubscription_bigqueryAnalyticshubSubscriptionBasicExample|TestAccDataprocJobIamPolicy|TestAccDataSourceGoogleServiceAccountAccessToken_basic|TestAccDataSourceGoogleServiceAccountJwt
$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocJobIamPolicy[Debug log]
TestAccDataSourceGoogleServiceAccountAccessToken_basic[Debug log]
TestAccDataSourceGoogleServiceAccountJwt[Debug log]
Rerun these tests in REPLAYING mode to catch issues
$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$
$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccBigqueryAnalyticsHubSubscription_bigqueryAnalyticshubSubscriptionBasicExample[Error message] [Debug log]
$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$ View the build log or the debug log for each test
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
Terraform GA: Diff ( 5 files changed, 1079 insertions(+), 2 deletions(-)) Terraform Beta: Diff ( 5 files changed, 1079 insertions(+), 2 deletions(-)) TF Conversion: Diff ( 1 file changed, 169 insertions(+)) TF OiCS: Diff ( 4 files changed, 144 insertions(+))
Tests analytics
Total tests: 3280
Passed tests 2942
Skipped tests: 334
Affected tests: 4
Action taken
Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccBigqueryAnalyticsHubSubscription_bigqueryAnalyticshubSubscriptionBasicExample|TestAccDataprocClusterIamPolicy|TestAccDataprocJobIamPolicy|TestAccDataSourceGoogleServiceAccountAccessToken_basic
$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]
TestAccDataprocJobIamPolicy[Debug log]
TestAccDataSourceGoogleServiceAccountAccessToken_basic[Debug log]
Rerun these tests in REPLAYING mode to catch issues
$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$
$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccBigqueryAnalyticsHubSubscription_bigqueryAnalyticshubSubscriptionBasicExample[Error message] [Debug log]
$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$ View the build log or the debug log for each test
Commenting to say feel free to ask for any assistance if you need it :)
Commenting to say feel free to ask for any assistance if you need it :)
@NickElliot Thanks! I'm currently fighting the "diff-of-doom" due to the API not returning all the properties of the subscription on its GET call. I generally know how to fix it, but before I put in all the effort in doing so, I'm trying to get to a state where I can have a static decoder make the diff appear as I need it to.
Additionally, subscriptions are polymorphic I just found out, so I'm going to split this into two resources as the behavior is sufficiently different between their use cases.
Right now, the static (well, almost static, as the dataset_id is dynamic) decoder isn't making the plan exclude the fields that are static in my decoder from the diff. I'm going to keep tinkering with it, but if you have any thoughts, feel free to chime in!
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
Terraform GA: Diff ( 5 files changed, 1109 insertions(+), 2 deletions(-)) Terraform Beta: Diff ( 5 files changed, 1109 insertions(+), 2 deletions(-)) TF Conversion: Diff ( 1 file changed, 169 insertions(+)) TF OiCS: Diff ( 4 files changed, 144 insertions(+))
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
Terraform GA: Diff ( 5 files changed, 1110 insertions(+), 2 deletions(-)) Terraform Beta: Diff ( 5 files changed, 1110 insertions(+), 2 deletions(-)) TF Conversion: Diff ( 1 file changed, 169 insertions(+)) TF OiCS: Diff ( 4 files changed, 144 insertions(+))
Tests analytics
Total tests: 3280
Passed tests 2945
Skipped tests: 334
Affected tests: 1
Action taken
Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccBigqueryAnalyticsHubListingSubscription_bigqueryAnalyticshubListingSubscriptionBasicExample
$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccBigqueryAnalyticsHubListingSubscription_bigqueryAnalyticshubListingSubscriptionBasicExample[Error message] [Debug log]
$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$ View the build log or the debug log for each test
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
Terraform GA: Diff ( 5 files changed, 1196 insertions(+), 2 deletions(-)) Terraform Beta: Diff ( 5 files changed, 1196 insertions(+), 2 deletions(-)) TF Conversion: Diff ( 1 file changed, 169 insertions(+)) TF OiCS: Diff ( 4 files changed, 144 insertions(+))
Tests analytics
Total tests: 3291
Passed tests 2953
Skipped tests: 337
Affected tests: 1
Action taken
Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccBigqueryAnalyticsHubListingSubscription_bigqueryAnalyticshubListingSubscriptionBasicExample
$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccBigqueryAnalyticsHubListingSubscription_bigqueryAnalyticshubListingSubscriptionBasicExample[Error message] [Debug log]
$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$ View the build log or the debug log for each test
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
Terraform GA: Diff ( 5 files changed, 1128 insertions(+), 2 deletions(-)) Terraform Beta: Diff ( 5 files changed, 1128 insertions(+), 2 deletions(-)) TF Conversion: Diff ( 1 file changed, 169 insertions(+)) TF OiCS: Diff ( 4 files changed, 144 insertions(+))
Tests analytics
Total tests: 3291
Passed tests 2953
Skipped tests: 337
Affected tests: 1
Action taken
Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccBigqueryAnalyticsHubListingSubscription_bigqueryAnalyticshubListingSubscriptionBasicExample
$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccBigqueryAnalyticsHubListingSubscription_bigqueryAnalyticshubListingSubscriptionBasicExample[Error message] [Debug log]
$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$ View the build log or the debug log for each test
@NickElliot and @rileykarson
There are a few more changes I need to make, but the core concept works today. However, during the VCR-test step on here, it's building for both beta and ga. This resource will never work for the beta endpoint, and I want it to skip building on beta. I set the min version to ga, but it's still building on it.
Any thoughts?
There is a base policy for terraform compatibility that the beta API is a strict superset of GA, so a minimum version of GA will include beta -- there are potential workarounds but they're non standard and go against that policy (albeit with a more unique case variant here with the entire specific resource being GA only)
There is a base policy for terraform compatibility that the beta API is a strict superset of GA, so a minimum version of GA will include beta -- there are potential workarounds but they're non standard and go against that policy (albeit with a more unique case variant here with the entire specific resource being GA only)
This is physically impossible. The beta API will never be able to support Terraform, so this needs an exception -- how do we go about this?
We can support only GA, and that will appear in all our providers (ga/beta/eap). If beta features are supported, beta must be a superset of GA.
@rileykarson
We can support only GA, and that will appear in all our providers (ga/beta/eap).
That's fine w/ me, and I'm happy to do the work to get it done.
If beta features are supported, beta must be a superset of GA.
The beta API for this resource, while it does exist, is broken and unusable entirely, and it is impossible to determine any state of the remote resource.
What should I do next to get this out the door safely/correctly?
Nothing in the product has a beta constraint, so we can probably drop the beta API from https://github.com/Cidan/magic-modules/blob/c813300ca003cadaca4ee55d941ba8dcbfe61c4e/mmv1/products/bigqueryanalyticshub/product.yaml#L20-L22. That will automatically use the GA API in all resources & provider versions in the product.
If there are different rules per-resource (i.e. this resource must use ga or beta, this resource can only use ga) we need to use different product folders, one per set of rules.
@Cidan per b/291746281, I would assume that https://github.com/GoogleCloudPlatform/magic-modules/pull/9350 is the solution that is owned by the service team, but I saw your note and was confused. Do you know which solution is preferred here? For example, even the resource names that were chosen are different between these PRs. I just want to make sure we don't have competing approaches being worked on in isolation.
@roaks3 Agreed. I reached out to the author of the other PR previously and have yet to hear back. I've been in a holding pattern from the holidays. I believe this PR is slightly more rounded out than the other as it covers more cases (import, etc).
I'm fine merging and/or handing off efforts as well. The Analytics Hub team has no preference as the approach here.
Also, quick note on resource names -- the names must be currently differentiated between subscriptions of listings and exchanges, because one is a clean room, and the other is a linked dataset. This is implicit in the link target, but it is difficult to ascertain what kind of subscription something is from the API unfortunately, as it's polymorphic with only a single possible way of figuring out what kind of subscription the resource is. I decided to be explicit and make two different resources so that users have a clear path to creating a clean room versus a link, and so that imports are easier and less error prone as well.
It looks like we still haven't gotten a response from the other author on Github, so I've pinged them internally. Hopefully we will get a response in the next 24 hrs or so and can decide on a path forward here.
On your last point, thanks for explaining the thought process. To me it seems like a reasonable choice for this API, but I would just note that if we want to go in that direction, we may want to double check if we've come across this same pattern in other APIs. If we have, we may want to favor an existing approach for consistency.
Yeah, it's a really weird case that should have really never happened, unfortunately. In this case, a single API endpoint is creating what should be two different resources explicitly.
I expect for version 2, we will change the API entirely such that subscriptions and clean rooms are two different, explicit resources and clean rooms will stop "piggy backing" on the subscriptions API. This is another reason I want to split the resources now, so that in the future we can split the API without breaking existing user-facing Terraform state.