plugins/juju-backup: tweaks

plugins/juju-backup: tweaks This is to address comments I made too late on the original CL. I've tried to reduce moving parts where possible and reduce vulnerability to bash quoting issues. I changed the indentation from two spaces to tabs, as I find two spaces hard to see and we use tabs elsewhere in the code base. I realise that this may be a religious issue - 4 spaces would also be a viable option if tabs are not considered OK. I've left the output pretty much unchanged. Here's a sample: % juju-backup Connecting to machine 0 ************************************************************** Preparing to perform backup ************************************************************** Older juju backup exists, moving to juju-backup-previous Removing existing backup archive.....SUCCESS Archiving backup.....SUCCESS Making backup directory.....SUCCESS ************************************************************** Backing up mongo database ************************************************************** Stopping mongo.....SUCCESS Backing up mongo.....SUCCESS Backing up environ config.....SUCCESS Starting mongo.....SUCCESS ************************************************************** Copying Juju configuration ************************************************************** Copying /etc/init/juju-db.conf.....SUCCESS Copying /etc/init/jujud-machine-0.conf.....SUCCESS Copying /var/lib/juju/agents/machine-0.....SUCCESS Copying /var/lib/juju/tools.....SUCCESS Copying /var/lib/juju/server.pem.....SUCCESS Copying /home/ubuntu/.ssh/authorized_keys.....SUCCESS Copying /etc/rsyslog.d/25-juju.conf.....SUCCESS Copying /var/log/juju/all-machines.log.....SUCCESS Copying /var/log/juju/machine-0.log.....SUCCESS ************************************************************** Creating tarball ************************************************************** Performing tar.....SUCCESS Changing ownership of backup files to ubuntu.....SUCCESS Juju backup finished. Connection to ec2-23-20-40-203.compute-1.amazonaws.com closed. Copying tarball to /home/rog/src/go/src/launchpad.net/juju-core/cmd/plugins/juju-backup/juju-backup-20131129-1540.tar.gz ... juju-backup.tar.gz 100% 4650KB 357.7KB/s 00:13 % https://code.launchpad.net/~rogpeppe/juju-core/471-juju-backup-tweaks/+merge/197224 (do not edit description out of merge proposal)

[rog, 2013-11-29 15:54:10]

Please take a look.

[fwereade, 2013-12-02 09:32:51]

LGTM assuming it's been tested live.

[dimitern, 2013-12-02 09:59:12]

LGTM https://codereview.appspot.com/34710044/diff/20001/cmd/plugins/juju-backup/ju... File cmd/plugins/juju-backup/juju-backup (right): https://codereview.appspot.com/34710044/diff/20001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:38: cd juju-backup I think we need to do cd /root/ somewhere in the beginning, just to make sure we're at the correct path (I know ssh by default goes to the user's home dir, but just to be on the safe side). https://codereview.appspot.com/34710044/diff/20001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:59: dest=$(dirname "./root/$i") why ./root/ here ? why not /root/ ?

[rog, 2013-12-02 11:39:11]

https://codereview.appspot.com/34710044/diff/20001/cmd/plugins/juju-backup/ju... File cmd/plugins/juju-backup/juju-backup (right): https://codereview.appspot.com/34710044/diff/20001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:38: cd juju-backup On 2013/12/02 09:59:12, dimitern wrote: > I think we need to do cd /root/ somewhere in the beginning, just to make sure > we're at the correct path (I know ssh by default goes to the user's home dir, > but just to be on the safe side). I'll change it so it explicitly creates and chdirs to ~/juju-backup. (I'm not sure why you're suggesting /root/ here though). https://codereview.appspot.com/34710044/diff/20001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:59: dest=$(dirname "./root/$i") On 2013/12/02 09:59:12, dimitern wrote: > why ./root/ here ? why not /root/ ? Because we copy all files into ~/juju-backup/root. Is there some issue with that?

[dimitern, 2013-12-02 12:25:08]

https://codereview.appspot.com/34710044/diff/20001/cmd/plugins/juju-backup/ju... File cmd/plugins/juju-backup/juju-backup (right): https://codereview.appspot.com/34710044/diff/20001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:38: cd juju-backup On 2013/12/02 11:39:11, rog wrote: > On 2013/12/02 09:59:12, dimitern wrote: > > I think we need to do cd /root/ somewhere in the beginning, just to make sure > > we're at the correct path (I know ssh by default goes to the user's home dir, > > but just to be on the safe side). > > I'll change it so it explicitly creates and chdirs to ~/juju-backup. > (I'm not sure why you're suggesting /root/ here though). My mistake (see below). https://codereview.appspot.com/34710044/diff/20001/cmd/plugins/juju-backup/ju... cmd/plugins/juju-backup/juju-backup:59: dest=$(dirname "./root/$i") On 2013/12/02 11:39:11, rog wrote: > On 2013/12/02 09:59:12, dimitern wrote: > > why ./root/ here ? why not /root/ ? > > Because we copy all files into ~/juju-backup/root. > Is there some issue with that? Ah, sorry, I should've written $HOME or ~. I was under the impression we're connecting as root.

[rog, 2013-12-03 18:21:35]

Please take a look.