|
|
|
cmd/plugins: juju restore plugin
We also tweak the juju backup plugin so that it preserves
user name and other file metadata.
Verified live.
https://code.launchpad.net/~rogpeppe/juju-core/470-juju-restore/+merge/199711
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : cmd/plugins: juju restore plugin #
Total comments: 7
Patch Set 3 : cmd/plugins: juju restore plugin #Patch Set 4 : cmd/plugins: juju restore plugin #
Total messages: 6
|
rog
Please take a look.
|
12 years ago (2013年12月19日 20:06:26 UTC) #1 | ||||||||||||||||||||||||||||||||||||||
Please take a look.
https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... File cmd/plugins/juju-restore/restore.go (right): https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... cmd/plugins/juju-restore/restore.go:82: return nil Many commands use 'return cmd.CheckEmpty(args[1:])' in this situation to explicitly error if given too many positional args. https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... cmd/plugins/juju-restore/restore.go:87: tar xzf juju-backup.tgz what if the filename isn't juju-backup.tgz?
Does this look good to you otherwise? https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... File cmd/plugins/juju-restore/restore.go (right): https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... cmd/plugins/juju-restore/restore.go:82: return nil On 2013年12月19日 21:59:03, thumper wrote: > Many commands use 'return cmd.CheckEmpty(args[1:])' in this situation to > explicitly error if given too many positional args. I prefer a better error message. https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... cmd/plugins/juju-restore/restore.go:87: tar xzf juju-backup.tgz On 2013年12月19日 21:59:03, thumper wrote: > what if the filename isn't juju-backup.tgz? That is what we copy the file to. It would probably be more obvious if this script was defined closer to its use, below
LGTM - although I still think you should be explicit in the handling of unused positional args. https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... File cmd/plugins/juju-restore/restore.go (right): https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... cmd/plugins/juju-restore/restore.go:82: return nil On 2013年12月20日 00:11:37, rog wrote: > On 2013年12月19日 21:59:03, thumper wrote: > > Many commands use 'return cmd.CheckEmpty(args[1:])' in this situation to > > explicitly error if given too many positional args. > > I prefer a better error message. What error message? You don't error out if they give you parameters that you are not going to use. https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... cmd/plugins/juju-restore/restore.go:87: tar xzf juju-backup.tgz On 2013年12月20日 00:11:37, rog wrote: > On 2013年12月19日 21:59:03, thumper wrote: > > what if the filename isn't juju-backup.tgz? > > That is what we copy the file to. It would probably be more obvious if this > script was defined closer to its use, below Ah... I see now
Please take a look. https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... File cmd/plugins/juju-restore/restore.go (right): https://codereview.appspot.com/44350043/diff/20001/cmd/plugins/juju-restore/r... cmd/plugins/juju-restore/restore.go:82: return nil On 2013年12月20日 01:17:28, thumper wrote: > On 2013年12月20日 00:11:37, rog wrote: > > On 2013年12月19日 21:59:03, thumper wrote: > > > Many commands use 'return cmd.CheckEmpty(args[1:])' in this situation to > > > explicitly error if given too many positional args. > > > > I prefer a better error message. > > What error message? You don't error out if they give you parameters that you are > not going to use. Ah, sorry, I'd missed that. Done.