Skip to content
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 a template_suffix parameter to BigQuery Table.insert_data #1562

Merged
merged 4 commits into from
Mar 14, 2016

Conversation

barakcoh
Copy link

@barakcoh barakcoh commented Mar 3, 2016

Enable the use of template tables by adding the template_suffix parameter.
Documentation: https://cloud.google.com/bigquery/streaming-data-into-bigquery#template-tables

Would love tips on how you feel this should be tested.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Mar 3, 2016
@barakcoh
Copy link
Author

barakcoh commented Mar 3, 2016

I signed it!

@tseaver tseaver self-assigned this Mar 3, 2016
:type template_suffix: string or ``NoneType``
:param template_suffix: treat ``name`` as a template table and provide
a suffix. BigQuery will create the table
``<name> + <template_suffix>`` based on the

This comment was marked as spam.

@tseaver
Copy link
Contributor

tseaver commented Mar 3, 2016

We need to add a unit test which covers the case that template_suffix is passed to insert_data.

@tseaver
Copy link
Contributor

tseaver commented Mar 3, 2016

@jgeewax can you figure out the CLA issue?

@barakcoh
Copy link
Author

barakcoh commented Mar 3, 2016

@jgeewax @tseaver sorry, I misunderstood the CLA process and thought I signed it when I filled out the form but then got the DocuSign. we're reviewing it internally and will sign it soon.

@barakcoh
Copy link
Author

barakcoh commented Mar 3, 2016

@tseaver re coverage, i can update existing tests with template_suffix='xyz' and I guess the unit tests will pass as it'll have no real effect and so will the coverage.

don't know about system tests as I haven't gone through the steps to run them locally. frankly, it's quite a hassle (at least for such a small change)

@tseaver
Copy link
Contributor

tseaver commented Mar 3, 2016

@barakcoh if you can add the template_suffix parameter to just one of the tests for insert_data (e.g., to test_insert_data_with_bound_client) and fix the assertion for the posted data, that will be sufficient.

@tseaver
Copy link
Contributor

tseaver commented Mar 3, 2016

It would actually fit better in test_insert_data_with_alternate_client, which already passes the other optional fields.

@barakcoh
Copy link
Author

barakcoh commented Mar 6, 2016

@tseaver done. also rebased the latest upstream (not sure if you prefer it over a merge).

@tseaver
Copy link
Contributor

tseaver commented Mar 7, 2016

LGTM as soon as the CLAbot says we're good.

@liorr
Copy link

liorr commented Mar 7, 2016

I signed it

@tseaver
Copy link
Contributor

tseaver commented Mar 7, 2016

@jgeewax can you verify, or kick the bot to check again?

@jgeewax
Copy link
Contributor

jgeewax commented Mar 8, 2016

@barakcoh Looks like the e-mail used in these commits (ba....@wonder....com) hasn't signed the CLA. Can you confirm the e-mail address you signed with?

Some options...

  • re-sign with the address on the commits (as listed on the patch) CLA signing link
  • amend the commit author e-mail for the commits you pushed to the e-mail you signed with (since they weren't merged in yet, this seems fine to me, @tseaver tell me if that's not OK)

Sorry about this. I promise it's not that we don't want your commits ... it's just one of those things we need before we can accept outside contributions.

@barakcoh
Copy link
Author

barakcoh commented Mar 8, 2016

@jgeewax the commits are under my email (as you've noted above) but CLA was eventually signed by my boss @liorr ([email protected]), I just tried signing it again using the link you provided but got It looks like you've already signed this CLA..

Do we have to submit all patches using just the email that signed the CLA? Seems kind of a turn off.. hope we can whitelist several emails / GH users under the CLA.

@jgeewax
Copy link
Contributor

jgeewax commented Mar 10, 2016

@barakcoh Seems weird that it says you've already signed... I can't find your GitHub username or the e-mail attached the commits in this PR, nor @liorr 's username. Is there a specific company that signed the corporate CLA so I can check on that?

The link should show a list of agreements you've signed, that looks like this:

cla

Do we have to submit all patches using just the email that signed the CLA? Seems kind of a turn off.. hope we can whitelist several emails / GH users under the CLA.

Any e-mail that is set as the "author" of a commit in a PR needs to have signed the CLA (otherwise, we don't necessarily know if those commits are yours...). So if you author commits as [email protected] and [email protected] and send a single pull request, both of those e-mails must have signed the CLA -- it's OK to have multiple in a single PR though.

If you have a ton of people and want to do a blanket signature, there's a way to sign "for the company" (here) and you just maintain a Google Group with all the e-mails that are "authorized".

@barakcoh
Copy link
Author

thanks @jgeewax, here's what I have:

  1. Corporate CLA covering @Wondermall w/ Google Group - https://groups.google.com/forum/#!managemembers/google-conrtibutors-wondermall/members/active
  2. I'm in the group with my @Wondermall email account: bar...@wond...com
  3. All commits are using my @Wondermall email account

what am I missing here?

screen shot 2016-03-13 at 12 10 38 pm
screen shot 2016-03-13 at 12 14 40 pm
screen shot 2016-03-13 at 12 09 00 pm

I've also asked the signed (@liorr) to join the group but that's pending.

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Mar 13, 2016
@barakcoh
Copy link
Author

oh thank god! thanks so much @jgeewax !

@jgeewax
Copy link
Contributor

jgeewax commented Mar 14, 2016 via email

@barakcoh
Copy link
Author

@jgeewax I figured that much, but still appreciative of the hand-holding. looks like the bot caught on exactly as I submitted my a comment.

@tseaver we good to merge?

dhermes added a commit that referenced this pull request Mar 14, 2016
Add a template_suffix parameter to BigQuery Table.insert_data
@dhermes dhermes merged commit 260cb35 into googleapis:master Mar 14, 2016
@tseaver
Copy link
Contributor

tseaver commented Mar 14, 2016

@dhermes ships that cross. I was re-running the tests locally prior to merging.

@dhermes
Copy link
Contributor

dhermes commented Mar 14, 2016

My bad. No lingering issues?

@tseaver
Copy link
Contributor

tseaver commented Mar 14, 2016

Nope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants