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

docs: reorganize documentation to split driver-specific info into separate files #409

Merged
merged 18 commits into from
Feb 26, 2021

Conversation

shubha-rajan
Copy link
Contributor

@shubha-rajan shubha-rajan commented Feb 19, 2021

Change Description

Reorganized docs to make information more organized and easier to navigate. Also updated the version updater script to find and replace versions in the new files instead of the main README and rewrote CONTRIBUTING.md for consistency with corresponding doc in Cloud SQL Proxy repo.

Checklist

  • Make sure to open an issue as a
    bug/issue
    before writing your code! That way we can discuss the change, evaluate
    designs, and agree on the general idea.
  • Ensure the tests and linter pass
  • Appropriate documentation is updated (if necessary)

Relevant issues:

README.md Outdated
Comment on lines 12 to 17
* [Connecting to MySQL using JDBC](docs/jdbc-mysql)
* [Connecting to Postgres using JDBC](docs/jdbc-postgres.md)
* [Connecting to SQL Server using JDBC](docs/jdbc-sqlserver.md)
* [Connecting to MySQL using R2DBC](docs/r2dbc-mysql.md)
* [Connecting to Postgres using R2DBC](docs/r2dbc-postgres.md)
* [Connecting to SQL Server using R2DBC](docs/r2dbc-sqlserver.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about separating this into a list for JDBC and a list for R2DBC? (It feels hard to skim since most of the letters are similar)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

README.md Outdated
## Authentication

This library uses the [Application Default Credentials](
The Cloud SQL Connector for Java is a library for the MySQL/PostgreSQL/SQL Server JDBC and R2DBC drivers that allows a user
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to work shop the description we are using for connectors. See my comment here: GoogleCloudPlatform/cloud-sql-proxy#621 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this to look more like your suggestion in that comment. Let me know if there's anything else I need to change.

README.md Outdated
```
jdbc:mysql:///<DATABASE_NAME>?unixSocketPath=</PATH/TO/UNIX/SOCKET>&cloudSqlInstance=<INSTANCE_CONNECTION_NAME>&socketFactory=com.google.cloud.sql.mysql.SocketFactory&user=<MYSQL_USER_NAME>&password=<MYSQL_USER_PASSWORD>
```
For examples of this library being used in the context of an application, check out the sample applications located
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest moving this line up into the usage section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

compile 'com.google.cloud.sql:mysql-socket-factory-connector-j-8:1.2.1'
```

### Create the JDBC URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Create the JDBC URL
### Creating the JDBC URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


## Setup and Usage

### Add library as a dependency
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Add library as a dependency
### Adding the library as a dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

### Specifying IP Types

The `ipTypes` argument can be used to specify a comma delimited list of preferred IP types for
connecting to a Cloud SQL instance. The argument `ipTypes=PRIVATE` will force the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the phrasing here could be improved. Maybe something like:

"The `ipTypes` argument is used to specify a preferred order of IP types used to connect via a comma delimited list. For example, `ipTypes=PUBLIC,PRIVATE` will use the instance's Public IP if it exists, otherwise private. The value `ipTypes=PRIVATE` will force the Cloud SQL instance to connect via it's private IP. If not specified, the default used is `ipTypes=PUBLIC,PRIVATE`.

Also please add a link to the private IP requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

README.md Outdated

## Examples
JDBC:
* [Connecting to MySQL using JDBC](docs/jdbc-mysql)
Copy link
Contributor

Choose a reason for hiding this comment

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

.md?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason we're using the docs folder? Would it be better to put in the README for the artifact itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for consistency with the documentation structure of the proxy repo. We can do artifact READMEs instead if you think that's better.

core/pom.xml Outdated
@@ -10,7 +10,7 @@
<artifactId>jdbc-socket-factory-core</artifactId>
<packaging>jar</packaging>

<name>Cloud SQL MySQL Socket Factory (Core Library, don't depend on this directly)</name>
<name>Cloud SQL Core Socket Factory (Core Library, don't depend on this directly)</name>
<description>
Core library for establishing secure connections. End users should depend on the the
mysql-socket-factory
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch - can you update the description as well?


### Specifying IP Types

"The `ipTypes` argument is used to specify a preferred order of IP types used to connect via a comma delimited list. For example, `ipTypes=PUBLIC,PRIVATE` will use the instance's Public IP if it exists, otherwise private. The value `ipTypes=PRIVATE` will force the Cloud SQL instance to connect via it's private IP. If not specified, the default used is `ipTypes=PUBLIC,PRIVATE`. For more info on connecting using a private IP address, see [Requirements for Private IP](https://cloud.google.com/sql/docs/mysql/private-ip#requirements_for_private_ip).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd suggest moving the last sentence into it's own paragraph.

jdbc:mysql:///<DATABASE_NAME>?unixSocketPath=</PATH/TO/UNIX/SOCKET>&cloudSqlInstance=<INSTANCE_CONNECTION_NAME>&socketFactory=com.google.cloud.sql.mysql.SocketFactory&user=<MYSQL_USER_NAME>&password=<MYSQL_USER_PASSWORD>
```

## Examples
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point we discussed referencing the Serverless docs for GAE, etc. Do we still want to do that somewhere?

shubha-rajan and others added 2 commits February 22, 2021 10:29
Co-authored-by: Kurtis Van Gent <31518063+kurtisvg@users.noreply.github.com>
Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

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

Looks great!

@@ -20,5 +20,5 @@ mvn versions:set -DnewVersion=$VERSION -DgenerateBackupPoms=false
# Update versions in README if not snapshot release
if ! [[ $VERSION =~ .*SNAPSHOT ]]
then
sed -Ei '' 's/[0-9]+\.[0-9]+\.[0-9]+/'"$VERSION"'/g' README.md
sed -Ei '' 's/[0-9]+\.[0-9]+\.[0-9]+/'"$VERSION"'/g' docs/*.md
fi
Copy link
Member

Choose a reason for hiding this comment

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

Nit: There should be a newline terminating the file.

The Cloud SQL Connector for Java is a library for the MySQL/PostgreSQL JDBC and R2DBC drivers that allows a user
with the appropriate permissions to connect to a Cloud SQL database without having to deal with IP
whitelisting or SSL certificates manually.
![CI Java 8][ci-badge-java-8]
Copy link
Member

Choose a reason for hiding this comment

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

Is there a link to the continuous build available to the public? It might be nice to add it here if so. Ditto with the line below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that there is. The only one I'm aware of is the one on Fusion.


## Examples
JDBC:
* [Connecting to MySQL using JDBC](docs/jdbc-mysql.md)
Copy link
Member

Choose a reason for hiding this comment

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

This is a fantastic improvement!

Copy link
Contributor

@kurtisvg kurtisvg left a comment

Choose a reason for hiding this comment

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

This feels way more organized than before. Thank you!

@@ -0,0 +1,77 @@
# Connecting to MySQL using JDBC
Copy link
Contributor

Choose a reason for hiding this comment

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

When we reshuffle the modules, I think we should revisit keeping these in the docs folder or with the module.

@shubha-rajan shubha-rajan merged commit 453e04d into main Feb 26, 2021
@shubha-rajan shubha-rajan deleted the rewrite-docs branch February 26, 2021 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve documentation about possible authentication options and required roles, if any?
3 participants