MORAY-442 node-moray cli option for readOnlyOverride

[joyent-automation]

MORAY-442 node-moray cli option for readOnlyOverride

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/2763/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@davepacheco commented at 2017-10-11T19:52:17

Patch Set 1:

(4 comments)

Thanks for doing this!

Patch Set 1 code comments

bin/sql#21 @davepacheco

If this option doesn't take an argument, the synopsis part should just say "[-r]".

docs/man/man1/sql.md#5 @davepacheco

This summary looks like a copy/paste of ping. Since you're changing it anyway, would you mind fixing it?

docs/man/man1/sql.md#25 @davepacheco

I would suggest starting with the fact that electric-moray normally prohibits raw SQL operations when any vnode is read-only in order to avoid modifications to the read-only metadata. (Without that, I think the rest of this is somewhat confusing.) Then I would just tell users that when using this flag, that check is skipped, and the user is responsible for ensuring that the query they're running will not modify read-only vnodes, and that the result could be data corruption.

Note that the description above already explains that this command requires knowing what you're doing, should usually be used only for read-only queries, that mistakes can cause data corruption, and that there's almost no validation on the input. I don't think this flag actually makes the command much more dangerous than it already is.

docs/man/man1/sql.md#27 @davepacheco

I would say "some vnodes have", since there's no particular vnode here.

@davepacheco commented at 2017-10-12T00:59:17

Patch Set 2: Code-Review+1

Thanks!

@davepacheco commented at 2017-10-12T01:00:20

Patch Set 2:

For IA: does the moray test suite run successfully? I think there are some CLI tests as well and I'm not sure if we want to update them to reflect this new flag.

@princesspretzel commented at 2017-10-21T00:07:09

Uploaded patch set 3: Patch Set 2 was rebased.

@princesspretzel commented at 2017-10-21T00:07:48

Patch Set 3:

Dave -- it does, but I can add those tests!

@melloc commented at 2017-11-02T21:46:59

Patch Set 3:

(2 comments)

We should also update the troff in man/ with this change. Unfortunately, doing this is a bit of a pain, since you need to install md2man. You can do this by:

pfexec pkgin in ruby-2.2.4
mkdir ~/ruby
export GEM_HOME=/home/<user>/ruby
export PATH=~/ruby/bin:$PATH
gem install md2man
make man

Patch Set 3 code comments

docs/man/man1/sql.md#27 @melloc

"modifications"

docs/man/man1/sql.md#31 @melloc

"as noted"