Code review comment for lp:~axwalk/juju-core/ssh-gocrypto-client

Revision history for this message
Tim Penhey (thumper) wrote :

I think the general approach is good, but there will be issues with the
identity file that my branch has introduced.

https://codereview.appspot.com/48370043/diff/1/cloudinit/sshinit/configure.go
File cloudinit/sshinit/configure.go (right):

https://codereview.appspot.com/48370043/diff/1/cloudinit/sshinit/configure.go#newcode54
cloudinit/sshinit/configure.go:54: cmd.Stdin = strings.NewReader(script)
One thing I noticed was the need for a carriage return on the end.
Does the script end with a new line?

https://codereview.appspot.com/48370043/diff/1/environs/config/authkeys.go
File environs/config/authkeys.go (right):

https://codereview.appspot.com/48370043/diff/1/environs/config/authkeys.go#newcode40
environs/config/authkeys.go:40: var err error
Wondering if this function should perhaps be moved into utils/ssh?

https://codereview.appspot.com/48370043/diff/1/environs/manual/fakessh.go
File environs/manual/fakessh.go (right):

https://codereview.appspot.com/48370043/diff/1/environs/manual/fakessh.go#newcode33
environs/manual/fakessh.go:33: head >/dev/null
Given the number of different places we mock out the ssh executable,
wondering if we should have some standard place for things like this,
perhaps "testing/ssh" ?

https://codereview.appspot.com/48370043/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/48370043/diff/1/environs/sshstorage/storage.go#newcode48
environs/sshstorage/storage.go:48: return ssh.Command(host,
[]string{"bash", "-c", command}, &options)
bash or /bin/bash ??

We have both in the codebase, and I think we should be consistent with
ourselves.

Personally I don't care too much which we choose, just that we have one.

https://codereview.appspot.com/48370043/diff/1/utils/ssh/ssh.go
File utils/ssh/ssh.go (right):

https://codereview.appspot.com/48370043/diff/1/utils/ssh/ssh.go#newcode18
utils/ssh/ssh.go:18: passwordAuthDisabled bool
hmm.. one of my branches adds an identity file option using the old way.
  This will need to be taken into account some how.

https://codereview.appspot.com/48370043/diff/1/utils/ssh/ssh_gocrypto.go
File utils/ssh/ssh_gocrypto.go (right):

https://codereview.appspot.com/48370043/diff/1/utils/ssh/ssh_gocrypto.go#newcode39
utils/ssh/ssh_gocrypto.go:39: func (c *GoCryptoClient) Command(host
string, command []string, options *Options) *Cmd {
Lots of public functions missing docs.

https://codereview.appspot.com/48370043/diff/1/utils/trivial.go
File utils/trivial.go (right):

https://codereview.appspot.com/48370043/diff/1/utils/trivial.go#newcode66
utils/trivial.go:66: // quotes as necessary; each argument is
single-quoted.
this isn't escaping slashes... is it?

https://codereview.appspot.com/48370043/

« Back to merge proposal