-
Notifications
You must be signed in to change notification settings - Fork 451
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
Add language-specific instructions to the "Connect to the Cluster" doc #6077
Conversation
c741cad
to
6fe8f33
Compare
@jseldess @mjibson I need help with the formatting on this page: What I want to do is make the "Python" section be pre-clicked and expanded by default. But as of now, the user needs to click one of the languages to expand the sections. My research says that the reason is that the previous section ("Use the CockroachDB client") also has section selectors, and only one section selector can be "active" at a time on a page. Is this something you have seen before? Any ideas on how to fix this? |
Yes, I think you’re right. We might be able to trick things by using divs
for the Mac/Linux toggles and sections for the language toggles. If that
doesn’t work, I’d just remove the Mac/Linux toggles and replace those with
bullets. I can track down an example or two of you need.
On Mon, Dec 2, 2019 at 8:20 PM Amruta Ranade ***@***.***> wrote:
@jseldess <https://github.com/jseldess> @mjibson
<https://github.com/mjibson> I need help with the formatting on this page:
http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/a430755e5838c1703c0bb26db7ba8d735aa70cc9/cockroachcloud/stable/cockroachcloud-connect-to-your-cluster.html#use-a-postgres-driver-or-orm
What I want to do is make the "Python" section be pre-clicked and expanded
by default. But as of now, the user needs to click one of the languages to
expand the sections. My research says that the reason is that the previous
section ("Use the CockroachDB client") also has section selectors, and only
one section selector can be "active" at a time on a page. Is this something
you have seen before? Any ideas on how to fix this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6077?email_source=notifications&email_token=ADZIH4TOLA77BWFNDTJMV5DQWWX55A5CNFSM4JR6R2OKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFXXYWA#issuecomment-560954456>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZIH4TN3EIRN2WRTL7W46LQWWX55ANCNFSM4JR6R2OA>
.
--
Jesse Seldess
VP of Education, Cockroach Labs
https://www.cockroachlabs.com/
|
Hmm..the divs/sections trick didn't work. I like the idea of replacing the Mac/Linux toggles with bullet points. Going with it for now. Thanks, Jesse! |
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.
Nice progress, @Amruta-Ranade. I'm a bit concerned about providing just the connection string code fragment without explicitly mentioning prerequisites, like installing the driver or ORM. I'm still wondering whether it will be more helpful to CC devs to have a full "hello world" sample from which they can easily extract the details they need. Thoughts?
If you prefer to move forward with this approach for now, I'd like you to have a developer run through this for real, pretending to be a beginner, before I give my review.
Some other issues issues:
- Reading through, it's strange to me that step 3 (generating the connection string) is not in the context of step 4 (connect to the cluster). You need to choose specific options in step 3 for step 4. Can you think about this more?
- I think all of the bullets nested under number steps are not rendering correctly.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jseldess)
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.
Everything worked as expected!
(also worth noting that this is the first time I've connected to one of my dbs)
For the "Use a Postgres driver or ORM" step I followed the "Node.js pg driver" instructions to test it out. The actual code I used:
// connect-test.js
const fs = require("fs");
const { Client } = require("pg");
// copied config from tutorial and added password
const config = {...};
const client = new Client(config);
client.connect();
console.log("connected");
client.query("SELECT NOW()", (err, res) => {
console.log(err, res);
client.end();
console.log("disconnected");
});
@@ -94,7 +94,7 @@ On the machine from which you want to connect to your cluster: | |||
|
|||
The **Connect** tab is displayed. | |||
|
|||
<img src="{{ 'images/v19.1/cockroachcloud/connect-tab.png' | relative_url }}" alt="Connect to cluster" style="border:1px solid #eee;max-width:100%" /> | |||
<img src="{{ 'images/v19.2/cockroachcloud/connect-tab.png' | relative_url }}" alt="Connect to cluster" style="border:1px solid #eee;max-width:50%" /> | |||
|
|||
6. Select a connection option: |
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.
The order of the tabs on the connection modal has recently been updated (CockroachDB Client is the default now).
// Connect to the "bank" database. | ||
var config = { | ||
user: '<username>', | ||
host: '<region>.<cluster_name>', |
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.
I think my host was <cluster_name>.<region>
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.
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.
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.
Woah..that's weird! Let's ask on Slack if this is an issue or if it's okay to have two formats.
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.
Confirmed: Both are correct, but differ based on version. <host>
might be the safest way to go.
~~~ js | ||
// Connect to CockroachDB through Sequelize. | ||
var sequelize = new Sequelize('<database_name>', '<username>', '<password>', { | ||
host: '<region>.<cluster_name>', |
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.
Same here --<cluster_name>.<region>
conn = psycopg2.connect( | ||
user='<username>', | ||
password='<password>', | ||
host='<region>.<cluster_name>', |
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.
Same here --<cluster_name>.<region>
On the machine where you want to run your application: | ||
# Create an engine to communicate with the database. The "cockroachdb://" prefix | ||
# for the engine URL indicates that we are connecting to CockroachDB. | ||
engine = create_engine('cockroachdb://<username>:<password>@<region>.<cluster_name>:26257/<database>?sslmode=verify-full&sslrootcert=<absolute path to CA certificate>') |
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.
Same here --<cluster_name>.<region>
~~~ go | ||
|
||
// Connect to the database. | ||
const addr = "postgresql://<username>:<password>@<region>.<cluster_name>:26257/<database>?sslmode=verify-full&sslrootcert=<absolute path to CA certificate>" |
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.
Same here --<cluster_name>.<region>
2. Run the code: | ||
// Configure the database connection. | ||
PGSimpleDataSource ds = new PGSimpleDataSource(); | ||
ds.setServerName("<region>.<cluster_name>"); |
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.
Same here --<cluster_name>.<region>
//Database connection settings | ||
<property name="hibernate.connection.driver_class">org.postgresql.Driver</property> | ||
<property name="hibernate.dialect">org.hibernate.dialect.PostgreSQL95Dialect</property> | ||
<property name="hibernate.connection.url"><![CDATA[jdbc:postgresql://<username>:<password>@<region>.<cluster_name>:26257/<database>?sslmode=verify-full&sslrootcert=<absolute path to CA certificate]]></property> |
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.
Same here --<cluster_name>.<region>
{{site.data.alerts.callout_info}} | ||
The [CockroachDB Python package](https://github.com/cockroachdb/cockroachdb-python) installed earlier is triggered by the `cockroachdb://` prefix in the engine URL. Using `postgres://` to connect to your cluster will not work. | ||
{{site.data.alerts.end}} | ||
// Connect to the "bank" database. |
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.
What does "bank" database
mean?
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.
Ah..included it by mistake. Fixed.
The [CockroachDB Python package](https://github.com/cockroachdb/cockroachdb-python) installed earlier is triggered by the `cockroachdb://` prefix in the engine URL. Using `postgres://` to connect to your cluster will not work. | ||
{{site.data.alerts.end}} | ||
// Connect to the "bank" database. | ||
var config = { |
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.
I had to add password
to the config to get it to connect.
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.
Yep. Good catch. Fixed.
{% include copy-clipboard.html %} | ||
~~~ shell | ||
$ sudo cp -i cockroach-{{ page.release_info.version }}.linux-amd64/cockroach /usr/local/bin/ | ||
~~~ | ||
</section> | ||
|
||
3. Use the `cockroach sql` command to open an interactive SQL shell, replacing placeholders in the [client connection string](#step-3-generate-the-connection-string) with the correct username, password, and path to the `ca.cert`: |
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.
Forgot one. Looks like the connection strings below missed the certs/ca.crt
to <path to the CA certificate>
update.
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.
Fixed!
Excellent feedback @laurenbarker! Thank you so much! |
1 similar comment
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.
Sorry for the delay, @Amruta-Ranade. I didn't know you were ready for a re-review. In the future, please mention me in a comment once you're ready.
LGTM, with a few nits. I still find steps 3 and 4 a bit confusing, but let's go with this for now.
Also, you
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade and @laurenbarker)
v19.2/cockroachcloud-connect-to-your-cluster.md, line 37 at r8 (raw file):
5. From the **Network** dropdown, select: - **New Network** to authorize your application server’s network or application server’s network. Enter the public IPv4 address of the machine in the **Network** field.
These bullets aren't rendering properly. Please fix.
v19.2/cockroachcloud-connect-to-your-cluster.md, line 47 at r8 (raw file):
{{site.data.alerts.end}} If you need to add a range of IP addresses, use the CIDR (Classless Inter-Domain Routing) notation.
nit: If you need to add
> To add
. Also, no reason for this sentence to be its own paragraph. Please make it the first sentence of the following paragraph.
v19.2/cockroachcloud-connect-to-your-cluster.md, line 79 at r8 (raw file):
## Step 3. Select a connection method On the machine from which you want to connect to your cluster:
This intro doesn't make sense for all cases. For example, you might grab the connection string on your laptop but then use it in app code running on a server. Do you think this intro is necessary?
v19.2/cockroachcloud-connect-to-your-cluster.md, line 151 at r8 (raw file):
~~~ 3. Use the `cockroach sql` command to open an interactive SQL shell, replacing placeholders in the [client connection string](#step-3-generate-the-connection-string) with the correct username, password, and path to the `ca.cert`:
This anchor link is broken.
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.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade, @jseldess, and @laurenbarker)
v19.2/cockroachcloud-connect-to-your-cluster.md, line 99 at r5 (raw file):
Previously, laurenbarker (Lauren Barker) wrote…
The order of the tabs on the connection modal has recently been updated (CockroachDB Client is the default now).
Done.
v19.2/cockroachcloud-connect-to-your-cluster.md, line 37 at r8 (raw file):
Done.
v19.2/cockroachcloud-connect-to-your-cluster.md, line 79 at r8 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
This intro doesn't make sense for all cases. For example, you might grab the connection string on your laptop but then use it in app code running on a server. Do you think this intro is necessary?
Done.
v19.2/cockroachcloud-connect-to-your-cluster.md, line 151 at r8 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
This anchor link is broken.
Done.
Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/678074fb1e1beea76d038bc50a7841d30459c473/ Edited pages: |
Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/bf493488198a11fecb08b45f5831bf03ff162eca/ Edited pages: |
Addresses #5738
To-Do: Replicate changes to 20.1 docs