Add multi-region support to listings and subscriptions#16021
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: For automatic test runs see go/terraform-auto-test-runs. @ScottSuarez, 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. |
de76826 to
aee82a3
Compare
|
@ScottSuarez This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
aee82a3 to
da1b37a
Compare
1cc0c01 to
cd05f62
Compare
|
@GoogleCloudPlatform/terraform-team @ScottSuarez This PR has been waiting for review for 1 week. Please take a look! Use the label |
cd05f62 to
58bab0e
Compare
ScottSuarez
left a comment
There was a problem hiding this comment.
This is quite a lot of changes and it is hard to follow. I would prefer for the test migrations and test file renaming to not be done within this change if possible. If needed or preferable could you let me know as to why?
| method_name_separator: ':' | ||
| fetch_iam_policy_verb: 'POST' | ||
| parent_resource_attribute: 'listing_id' | ||
| example_config_body: 'templates/terraform/iam/iam_attributes.go.tmpl' |
There was a problem hiding this comment.
This shouldn't be needed?
There was a problem hiding this comment.
Correct, I have removed this line.
|
|
||
| func AddBigQueryDatasetReplica(projectID string,datasetID string,primaryLocation string,replicaLocation string) (string, error){ | ||
| ctx := context.Background() | ||
|
|
||
| client, err := bigquery.NewClient(ctx, projectID) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create BigQuery client: %w", err) | ||
| } | ||
| defer client.Close() | ||
|
|
||
| log.Printf("INFO: Attempting to create dataset '%s' in project '%s' at location '%s'", datasetID, projectID, primaryLocation) | ||
| datasetRef := client.Dataset(datasetID) | ||
| datasetMetadata := &bigquery.DatasetMetadata{ | ||
| Location: primaryLocation, | ||
| } | ||
|
|
||
| err = datasetRef.Create(ctx, datasetMetadata) | ||
| if err != nil { | ||
| log.Printf("failed to create BigQuery dataset '%s'.Dataset already exists.", datasetID) | ||
| } else { | ||
| log.Printf("INFO: Successfully created BigQuery dataset '%s' at location '%s'.", datasetID, primaryLocation) | ||
| } | ||
|
|
||
| sqlQuery := fmt.Sprintf( | ||
| "ALTER SCHEMA `%s` ADD REPLICA `%s` OPTIONS(location=`%s`)", | ||
| datasetID, | ||
| replicaLocation, | ||
| replicaLocation, | ||
| ) | ||
|
|
||
| log.Printf("INFO: Executing BigQuery DDL query: %s", sqlQuery) | ||
|
|
||
| query := client.Query(sqlQuery) | ||
|
|
||
| job, err := query.Run(ctx) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to submit BigQuery DDL job: %w", err) | ||
| } | ||
|
|
||
| status, err := job.Wait(ctx) | ||
| if err != nil { | ||
| log.Printf("failed to wait for BigQuery job completion: %v", err) | ||
| } | ||
|
|
||
| if status.Err() != nil { | ||
| log.Printf("BigQuery job completed with an error: %v", status.Err()) | ||
| } | ||
|
|
||
| log.Println("Successfully added BigQuery dataset replica using the Go client library.") | ||
|
|
||
| fullDatasetLocation := fmt.Sprintf("projects/%s/datasets/%s", projectID, datasetID) | ||
| return fullDatasetLocation, nil | ||
| } | ||
|
|
||
| func CleanupBigQueryDatasetAndReplica(projectID, datasetID, replicaLocation string) { | ||
| log.Printf("[DEBUG] Cleanup: Starting cleanup for BigQuery dataset: projects/%s/datasets/%s", projectID, datasetID) | ||
| cleanupCtx := context.Background() | ||
|
|
||
| client, cerr := bigquery.NewClient(cleanupCtx, projectID) | ||
| if cerr != nil { | ||
| log.Printf("[ERROR] Cleanup: Failed to create BigQuery client for dataset %s: %v", datasetID, cerr) | ||
| return | ||
| } | ||
| defer client.Close() | ||
|
|
||
| // Attempt to remove the replica first | ||
| dropReplicaSQL := fmt.Sprintf( | ||
| "ALTER SCHEMA `%s` DROP REPLICA `%s`", | ||
| datasetID, | ||
| replicaLocation, | ||
| ) | ||
| log.Printf("[DEBUG] Cleanup: Dropping replica with SQL: %s", dropReplicaSQL) | ||
| dropQuery := client.Query(dropReplicaSQL) | ||
| dropJob, dropErr := dropQuery.Run(cleanupCtx) | ||
| if dropErr != nil { | ||
| log.Printf("[ERROR] Cleanup: Failed to submit BigQuery DDL job for dropping replica %s of dataset %s: %v", replicaLocation, datasetID, dropErr) | ||
| } else { | ||
| dropStatus, dropWaitErr := dropJob.Wait(cleanupCtx) | ||
| if dropWaitErr != nil { | ||
| log.Printf("[ERROR] Cleanup: Failed to wait for BigQuery job completion for dropping replica %s of dataset %s: %v", replicaLocation, datasetID, dropWaitErr) | ||
| } else if dropStatus.Err() != nil { | ||
| log.Printf("[ERROR] Cleanup: BigQuery job for dropping replica %s of dataset %s completed with an error: %v", replicaLocation, datasetID, dropStatus.Err()) | ||
| } else { | ||
| log.Printf("[INFO] Cleanup: Successfully dropped BigQuery dataset replica: %s for dataset %s", replicaLocation, datasetID) | ||
| } | ||
| } | ||
|
|
||
| //Delete the main dataset (including any remaining contents) | ||
| log.Printf("[DEBUG] Cleanup: Deleting main BigQuery dataset: %s", datasetID) | ||
| err := client.Dataset(datasetID).DeleteWithContents(cleanupCtx) | ||
| if err != nil { | ||
| // Cannot differentiate. | ||
| log.Printf("[ERROR] Cleanup: Failed to delete BigQuery dataset %s: %v", datasetID, err) | ||
| } else { | ||
| log.Printf("[INFO] Cleanup: Successfully deleted BigQuery dataset: %s", datasetID) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why are these functions added. They are not used anywhere. Can we remove them
There was a problem hiding this comment.
these functions are for bootstrapping the multi region acceptance tests. since terraform's google_bigquery_dataset cannot natively create replicas, we use these go client helpers to set up the required environment before the terraform tests run. i have restored the multiregion tests that explicitly call these functions
58bab0e to
5c84a6b
Compare
the test files were renamed to .go.tmpl to implement Beta-only scoping. Since the multi-region support is currently restricted to the google-beta provider, standard .go tests would cause the GA provider build to fail because the GA schema does not recognize the new replica_locations field. |
5c84a6b to
2476981
Compare
|
@ScottSuarez This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 5891 Click here to see the affected service packages
Action takenFound 31 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
| } | ||
|
|
||
| {{- if ne $.TargetVersionName "ga" }} | ||
| func testAccBigqueryAnalyticsHubListingSubscription_multiregion(context map[string]interface{}) string { |
There was a problem hiding this comment.
Need provider = google-beta for each resource, see https://googlecloudplatform.github.io/magic-modules/test/test/. Same for the other test file.
|
@GoogleCloudPlatform/terraform-team @ScottSuarez This PR has been waiting for review for 1 week. Please take a look! Use the label |
|
@GoogleCloudPlatform/terraform-team @ScottSuarez This PR has been waiting for review for 2 weeks. Please take a look! Use the label |
ScottSuarez
left a comment
There was a problem hiding this comment.
Hi, apologies, wanted to check in on this. Could you pull the latest changes from main into your branch.
2476981 to
2d42f69
Compare
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
ScottSuarez
left a comment
There was a problem hiding this comment.
compilation errors downstream from the go mod file
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 0 Click here to see the affected service packages
🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR. View the build log |
| listing_id: 'my_listing' | ||
| desc: 'example data exchange' | ||
| steps: | ||
| - name: 'basic' |
There was a problem hiding this comment.
Because the name for every test is now basic the documentation is changed in a negative direction. I would prefer we retain the names from the old examples which originated from the file names on the old system.
| base := parts[0] | ||
| query := parts[1] | ||
|
|
||
| query = strings.ReplaceAll(query, "%2C", ",") |
There was a problem hiding this comment.
Apologies if this was already covered in prior comments but why is this being removed?
| if len(parts) == 2 { | ||
| base := parts[0] | ||
| query := parts[1] | ||
|
|
There was a problem hiding this comment.
Why not include replacement of "%2c" here?
|
Hey, as this PR was getting a bit messy and hard to follow due to re-occuring conflicts, I am closing this PR. I have created a fresh PR where I have addressed all the comments received so far. Linking it below, I request all the reviewers to have a look. Thanks. |
Description
This PR adds multi-region support to the BigQuery Analytics Hub
listingandlisting_subscriptionresources in thegoogle-betaprovider. This enhancement allows publishers to specify existing dataset replicas and allows subscribers to request replicas for their destination datasets.This work takes over and finalizes the logic from the original PR #14481.
Key Changes
replica_locationsandeffective_replicasfields togoogle_bigquery_analytics_hub_listing.replica_locationsfield togoogle_bigquery_analytics_hub_listing_subscription.min_version: 'beta'in YAML and implemented version guards ({{- if ne ... }}) in renamed.go.tmpltest files to correctly restrict the feature to the beta provider.tpgresource.CaseDiffSuppresstolocationfields to allow case-insensitive region input.CaseInsensitiveHashfunction incommon_diff_suppress.goand configuredset_hash_funcin YAML to ensure thereplica_locationsSet handles case differences (e.g., "eu" vs "EU") without triggering a diff.ProtoV5ProviderBetaFactoriesfor beta-specific acceptance tests.Testing Performed
Manual verification in a test project confirmed:
Bypass Note
BYPASS_BQUI_SCREENSHOT_BAN_PRESUBMIT=Updating existing screenshot goldens to reflect Terraform provider changes for the multi-region feature.
Additional attachments: https://docs.google.com/document/d/1V_377q8WQ2pNpvVjmRNSVQnOTzXIfBIJ4OjSIxrVVRU/edit?usp=sharing