refactor Cloud SQL samples #3952

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

shubha-rajan merged 6 commits into GoogleCloudPlatform:master from SaketramDurbha:cloudsql-refactor

Jun 5, 2020

Contributor

SaketramDurbha commented Jun 3, 2020

Rename environment variable DB_SOCKET_PATH to DB_SOCKET_DIR for better description of its contents
Move region tags and refactor code so as to give users a better sample experience

SaketramDurbha requested a review from a team as a code owner

June 3, 2020 21:56

blunderbuss-gcf bot assigned busunkim96

googlebot added the cla: yes label

SaketramDurbha force-pushed the cloudsql-refactor branch from 6ddf64b to 783505e Compare

June 3, 2020 22:00

kurtisvg assigned kurtisvg and unassigned busunkim96

kurtisvg requested a review from shubha-rajan

June 3, 2020 22:00

SaketramDurbha force-pushed the cloudsql-refactor branch from 783505e to 055a202 Compare

June 3, 2020 22:00

kurtisvg suggested changes

View reviewed changes

cloud-sql/mysql/sqlalchemy/main.py Outdated

Comment on lines 102 to 104

    
                  db_user = os.environ.get("DB_USER")

                  db_pass = os.environ.get("DB_PASS")

                  db_name = os.environ.get("DB_NAME")

Contributor

kurtisvg Jun 3, 2020

Let's make these required, since the application will fail without them:

Suggested change

      
                db_user = os.environ.get("DB_USER")
          
                db_pass = os.environ.get("DB_PASS")
          
                db_name = os.environ.get("DB_NAME")
          
                db_user = os.environ["DB_USER"]
          
                db_pass = os.environ["DB_PASS"]
          
                db_name = os.environ["DB_NAME"]

Contributor Author

SaketramDurbha Jun 3, 2020

Thanks. I'm making all the env vars except the "DB_SOCKET_DIR" required. Including "DB_HOST" for TCP connections and "CLOUD_SQL_CONNECTION_NAME" for Unix connections.

cloud-sql/mysql/sqlalchemy/main.py

    
                  db_socket_dir = os.environ.get("DB_SOCKET_DIR", "/cloudsql")

                  cloud_sql_connection_name = os.environ.get("CLOUD_SQL_CONNECTION_NAME")

                  pool = sqlalchemy.create_engine(

                      # Equivalent URL:

                      # mysql+pymysql://<db_user>:<db_pass>@/<db_name>?unix_socket=<socket_path>/<cloud_sql_instance_name>

                      sqlalchemy.engine.url.URL(

Contributor

kurtisvg Jun 3, 2020

Can we add init examples next to these values?

username=db_user, # e.g. "my-database-user"

kurtisvg approved these changes

View reviewed changes

cloud-sql/mysql/sqlalchemy/main.py Outdated

Comment on lines 73 to 75

    
                  # Extract host and port from db_host socket address

                  db_hostname = db_host_parts[0]

                  db_port = int(db_host_parts[1])

Contributor

kurtisvg Jun 3, 2020

Can we condense this a bit?

Suggested change

      
                # Extract host and port from db_host socket address
          
                db_hostname = db_host_parts[0]
          
                db_port = int(db_host_parts[1])
          
                # Extract host and port from db_host socket address
          
                host_args = db_host.split(":")
          
                db_hostname, db_port = host_args[0], int(host_args[1])

shubha-rajan approved these changes

View reviewed changes

Contributor

shubha-rajan commented Jun 3, 2020

Approved! @SaketramDurbha once you address Kurtis's comments, let me know and I'll merge it

kurtisvg suggested changes

View reviewed changes

Contributor

kurtisvg left a comment

Additional thought - do we need to update sqlserver as well?

Contributor

shubha-rajan commented Jun 3, 2020

sqlserver has a hard-coded string for the host. We might want to change it to an env var for consistency's sake but I think the hard coded version might be cleaner. We're not using the presence of DB_HOST to decide between connection methods there

shubha-rajan reviewed

View reviewed changes

cloud-sql/mysql/sqlalchemy/README.md Outdated

    
            @@ -52,6 +41,8 @@ export DB_USER='<DB_USER_NAME>'
          
              export DB_PASS='<DB_PASSWORD>'

              export DB_NAME='<DB_NAME>'

              ```

              Note: Saving credentials in environment variables is convenient, but not secure - consider a more

              secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.

Contributor

shubha-rajan Jun 5, 2020

Suggested change

      
            secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.
          
            secure solution such as [Secret Manager](https://cloud.google.com/secret-manager/docs/overview) to help keep secrets safe.

cloud-sql/mysql/sqlalchemy/README.md Outdated

    
            @@ -67,6 +58,8 @@ $env:DB_USER="<DB_USER_NAME>"
          
              $env:DB_PASS="<DB_PASSWORD>"

              $env:DB_NAME="<DB_NAME>"

              ```

              Note: Saving credentials in environment variables is convenient, but not secure - consider a more

              secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.

Contributor

shubha-rajan Jun 5, 2020

Suggested change

      
            secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.
          
            secure solution such as [Secret Manager](https://cloud.google.com/secret-manager/docs/overview) to help keep secrets safe.

cloud-sql/mysql/sqlalchemy/README.md Outdated

    
            @@ -98,10 +91,12 @@ export DB_USER='<DB_USER_NAME>'
          
              export DB_PASS='<DB_PASSWORD>'

              export DB_NAME='<DB_NAME>'

              ```

              Note: Saving credentials in environment variables is convenient, but not secure - consider a more

              secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.

Contributor

shubha-rajan Jun 5, 2020

Suggested change

      
            secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.
          
            secure solution such as [Secret Manager](https://cloud.google.com/secret-manager/docs/overview) to help keep secrets safe.

cloud-sql/postgres/sqlalchemy/README.md Outdated

    
            @@ -51,6 +40,8 @@ export DB_USER='<DB_USER_NAME>'
          
              export DB_PASS='<DB_PASSWORD>'

              export DB_NAME='<DB_NAME>'

              ```

              Note: Saving credentials in environment variables is convenient, but not secure - consider a more

              secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.

Contributor

shubha-rajan Jun 5, 2020

Suggested change

      
            secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.
          
            secure solution such as [Secret Manager](https://cloud.google.com/secret-manager/docs/overview) to help keep secrets safe.

cloud-sql/postgres/sqlalchemy/README.md Outdated

    
            @@ -66,6 +57,8 @@ $env:DB_USER="<DB_USER_NAME>"
          
              $env:DB_PASS="<DB_PASSWORD>"

              $env:DB_NAME="<DB_NAME>"

              ```

              Note: Saving credentials in environment variables is convenient, but not secure - consider a more

              secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.

Contributor

shubha-rajan Jun 5, 2020

Suggested change

      
            secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.
          
            secure solution such as [Secret Manager](https://cloud.google.com/secret-manager/docs/overview) to help keep secrets safe.

cloud-sql/postgres/sqlalchemy/README.md Outdated

    
            @@ -96,10 +89,12 @@ export DB_USER='<DB_USER_NAME>'
          
              export DB_PASS='<DB_PASSWORD>'

              export DB_NAME='<DB_NAME>'

              ```

              Note: Saving credentials in environment variables is convenient, but not secure - consider a more

              secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.

Contributor

shubha-rajan Jun 5, 2020

Suggested change

      
            secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.
          
            secure solution such as [Secret Manager](https://cloud.google.com/secret-manager/docs/overview) to help keep secrets safe.

cloud-sql/sql-server/sqlalchemy/README.md Outdated

    
              $env:DB_HOST="127.0.0.1:1433"

              ```

              Note: Saving credentials in environment variables is convenient, but not secure - consider a more

              secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.

Contributor

shubha-rajan Jun 5, 2020

Suggested change

      
            secure solution such as [Cloud KMS](https://cloud.google.com/kms/) to help keep secrets safe.
          
            secure solution such as [Secret Manager](https://cloud.google.com/secret-manager/docs/overview) to help keep secrets safe.

SaketramDurbha changed the title ~~refactor Cloud SQL PostgreSQL & MySQL samples~~ refactor Cloud SQL samples

SaketramDurbha requested review from shubha-rajan and kurtisvg

June 5, 2020 17:01

kurtisvg approved these changes

View reviewed changes

shubha-rajan approved these changes

View reviewed changes

shubha-rajan added the kokoro:run label

kurtisvg added kokoro:force-run and removed kokoro:run labels

kokoro-team removed the kokoro:force-run label

Contributor

shubha-rajan commented Jun 5, 2020

@SaketramDurbha looks like the lint check is failing and it has to do with an unused variable.

SaketramDurbha requested review from sirtorry and telpirion as code owners

June 5, 2020 19:23

googlebot commented Jun 5, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to determine that you authored the commits in this PR. Maybe you used a different email address in the git commits than was used to sign the CLA? If someone else authored these commits, then please add them to this pull request and have them confirm that they're okay with them being contributed to Google. If there are co-authors, make sure they're formatted properly.

In order to pass this check, please resolve this problem and then comment@googlebot I fixed it... If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot added cla: no and removed cla: yes labels

Saketram Durbha added 6 commits

June 5, 2020 15:28


          refactor Cloud SQL PostgreSQL & MySQL samples

e5aa603

* Rename environment variable DB_SOCKET_PATH to  DB_SOCKET_DIR for better description of its contents
* Move region tags and refactor code so as to give users a better sample experience


          minor code style and performance updates for Cloud SQL PostgreSQL and…

b4bbea0

… MySQL samples

* make necessary environment variables such as "DB_USER" required
* add comments to show examples for some input arguments
* rewrite small piece of code to be more condensed and concise


          refactor Cloud SQL SQL Server samples

9fa6f6e

* use environment varialbe for proxy connection hostname over hardcoding it
* make all environment variables required
* include instructions in README for running locally on Windows
* make code structure consistent with PostgreSQL and MySQL samples


          fix Cloud SQL MySQL & PostgreSQL READMEs to remove unneecessary instr…

17298c5

…uctions


          replace references in Cloud SQL samples to Key Management Service to …

cf283cf

…Secret Manager


          fix typo in Cloud SQL Postgres sample and refactor SQL Server sample …

734c833

…for consistency across Cloud SQL samples

SaketramDurbha force-pushed the cloudsql-refactor branch from 6a2d27e to 734c833 Compare

June 5, 2020 19:30

googlebot commented Jun 5, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot added cla: yes and removed cla: no labels

shubha-rajan removed request for telpirion and sirtorry

June 5, 2020 19:31

shubha-rajan added the kokoro:force-run label

kokoro-team removed the kokoro:force-run label

shubha-rajan merged commit 86080b8 into GoogleCloudPlatform:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels