-
Notifications
You must be signed in to change notification settings - Fork 455
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
Hello World tutorial for Go pgx #7698
Conversation
@vy-ton FYI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I don't know anything about that ServerName
config option.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @ericharmeling)
_includes/v20.1/app/txn-sample-pgx.go, line 15 at r1 (raw file):
// Read the balance. var fromBalance int if err := tx.QueryRow(context.Background(),
nit: instead of context.Background()
everywhere, make transferFunds()
take in a ctx
, and have the caller pass it the background ctx.
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 16 at r1 (raw file):
This tutorial shows you how build a simple Go application with CockroachDB and the Go pq driver. We have tested the [Go pq driver](https://godoc.org/github.com/lib/pq) enough to claim **beta-level** support. If you encounter problems, please [open an issue](https://github.com/cockroachdb/cockroach/issues/new) with details to help us make progress toward full support.
What's this "beta-level" about? It's the driver we tested most of them all, isn't it. Although it has major bugs like lib/pq#939, but they're not crdb-specific.
6020cc0
to
4b465c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR, Andrei!
@rafiss
Rafi, is setting the ServerName
option a known requirement for secure apps using crdbpgx?
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)
_includes/v20.1/app/txn-sample-pgx.go, line 15 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
nit: instead of
context.Background()
everywhere, maketransferFunds()
take in actx
, and have the caller pass it the background ctx.
Done for transferFunds()
.
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 16 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
What's this "beta-level" about? It's the driver we tested most of them all, isn't it. Although it has major bugs like lib/pq#939, but they're not crdb-specific.
Removed from lib/pq tutorial.
The It looks like |
log.Fatal("error configuring the database: ", err) | ||
} | ||
|
||
config.TLSConfig.ServerName = "localhost" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well actually, I wouldn't expect ServerName to be required here (in the insecure example). does this fail without specifying ServerName
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. In the insecure example, it doesn't fail. I'll remove that line from the insecure example.
@lnhsingh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall, ! nice job. added a few small edits / suggestions.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @ericharmeling, @lnhsingh, and @rafiss)
v20.1/build-a-go-app-with-cockroachdb.md, line 78 at r3 (raw file):
{% include copy-clipboard.html %} ~~~ shell go mod init basic-sample-pgx
Add $
at the start
v20.1/build-a-go-app-with-cockroachdb.md, line 170 at r3 (raw file):
{% include copy-clipboard.html %} ~~~ shell go mod init basic-sample-pgx
Add $
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 37 at r3 (raw file):
## Step 3. Generate a certificate for the `maxroach` user Create a certificate and key for the `maxroach` user by running the following command. The code samples will run as this user.
Suggestion: add colon after ...running the following command:
and put The code samples will run as this user.
after the code block
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 49 at r3 (raw file):
{{site.data.alerts.callout_success}} To clone a version of the code below that connects to insecure clusters, run the command below. Note that you will need to edit the connection string to use the certificates that you generated when you set up your secure cluster.
Suggested edit:
To clone a version of the code below that connects to insecure clusters, run the following:
git clone https://github.com/cockroachlabs/hello-world-go-pq/
Note that you will need to edit the connection string to use the certificates that you generated when you set up your secure cluster.
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 91 at r3 (raw file):
~~~ CockroachDB may require the [client to retry a transaction](transactions.html#transaction-retries) in case of read/write contention. CockroachDB provides a generic **retry function** that runs inside a transaction and retries it as needed. For Go, the CockroachDB retry function is in the `crdb` package of the CockroachDB Go client. To install Clone the library into your `$GOPATH` as follows:
Edit for last sentence: To install, clone the library...
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 118 at r3 (raw file):
~~~ shell Success
This is rendering with a $
at the front, maybe remove the shell
tag?
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 129 at r3 (raw file):
~~~ +----+---------+
nit: this table format looks different than what displays (shouldn't have outer pipes or first / last row of +--+
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 224 at r3 (raw file):
~~~ +----+---------+ | id | balance |
Also the format on this table
e85dd25
to
542d083
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR @lnhsingh !
I copied a lot of your suggestions from the lib/pq over to the PGX tutorial, and then copied all changes into 19.2 and 20.2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @lnhsingh, and @rafiss)
v20.1/build-a-go-app-with-cockroachdb.md, line 78 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
Add
$
at the start
Done.
v20.1/build-a-go-app-with-cockroachdb.md, line 170 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
Add
$
Done.
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 37 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
Suggestion: add colon after
...running the following command:
and putThe code samples will run as this user.
after the code block
Done.
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 49 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
Suggested edit:
To clone a version of the code below that connects to insecure clusters, run the following:
git clone https://github.com/cockroachlabs/hello-world-go-pq/
Note that you will need to edit the connection string to use the certificates that you generated when you set up your secure cluster.
Done.
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 91 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
Edit for last sentence:
To install, clone the library...
Done.
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 118 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
This is rendering with a
$
at the front, maybe remove theshell
tag?
Done.
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 129 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
nit: this table format looks different than what displays (shouldn't have outer pipes or first / last row of
+--+
Done.
v20.1/build-a-go-app-with-cockroachdb-pq.md, line 224 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
Also the format on this table
Done.
Fixes #7185.
Fixes #7011.
Related to #6866.
This PR includes the following changes:
build-a-go-app-with-cockroachdb.html
tobuild-a-go-app-with-cockroachdb-pq.html
.build-a-go-app-with-cockroachdb.html
. This tutorial instructs users to set up and run a simple app that uses pgx to connect to CRDB. It is now the default Go tutorial.drivers.md
andtooling.md
include files to list pgx at full support, and link to new tutorial.These changes have only been made to the v20.1 docs. After this PR has been reviewed and the changes finalized, I'll make copies for v19.2 and v20.2.
Separately, I created the hello-world-go-pgx repository for a single-file version of the code samples. Before merging this PR, I'll make that repo public.
@andreimatei
Adding you for technical review, Andrei. As you can see, I adapted the lib/pq code samples (https://github.com/cockroachdb/docs/blob/master/_includes/v20.1/app/basic-sample.go and https://github.com/cockroachdb/docs/blob/master/_includes/v20.1/app/txn-sample.go) to use pgx.
Note that I had to explicitly define
tls.Config.ServerName
in the connection configuration. Without this defined explicitly, the following error was reported for the secure sample:This step wasn't documented at https://pkg.go.dev/github.com/jackc/pgx?tab=doc or https://pkg.go.dev/github.com/cockroachdb/[email protected]+incompatible/crdb/crdbpgx?tab=doc, so it could be some kind of bug? Or I could have missed something?