Code review comment for lp:~axwalk/juju-core/gocryptossh-proxycommand

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

On 2014/03/28 13:38:15, fwereade wrote:
> LGTM, if you drop the local config check -- it adds an implicit
dependency to
> cmd, on an implementation detail of environs/config; and we already
have a conn
> open, so it shouldn't take too long to just ask remotely. Ping me to
discuss if
> you feel really strongly...

Fair call. I'll remove the check, and also make proxy-ssh mutable.

> https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go
> File cmd/juju/ssh.go (right):

https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go#newcode151
> cmd/juju/ssh.go:151: // this avoids another API round-trip.
> On 2014/03/27 13:50:54, axw wrote:
> > On 2014/03/27 12:28:57, fwereade wrote:
> > > Not valid, sadly: we can't depend on bootstrap config being
available.
> >
> > Updated to use bootstrap config if there, else go via API. Is this
valid?

> Well, I don't love depending on the immutability, and we already have
a conn
> open. How much time do we save here, and how often?

It adds about 1s in my tests against Azure (10s vs. 9s). Not a big deal
I suppose.
I just realised that the second "juju ssh" call is making an unnecessary
API connection, so I'll fix that up.

https://codereview.appspot.com/77300045/

« Back to merge proposal