Secure app examples #5028
Secure app examples #5028
Conversation
|
This change is |
There was a problem hiding this comment.
Clojure parts LGTM! I left some annoying Lisp nerd comments but they are non-blocking. Examples tested and they work as expected. Docs pages read/work well.
I will review the C++ ASAP -- later today or first thing tomorrow (lots of meetings today, sorry!).
(PS For future work we should consider making the instructions a bit more idiomatic for Clojure users, esp. around lein usage and project layout. I filed https://github.com/cockroachdb/docs/issues/5031. But that's not in scope for this PR of course! Just FYI.)
|
Oh @ericharmeling one last thing. Would you mind squashing this before merging? It's a bit hard to read the history otherwise |
For sure! |
ericharmeling
force-pushed
the
eharm-secure-app-examples
branch
from
3a78b3c
to
8b9ccd1
Compare
There was a problem hiding this comment.
Awesome work, @ericharmeling! I left a few nits and suggestions. Once you've made final edits to all of these 19.1 files, please then port everything to the corresponding 19.2 files as part of this PR. At that point, will
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling, @jseldess, and @rmloveland)
_includes/v19.1/app/basic-sample.cpp, line 1 at r3 (raw file):
// Build with g++ -std=c++11 basic-sample.cpp -lpq -lpqxx
Shouldn't we make this a more explicit user action in the tutorial itself, like we do in the Closure and other language tutorials? I find leaving this as a comment a bit too subtle.
This particular issue is unrelated to your changes, but we might as well make this usability improvement while we're here.
_includes/v19.1/app/txn-sample.cpp, line 1 at r3 (raw file):
// Build with g++ -std=c++11 txn-sample.cpp -lpq -lpqxx
Same comment as above: Shouldn't we make this a more explicit user action in the tutorial itself, like we do in the Closure and other language tutorials? I find leaving this as a comment a bit too subtle.
This particular issue is unrelated to your changes, but we might as well make this usability improvement while we're here.
_includes/v19.1/app/insecure/basic-sample.cpp, line 1 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
Sounds good to me! I think there's a lot of work in general to be done on the application examples to make projects a little more realistic and language-specific (like what you did for the Java example). Makefiles, for example, for C++ and C (which don't yet exist...) examples.
Same comment as above: Shouldn't we make this a more explicit user action in the tutorial itself, like we do in the Closure and other language tutorials? I find leaving this as a comment a bit too subtle.
This particular issue is unrelated to your changes, but we might as well make this usability improvement while we're here.
_includes/v19.1/app/insecure/txn-sample.cpp, line 1 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
Right on.
Same comment as above: Shouldn't we make this a more explicit user action in the tutorial itself, like we do in the Closure and other language tutorials? I find leaving this as a comment a bit too subtle.
This particular issue is unrelated to your changes, but we might as well make this usability improvement while we're here.
_includes/v19.1/misc/external-urls.md, line 14 at r3 (raw file):
| S3-compatible services [<sup>5</sup>](#considerations) | `s3` | Bucket name | `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, `AWS_SESSION_TOKEN`, `AWS_REGION` [<sup>6</sup>](#considerations) (optional), `AWS_ENDPOINT` | {{site.data.alerts.callout_danger}}
This change seems unrelated. Are you resolving a separate issue in the same PR?
_includes/v19.2/misc/external-urls.md, line 14 at r3 (raw file):
| S3-compatible services [<sup>5</sup>](#considerations) | `s3` | Bucket name | `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, `AWS_SESSION_TOKEN`, `AWS_REGION` [<sup>6</sup>](#considerations) (optional), `AWS_ENDPOINT` | {{site.data.alerts.callout_danger}}
Same as above: This change seems unrelated. Are you resolving a separate issue in the same PR?
v19.1/build-a-c++-app-with-cockroachdb.md, line 78 at r3 (raw file):
~~~ +----+---------+
nit: Can you update this output? In recent versions of CockroachDB, the output formatting has changed.
v19.1/build-a-c++-app-with-cockroachdb.md, line 134 at r3 (raw file):
~~~ +----+---------+
nit: Can you update this output? In recent versions of CockroachDB, the output formatting has changed.
v19.1/build-a-clojure-app-with-cockroachdb.md, line 116 at r3 (raw file):
~~~ +----+---------+
nit: Can you update this output? In recent versions of CockroachDB, the output formatting has changed.
v19.1/build-a-clojure-app-with-cockroachdb.md, line 185 at r3 (raw file):
Copy the code below to `myapp/src/test/test.clj` or <a href="https://raw.githubusercontent.com/cockroachdb/docs/master/_includes/{{ page.version.version }}/app/insecure/txn-sample.clj" download>download it directly</a>. Again, preserve the file name `test.clj`. {{site.data.alerts.callout_info}}
Not causing any rendering issues, but would you mind adding a blank line before the start of this callout?
ericharmeling
force-pushed
the
eharm-secure-app-examples
branch
from
8b9ccd1
to
97d8715
Compare
There was a problem hiding this comment.
Reviewable status:
_includes/v19.1/app/basic-sample.clj, line 9 at r1 (raw file):
:user "maxroach" :password ""})
_includes/v19.1/app/basic-sample.clj, line 15 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
You're right... If you try to use the wrong key type (not the .pk8),
leinwith give you an error that you need to specify a password. I don't think this is worth mentioning though. I'm sure you ran into the same type of thing when you created the secure Java example.
Done.
_includes/v19.1/app/basic-sample.cpp, line 1 at r3 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Shouldn't we make this a more explicit user action in the tutorial itself, like we do in the Closure and other language tutorials? I find leaving this as a comment a bit too subtle.
This particular issue is unrelated to your changes, but we might as well make this usability improvement while we're here.
That makes sense! I will make this change for all four .cpp files.
_includes/v19.1/app/txn-sample.clj, line 6 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
I tried out using
:dbtypeand friends and they worked for the insecure example! I'll change this to make it standard across the insecure and secure examples.
Done.
_includes/v19.1/app/txn-sample.cpp, line 1 at r3 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Same comment as above: Shouldn't we make this a more explicit user action in the tutorial itself, like we do in the Closure and other language tutorials? I find leaving this as a comment a bit too subtle.
This particular issue is unrelated to your changes, but we might as well make this usability improvement while we're here.
Done.
_includes/v19.1/app/insecure/basic-sample.clj, line 26 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
Let's take it out then!
Done.
_includes/v19.1/app/insecure/basic-sample.cpp, line 1 at r1 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Same comment as above: Shouldn't we make this a more explicit user action in the tutorial itself, like we do in the Closure and other language tutorials? I find leaving this as a comment a bit too subtle.
This particular issue is unrelated to your changes, but we might as well make this usability improvement while we're here.
Done.
_includes/v19.1/app/insecure/txn-sample.clj, line 6 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
Agreed! I just opted for not touching the insecure examples, but I will update to standardize. Same for the C++ examples. Most of my syntax is based on some API reference or other than I found.
Done.
_includes/v19.1/app/insecure/txn-sample.clj, line 9 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
I ran into some "password required" errors, but I just ran through both examples again with the password parameter and everything worked. Must have been for the secure example when I was using the wrong type of key. I'll remove that parameter from both!
Done.
_includes/v19.1/app/insecure/txn-sample.cpp, line 1 at r1 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Same comment as above: Shouldn't we make this a more explicit user action in the tutorial itself, like we do in the Closure and other language tutorials? I find leaving this as a comment a bit too subtle.
This particular issue is unrelated to your changes, but we might as well make this usability improvement while we're here.
Done.
_includes/v19.1/misc/external-urls.md, line 14 at r3 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
This change seems unrelated. Are you resolving a separate issue in the same PR?
You're totally right. I pulled these into the PR on accident... I can remove them.
_includes/v19.2/misc/external-urls.md, line 14 at r3 (raw file):
You're totally right. I pulled these into the PR on accident... I can remove them.
v19.1/build-a-c++-app-with-cockroachdb.md, line 22 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
Yes, I will do that. I installed 4.0.0 for some reason(?), and it didn't have this mac-specific bug fixed yet: Homebrew/legacy-homebrew#23627. Your language is more accurate.
Done.
v19.1/build-a-c++-app-with-cockroachdb.md, line 78 at r3 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
nit: Can you update this output? In recent versions of CockroachDB, the output formatting has changed.
Sure thing! I'll make this change to all
v19.1/build-a-clojure-app-with-cockroachdb.md, line 199 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
Ah OK. I will add those back in.
Done.
v19.1/build-a-clojure-app-with-cockroachdb.md, line 185 at r3 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Not causing any rendering issues, but would you mind adding a blank line before the start of this callout?
Sure thing!
rmloveland review comments jseldess review comments 19.1 files copied to 19.2 folders
ericharmeling
force-pushed
the
eharm-secure-app-examples
branch
from
97d8715
to
5067d9d
Compare
|
Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/5067d9d6a7c386dc36cb52c803c2dbeaacabb63c/ Edited pages: |
Added the following to the "Build an App with CockroachDB" examples: