Issue 12603047: Extra options for ssh/scp don't cause errors.

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years, 9 months ago by Michael Nelson

Modified:
10 years, 8 months ago

Reviewers:
mp+179422, fwereade, rog

Visibility:
Public.

Description

Extra options for ssh/scp don't cause errors. https://code.launchpad.net/~michael.nelson/juju-core/1183159-ssh-options/+merge/179422 (do not edit description out of merge proposal)

Patch Set 2 : Extra options for ssh/scp don't cause errors. #

Patch Set 3 : Extra options for ssh/scp don't cause errors. #

Patch Set 4 : Extra options for ssh/scp don't cause errors. #

Patch Set 5 : Extra options for ssh/scp don't cause errors. #

Total comments: 10

Patch Set 6 : Extra options for ssh/scp don't cause errors. #

Patch Set 7 : Extra options for ssh/scp don't cause errors. #

Created: 10 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+48 lines, -21 lines)			Patch
A	[revision details]	View	1 2 3 4 5	1 chunk	+2 lines, -0 lines	0 comments	Download
M	cmd/cmd.go	View	1 2 3 4 5	4 chunks	+11 lines, -13 lines	0 comments	Download
M	cmd/juju/debuglog.go	View	1 2 3 4 5	1 chunk	+4 lines, -0 lines	0 comments	Download
M	cmd/juju/ssh.go	View	1 2 3	1 chunk	+7 lines, -0 lines	0 comments	Download
M	cmd/juju/ssh_test.go	View		2 chunks	+14 lines, -6 lines	0 comments	Download
M	cmd/supercommand.go	View	1 2 3 4 5	2 chunks	+9 lines, -1 line	0 comments	Download
M	testing/cmd.go	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

fwereade

LGTM apart from the one comment. https://codereview.appspot.com/12603047/diff/11001/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/12603047/diff/11001/cmd/juju/debuglog.go#newcode35 cmd/juju/debuglog.go:35: return c.sshCmd.AllowInterspersedFlags() I ...

10 years, 8 months ago (2013-08-13 15:24:41 UTC) #4

rog

LGTM modulo the below suggestions. Thanks. https://codereview.appspot.com/12603047/diff/11001/cmd/cmd.go File cmd/cmd.go (right): https://codereview.appspot.com/12603047/diff/11001/cmd/cmd.go#newcode40 cmd/cmd.go:40: AllowInterspersedFlags() bool // ...

10 years, 8 months ago (2013-08-13 15:28:43 UTC) #5

Michael Nelson

10 years, 8 months ago (2013-08-14 07:13:08 UTC) #6

Please take a look.

https://codereview.appspot.com/12603047/diff/11001/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/12603047/diff/11001/cmd/cmd.go#newcode40
cmd/cmd.go:40: AllowInterspersedFlags() bool
On 2013/08/13 15:28:44, rog wrote:
> // AllowInterspersedFlags returns whether the command allows flag
> // arguments to be interspersed with non-flag arguments.

Done.

https://codereview.appspot.com/12603047/diff/11001/cmd/cmd.go#newcode164
cmd/cmd.go:164: if rc, done := handleCommandError(c, ctx, ParseArgs(c, f, args),
f); done {
On 2013/08/13 15:28:44, rog wrote:
> if rc, done := handleCommandError(c, ctx, f.Parse(c.AllowInterspersedFlags(),
> args), f); done {

Done. There as well as in testing/cmd.go:InitCommand.

https://codereview.appspot.com/12603047/diff/11001/cmd/juju/debuglog.go
File cmd/juju/debuglog.go (right):

https://codereview.appspot.com/12603047/diff/11001/cmd/juju/debuglog.go#newco...
cmd/juju/debuglog.go:35: return c.sshCmd.AllowInterspersedFlags()
On 2013/08/13 15:24:41, fwereade wrote:
> I don't think we want to pass additional args into SSH here.

Done. Just returned true like CommandBase.

https://codereview.appspot.com/12603047/diff/11001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/12603047/diff/11001/cmd/supercommand.go#newcod...
cmd/supercommand.go:213: }
On 2013/08/13 15:28:44, rog wrote:
> func (c *SuperCommand) AllowInterspersedFlags() bool {
>     return false
> }

Done - I copied the useful part of the comment from ParseArgs to here too.

Expand All Messages | Collapse All Messages