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

[bigtable] - Add an example of a cloud function with Bigtable #1742

Closed
wants to merge 10 commits into from

Conversation

billyjacobson
Copy link
Member

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 22, 2020
@fhinkel
Copy link
Contributor

fhinkel commented Apr 23, 2020

👋 Can you also add yourself as codeowner in the OWNERS file please.

@billyjacobson billyjacobson changed the title Bigtable functions [bigtable] - Add an example of a cloud function with Bigtable Apr 23, 2020
@billyjacobson billyjacobson marked this pull request as ready for review April 23, 2020 14:54
@billyjacobson billyjacobson requested a review from fhinkel as a code owner April 23, 2020 15:03
Copy link
Contributor

@ace-n ace-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -1 on mocking APIs unless absolutely necessary (mock tests may not detect client library surface changes), but other than that LGTM.


return {
program: proxyquire('../', {
'@google-cloud/bigtable': {Bigtable: BigtableMock},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth testing this directly instead of using a mocked client library?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion about this, I'm just following what every other sample has done.

@grant grant removed their request for review April 27, 2020 17:40
@ace-n
Copy link
Contributor

ace-n commented Apr 27, 2020 via email

@fhinkel
Copy link
Contributor

fhinkel commented Apr 27, 2020

I agree, system tests often give us a better indication if something breaks. That being said, considering that we have lots of samples in this repo that use proxyquire, I suggest we merge this PR as it is. @billyjacobson, is it OK if somebody else revisits the tests in the future even though you're code owner?

@fhinkel fhinkel added the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 27, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 27, 2020
@fhinkel fhinkel added the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 28, 2020
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Apr 28, 2020
@billyjacobson
Copy link
Member Author

I agree, system tests often give us a better indication if something breaks. That being said, considering that we have lots of samples in this repo that use proxyquire, I suggest we merge this PR as it is. @billyjacobson, is it OK if somebody else revisits the tests in the future even though you're code owner?

Happy to help provide a review in the future on the tests

@ace-n
Copy link
Contributor

ace-n commented Apr 28, 2020

Oh - one more thing that I almost forgot: these samples should live in bigtable/functions, not functions/bigtable.

(@grant and I previously decided that the top-level functions/ should only be for samples used in the Functions docs.)

@billyjacobson
Copy link
Member Author

Oh - one more thing that I almost forgot: these samples should live in bigtable/functions, not functions/bigtable.

(@grant and I previously decided that the top-level functions/ should only be for samples used in the Functions docs.)

these will be in the functions doc and potentially the bigtable docs, just like the Spanner ones

@fhinkel
Copy link
Contributor

fhinkel commented Apr 29, 2020

ping @ace-n does your objection still hold with @billyjacobson 's clarification?

@ace-n
Copy link
Contributor

ace-n commented Apr 29, 2020

@fhinkel I'll defer to @labtopia on that (GCF TW).

(N.B: they're quite busy with ongoing GCF asks, so they may take awhile to get to this.)

@ace-n
Copy link
Contributor

ace-n commented Apr 29, 2020

FYI: chatted with @labtopia offline, we are 👎on adding these to the functions docs.

(The "spanner tutorial" you mentioned likely should either be removed entirely [given that we have this page], or moved to the spanner/ docs.)

What we are in favor of is linking out from the functions/ docs' left-nav in the same way the above-mentioned Spanner tutorial does (as shown at the bottom of this image):

Screen Shot 2020-04-29 at 4 56 02 PM

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.

5 participants