-
Notifications
You must be signed in to change notification settings - Fork 286
feat(bigquery): Add example for creating an authorized view #872
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
feat(bigquery): Add example for creating an authorized view #872
Conversation
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
| dataset_id = "authdataset" | ||
| description = "Dataset for authorized view" | ||
| location = "us-west1" | ||
|
|
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.
nit: can remove this empty line
| query = "SELECT item_id, avg(rating) FROM `myproject.movie_dataset.movie_ratings` GROUP BY item_id ORDER BY item_id;" | ||
| use_legacy_sql = false | ||
| } | ||
| depends_on = [ |
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.
Since e.g. project and dataset_id accesses google_bigquery_dataset.default directly, Terraform can infer the dependency and we don't need depends_on explicitly, see https://developer.hashicorp.com/terraform/language/meta-arguments/depends_on.
Same in the other places in this example.
| resource "google_bigquery_table_iam_policy" "default" { | ||
| project = google_bigquery_table.default.project | ||
| dataset_id = google_bigquery_dataset.default.dataset_id | ||
| table_id = "authview" |
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.
google_bigquery_table.default.table_id
| */ | ||
| resource "google_bigquery_table_iam_policy" "default" { | ||
| project = google_bigquery_table.default.project | ||
| dataset_id = google_bigquery_dataset.default.dataset_id |
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.
google_bigquery_table.default.dataset_id
glasnt
left a comment
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.
Some nits, but some blockers.
| /* | ||
| Create a dataset to contain the authorized view. | ||
| */ |
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.
These comments can be useful, and if you want to keep the, I would suggest using single-line comments to reduce the lines of the demo, to make it more accessible/visible on the tutorial page.
If you want to present each resource in a different block in the codes, you can use region tags around each block.
| /* | |
| Create a dataset to contain the authorized view. | |
| */ | |
| # Create a dataset to contain the authorized view. |
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.
Changed in latest commit.
| dataset_id = google_bigquery_dataset.default.dataset_id | ||
| table_id = "authview" | ||
| description = "View to authorize" | ||
| deletion_protection = false # set to "true" in production |
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.
Since this sample is not being tested, remove this line and ensure the configuration is production ready. Deletion protection is true by default
| deletion_protection = false # set to "true" in production |
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.
Changed in latest commit.
| Create a dataset to contain the authorized view. | ||
| */ | ||
| resource "google_bigquery_dataset" "default" { | ||
| dataset_id = "authdataset" |
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.
Query: is the dataset "authdataset", or "movie_dataset"? You're assigning dataset access to a different dataset later int his sample.
You can reference google_bigquery_dataset.default.dataset_id in other resources to ensure a consistent name.
It's helpful to clearly show which is the original dataset and which is the authorised view of the data with clear names (unsure what "movies" is in this context. )
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 re-named the resources for clarity and tested again to make sure it works.
…m-google-modules#872) * feat(bigquery): Add example for creating an authorized view * feat(bigquery): Add example for creating an authorized view * feat(bigquery): Add example for creating an authorized view * feat(bigquery): Add example for creating an authorized view * feat(bigquery): Add example for creating an authorized view --------- Co-authored-by: Katie McLaughlin <[email protected]>
Description
Fixes b/361168902
Note: If you are not associated with Google, open an issue for discussion before submitting a pull request.
Checklist
Readiness
Style
guide
Testing
I have performed tests described in the Contributing guide:
terraform applyterraform fmtcheckIntended location
Yes, this sample will be (or already is) included on cloud.google.com
Location(s): https://cloud.google.com/bigquery/docs/create-authorized-views
No, this sample won't be included on cloud.google.com
Reason:
API enablement
Review