-
Notifications
You must be signed in to change notification settings - Fork 44
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: Update docs for the v0.5 release #1320
Conversation
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.
praise: Looks really good - thank you for taking the time to go over this.
Approving now with a few minor comments added
docs/cli/defradb_start.md
Outdated
--no-p2p Disable the peer-to-peer network synchronization system | ||
--p2paddr string Listener address for the p2p network (formatted as a libp2p MultiAddr) (default "/ip4/0.0.0.0/tcp/9171") | ||
--peers string List of peers to connect to | ||
--privkeypath string Path to the private key for tls (default "/home/jsimnz/.defradb/certs/server.crt") | ||
--pubkeypath string Path to the public key for tls (default "/home/jsimnz/.defradb/certs/server.key") | ||
--privkeypath string Path to the private key for tls (default "/Users/o/.defradb/certs/server.crt") |
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.
suggestion: This looks inaccurate, suggest changing to $HOME/.defradb/......
or perhaps $DEFRA_ROOTDIR/.defradb/......
? I'm not sure about the behaviour here though.
(same goes for the line below too)
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 prefer $HOME
.
What is $DEFRA_ROOTDIR
, do we maintain exporting this properly?
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.
Btw: here is what above in the docs it shows:
--rootdir string Directory for data and configuration to use (default "$HOME/.defradb")
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 prefer
$HOME
. What is$DEFRA_ROOTDIR
, do we maintain exporting this properly?
It is documented in the config file template, but it might actually be out of date - the only other ref I can see to it is a test that looks like it might not be testing what it says it is (TestLoadValidationEnvDoesntSupportRootdir
- The call to FixtureEnvKeyValue
looks like it has no impact on the test)
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.
Ah - the test is a negative test, but I'll remove it as we could test an almost infinite amount of unsupported environment variables.
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.
Orpheus has actually already removed the reference to this from the template in #1318
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 this is an anomaly of how we treat paths in the current config system. I'd like to do/see a refactor in v0.6 which would allow writing relative paths in the default config file and docs.
d7a7ace
to
2e53749
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1320 +/- ##
===========================================
+ Coverage 70.65% 70.68% +0.03%
===========================================
Files 184 184
Lines 17803 17803
===========================================
+ Hits 12578 12584 +6
+ Misses 4272 4269 -3
+ Partials 953 950 -3 |
Relevant issue(s)
Resolves #1171
Description
Update in-repo docs in light of release v0.5.
Tasks
How has this been tested?
Human readers.