8000 Add GCF Python SQL samples by ace-n · Pull Request #1655 · GoogleCloudPlatform/python-docs-samples · GitHub
[go: up one dir, main page]
Skip to content

Add GCF Python SQL samples #1655

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
Merged
merged 10 commits into from
Sep 12, 2018
Merged

Add GCF Python SQL samples #1655

merged 10 commits into from
Sep 12, 2018

Conversation

ace-n
Copy link
@ace-n ace-n commented Aug 25, 2018 •
edited
Loading

Do not merge until CI is set up.

@stew-r FYI

@andrewsg FYI - psycopg2 gives a warning about binary modules. I assume that we can ignore that?

Add GCF Python SQL samples
722b0fd 
Change-Id: Ib60712a5f3808b1fb99088e5e637304ef21e98c7
@ace-n ace-n requested a review from andrewsg August 25, 2018 02:26
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 25, 2018
Update test SQL instance name
292eaa0 
Change-Id: I0c6ca84695a1a593f46ddd922507eb0cb1e56879
@ace-n
Copy link
Author
ace-n commented Aug 27, 2018 •
edited
Loading

TODO Add CI

@ace-n ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 27, 2018
Add SQL Kokoro test files
df9a6ba 
Change-Id: Ia59f803bb2bd50842f047a6570ebbc86171d2898
@ace-n ace-n removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 29, 2018
@andrewsg
Copy link
Member

Any idea why Travis build is failing?
10000

andrewsg
andrewsg reviewed Aug 30, 2018
View reviewed changes
functions/sql/mysql_sample.py

import pymysql

is_production = getenv('SUPERVISOR_HOSTNAME') is not None
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any more straightforward methods of detecting the environment? Even better, could we do try/except to simply try to connect to the unix socket and fallback to user settings if it doesn't work? (properly commented)

Copy link

Choose a reason for hiding this comment

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

We ideally should not use SUPERVISOR_HOSTNAME. It may not be there in the future. You opt for a separate env var to contain the "key". So.. INSTANCE_CONNECTION_TYPE with either unix_socket (or empty string for default), of host for traditional connections.

Copy link
Author

Choose a reason for hiding this comment

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

The Node samples use the NODE_ENV env var to determine whether a function has been deployed. For the sake of maintaining patterns across samples, is there a way we can do this here?

@ace-n
Copy link
Author
ace-n commented Aug 30, 2018

RE Travis build failure: Travis doesn't have the Cloud SQL proxy enabled. I believe we're intending to deprecate Travis in favor of Kokoro, but I don't know what the timeline on that is.

@SurferJeffAtGoogle should I disable this test (on Travis) for now and move it to Kokoro, or will y'all take care of that later on?

@ace-n
Copy link
Author
ace-n commented Aug 31, 2018 •
edited
Loading

TODO add best practices samples.

Edit 9/10/18: "best practices" samples are being merged into existing ones, so this should be good for review..

Note: we'll have to use a different library for MySQL, as PyMySQL supports neither connection pooling nor auto-reconnect connections.

mysql-python supports pooling, but doesn't support auto-reconnection - I've gone ahead with the PyMySQL version, and added a code comment to that effect.

@andrewsg this should be good to review. 😄

@ace-n ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 31, 2018
Ace Nassri added 6 commits September 7, 2018 10:50
Address comments
94b76b5 
Change-Id: I0bad3298fa138430a4073ce108412fd4a6af5854
Switch python samples to pools
f426dda 
Change-Id: Ibf5a90b3b8c02596f441b7d77cdf11b416260437
Use connections for Python MySQL, as libraries don't support pooling
8a6e7f8 
Change-Id: I3efeabd192be9c2c987846b0df7dcced016ddc49
Fix lint + tests
be10b39 
Change-Id: I9d2901a737ab0bb7d74edefa69876704846fdf10
Fix nits
686ee00 
Change-Id: I0bbcf8e32e6fd2f8fa8ef942d3b089418f01f502
Make Python config consistent with Node samples
5530250 
Change-Id: Ibb0341f51861ec5adcba136903c1df0567e97398
@ace-n ace-n added kokoro:run Add this label to force Kokoro to re-run the tests. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Sep 11, 2018
andrewsg
andrewsg approved these changes Sep 11, 2018
View reviewed changes
Copy link
Member
@andrewsg andrewsg left a comment

Choose a reason for hiding this comment

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

lgtm but please hold off on merge until @SurferJeffAtGoogle has a chance to review changes to testing/test-env.tmpl.sh and others.

@SurferJeffAtGoogle SurferJeffAtGoogle self-assigned this Sep 11, 2018
SurferJeffAtGoogle
SurferJeffAtGoogle reviewed Sep 12, 2018
View reviewed changes
testing/test-env.tmpl.sh
export POSTGRES_INSTANCE=
export POSTGRES_USER=
export POSTGRES_PASSWORD=
export POSTGRES_DATABASE=
Copy link
Contributor

Choose a reason for hiding this comment

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

We haven't standardized these names across languages/repos, and these names look fine.

@ace-n ace-n merged commit 0eb0ebd into master Sep 12, 2018
@ace-n ace-n deleted the gcf-sql branch September 12, 2018 22:02
@ace-n ace-n mentioned this pull request Sep 12, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonpolites jasonpolites jasonpolites left review comments

@andrewsg andrewsg andrewsg approved these changes

@SurferJeffAtGoogle SurferJeffAtGoogle SurferJeffAtGoogle approved these changes

Assignees

@SurferJeffAtGoogle SurferJeffAtGoogle

Labels
cla: yes This human has signed the Contributor License Agreement. kokoro:run Add this label to force Kokoro to re-run the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ace-n @andrewsg @jasonpolites @SurferJeffAtGoogle @googlebot
0