Code review - Issue 94330043: Corrected restore tu support current juju codehttps://codereview.appspot.com/2014-05-12T16:10:25+00:00rietveld
Message from unknown
2014-05-09T14:29:15+00:00hduranurn:md5:64d3d661e27f69434ce84329901091b8
Message from horacio.duran@canonical.com
2014-05-09T14:29:29+00:00hduranurn:md5:91d09285f3e67f49e316ac7ef37b33f8
Please take a look.
Message from vladislav.klyachin@canonical.com
2014-05-09T15:33:49+00:00vladislav.klyachinurn:md5:d015534988875846903ae0a66fca058f
LGTM with some notices
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go
File cmd/plugins/juju-restore/restore.go (right):
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode107
cmd/plugins/juju-restore/restore.go:107: set -x
I consider -e as good practice option
If you want to ignore result code of command, use:
command || true
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode120
cmd/plugins/juju-restore/restore.go:120: mkdir /tmp/oldbackup/inits
Use mkdir -p to get ok result code if directory exists and to create several directories
Although, in this place, I would remove old backup before moving the new one.
I am nor sure that /tmp is a good place for backups. Ofhen /tmp is located on ramdisk and it will consume the memory. I would use /var/lib/juju.backup.
The second 'mv' has invalid indent (spaces vs tabs).
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode131
cmd/plugins/juju-restore/restore.go:131: fi
IMHO export is not needed here and before
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode137
cmd/plugins/juju-restore/restore.go:137: mongo --ssl -u admin -p {{.Creds.OldPassword | printf "%q" }} localhost:37017/admin --eval "$1"
I would use 'shquote' instead of 'printf "%q"' here and further, because 'string' is better than "string" , because bash processes '/' and '$' inside of "string".
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode146
cmd/plugins/juju-restore/restore.go:146: for i in $(seq 1 50)
Shortcut for $(seq 1 50): {1..50}
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode348
cmd/plugins/juju-restore/restore.go:348: OldPassword string
OldPassword is confusing, why not use AdminPassword
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode387
cmd/plugins/juju-restore/restore.go:387: if !ok || password == "" {
Go style is oldPassword rather than olspassword
typo: oldpassword == ""
Message from unknown
2014-05-09T18:01:00+00:00hduranurn:md5:15460b53853fd86dc1f52f2c3a1d5b60
Message from horacio.duran@canonical.com
2014-05-09T18:01:03+00:00hduranurn:md5:5fb8d793d6f4471a0573aaced715afaa
Please take a look.
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go
File cmd/plugins/juju-restore/restore.go (right):
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode120
cmd/plugins/juju-restore/restore.go:120: mkdir /tmp/oldbackup/inits
On 2014/05/09 15:33:49, vladislav.klyachin wrote:
> Use mkdir -p to get ok result code if directory exists and to create several
> directories
> Although, in this place, I would remove old backup before moving the new one.
> I am nor sure that /tmp is a good place for backups. Ofhen /tmp is located on
> ramdisk and it will consume the memory. I would use /var/lib/juju.backup.
> The second 'mv' has invalid indent (spaces vs tabs).
Done.
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode146
cmd/plugins/juju-restore/restore.go:146: for i in $(seq 1 50)
On 2014/05/09 15:33:49, vladislav.klyachin wrote:
> Shortcut for $(seq 1 50): {1..50}
Noted although I will try to keep the other way since it seems more likely that less bash savvy people will understand it.
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode348
cmd/plugins/juju-restore/restore.go:348: OldPassword string
On 2014/05/09 15:33:49, vladislav.klyachin wrote:
> OldPassword is confusing, why not use AdminPassword
Because this is referred to as old password everywhere, since it is the old mongo access password (used for admin)
https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/restore.go#newcode387
cmd/plugins/juju-restore/restore.go:387: if !ok || password == "" {
On 2014/05/09 15:33:49, vladislav.klyachin wrote:
> Go style is oldPassword rather than olspassword
> typo: oldpassword == ""
Done.
Message from john.meinel@canonical.com
2014-05-12T12:59:53+00:00jameinelurn:md5:152a43226dcf0f76740613bfdd141887
I have a couple of suggestions, if you're fine with them, then the rest LGTM. (if you can justify why we want to get rid of 'set -e' that can be ok, but I do think we want -u for sure, and I'd really like -e unless there is a strong reason not to.)
https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go
File cmd/plugins/juju-restore/restore.go (right):
https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode107
cmd/plugins/juju-restore/restore.go:107: set -x
I'd rather see us use || true syntax than let failing things be missed by the script.
Also, we should be setting -u (treat unknown environment variables as an error rather than silently passing "").
for -e, I'm guessing we are worried that we might set something up but fail to cleanup afterwards?
https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode118
cmd/plugins/juju-restore/restore.go:118: rm -r /var/log/juju
it looks like you mixed tabs and spaces here, its probably better to be consistent.
https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode142
cmd/plugins/juju-restore/restore.go:142: for i in $(seq 1 50)
this change seems odd to me, is it just to make testing faster?
https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode316
cmd/plugins/juju-restore/restore.go:316: paddr, err := conn.State.Client().PrivateAddress("0")
I really think we should be grabbing the Client() as a variable and using it repeatedly, rather than re-instantiating it for each call.
Message from unknown
2014-05-12T16:10:20+00:00hduranurn:md5:c9786e57645ad6ed6041ebf9b975dcd5
Message from horacio.duran@canonical.com
2014-05-12T16:10:25+00:00hduranurn:md5:7cd857c932b1baf35ac926ce7831f1b3
Please take a look.
https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go
File cmd/plugins/juju-restore/restore.go (right):
https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode107
cmd/plugins/juju-restore/restore.go:107: set -x
On 2014/05/12 12:59:53, jameinel wrote:
> I'd rather see us use || true syntax than let failing things be missed by the
> script.
> Also, we should be setting -u (treat unknown environment variables as an error
> rather than silently passing "").
>
> for -e, I'm guessing we are worried that we might set something up but fail to
> cleanup afterwards?
Done.
https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode118
cmd/plugins/juju-restore/restore.go:118: rm -r /var/log/juju
On 2014/05/12 12:59:53, jameinel wrote:
> it looks like you mixed tabs and spaces here, its probably better to be
> consistent.
Done.
https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode142
cmd/plugins/juju-restore/restore.go:142: for i in $(seq 1 50)
On 2014/05/12 12:59:53, jameinel wrote:
> this change seems odd to me, is it just to make testing faster?
This was a bug, I made the change to actually produce a failure and forgot to remove it. Tx for spotting it.
https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/restore.go#newcode316
cmd/plugins/juju-restore/restore.go:316: paddr, err := conn.State.Client().PrivateAddress("0")
On 2014/05/12 12:59:53, jameinel wrote:
> I really think we should be grabbing the Client() as a variable and using it
> repeatedly, rather than re-instantiating it for each call.
Done.