Add TLS support to increment command #3257
Conversation
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @codexnull)
x/tls_helper.go, line 60 at r1 (raw file):
flag.Bool("tls_use_system_ca", true, "Include System CA into CA Certs.") flag.String("tls_server_name", "", "Used to verify the server hostname.") flag.String("tls_cert", "", "(optional) The Cert file provided by the client to the server.")
The documentation for these flags should probably state that they are required if tls_cacert is specified or something to similar effect.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @martinmr)
x/tls_helper.go, line 60 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
flag.Bool("tls_use_system_ca", true, "Include System CA into CA Certs.") flag.String("tls_server_name", "", "Used to verify the server hostname.") flag.String("tls_cert", "", "(optional) The Cert file provided by the client to the server.")The documentation for these flags should probably state that they are required if tls_cacert is specified or something to similar effect.
Only --tls_cacert is required to enable TLS. The rest of the --tls_* options are optional. I'm planning on making this more clear for all dgraph commands in a separate PR.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
One comment, I strongly think we should keep the current addr flag. Will leave it to @martinmr for final approval.
Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @codexnull)
dgraph/cmd/counter/increment.go, line 49 at r1 (raw file):
flag := Increment.Cmd.Flags() flag.String("dgraph", "localhost:9080", "Address of Dgraph alpha.")
The command would be: dgraph increment --dgraph? I think --addr is a better argument.
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @martinmr)
dgraph/cmd/counter/increment.go, line 49 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
The command would be:
dgraph increment --dgraph? I think--addris a better argument.
Reverted back to --addr.
|
You're right, it doesn't look good, but I was trying to be consistent with other commands. We already have |
|
|
* Fix dgraph increment options. * Show error if TLS options are incomplete. * Use TLS connection with dgraph increment if requested. * Replace hard-coded alpha port in tests with z.SockAddr. * Remove unnecessary comment.
* Fix dgraph increment options. * Show error if TLS options are incomplete. * Use TLS connection with dgraph increment if requested. * Replace hard-coded alpha port in tests with z.SockAddr. * Remove unnecessary comment.
Change summary:
This change is