-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add support for BigQuery External Data Tables #1466
Add support for BigQuery External Data Tables #1466
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs. Thanks for your contribution! A human will be with you soon. |
I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
004401b
to
cf1e8a4
Compare
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.
Made a quick first pass, I have a few initial questions/comments
// BigtableOptions: [Optional] Additional options if sourceFormat is set to BIGTABLE. | ||
"bigtable_options": { | ||
<% if version.nil? || version == 'ga' -%> | ||
Removed: "This field is in beta. Use it in the the google-beta provider instead. See https://terraform.io/docs/providers/google/provider_versions.html for more details.", |
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.
is this in beta? It looks like it's just part of the v2
API like everything else.
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.
externalDataConfiguration.sourceFormat | string | [Required] The data format. For CSV files, specify "CSV". For Google sheets, specify "GOOGLE_SHEETS". For newline-delimited JSON, specify "NEWLINE_DELIMITED_JSON". For Avro files, specify "AVRO". For Google Cloud Datastore backups, specify "DATASTORE_BACKUP". [Beta] For Google Cloud Bigtable, specify "BIGTABLE".
https://cloud.google.com/bigquery/docs/reference/rest/v2/tables
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.
So maybe this should be included? But source_format
disallow BIGTABLE
unless using google-beta
provider (as it already does). What do you think?
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.
Ah, nope- you're correct. Let's omit bigtable_options
entirely from the google
provider, nested removed fields don't work well: https://github.com/hashicorp/terraform/issues/20756
8e7d99e
to
54cc6c4
Compare
74a8673
to
fe7f47b
Compare
This is ready for review. I have done manual testing of most of this, but might add some test the coming days. |
fe7f47b
to
df85fb3
Compare
df85fb3
to
3ca38e9
Compare
Rebased |
What is the status of this PR , it seems like it's currently in conflict ? I can't wait to be able to define external tables with terraform, currently it's the only part of of our data pipeline that we have still manually defined |
@allanlegalstart - ah, I managed to lose track of this unfortunately. @rickard-von-essen - Really sorry for losing track of this! Do you mind rebasing again so I can make another pass? Alternatively, I can take over on the PR for you and get this merged if you've no longer got the time. |
@rileykarson would be awesome if this can get merged in a near future :) |
One sec. |
3ca38e9
to
91f2eed
Compare
Rebased |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in terraform-google-conversion. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
And fixed wronly named option in docs s/schema_format/source_format/
…rom '"' (default) to ''
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
7c178e3
to
ed5b784
Compare
Hi, I noticed BigTable support was added then removed (6165666) and I don't see it in the lastest Terraform. Is there any planned support for external bigtable tables? |
Not that I'm aware of- I believe the feature was disabled or otherwise untestable at the time of this PR. Can you file an issue asking for support against the terraform-provider-google repo? |
@alfredo-gimenez It was part of this PR for a short while (my goal was to support all external table sources) but I decided to remove the BigTable part since it was a huge part of the implementation, I had no enough time to test it properly, and I didn't have much knowledge of BigTable. |
Gotcha, I’ll file an issue, thanks! |
This adds support for BigQuery external data tables in Terraform.
This needs:
resourceBigQueryTableRead
[terraform] BQ - Support for External Data Tables