Corrected restore tu support current juju code

Corrected restore tu support current juju code

Current restore plugin, takes a backup which contains
a dump of the mongodb for the backed up machine and 
several configuration files, it bootstraps a new
machine and:
Deletes the fresh db
Loads the dump
Places the files instead of the fresh machine ones
Restarts services.
Since replicaset was introduced the db load was not
working properly. A replicaset initiate with a fresh
config was added to solve it.

https://code.launchpad.net/~hduran-8/juju-core/upgrade_restore_for_replicaset/+merge/218998

(do not edit description out of merge proposal)

[hduran, 2014-05-09 14:29:29]

Please take a look.

[vladislav.klyachin, 2014-05-09 15:33:49]

LGTM with some notices

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/resto...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/resto...
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/resto...
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/resto...
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/resto...
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/resto...
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/resto...
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/resto...
cmd/plugins/juju-restore/restore.go:387: if !ok || password == "" {
Go style is oldPassword rather than olspassword
typo: oldpassword == ""

[hduran, 2014-05-09 18:01:03]

Please take a look.

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/resto...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/94330043/diff/1/cmd/plugins/juju-restore/resto...
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/resto...
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/resto...
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/resto...
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.

[jameinel, 2014-05-12 12:59:53]

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/r...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/r...
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/r...
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/r...
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/r...
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.

[hduran, 2014-05-12 16:10:25]

Please take a look.

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/r...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/94330043/diff/20001/cmd/plugins/juju-restore/r...
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/r...
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/r...
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/r...
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.