Support Bigquery Views #230
Conversation
There was a problem hiding this comment.
Thanks @jimmy-btn! Just a few comments but looks good overall.
google/resource_bigquery_dataset.go
Outdated
| @@ -238,6 +238,12 @@ func resourceBigQueryDatasetRead(d *schema.ResourceData, meta interface{}) error | |||
| d.Set("dataset_id", res.DatasetReference.DatasetId) | |||
| d.Set("default_table_expiration_ms", res.DefaultTableExpirationMs) | |||
|
|
|||
| // Older Tables in BigQuery have no Location set in the API response. We can safely assume that these tables | |||
There was a problem hiding this comment.
This is only going to come up during import, right? If so I wouldn't mind a comment that mentions that just to be extra clear. Also mind moving the other d.Set for location into an else with this clause?
There was a problem hiding this comment.
Great idea - Done!
google/resource_bigquery_table.go
Outdated
| MaxItems: 1, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| // Type: [Required] The only type supported is DAY, which will generate |
There was a problem hiding this comment.
I think this comment was accidentally pasted from a different one
There was a problem hiding this comment.
It was! Thank you for catching. I've updated using the language from the API documentation.
| Optional: true, | ||
| MaxItems: 1, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ |
There was a problem hiding this comment.
Do you think it makes sense to add the other view attributes, or do they not make sense in terraform? (useLegacySql, userDefinedFunctionResources)
There was a problem hiding this comment.
I've added support for useLegacySQL - I think that makes a lot of sense.
It makes sense to capture userDefinedFunctionResources in terraform, but I'm not particularly familiar with them. I'll try and carve out some time to ramp up and get them added, but I'd recommend doing that in a second CL/pass.
google/resource_bigquery_table.go
Outdated
| @@ -202,12 +219,17 @@ func resourceTable(d *schema.ResourceData, meta interface{}) (*bigquery.Table, e | |||
| }, | |||
| } | |||
|
|
|||
| // TODO(jmcgill): How do I do nested objects in a terraform configuration. | |||
There was a problem hiding this comment.
do you still need this TODO?
| @@ -64,11 +85,40 @@ func testAccBigQueryTableExists(n string) resource.TestCheckFunc { | |||
| return fmt.Errorf("No ID is set") | |||
| } | |||
| config := testAccProvider.Meta().(*Config) | |||
| _, err := config.clientBigQuery.Tables.Get(config.Project, rs.Primary.Attributes["dataset_id"], rs.Primary.Attributes["name"]).Do() | |||
| table, err := config.clientBigQuery.Tables.Get(config.Project, rs.Primary.Attributes["dataset_id"], rs.Primary.Attributes["table_id"]).Do() | |||
There was a problem hiding this comment.
Good catch, thanks! Want to fix that up in testAccCheckBigQueryTableDestroy too?
|
For tracking purposes: this fixes #190 |
| vd := &bigquery.ViewDefinition{Query: raw["query"].(string)} | ||
|
|
||
| if v, ok := raw["use_legacy_sql"]; ok { | ||
| vd.UseLegacySql = v.(bool) |
There was a problem hiding this comment.
You'll also need to set vd.ForceSendFields = append(vd.ForceSendFields, "UseLegacySql" so if it gets updated to false, it still gets sent along the wire (since false values end up defaulting to empty)
There was a problem hiding this comment.
Good catch - thank you!
Would you like me to add an end to end test for this particular case (UseLegaclySQL = false)? Running these AF tests costs $$$ so I wasn't sure on how to balance coverage vs cost.
There was a problem hiding this comment.
Sure! You'd want to do a test specifically for updating it, since the default is false so if you set it to false explicitly on create it won't make a difference whether you forced it to send. If cost is an issue for you let me know and I'm happy to run it and post the results.
There was a problem hiding this comment.
Perfect! Tests added for converting useLegacySQL from true to false - The API is kind enough to reject views with incorrect syntax when using new-SQL so I am now extra confident that this is working as expected!
google/resource_bigquery_table.go
Outdated
| func flattenView(vd *bigquery.ViewDefinition) []map[string]interface{} { | ||
| result := map[string]interface{}{"query": vd.Query} | ||
|
|
||
| if vd.UseLegacySql { |
There was a problem hiding this comment.
Fun fact: this already actually gets stored in state whether it's true or false because of the way terraform handles objects, so we may as well just simplify the logic here and always set it anyway.
There was a problem hiding this comment.
Done! Should I skip the check on line 436 (in expandView) too?
There was a problem hiding this comment.
Actually nevermind; I need to expand the value anyway. That extra error check is harmless.
* Support views in Terraform.BigQuery * Add tests for Table with view, and fix existing Table test * Remove dead code * run gofmt * Address comments * Address review comments and add support for use_legacy_sql * Force transmission/storage of UseLegacySQL * Trying to fix tests * add tests for useLegacySQL
<!-- This change is generated by MagicModules. --> /cc @rileykarson
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
Extend the Google provider for Terraform to support Bigquery Views.
Make a few drive by cleanups in the process: