Merge lp:~axwalk/juju-core/lp1298115-utils-ssh-testing into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

utils/ssh: revert to using slices for command args

Reverts part of https://codereview.appspot.com/66340045/
which changed OpenSSH-based client args from using a slice
to a map.

Fixes lp:1298115

https://codereview.appspot.com/81180043/

utils/ssh: revert to using slices for command args

Reverts part of https://codereview.appspot.com/66340045/
which changed OpenSSH-based client args from using a slice
to a map.

Fixes lp:1298115

https://codereview.appspot.com/81180043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+212977_code.launchpad.net,

Message:
Please take a look.

Description:
utils/ssh: revert to using slices for command args

Reverts part of https://codereview.appspot.com/66340045/
which changed OpenSSH-based client args from using a slice
to a map.

Fixes lp:1298115

https://code.launchpad.net/~axwalk/juju-core/lp1298115-utils-ssh-testing/+merge/212977

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/81180043/

Affected files (+15, -45 lines):
   A [revision details]
   M utils/ssh/ssh_openssh.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140326230643-olixj5ukxzhgt4cv
+New revision: <email address hidden>

Index: utils/ssh/ssh_openssh.go
=== modified file 'utils/ssh/ssh_openssh.go'
--- utils/ssh/ssh_openssh.go 2014-03-19 03:18:41 +0000
+++ utils/ssh/ssh_openssh.go 2014-03-27 02:42:08 +0000
@@ -14,7 +14,7 @@
   "launchpad.net/juju-core/utils"
  )

-var opensshCommonOptions = map[string][]string{"-o":
[]string{"StrictHostKeyChecking no"}}
+var opensshCommonOptions = []string{"-o", "StrictHostKeyChecking no"}

  // default identities will not be attempted if
  // -i is specified and they are not explcitly
@@ -63,22 +63,19 @@
   return &c, nil
  }

-func opensshOptions(options *Options, commandKind opensshCommandKind)
map[string][]string {
- args := make(map[string][]string)
- for k, v := range opensshCommonOptions {
- args[k] = v
- }
+func opensshOptions(options *Options, commandKind opensshCommandKind)
[]string {
+ args := append([]string{}, opensshCommonOptions...)
   if options == nil {
    options = &Options{}
   }
   if len(options.proxyCommand) > 0 {
- args["-o"] =
append(args["-o"], "ProxyCommand "+utils.CommandString(options.proxyCommand...))
+ args =
append(args, "-o", "ProxyCommand "+utils.CommandString(options.proxyCommand...))
   }
   if !options.passwordAuthAllowed {
- args["-o"] = append(args["-o"], "PasswordAuthentication no")
+ args = append(args, "-o", "PasswordAuthentication no")
   }
   if options.allocatePTY {
- args["-t"] = []string{}
+ args = append(args, "-t", "-t") // twice to force
   }
   identities := append([]string{}, options.identities...)
   if pk := PrivateKeyFiles(); len(pk) > 0 {
@@ -100,54 +97,29 @@
    }
   }
   for _, identity := range identities {
- args["-i"] = append(args["-i"], identity)
+ args = append(args, "-i", identity)
   }
   if options.port != 0 {
    port := fmt.Sprint(options.port)
    if commandKind == scpKind {
     // scp uses -P instead of -p (-p means preserve).
- args["-P"] = []string{port}
+ args = append(args, "-P", port)
    } else {
- args["-p"] = []string{port}
+ args = append(args, "-p", port)
    }
   }
   return args
  }

-func expandArgs(args map[string][]string, quote bool) []string {
- var list []string
- for opt, vals := range args {
- if len(vals) == 0 {
- list = append(list, opt)
- if opt == "-t" {
- // In order to force a PTY to be allocated, we need to
- // pass -t twice.
- ...

[Dave Cheney (dave-cheney)]

LGTM.

I'm a bit sad because options really shouldn't be position dependent. It
feels as if the code is being forced to conform to a specific output
just so we can write the test.

That said, without what you have done testing the output would have been
really difficult as the line would have to be parsed and split into a
set of atoms, taking care to worry about quoting and flags that have or
don't have options.

All in all, this is the right choice.

https://codereview.appspot.com/81180043/

=== modified file 'utils/ssh/ssh_openssh.go'
--- utils/ssh/ssh_openssh.go	2014-03-19 03:18:41 +0000
+++ utils/ssh/ssh_openssh.go	2014-03-27 02:55:06 +0000
@@ -14,7 +14,7 @@
 	"launchpad.net/juju-core/utils"
 )
 
-var opensshCommonOptions = map[string][]string{"-o": []string{"StrictHostKeyChecking no"}}
+var opensshCommonOptions = []string{"-o", "StrictHostKeyChecking no"}
 
 // default identities will not be attempted if
 // -i is specified and they are not explcitly
@@ -63,22 +63,19 @@
 	return &c, nil
 }
 
-func opensshOptions(options *Options, commandKind opensshCommandKind) map[string][]string {
-	args := make(map[string][]string)
-	for k, v := range opensshCommonOptions {
-		args[k] = v
-	}
+func opensshOptions(options *Options, commandKind opensshCommandKind) []string {
+	args := append([]string{}, opensshCommonOptions...)
 	if options == nil {
 		options = &Options{}
 	}
 	if len(options.proxyCommand) > 0 {
-		args["-o"] = append(args["-o"], "ProxyCommand "+utils.CommandString(options.proxyCommand...))
+		args = append(args, "-o", "ProxyCommand "+utils.CommandString(options.proxyCommand...))
 	}
 	if !options.passwordAuthAllowed {
-		args["-o"] = append(args["-o"], "PasswordAuthentication no")
+		args = append(args, "-o", "PasswordAuthentication no")
 	}
 	if options.allocatePTY {
-		args["-t"] = []string{}
+		args = append(args, "-t", "-t") // twice to force
 	}
 	identities := append([]string{}, options.identities...)
 	if pk := PrivateKeyFiles(); len(pk) > 0 {
@@ -100,54 +97,29 @@
 		}
 	}
 	for _, identity := range identities {
-		args["-i"] = append(args["-i"], identity)
+		args = append(args, "-i", identity)
 	}
 	if options.port != 0 {
 		port := fmt.Sprint(options.port)
 		if commandKind == scpKind {
 			// scp uses -P instead of -p (-p means preserve).
-			args["-P"] = []string{port}
+			args = append(args, "-P", port)
 		} else {
-			args["-p"] = []string{port}
+			args = append(args, "-p", port)
 		}
 	}
 	return args
 }
 
-func expandArgs(args map[string][]string, quote bool) []string {
-	var list []string
-	for opt, vals := range args {
-		if len(vals) == 0 {
-			list = append(list, opt)
-			if opt == "-t" {
-				// In order to force a PTY to be allocated, we need to
-				// pass -t twice.
-				list = append(list, opt)
-			}
-		}
-		for _, val := range vals {
-			list = append(list, opt)
-			if quote {
-				val = fmt.Sprintf("%q", val)
-			}
-			list = append(list, val)
-		}
-	}
-	return list
-}
-
 // Command implements Client.Command.
 func (c *OpenSSHClient) Command(host string, command []string, options *Options) *Cmd {
-	opts := opensshOptions(options, sshKind)
-	args := expandArgs(opts, false)
+	args := opensshOptions(options, sshKind)
 	args = append(args, host)
 	if len(command) > 0 {
 		args = append(args, command...)
 	}
 	bin, args := sshpassWrap("ssh", args)
-	optsList := strings.Join(expandArgs(opts, true), " ")
-	fullCommand := strings.Join(command, " ")
-	logger.Debugf("running: %s %s %q '%s'", bin, optsList, host, fullCommand)
+	logger.Debugf("running: %s %s", bin, utils.CommandString(args...))
 	return &Cmd{impl: &opensshCmd{exec.Command(bin, args...)}}
 }
 
@@ -158,18 +130,14 @@
 		options = *userOptions
 		options.allocatePTY = false // doesn't make sense for scp
 	}
-	opts := opensshOptions(&options, scpKind)
-	args := expandArgs(opts, false)
+	args := opensshOptions(&options, scpKind)
 	args = append(args, extraArgs...)
 	args = append(args, targets...)
 	bin, args := sshpassWrap("scp", args)
 	cmd := exec.Command(bin, args...)
 	var stderr bytes.Buffer
 	cmd.Stderr = &stderr
-	allOpts := append(expandArgs(opts, true), extraArgs...)
-	optsList := strings.Join(allOpts, " ")
-	targetList := `"` + strings.Join(targets, `" "`) + `"`
-	logger.Debugf("running: %s %s %s", bin, optsList, targetList)
+	logger.Debugf("running: %s %s", bin, utils.CommandString(args...))
 	if err := cmd.Run(); err != nil {
 		stderr := strings.TrimSpace(stderr.String())
 		if len(stderr) > 0 {