impl(google_bigquery_table): exposing TableMetadataView query param #13240
Conversation
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @c2thorn, 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 reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 129 Click here to see the affected service packages
Action takenFound 71 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. |
sachinpro
force-pushed
the
table_metadata_view
branch
from
3fe6669 to
238e9d4
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.
|
Tests analyticsTotal tests: 129 Click here to see the affected service packages
Action takenFound 40 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. |
sachinpro
force-pushed
the
table_metadata_view
branch
from
238e9d4 to
3b6d742
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.
|
Tests analyticsTotal tests: 129 Click here to see the affected service packages
Action takenFound 13 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. |
sachinpro
force-pushed
the
table_metadata_view
branch
2 times, most recently
from
aa28c78 to
b80cb4a
Compare
… as 'table_metadata_view'
sachinpro
force-pushed
the
table_metadata_view
branch
from
b80cb4a to
937b666
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.
|
1 similar comment
|
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: 130 Click here to see the affected service packages
Action takenFound 41 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Tests analyticsTotal tests: 130 Click here to see the affected service packages
Action takenFound 41 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
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: 130 Click here to see the affected service packages
Action takenFound 67 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. |
|
Test |
| @@ -1834,7 +1839,14 @@ func resourceBigQueryTableRead(d *schema.ResourceData, meta interface{}) error { | |||
| datasetID := d.Get("dataset_id").(string) | |||
| tableID := d.Get("table_id").(string) | |||
|
|
|||
| res, err := config.NewBigQueryClient(userAgent).Tables.Get(project, datasetID, tableID).Do() | |||
| tableMetadataView := d.Get("table_metadata_view").(string) | |||
There was a problem hiding this comment.
How is if tableMetadataView, ok := d.GetOk("table_metadata_view"); ok { ... }?
There was a problem hiding this comment.
Resolved in the commit.
| @@ -1834,7 +1839,14 @@ func resourceBigQueryTableRead(d *schema.ResourceData, meta interface{}) error { | |||
| datasetID := d.Get("dataset_id").(string) | |||
| tableID := d.Get("table_id").(string) | |||
|
|
|||
| res, err := config.NewBigQueryClient(userAgent).Tables.Get(project, datasetID, tableID).Do() | |||
There was a problem hiding this comment.
In resourceBigQueryTableColumnDrop there is a Tables.Get too that we need to handle
There was a problem hiding this comment.
Resolved in the commit.
There was a problem hiding this comment.
Was thinking this:
client := config.NewBigQueryClient(userAgent).Tables.Get(project, datasetID, tableID)
if tableMetadataViewRaw, ok := d.GetOk("table_metadata_view"); ok {
client = client.View(tableMetadataViewRaw.(string))
}
res, err := client.Do()
There was a problem hiding this comment.
This does look better. Added this in the commit.
There was a problem hiding this comment.
We should handle the virtual field in Update and Import too, see lines 2075 and 3392
There was a problem hiding this comment.
Resolved in the commit.
|
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: 131 Click here to see the affected service packages
Action takenFound 40 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. 🟢 All tests passed! |
| @@ -1834,7 +1839,14 @@ func resourceBigQueryTableRead(d *schema.ResourceData, meta interface{}) error { | |||
| datasetID := d.Get("dataset_id").(string) | |||
| tableID := d.Get("table_id").(string) | |||
|
|
|||
| res, err := config.NewBigQueryClient(userAgent).Tables.Get(project, datasetID, tableID).Do() | |||
There was a problem hiding this comment.
Was thinking this:
client := config.NewBigQueryClient(userAgent).Tables.Get(project, datasetID, tableID)
if tableMetadataViewRaw, ok := d.GetOk("table_metadata_view"); ok {
client = client.View(tableMetadataViewRaw.(string))
}
res, err := client.Do()
| @@ -3382,6 +3406,12 @@ func resourceBigQueryTableImport(d *schema.ResourceData, meta interface{}) ([]*s | |||
| return nil, fmt.Errorf("Error setting deletion_protection: %s", err) | |||
| } | |||
|
|
|||
| // The default for `table_metadata_view` is "STORAGE_STATS" for the API. | |||
There was a problem hiding this comment.
Sorry I was mistaken in my previous comment. Since table_metadata_view doesn't have Default: true unlike deletion_protection, we don't need to set it in Import. Setting it to BASIC here makes it unclear to the user that they "lose" the default level of storage stats upon importing into Terraform https://cloud.google.com/bigquery/docs/reference/rest/v2/tables/get#TableMetadataView.
Can we update the comment in Line 3404 to say "// Explicitly set virtual fields with default values to their default values on import"?
The tests should also pass without having table_metadata_view in ImportStateVerifyIgnore like in your original commit.
There was a problem hiding this comment.
I have reverted the default value change.
The comment is already here. Isn't that sufficient?
There was a problem hiding this comment.
Yes the original comment is there, it should help for future reference to emphasize on "// Explicitly set virtual fields with default values to their default values on import" so contributors know to set/not set depending on whether the virtual field has/doesn't have a Default.
There was a problem hiding this comment.
Added the comment.
sachinpro
force-pushed
the
table_metadata_view
branch
from
38f670f to
2b35513
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.
|
Tests analyticsTotal tests: 131 Click here to see the affected service packages
Action takenFound 40 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. 🟢 All tests passed! |
|
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: 131 Click here to see the affected service packages
🟢 All tests passed! View the build log |
0913590
We are using TablesGetCall.View to set TableMetadataView through a virtual field
table_metadata_view.b/398215519
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.