fix(bigquery_dataset): fixed handling of non-legacy roles for access … #14569

sachinpro · 2025-07-18T18:42:16Z

Added a flag to prevent Terraform from showing diff for server generated schema columns(like hive partitioned ones)

Fixes hashicorp/terraform-provider-google#8370

Release Note Template for Downstream PRs (will be copied)

bigquery: fixed handling of non-legacy roles for access block inside `google_bigquery_dataset`

modular-magician · 2025-07-18T18:55:29Z

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.

google provider: Diff ( 3 files changed, 29 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 3 files changed, 29 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 3 files changed, 52 insertions(+), 2 deletions(-))
Open in Cloud Shell: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2025-07-18T18:58:51Z

Tests analytics

Total tests: 17
Passed tests: 10
Skipped tests: 7
Affected tests: 0

Click here to see the affected service packages

bigquery

🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR.

View the build log

modular-magician · 2025-07-18T19:14:16Z

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.

google provider: Diff ( 3 files changed, 29 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 3 files changed, 29 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 3 files changed, 52 insertions(+), 2 deletions(-))
Open in Cloud Shell: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2025-07-18T19:17:55Z

Tests analytics

Total tests: 69
Passed tests: 60
Skipped tests: 8
Affected tests: 1

Click here to see the affected service packages

bigquery

#### 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

TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionIDAndHivePartitioning

Get to know how VCR tests work

modular-magician · 2025-07-18T19:18:41Z

🔴 Tests failed during RECORDING mode:
TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionIDAndHivePartitioning [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

modular-magician · 2025-07-18T19:33:42Z

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.

google provider: Diff ( 3 files changed, 29 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 3 files changed, 29 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 3 files changed, 52 insertions(+), 2 deletions(-))
Open in Cloud Shell: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2025-07-18T19:37:19Z

Tests analytics

Total tests: 83
Passed tests: 71
Skipped tests: 8
Affected tests: 4

Click here to see the affected service packages

bigquery

#### 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

TestAccBigQueryDataset_bigqueryDatasetBasicExample

TestAccBigQueryDataset_bigqueryDatasetDefaultCollationSetExample

TestAccBigQueryDataset_bigqueryDatasetWithMaxTimeTravelHoursExample

TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionIDAndHivePartitioning

Get to know how VCR tests work

github-actions · 2025-07-18T19:38:01Z

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.

@melinath, 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.

sachinpro · 2025-07-18T19:38:16Z

🔴 Tests failed during RECORDING mode: TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionIDAndHivePartitioning [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

@BBBmau, I am not sure why this is failing. TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionIDAndHivePartitioning shouldn't be affected by this change.

modular-magician · 2025-07-18T19:38:55Z

🟢 Tests passed during RECORDING mode:
TestAccBigQueryDataset_bigqueryDatasetBasicExample [Debug log]
TestAccBigQueryDataset_bigqueryDatasetDefaultCollationSetExample [Debug log]
TestAccBigQueryDataset_bigqueryDatasetWithMaxTimeTravelHoursExample [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

🔴 Tests failed during RECORDING mode:
TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionIDAndHivePartitioning [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

wj-chen

General question: Is there any difference between implementing it this way vs through a diff suppression?

wj-chen · 2025-07-18T22:23:55Z

mmv1/templates/terraform/examples/bigquery_dataset_basic.tf.tmpl

@@ -10,7 +10,7 @@ resource "google_bigquery_dataset" "{{$.PrimaryResourceId}}" {
  }

  access {


In the documentation we can advertise the user to use the new role syntax. In the test at least, we should keep the old case and add a new test for the new role so we have both to prevent regression. We also want to make sure the case in hashicorp/terraform-provider-google#8370 (comment) is tested and handled.

In the documentation we can advertise the user to use the new role syntax.

It is not shown in the diff but this test validates both cases:

access { role = "roles/bigquery.dataOwner" user_by_email = google_service_account.bqowner.email } access { role = "READER" domain = "hashicorp.com" }

This test is reflected in the documentation showing both ways to specify role.
Do you still think we need another test?

We also want to make sure the case in hashicorp/terraform-provider-google#8370 (comment) is tested and handled.

Let me check this. I don't think it is an issue anymore.

If the test covers both legacy and new syntax then it should be sufficient.

We also want to make sure the case in hashicorp/terraform-provider-google#8370 (comment) is tested and handled.

I couldn't reproduce this by adding access blocks with group_by_email and special_group:

access { role = "READER" group_by_email = "[email protected]" } access { role = "READER" special_group = "projectWriters" }

I did find one issue with case sensitivity though, which is there on main and this branch. Updating this line to domain = "hashicorp.coM"(for example) will give this error:

| # google_bigquery_dataset.dataset will be updated in-place | ~ resource "google_bigquery_dataset" "dataset" { | id = "<redacted>" | # (18 unchanged attributes hidden) | | - access { | - role = "OWNER" -> null | - user_by_email = "<redacted>" -> null | } | - access { | - domain = "hashicorp.com" -> null | - role = "READER" -> null | } | + access { | + domain = "hashicorp.coM" | + role = "READER" | } | + access { | + role = "OWNER" | + user_by_email = "<redacted>" | } | }

Case sensitivity doesn't seem to cause error in that comment though.

Thanks for testing. The case permadiff sounds related to hashicorp/terraform-provider-google#21928. I added this observation in the tracking issue and we can track and fix that separately.

sachinpro · 2025-07-21T13:46:00Z

General question: Is there any difference between implementing it this way vs through a diff suppression?

@wj-chen The DiffSuppressFunc on a field within a set element doesn't work because Terraform hashes the entire element to see if it has changed. Since "OWNER" and "roles/bigquery.dataOwner" are different strings, they produce different hashes, and Terraform sees one access block being removed and another being added, rather than an in-place change.
That's why I have to use a custom hash function.

I can make the change if there is a better way to handle this.

wj-chen · 2025-07-21T17:33:18Z

🔴 Tests failed during RECORDING mode: TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionIDAndHivePartitioning [Error message] [Debug log]
🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.
View the build log or the debug log for each test

@BBBmau, I am not sure why this is failing. TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionIDAndHivePartitioning shouldn't be affected by this change.

Could you check if it's related to the test added in #14483?

Error message:

---[ REQUEST ]---------------------------------------
POST /bigquery/v2/projects/ci-test-project-188019/queries?alt=json&prettyPrint=false HTTP/1.1
Host: bigquery.googleapis.com
User-Agent: google-api-go-client/0.5 Terraform/1.11.0 (+https://www.terraform.io) Terraform-Plugin-SDK/2.36.0 terraform-provider-google-beta/acc
Content-Length: 142
Content-Type: application/json
X-Goog-Api-Client: gl-go/1.23.11 gdcl/0.238.0
Accept-Encoding: gzip

{
 "query": "ALTER TABLE `ci-test-project-188019.tf_test_1byqthgjxe.tf_test_6w7ugd6t64` DROP COLUMN `country_partitioned`",
 "useLegacySql": false
}

-----------------------------------------------------
2025/07/19 00:07:21 [DEBUG] Google API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 404 Not Found
Content-Type: application/json; charset=UTF-8
Date: Sat, 19 Jul 2025 00:07:21 GMT
Server: ESF
Vary: Origin
Vary: X-Origin
Vary: Referer
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 0

{
  "error": {
    "code": 404,
    "message": "Not found: Connection tf_test_nyfa7l3x1h was not found for table ci-test-project-188019:tf_test_1byqthgjxe.tf_test_6w7ugd6t64",
    "errors": [
      {
        "message": "Not found: Connection tf_test_nyfa7l3x1h was not found for table ci-test-project-188019:tf_test_1byqthgjxe.tf_test_6w7ugd6t64",
        "domain": "global",
        "reason": "notFound"
      }
    ],
    "status": "NOT_FOUND"
  }
}

So looks like our table column drop logic still tried to run on the partition column, and it happened after the connection resource was deleted to cause the failure. Let's prioritize fixing this.

github-actions · 2025-07-23T09:46:08Z

@BBBmau This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

github-actions · 2025-07-25T09:45:26Z

@GoogleCloudPlatform/terraform-team @BBBmau This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

wj-chen · 2025-07-28T21:47:02Z

General question: Is there any difference between implementing it this way vs through a diff suppression?

@wj-chen The DiffSuppressFunc on a field within a set element doesn't work because Terraform hashes the entire element to see if it has changed. Since "OWNER" and "roles/bigquery.dataOwner" are different strings, they produce different hashes, and Terraform sees one access block being removed and another being added, rather than an in-place change. That's why I have to use a custom hash function.

I can make the change if there is a better way to handle this.

This approach LGTM.

sachinpro · 2025-07-29T08:46:04Z

@BBBmau This PR is ready for review. The test failure TestAccBigQueryExternalDataTable_CSV_WithSchemaAndConnectionIDAndHivePartitioning(not caused by this change) is fixed in a separate PR(#14659).

modular-magician · 2025-07-31T17:50:17Z

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.

google provider: Diff ( 3 files changed, 29 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 3 files changed, 29 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 3 files changed, 52 insertions(+), 2 deletions(-))
Open in Cloud Shell: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2025-07-31T17:54:59Z

Tests analytics

Total tests: 137
Passed tests: 126
Skipped tests: 10
Affected tests: 1

Click here to see the affected service packages

bigquery

#### 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

TestAccBigQueryDataset_access

Get to know how VCR tests work

modular-magician · 2025-07-31T17:56:19Z

🟢 Tests passed during RECORDING mode:
TestAccBigQueryDataset_access [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

🟢 All tests passed!

View the build log or the debug log for each test

github-actions · 2025-08-01T09:46:17Z

@GoogleCloudPlatform/terraform-team @BBBmau This PR has been waiting for review for 2 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

melinath · 2025-08-05T15:26:25Z

@modular-magician reassign-reviewer

mmv1/templates/terraform/constants/bigquery_dataset.go.tmpl

modular-magician · 2025-08-06T07:27:38Z

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.

google provider: Diff ( 3 files changed, 29 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 3 files changed, 29 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 2 files changed, 27 insertions(+), 2 deletions(-))
Open in Cloud Shell: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2025-08-06T07:32:48Z

Tests analytics

Total tests: 137
Passed tests: 126
Skipped tests: 10
Affected tests: 1

Click here to see the affected service packages

bigquery

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

TestAccBigQueryDataset_bigqueryDatasetAuthorizedDatasetExample

Get to know how VCR tests work

modular-magician · 2025-08-06T07:34:01Z

🟢 Tests passed during RECORDING mode:
TestAccBigQueryDataset_bigqueryDatasetAuthorizedDatasetExample [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

🟢 All tests passed!

View the build log or the debug log for each test

…block inside bigquery_dataset resource

modular-magician · 2025-08-06T07:47:43Z

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.

google provider: Diff ( 3 files changed, 35 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 3 files changed, 35 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 3 files changed, 39 insertions(+), 2 deletions(-))
Open in Cloud Shell: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2025-08-06T07:52:43Z

Tests analytics

Total tests: 137
Passed tests: 127
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages

bigquery

🟢 All tests passed!

View the build log

melinath

LGTM for Terraform.

melinath · 2025-08-06T22:53:24Z

Moving forward with merge since it looks like there aren't meaningful changes since @wj-chen's last review.

As a side note, I tested this manually locally & confirmed that there's no diff on upgrade. Like with any diff suppress, users will see a more complex diff if anything in the access block does change, but that's expected.

GoogleCloudPlatform#14569)

modular-magician added the service/bigquery label Jul 18, 2025

modular-magician requested a review from wj-chen July 18, 2025 18:55

sachinpro force-pushed the fix_dataset_non_legacy_roles branch from a57930b to 319e962 Compare July 18, 2025 19:00

sachinpro force-pushed the fix_dataset_non_legacy_roles branch from 319e962 to 62042a2 Compare July 18, 2025 19:21

sachinpro marked this pull request as ready for review July 18, 2025 19:37

github-actions bot requested a review from BBBmau July 18, 2025 19:38

wj-chen reviewed Jul 18, 2025

View reviewed changes

wj-chen approved these changes Jul 28, 2025

View reviewed changes

sachinpro force-pushed the fix_dataset_non_legacy_roles branch from 62042a2 to 63af990 Compare July 31, 2025 17:37

modular-magician requested a review from wj-chen July 31, 2025 17:50

github-actions bot requested review from melinath and removed request for BBBmau August 5, 2025 15:27

melinath requested changes Aug 5, 2025

View reviewed changes

mmv1/templates/terraform/constants/bigquery_dataset.go.tmpl Outdated Show resolved Hide resolved

sachinpro force-pushed the fix_dataset_non_legacy_roles branch from 63af990 to 4bc4d81 Compare August 6, 2025 07:14

github-actions bot requested a review from melinath August 6, 2025 07:15

fix(bigquery_dataset): fixed handling of non-legacy roles for access …

7beb1f2

…block inside bigquery_dataset resource

sachinpro force-pushed the fix_dataset_non_legacy_roles branch from 4bc4d81 to 7beb1f2 Compare August 6, 2025 07:34

melinath approved these changes Aug 6, 2025

View reviewed changes

melinath added this pull request to the merge queue Aug 6, 2025

Merged via the queue into GoogleCloudPlatform:main with commit dc61202 Aug 6, 2025
26 checks passed

sachinpro deleted the fix_dataset_non_legacy_roles branch August 7, 2025 11:12

zhihaos pushed a commit to zhihaos/magic-modules that referenced this pull request Aug 12, 2025

fix(bigquery_dataset): fixed handling of non-legacy roles for access … (

f4472fd

GoogleCloudPlatform#14569)

		@@ -10,7 +10,7 @@ resource "google_bigquery_dataset" "{{$.PrimaryResourceId}}" {
		}

		access {

fix(bigquery_dataset): fixed handling of non-legacy roles for access … #14569

fix(bigquery_dataset): fixed handling of non-legacy roles for access … #14569

Uh oh!

Conversation

sachinpro commented Jul 18, 2025

Uh oh!

modular-magician commented Jul 18, 2025

Diff report

Uh oh!

modular-magician commented Jul 18, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 18, 2025

Diff report

Uh oh!

modular-magician commented Jul 18, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 18, 2025

Uh oh!

modular-magician commented Jul 18, 2025

Diff report

Uh oh!

modular-magician commented Jul 18, 2025

Tests analytics

Uh oh!

github-actions bot commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sachinpro commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

modular-magician commented Jul 18, 2025

Uh oh!

wj-chen left a comment

Choose a reason for hiding this comment

Uh oh!

wj-chen Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

sachinpro Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

wj-chen Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

sachinpro Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

wj-chen Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

sachinpro commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wj-chen commented Jul 21, 2025

Uh oh!

github-actions bot commented Jul 23, 2025

Uh oh!

github-actions bot commented Jul 25, 2025

Uh oh!

wj-chen commented Jul 28, 2025

Uh oh!

sachinpro commented Jul 29, 2025

Uh oh!

modular-magician commented Jul 31, 2025

Diff report

Uh oh!

modular-magician commented Jul 31, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 31, 2025

Uh oh!

github-actions bot commented Aug 1, 2025

Uh oh!

melinath commented Aug 5, 2025

Uh oh!

Uh oh!

modular-magician commented Aug 6, 2025

Diff report

github-actions bot commented Jul 18, 2025 •

edited

Loading

sachinpro commented Jul 18, 2025 •

edited

Loading

sachinpro commented Jul 21, 2025 •

edited

Loading