juju-core

cmd/juju/cmd_test.go (+15/-0)
cmd/juju/main.go (+1/-0)
cmd/juju/main_test.go (+1/-0)
cmd/juju/scp.go (+66/-0)
cmd/juju/scp_test.go (+72/-0)
cmd/juju/ssh.go (+12/-7)
cmd/juju/ssh_test.go (+18/-12)

To merge this branch:

bzr merge lp://staging/~dave-cheney/juju-core/005-cmd-juju-scp

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
The Go Language Gophers		2012-09-27	Pending
Review via email: mp+126623@code.staging.launchpad.net

Description of the change

cmd/juju: add scp subcommand

https://codereview.appspot.com/6565055/

Revision history for this message

Gustavo Niemeyer (niemeyer) wrote on 2012-09-27:

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

https://codereview.appspot.com/6565055/

Revision history for this message

Gustavo Niemeyer (niemeyer) wrote on 2012-09-28:

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 (<email address hidden> as a path?).

https://codereview.appspot.com/6565055/

Revision history for this message

Gustavo Niemeyer (niemeyer) wrote on 2012-09-28:

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:<email address hidden> 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 <email address hidden>:foo .\n",
I suggest putting the common arguments in a const, and using commonArgs
+ "foo".

https://codereview.appspot.com/6565055/