docs(samples): create connection sample for MySQL instance #147
Conversation
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
product-auto-label
bot
added
the
api: bigqueryconnection
loferris
changed the title
product-auto-label
bot
added
the
samples
| from google.cloud import bigquery_connection_v1 as bq_connection | ||
| from google.cloud.bigquery_connection_v1 import types as connection_types | ||
|
|
||
| """This sample shows how to create a BigQuery connection with a Cloud MySql database""" |
There was a problem hiding this comment.
resolved in recent commit!
| # [START_EXCLUDE] | ||
| original_project_id = project_id | ||
| original_location = location | ||
| original_connection_id = connection_id | ||
| original_database = database | ||
| original_instance = instance | ||
| original_instance_location = instance_location | ||
| original_username = username | ||
| original_password = password | ||
| # [END_EXCLUDE] |
There was a problem hiding this comment.
What's up with these excludes and reassignments? Can we just leave this as is so the suer can edit it, and test the behavior of the function below?
There was a problem hiding this comment.
This is a pattern we've been following recently - I believe the purpose is to allow the function to be "live" and testable while still giving a clear view to the developer on how to fill in their own values. To me I think it makes more sense to have developer values set as statics above the function itself, but I'm not sure if that's consistent with what we've been doing. @tswast may have an opinion!
There was a problem hiding this comment.
The EXCLUDE pattern is probably not necessary if the snippet includes the whole function definition, as you've done here. Using default values there would serve the same purpose.
There was a problem hiding this comment.
Also, I tend to avoid EXCLUDE even with this pattern, as it shows "..." when rendered in the docs.
There was a problem hiding this comment.
https://github.com/googleapis/python-datacatalog/blob/main/samples/snippets/data_catalog_ptm_create_taxonomy.py is a more typical example.
In core BigQuery we have a lot of samples that have the tags within the function, so we've kept with that style.
There was a problem hiding this comment.
Slight update -- I've started to use the more standard style in some BigQuery samples: googleapis/python-bigquery#1137
There was a problem hiding this comment.
Attempted in recent commit! Would love feedback!
| cloud_sql_credential = bq_connection.CloudSqlCredential() | ||
| cloud_sql_credential.username = username | ||
| cloud_sql_credential.password = password | ||
| cloud_sql_properties = bq_connection.CloudSqlProperties() | ||
| cloud_sql_properties.type_ = bq_connection.CloudSqlProperties.DatabaseType.MYSQL | ||
| cloud_sql_properties.database = database | ||
| cloud_sql_properties.instance_id = instance_id | ||
| cloud_sql_properties.credential = cloud_sql_credential | ||
| connection = connection_types.Connection() | ||
| connection.cloud_sql = cloud_sql_properties | ||
| create_connection(project_id, location, connection_id, connection) |
There was a problem hiding this comment.
This is all stuff that should happen in the sample function below.
nit: you may also want to consider using the mapping value here for these protos e.g.:
cloud_sql_credential = bq_connection.CloudSqlCredential({
"username": username,
"password": password
})There was a problem hiding this comment.
resolved in recent commit!
| return connection | ||
|
|
||
|
|
||
| def create_connection( |
There was a problem hiding this comment.
nit: should it be "create_mysql_connection"?
There was a problem hiding this comment.
resolved in recent commit!
| connection = connection_types.Connection() | ||
| connection.cloud_sql = cloud_sql_properties | ||
| create_connection(project_id, location, connection_id, connection) | ||
| return connection |
There was a problem hiding this comment.
Ideally we avoid return types in the function to force the sample to interact with the created object
There was a problem hiding this comment.
resolved in recent commit!
| # [START_EXCLUDE] | ||
| original_project_id = project_id | ||
| original_location = location | ||
| original_connection_id = connection_id | ||
| original_database = database | ||
| original_instance = instance | ||
| original_instance_location = instance_location | ||
| original_username = username | ||
| original_password = password | ||
| # [END_EXCLUDE] |
There was a problem hiding this comment.
Slight update -- I've started to use the more standard style in some BigQuery samples: googleapis/python-bigquery#1137
parthea
changed the title
…on into feat_create_connection updating branch
…-bigquery-connection into feat_create_connection updating local
…-bigquery-connection into feat_create_connection merge conflicts
|
@kurtisvg All comments responded to! |
| # TODO(developer): Set project_id to the project ID containing the | ||
| # connection. | ||
| project_id = "your-project-id" | ||
| # TODO(developer): Set location to the location of the connection. | ||
| # See: https://cloud.google.com/bigquery/docs/locations for a list of | ||
| # available locations. | ||
| location = "US" | ||
| # TODO(developer): Set database to the name of the database in which you're creating the connection. | ||
| database = "my-database" | ||
| # TODO(developer): Set instance to the instance where you're creating the connection. | ||
| instance_name = "my-instance" | ||
| # TODO(developer): Set instance_location to the location of the instance where you are creating the connection. | ||
| instance_location = "my-instance-location" | ||
| # TODO(developer): Set username to the database username. | ||
| username = "my-username" | ||
| # TODO(developer): Set password to the database password. | ||
| password = "my-password" |
There was a problem hiding this comment.
nit: I would try to use just 1 TODO to represent all of these inputs to reduce the vertical space this takes up. If you feel like it needs additional context, try to leave a short comment in the follow up. E.g.:
username = "my-username" # username for your database
password = "my-password" # password for your database user| username = "my-username" | ||
| # TODO(developer): Set password to the database password. | ||
| password = "my-password" | ||
| cloud_sql_conn_name = f"{project_id}:{instance_location}:{instance_name}" |
There was a problem hiding this comment.
I don't think you need to have the user input these as 3 different fields. You can just ask them for the "instance connection name". This is a specific field in the instance metadata and is shown to the user in console, e.g.:
For reference, this is what we do in Cloud SQL samples:
…-bigquery-connection into feat_create_connection merge conflicts
loferris
requested review from
a team
and removed request for
stephaniewang526 and
tswast
…-bigquery-connection into feat_create_connection merge conflicts
loferris
added
kokoro:run
yoshi-kokoro
removed
the
kokoro:force-run
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: