cmd/juju: add scp subcommand

cmd/juju: add scp subcommand



https://code.launchpad.net/~dave-cheney/juju-core/005-cmd-juju-scp/+merge/126623

Requires: https://code.launchpad.net/~dave-cheney/juju-core/004-cmd-juju-ssh/+merge/126592

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-09-27 08:22:49]

Please take a look.

[niemeyer, 2012-09-27 22:49:52]

The comments made for the pre-req should be considered here as well. There's
probably not much, though, given the very nice reuse.

[dave_cheney.net, 2012-09-28 00:13:44]

Please take a look.

[niemeyer, 2012-09-28 01:06:02]

https://codereview.appspot.com/6565055/diff/5001/cmd/juju/scp.go
File cmd/juju/scp.go (right):

https://codereview.appspot.com/6565055/diff/5001/cmd/juju/scp.go#newcode51
cmd/juju/scp.go:51: // BUG(dfc) This will also not work if the local path has a
: in it.
Parsing the value we obtain and checking if it's a number of a IsUnitName would
remove most ambiguities as it will be quite rare to see a path with a
/<number>:/ component. This isn't much worse than the same issue in the stock
scp (foo@bar.com as a path?).

[dave_cheney.net, 2012-09-28 04:11:03]

Please take a look.

https://codereview.appspot.com/6565055/diff/5001/cmd/juju/scp.go
File cmd/juju/scp.go (right):

https://codereview.appspot.com/6565055/diff/5001/cmd/juju/scp.go#newcode51
cmd/juju/scp.go:51: // BUG(dfc) This will also not work if the local path has a
: in it.
On 2012/09/28 01:06:02, niemeyer wrote:
> Parsing the value we obtain and checking if it's a number of a IsUnitName
would
> remove most ambiguities as it will be quite rare to see a path with a
> /<number>:/ component. This isn't much worse than the same issue in the stock
> scp (mailto:foo@bar.com as a path?).

hostFromTarget does that for us. I'll drop the comment about local paths with
colons in them, it doesn't add anything over using raw ssh.

[niemeyer, 2012-09-28 13:09:51]

LGTM

https://codereview.appspot.com/6565055/diff/5001/cmd/juju/scp.go
File cmd/juju/scp.go (right):

https://codereview.appspot.com/6565055/diff/5001/cmd/juju/scp.go#newcode51
cmd/juju/scp.go:51: // BUG(dfc) This will also not work if the local path has a
: in it.
On 2012/09/28 04:11:03, dfc wrote:
> On 2012/09/28 01:06:02, niemeyer wrote:
> > Parsing the value we obtain and checking if it's a number of a IsUnitName
> would
> > remove most ambiguities as it will be quite rare to see a path with a
> > /<number>:/ component. This isn't much worse than the same issue in the
stock
> > scp (mailto:foo@bar.com as a path?).
> 
> hostFromTarget does that for us. I'll drop the comment about local paths with
> colons in them, it doesn't add anything over using raw ssh.

Sorry, I think I wasn't clear (or maybe I misunderstand you). The idea is to
*decide* whether it is a machine/unit prefix or not based on what it is, rather
than purely splitting at first colon no matter where it is, and then handing off
and breaking down if it isn't.

That said, I think this is a fine first implementation, and happy to have it in
as-is too.

https://codereview.appspot.com/6565055/diff/5001/cmd/juju/scp_test.go
File cmd/juju/scp_test.go (right):

https://codereview.appspot.com/6565055/diff/5001/cmd/juju/scp_test.go#newcode25
cmd/juju/scp_test.go:25: "-o StrictHostKeyChecking no -o PasswordAuthentication
no ubuntu@dummyenv-0.dns:foo .\n",
I suggest putting the common arguments in a const, and using commonArgs + "foo".

[dave_cheney.net, 2012-10-01 01:03:34]

*** Submitted:

cmd/juju: add scp subcommand

R=niemeyer
CC=
https://codereview.appspot.com/6565055

https://codereview.appspot.com/6565055/diff/5001/cmd/juju/scp_test.go
File cmd/juju/scp_test.go (right):

https://codereview.appspot.com/6565055/diff/5001/cmd/juju/scp_test.go#newcode25
cmd/juju/scp_test.go:25: "-o StrictHostKeyChecking no -o PasswordAuthentication
no ubuntu@dummyenv-0.dns:foo .\n",
On 2012/09/28 13:09:51, niemeyer wrote:
> I suggest putting the common arguments in a const, and using commonArgs +
"foo".

Done.