Code review comment for lp:~gz/juju-core/gosshnew_1312940

Revision history for this message
Andrew Wilkins (axwalk) wrote :

LGTM, with a few naming changes

Thanks for doing that, I keep on meaning to do it and then promptly
forget.

https://codereview.appspot.com/95770045/diff/1/utils/ssh/authorisedkeys.go
File utils/ssh/authorisedkeys.go (left):

https://codereview.appspot.com/95770045/diff/1/utils/ssh/authorisedkeys.go#oldcode48
utils/ssh/authorisedkeys.go:48: return nil, fmt.Errorf("invalid
authorized_key %q", line)
Include the ParseAuthorizedKey error in the message?

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

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto.go#newcode84
utils/ssh/ssh_gocrypto.go:84: conn *ssh.Client
s/conn/client/
(and wherever else we refer to Client as conn)
?

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto.go#newcode152
utils/ssh/ssh_gocrypto.go:152: conn.Conn.Close()
ssh.Client embeds Conn, so conn.Close (or client.Close) would be better
IMO

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

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto_test.go#newcode31
utils/ssh/ssh_gocrypto_test.go:31: Listener net.Listener
s/Listener/listener/
?
(I would prefer we don't export fields unless we need to for reflection
or whatever.)

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto_test.go#newcode154
utils/ssh/ssh_gocrypto_test.go:154: c.Assert(string(out), gc.Equals,
"abc value\n")
Yay! Thanks.

https://codereview.appspot.com/95770045/

« Back to merge proposal