-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Google Cloud: BigQuery DataSet and Table resources #3764
Conversation
@lwander @sparkprime anything I can do for this one or is it in the hopper, waiting its turn? |
bfeaa9b
to
eddbcd1
Compare
|
||
Schema: map[string]*schema.Schema{ | ||
"name": &schema.Schema{ | ||
Type: schema.TypeString, |
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.
It looks like you're missing quite a few features described here
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 am missing two general classes of things:
- the role access information, which should be included. I'll do that
- updating/patching an existing dataset. Update doesn't need to be implemented at all, in terraform's context that's handled by delete/create when a ForceNow config val is set. I will use patch to implement Update
Anything else you'd like from that page? I see list, which isn't useful in a terraform context, and a bunch of computed fields that don't have any obvious use in a terraform 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.
For starters
datasetReference
is described as being required. This resource as is shouldn't work as is.
Otherwise,
access[]
friendlyName
(which you've calledname
)description
defaultTableExpirationMs
location
are optional, but nice to have.
When it comes to exported attributes
self_link
is important to include ascomputed
, since other resources often refer to one another by GCE assigned URI.id
for similar reasons.lastModifiedTime
can be useful for people examining infrastructure managed by Terraform.
@lwander Thanks man! I really appreciate you taking the time to review! |
@coffeepac you got it! Thanks for doing all this work so far. |
eddbcd1
to
d1e7aa6
Compare
@lwander I wanted to get your thoughts on my changes to the Dataset resource before I do something similar to the table and then go implement the Patch operation. The only comment I didn't directly implement was the comment about 'datasetReference'. This is a struct composed of datasetId and projectId which I'm pulling from the provider configuration. If that's wrong/inconsistent lemme know and I'll rework it. |
Optional: true, | ||
}, | ||
|
||
"access": &schema.Schema{ |
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.
This gets a bit tricky, since we implemented ACLs for GCS a little differently, but that was only possible because the API is different. The idea there was, you just provide a list of role entity pairs like this. The GCS API basically squashed userByEmail
, groupByEmail
, and domain
all into one field, so all we had to do was have the user concatenate the contents of that field with the role
, and that would be the access control object. Second, there was no view
type object attached with each GCS ACL, since the ACL is an entirely separate resource for GCS objects and buckets. Basically, to match the GCS Terraform interface, we'd really have to contort the BigQuery API, which is a shame.
I'd say what you have here is best, I'm just leaving this comment in case people have questions about this decision in the future.
@sparkprime, what do you think?
When it comes to the Are you planning on flushing out the table resource as well? It has quite a few fields here. |
I am planning on fleshing out the table resource as well. I wanted some
|
BigQuery is a managed database-like service. It supports SQL as a query language and nearly infinite scale. This PR adds support to manage tables and datasets. first cut, need to finish tests fully functional and the tests are looking good. this is a really thin wrapper around the create/delete portion of the API removed optional parameters, there is no update defined add docs for bigquery resources
98c2579
to
9132344
Compare
I'm going to remove all but the top level of fields and add a json file config option as well. but I wanted to save this incase I want it arbitarily in the future
the table schema is an arbitrarily deep structure which I'm not sure how to represent in terraform's schema syntax. failing that I have introduced a work around: - one level deep of table structure in the standard terraform schema manner (as many fields as you want, no sub fields or fields of type 'record') - arbitrary depth but specify a file that has the layout in json I really like the idea of including an auxilarly file to represent the table structure but I'm open to it being spcified via the file built-in or something. I don't really want to let there be arbitrary depth configs in the terraform config file. that seems a bit heinous
saving because HDD is wonky. will squash later
now to add some tests for the new schema nonsense
adds a test for the inline table format (one level) and the json file format (arbitrary depth)
@lwander oh holidays... I haven't started working on patch semantics yet but I have added bigquery table. tables can have an arbitrary depth of record fields and I didn't see a clean way of doing that with helper/schema so I fudged it by having the schema in code allow for depth of one but I added a schemaFile that expects json and allows for arbitrary depth. the code to support arbitrary depth of record fields is there so if there's a clean way to add them into the schema I'm all for it if you have a way. either way, I like having the schema in a separate file so if there is a 4 layer deep table (my current use case) it isn't garbaging up the tf file. sound okay or is that no go? |
Could you point me to the |
line 46 in builtin/providers/google/resource_bigquery_table.go test using this file is at line 29 and the resource is defined at 105 in builtin/providers/google/resource_bigquery_table_test.go the test fixture for this is: builtin/providers/google/test-fixtures/fake_bigquery_table.json the test schemaFile goes three levels deep. |
@coffeepac I'd love to see support for BigQuery in Terraform. |
@heimweh feel free to take it over! I got pulled off and had to create an external plugin for this. its here: https://github.com/22Acacia/terraform-provider-googlebigquery Its currently in production and does what we need so we're just living with it for now. The big chunk of work I never got to was how to implement Update, its currently a big ball of lying No-Op. The golang docs for the bigquery library have taken a massive leap forward in the past year so it shouldn't be a slog anymore. It was basically just the auto-generated go doc stuff. Lemme know if you want me to review anything, open up permissions on a branch somewhere or have more questions. I'm happy to help! |
Hi all, Any plans to provide a resource for bigquery views? Would be super helpful!! Thanks for your hard work on these core resources..... |
1 similar comment
Hi all, Any plans to provide a resource for bigquery views? Would be super helpful!! Thanks for your hard work on these core resources..... |
this was merged elsewhere. going to close this PR. |
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
BigQuery is a managed database-like service. It supports SQL as
a query language and nearly infinite scale. This PR adds support
to manage tables and datasets.