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.

Sign up for GitHub

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

Jump to bottom

Allow defining BigQuery table fields inline #1113

Closed

dasch wants to merge 1 commit into hashicorp:main from dasch:dasch/bigquery-table-schema

Contributor

dasch commented Feb 21, 2018 •

edited

Loading

An implementation of #910.

I'm pretty new to Go, so I wanted to push this PR even if it's a bit rough.

Add a repeated field to google_bigquery_table resources, allowing for inline definition of table schemas.
Support setting the name of the field.
Support setting the type of the field.
Support setting the description of the field.
Support making a field required.
Support making a field repeated.

nat-henderson mentioned this pull request

Terraform plan permits renaming BigQuery column #1144

Closed

Contributor

nat-henderson commented Mar 12, 2018

This looks like a great idea.

danawillow mentioned this pull request

Allow defining a bigquery_table schema directly #910

Closed

danawillow suggested changes

View reviewed changes

Contributor

danawillow left a comment

I'm fine with the general idea of this as long as it's well-documented. Thanks @dasch for starting, and sorry it took so long to get around to reviewing! Here are some initial comments.

google/resource_bigquery_table.go Outdated

@@ @@ -93,6 +93,23 @@ func resourceBigQueryTable() *schema.Resource { @@
               				},
               			},
+              			"field": {
+              				Type:     schema.TypeSet,
+              				Required: true,

Contributor

danawillow May 8, 2018

This should be optional, since we still want to allow people to use the schema field

google/resource_bigquery_table.go Outdated

               		table.TimePartitioning = expandTimePartitioning(v)
               	}
+              	fields := d.Get("binding").(*schema.Set)

Contributor

danawillow May 8, 2018

since the attribute in the schema is called "field", this should be d.Get("field"). Also since we're going to make this optional, let's do a GetOk instead, since if no fields are set then I think d.Get will return nil (it's possible I'm wrong and that it's an empty set, but for consistency with the other d.GetOk calls in this function I think it's a good idea anyway)

Contributor Author

dasch May 16, 2018

Ah, copy-paste error. I think we need to use Get because field can be used multiple times – I copied this code from another place that had that behavior.

google/resource_bigquery_table.go Outdated

+              	for i, v := range fields.List() {
+              		field := v.(map[string]interface{})
+              		table.Schema.Fields[i] = &bigquery.TableFieldSchema{

Contributor

danawillow May 8, 2018

This will segfault because table.Schema is nil at this point- take a look around the codebase for some examples of how to fill this in.

Contributor Author

dasch May 16, 2018

I set table.Schema on line 289 – can it still be nil?

Contributor

danawillow May 16, 2018

Oh sorry, it's a different reason- the Fields array needs to be initialized: https://play.golang.org/p/2AsZv6v55tP is a stripped down version of what's happening here.

Contributor Author

dasch May 17, 2018

Ah, thanks!

dasch force-pushed the dasch/bigquery-table-schema branch 2 times, most recently from db2a556 to 899e4df Compare

May 16, 2018 13:09

dasch commented

View reviewed changes

google/resource_bigquery_table.go Show resolved Hide resolved

dasch force-pushed the dasch/bigquery-table-schema branch from 899e4df to 97fb0d8 Compare

May 16, 2018 13:52

danawillow suggested changes

View reviewed changes

Contributor

danawillow left a comment

This seems reasonable so far! Next up you'll want to add acceptance tests and documentation.

google/resource_bigquery_table.go Outdated

+              	for i, v := range fields.List() {
+              		field := v.(map[string]interface{})
+              		table.Schema.Fields[i] = &bigquery.TableFieldSchema{

Contributor

danawillow May 16, 2018

Oh sorry, it's a different reason- the Fields array needs to be initialized: https://play.golang.org/p/2AsZv6v55tP is a stripped down version of what's happening here.

google/resource_bigquery_table.go Show resolved Hide resolved

Contributor Author

dasch commented May 17, 2018

@danawillow sorry for all the questions, but I don't seem to be able to make the tests actually run – I try to break a test be e.g. screwing with the inline config, but make test reports success:

$ make test
==> Checking that code complies with gofmt requirements...
go test -i $(go list ./... |grep -v 'vendor') || exit 1
echo $(go list ./... |grep -v 'vendor') | \
		xargs -t -n4 go test  -timeout=30s -parallel=4
go test -timeout=30s -parallel=4 github.com/terraform-providers/terraform-provider-google github.com/terraform-providers/terraform-provider-google/google github.com/terraform-providers/terraform-provider-google/scripts
?   	github.com/terraform-providers/terraform-provider-google	[no test files]
ok  	github.com/terraform-providers/terraform-provider-google/google	0.090s
ok  	github.com/terraform-providers/terraform-provider-google/scripts	0.025s

Contributor Author

dasch commented May 17, 2018 •

edited

Loading

@danawillow one more: BQ only allows making a REQUIRED column NULLABLE and changing the description, all other column changes are forbidden. With TypeSet, it's difficult to work with columns as logical units, since the name has no special role. Would it make more sense to use TypeMap here, and handle field diffs specially, e.g. not allowing changes that are illegal?

Contributor

danawillow commented May 18, 2018

For running tests, you want testacc: check out the instructions at https://github.com/terraform-providers/terraform-provider-google/blob/master/.github/CONTRIBUTING.md and let me know if they can be improved!

As for your other comments, are you trying to add apply-time logic to catch these? If so, I would probably not bother- if the API throws an error, that'll happen at the exact same spot that it would if we threw the error from inside Terraform. If the API takes a long time to return or has a confusing error message, then I'd reconsider. For plan-time though, just make everything except for description and mode as ForceNew.

Contributor Author

dasch commented May 18, 2018 •

edited

Loading

Thanks!

This would be plan-time stuff. Will ForceNew work for Elem items in a TypeSet? If e.g. the description field is changed, Terraform doesn't recognize the item anymore – it's effectively a new item, since the hash has changed. As such, does it even make sense to say a field has "changed"? That's why I asked about using TypeMap instead, since each item would be stable even if some fields on it changes.

Also: adding a new field shouldn't force a new table to be created, and that's probably the most common change.

dasch force-pushed the dasch/bigquery-table-schema branch from dcac526 to 2d2e832 Compare

May 18, 2018 09:12


          Allow defining BigQuery table fields inline

6ec3735

dasch force-pushed the dasch/bigquery-table-schema branch from 2d2e832 to 6ec3735 Compare

May 18, 2018 10:44

Contributor

danawillow commented May 18, 2018

Hmm, without spending some time playing around with it I'm not sure. You may also be interested in looking at setting a custom hash function on the set.

(also unrelated fyi- if you could avoid squashing your commits, that would be great just so I can see what changed in between reviews 🙂)

Contributor Author

dasch commented May 22, 2018

Hmm, without spending some time playing around with it I'm not sure. You may also be interested in looking at setting a custom hash function on the set.

I'll try that.

(also unrelated fyi- if you could avoid squashing your commits, that would be great just so I can see what changed in between reviews 🙂)

Sure thing!

Contributor

paddycarver commented Jun 22, 2018

Hey @dasch! I just looked through this, and it's looking really good. Is this something you're still looking to get merged?

As for the REQUIRED to NULLABLE change, you could probably accomplish that with CustomizeDiff: loop through all fields in a set, and track changes. You could also, as Dana mentioned, use a custom SchemaSetFunc that only used the name of the column, making the keys stable as long as the name doesn't change.

paddycarver added the enhancement label

Contributor Author

dasch commented Aug 7, 2018

@paddycarver I'd love to have it merged, but unfortunately I won't be able to spend more time on it in the foreseeable future – feel free to merge as-is or take it over.

Contributor Author

dasch commented Oct 19, 2018

@danawillow @paddycarver would you be willing to merge this as-is and complete the changes yourselves? I'm afraid I've forgotten whatever Go I learned doing this PR...

Collaborator

rileykarson commented Jun 11, 2024

Closing PR as stale

rileykarson closed this

github-actions bot commented Jul 14, 2024

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels