juju-core

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

  1. lp1298115-utils-ssh-testing
  2. Merge into trunk
Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 2497
Proposed branch: lp:~axwalk/juju-core/lp1298115-utils-ssh-testing
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 121 lines (+13/-45)
1 file modified
utils/ssh/ssh_openssh.go (+13/-45)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1298115-utils-ssh-testing
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+212977@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (4.8 KiB)

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.
- ...

Read more...

Revision history for this message
Dave Cheney (dave-cheney) wrote :

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/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
14 "launchpad.net/juju-core/utils"14 "launchpad.net/juju-core/utils"
15)15)
1616
17var opensshCommonOptions = map[string][]string{"-o": []string{"StrictHostKeyChecking no"}}17var opensshCommonOptions = []string{"-o", "StrictHostKeyChecking no"}
1818
19// default identities will not be attempted if19// default identities will not be attempted if
20// -i is specified and they are not explcitly20// -i is specified and they are not explcitly
@@ -63,22 +63,19 @@
63 return &c, nil63 return &c, nil
64}64}
6565
66func opensshOptions(options *Options, commandKind opensshCommandKind) map[string][]string {66func opensshOptions(options *Options, commandKind opensshCommandKind) []string {
67 args := make(map[string][]string)67 args := append([]string{}, opensshCommonOptions...)
68 for k, v := range opensshCommonOptions {
69 args[k] = v
70 }
71 if options == nil {68 if options == nil {
72 options = &Options{}69 options = &Options{}
73 }70 }
74 if len(options.proxyCommand) > 0 {71 if len(options.proxyCommand) > 0 {
75 args["-o"] = append(args["-o"], "ProxyCommand "+utils.CommandString(options.proxyCommand...))72 args = append(args, "-o", "ProxyCommand "+utils.CommandString(options.proxyCommand...))
76 }73 }
77 if !options.passwordAuthAllowed {74 if !options.passwordAuthAllowed {
78 args["-o"] = append(args["-o"], "PasswordAuthentication no")75 args = append(args, "-o", "PasswordAuthentication no")
79 }76 }
80 if options.allocatePTY {77 if options.allocatePTY {
81 args["-t"] = []string{}78 args = append(args, "-t", "-t") // twice to force
82 }79 }
83 identities := append([]string{}, options.identities...)80 identities := append([]string{}, options.identities...)
84 if pk := PrivateKeyFiles(); len(pk) > 0 {81 if pk := PrivateKeyFiles(); len(pk) > 0 {
@@ -100,54 +97,29 @@
100 }97 }
101 }98 }
102 for _, identity := range identities {99 for _, identity := range identities {
103 args["-i"] = append(args["-i"], identity)100 args = append(args, "-i", identity)
104 }101 }
105 if options.port != 0 {102 if options.port != 0 {
106 port := fmt.Sprint(options.port)103 port := fmt.Sprint(options.port)
107 if commandKind == scpKind {104 if commandKind == scpKind {
108 // scp uses -P instead of -p (-p means preserve).105 // scp uses -P instead of -p (-p means preserve).
109 args["-P"] = []string{port}106 args = append(args, "-P", port)
110 } else {107 } else {
111 args["-p"] = []string{port}108 args = append(args, "-p", port)
112 }109 }
113 }110 }
114 return args111 return args
115}112}
116113
117func expandArgs(args map[string][]string, quote bool) []string {
118 var list []string
119 for opt, vals := range args {
120 if len(vals) == 0 {
121 list = append(list, opt)
122 if opt == "-t" {
123 // In order to force a PTY to be allocated, we need to
124 // pass -t twice.
125 list = append(list, opt)
126 }
127 }
128 for _, val := range vals {
129 list = append(list, opt)
130 if quote {
131 val = fmt.Sprintf("%q", val)
132 }
133 list = append(list, val)
134 }
135 }
136 return list
137}
138
139// Command implements Client.Command.114// Command implements Client.Command.
140func (c *OpenSSHClient) Command(host string, command []string, options *Options) *Cmd {115func (c *OpenSSHClient) Command(host string, command []string, options *Options) *Cmd {
141 opts := opensshOptions(options, sshKind)116 args := opensshOptions(options, sshKind)
142 args := expandArgs(opts, false)
143 args = append(args, host)117 args = append(args, host)
144 if len(command) > 0 {118 if len(command) > 0 {
145 args = append(args, command...)119 args = append(args, command...)
146 }120 }
147 bin, args := sshpassWrap("ssh", args)121 bin, args := sshpassWrap("ssh", args)
148 optsList := strings.Join(expandArgs(opts, true), " ")122 logger.Debugf("running: %s %s", bin, utils.CommandString(args...))
149 fullCommand := strings.Join(command, " ")
150 logger.Debugf("running: %s %s %q '%s'", bin, optsList, host, fullCommand)
151 return &Cmd{impl: &opensshCmd{exec.Command(bin, args...)}}123 return &Cmd{impl: &opensshCmd{exec.Command(bin, args...)}}
152}124}
153125
@@ -158,18 +130,14 @@
158 options = *userOptions130 options = *userOptions
159 options.allocatePTY = false // doesn't make sense for scp131 options.allocatePTY = false // doesn't make sense for scp
160 }132 }
161 opts := opensshOptions(&options, scpKind)133 args := opensshOptions(&options, scpKind)
162 args := expandArgs(opts, false)
163 args = append(args, extraArgs...)134 args = append(args, extraArgs...)
164 args = append(args, targets...)135 args = append(args, targets...)
165 bin, args := sshpassWrap("scp", args)136 bin, args := sshpassWrap("scp", args)
166 cmd := exec.Command(bin, args...)137 cmd := exec.Command(bin, args...)
167 var stderr bytes.Buffer138 var stderr bytes.Buffer
168 cmd.Stderr = &stderr139 cmd.Stderr = &stderr
169 allOpts := append(expandArgs(opts, true), extraArgs...)140 logger.Debugf("running: %s %s", bin, utils.CommandString(args...))
170 optsList := strings.Join(allOpts, " ")
171 targetList := `"` + strings.Join(targets, `" "`) + `"`
172 logger.Debugf("running: %s %s %s", bin, optsList, targetList)
173 if err := cmd.Run(); err != nil {141 if err := cmd.Run(); err != nil {
174 stderr := strings.TrimSpace(stderr.String())142 stderr := strings.TrimSpace(stderr.String())
175 if len(stderr) > 0 {143 if len(stderr) > 0 {

Subscribers

People subscribed via source and target branches

to status/vote changes: