Uh oh!
There was an error while loading. Please reload this page.
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.
|
| 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!
tswast
left a comment
There was a problem hiding this comment.
+1 to Kurtis's requested changes.
| # [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
…on into feat_create_connection updating branch
…-bigquery-connection into feat_create_connection updating local
…-bigquery-connection into feat_create_connection merge conflicts
loferris
commented
Apr 6, 2022
@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 databasepassword="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
…-bigquery-connection into feat_create_connection merge conflicts
shollyman
left a comment
There was a problem hiding this comment.
I need to pay more attention to how we're dealing with test resource setup/lifecycle, but that is outside the scope of this PR.
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: