Support juju debug-hooks

Support juju debug-hooks

This implementation is a departure from pyjuju, as we do not
have Zookeeper's ephemeral nodes to rely upon for locking.

The approach taken is described here:
  https://bugs.launchpad.net/juju-core/+bug/1027876/comments/11

Rather than periodically checking tmux, I've just gone for
a simple approach of calling "tmux has-session" each time a
hook is to be executed. If this proves to be too expensive,
then reassess.

https://code.launchpad.net/~axwalk/juju-core/lp1027876-implement-debug-hooks/+merge/177353

(do not edit description out of merge proposal)

[axw, 2013-07-30 07:14:13]

Please take a look.

[fwereade, 2013-07-31 02:02:07]

Thanks, this is looking really nice; I have a number of thoughts below, of
varying actionability (and perhaps sanity). Let me know what you think.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks.go
File cmd/juju/debughooks.go (right):

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks.go#newc...
cmd/juju/debughooks.go:79: return fmt.Errorf("unit %q does not contain hook %q",
unit.Name(), hook)
Would you check whether the python includes handling for, say, "db-relation-*"?
If so we should preserve it.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks.go#newc...
cmd/juju/debughooks.go:109: args := []string{"-l", "ubuntu", "-t", "-o",
"StrictHostKeyChecking no", "-o", "PasswordAuthentication no", host, "--"}
This looks awfully familiar. Can a useful set of common args be extracted?

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go
File cmd/juju/debughooks_test.go (right):

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go...
cmd/juju/debughooks_test.go:29: stderr string
Local convention would lean somewhat towards:

var debugHooksTests = []struct{
   ...
}{{
    args: []string{"mysql/0"},
    result: regexp.QuoteMeta(...),
}, {
    ...
}}

... ie tightening the indentation slightly and explicitly naming every non-zero
field.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go...
cmd/juju/debughooks_test.go:43: // "*" is a valid hook name: it means hook
everything.
I'd add these comments as an "info" field, and log them -- I find it really
helps when diagnosing surprising failures.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go...
cmd/juju/debughooks_test.go:114: c.Logf("testing juju debug-hooks %s", t.args)
Again by approximate mild convention:

for i, test := range debugHooksTests {
    c.Logf("test %d: %s\n%s", i, test.info, test.args)

...might be neatest. The "testing juju debug-hooks" bit is somewhat redundant in
the light of the test name, but the args are clearly interesting enough to log..

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go
File worker/uniter/context.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go#ne...
worker/uniter/context.go:13: 
I'd prefer to call this plain "debug".

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go#ne...
worker/uniter/context.go:156: 
It's big and distracting, and not really related to this CL; but, as you hit
packages using log it's nice to switch them over to loggo as you do. A followup
doing so would be much appreciated.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go#ne...
worker/uniter/context.go:158: 
maybe pull this block out into a runCharmHook() method?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client.go
File worker/uniter/debug/client.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client....
worker/uniter/debug/client.go:12: func (c *DebugHooksContext) ClientScript(hooks
[]string) string {
quibble quibble bikeshed -- how about:

func ClientScript(c *DebugHooksContext, hooks []string) string {

..? It's a little bit jarring to have the functionality of a single type spread
across files, and I think this works just as well as a utility func.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client....
worker/uniter/debug/client.go:23: s = strings.Replace(s, "{hook_args}",
strings.Join(hooks, " "), -1)
Hmm... maybe store these with a tiny bit more structure, with a view to easier
compatibility if and when we want to change anything related? Clients of version
X will doubtless at some stage connect to units of version Y, and a bit of
potential for flexibility could save a lot of headaches if and when any of this
stuff needs to change. YAML along the lines of `hooks: [foo, bar, baz]` would be
fairly normal. Thoughts?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client....
worker/uniter/debug/client.go:84: END
I think readability would be enhanced if you pulled this out into a tmuxConf
const and subbed it in at runtime.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client....
worker/uniter/debug/client.go:95: exec tmux attach -t {unit_name}
Shouldn't we kill the session here? Otherwise, next time the server runs a hook,
it'll find the session and start creating windows and waiting for input that'll
never come. Right?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/common.go
File worker/uniter/debug/common.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/common....
worker/uniter/debug/common.go:15: type DebugHooksContext struct {
I think we can drop the stuttering -- debug.HooksContext sounds fine to me.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/common....
worker/uniter/debug/common.go:25: basename := fmt.Sprintf("juju-%s-debug-hooks",
state.UnitTag(c.Unit))
imminently names.UnitTag, fwiw

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server.go
File worker/uniter/debug/server.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server....
worker/uniter/debug/server.go:18: hooks map[string]bool
utils/set.Strings?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server....
worker/uniter/debug/server.go:43: proc.Kill()
What happens in all these goroutines when we run N hook windows in a row without
exiting the client?

On balance, I think it's good that we don't try to kill the session here,
because staying in debug mode when the client unexpectedly disconnects is
actually pretty cool, if you have some reason to believe that the client is
likely to come back at some point; if so, they're likely to be pleased that we
waited for them.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server....
worker/uniter/debug/server.go:95: tmux new-session -d -s $JUJU_UNIT_NAME 2>&1 |
cat > /dev/null || true
Is this still needed? Aren't we now using the existence of this session as a
condition for triggering this script?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_...
File worker/uniter/debug/server_test.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_...
worker/uniter/debug/server_test.go:46: s.setenv("PATH",
s.fakebin+":"+os.Getenv("PATH"))
This also looks awfully familiar; would you have a quick look through the source
for similar constructions and maybe open a bug and tag it tech-debt if it seems
like there are wins to be had?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_...
worker/uniter/debug/server_test.go:109: // in exceptional situations.
Doesn't match test name, don't think is needed.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_...
worker/uniter/debug/server_test.go:147: // exit cleanly (as if the PID were real
and no longer running).
Hmm. Is there any way to fail a hook once we're debugging it? A `juju-fail` hook
command would be handy in a couple of contexts then: (1) charm authors debugging
hooks could fail out and retry with a clean context if they just did `juju
resolved --retry` after `juju-fail`ing and killing the original window, and (2)
we could also grab those messages and put them in status, and thus make it a
useful channel for authors to communicate charm-specific errors back to users.
I'm worried people would abuse it to spam users with, say, python tracebacks in
the middle of status... but we can make it part of the charm approval
guidelines, I guess?

[axw, 2013-07-31 06:03:03]

Please take a look.

[axw1, 2013-07-31 06:04:23]

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks.go
File cmd/juju/debughooks.go (right):

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks.go#newc...
cmd/juju/debughooks.go:79: return fmt.Errorf("unit %q does not contain hook %q",
unit.Name(), hook)
On 2013/07/31 02:02:07, fwereade wrote:
> Would you check whether the python includes handling for, say,
"db-relation-*"?
> If so we should preserve it.

It didn't before: it's just '*' by itself (i.e. the default), or a named hook
without wildcards. I think it makes sense to support it, but I'd rather leave it
till another changeset.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks.go#newc...
cmd/juju/debughooks.go:109: args := []string{"-l", "ubuntu", "-t", "-o",
"StrictHostKeyChecking no", "-o", "PasswordAuthentication no", host, "--"}
On 2013/07/31 02:02:07, fwereade wrote:
> This looks awfully familiar. Can a useful set of common args be extracted?

Done. I've modified it to defer to SSHCommand, and modified SSHCommand.Run to
check if Conn is already initialised.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go
File cmd/juju/debughooks_test.go (right):

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go...
cmd/juju/debughooks_test.go:29: stderr string
On 2013/07/31 02:02:07, fwereade wrote:
> Local convention would lean somewhat towards:
> 
> var debugHooksTests = []struct{
>    ...
> }{{
>     args: []string{"mysql/0"},
>     result: regexp.QuoteMeta(...),
> }, {
>     ...
> }}
> 
> ... ie tightening the indentation slightly and explicitly naming every
non-zero
> field.

Done.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go...
cmd/juju/debughooks_test.go:43: // "*" is a valid hook name: it means hook
everything.
On 2013/07/31 02:02:07, fwereade wrote:
> I'd add these comments as an "info" field, and log them -- I find it really
> helps when diagnosing surprising failures.

Done.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go...
cmd/juju/debughooks_test.go:114: c.Logf("testing juju debug-hooks %s", t.args)
On 2013/07/31 02:02:07, fwereade wrote:
> Again by approximate mild convention:
> 
> for i, test := range debugHooksTests {
>     c.Logf("test %d: %s\n%s", i, test.info, test.args)
> 
> ...might be neatest. The "testing juju debug-hooks" bit is somewhat redundant
in
> the light of the test name, but the args are clearly interesting enough to
log..

Done.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go
File worker/uniter/context.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go#ne...
worker/uniter/context.go:13: 
On 2013/07/31 02:02:07, fwereade wrote:
> I'd prefer to call this plain "debug".

Done.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go#ne...
worker/uniter/context.go:156: 
On 2013/07/31 02:02:07, fwereade wrote:
> It's big and distracting, and not really related to this CL; but, as you hit
> packages using log it's nice to switch them over to loggo as you do. A
followup
> doing so would be much appreciated.

Will do.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go#ne...
worker/uniter/context.go:158: 
On 2013/07/31 02:02:07, fwereade wrote:
> maybe pull this block out into a runCharmHook() method?

Done.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client.go
File worker/uniter/debug/client.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client....
worker/uniter/debug/client.go:12: func (c *DebugHooksContext) ClientScript(hooks
[]string) string {
On 2013/07/31 02:02:07, fwereade wrote:
> quibble quibble bikeshed -- how about:
> 
> func ClientScript(c *DebugHooksContext, hooks []string) string {
> 
> ..? It's a little bit jarring to have the functionality of a single type
spread
> across files, and I think this works just as well as a utility func.

Done.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client....
worker/uniter/debug/client.go:23: s = strings.Replace(s, "{hook_args}",
strings.Join(hooks, " "), -1)
On 2013/07/31 02:02:07, fwereade wrote:
> Hmm... maybe store these with a tiny bit more structure, with a view to easier
> compatibility if and when we want to change anything related? Clients of
version
> X will doubtless at some stage connect to units of version Y, and a bit of
> potential for flexibility could save a lot of headaches if and when any of
this
> stuff needs to change. YAML along the lines of `hooks: [foo, bar, baz]` would
be
> fairly normal. Thoughts?

SGTM. Done.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client....
worker/uniter/debug/client.go:84: END
On 2013/07/31 02:02:07, fwereade wrote:
> I think readability would be enhanced if you pulled this out into a tmuxConf
> const and subbed it in at runtime.

Done.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client....
worker/uniter/debug/client.go:95: exec tmux attach -t {unit_name}
On 2013/07/31 02:02:07, fwereade wrote:
> Shouldn't we kill the session here? Otherwise, next time the server runs a
hook,
> it'll find the session and start creating windows and waiting for input
that'll
> never come. Right?

The "tmux attach" attaches to the terminal; when the terminal is closed, the
session ends.

Actually that can be cleaned up to just "exec tmux new-session -s
{unit_name}...". The separate commands are not necessary anymore, since the
server never creates the session.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/common.go
File worker/uniter/debug/common.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/common....
worker/uniter/debug/common.go:15: type DebugHooksContext struct {
On 2013/07/31 02:02:07, fwereade wrote:
> I think we can drop the stuttering -- debug.HooksContext sounds fine to me.

Done.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/common....
worker/uniter/debug/common.go:25: basename := fmt.Sprintf("juju-%s-debug-hooks",
state.UnitTag(c.Unit))
On 2013/07/31 02:02:07, fwereade wrote:
> imminently names.UnitTag, fwiw

Merged from trunk and fixed, thanks.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server.go
File worker/uniter/debug/server.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server....
worker/uniter/debug/server.go:18: hooks map[string]bool
On 2013/07/31 02:02:07, fwereade wrote:
> utils/set.Strings?

Done.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server....
worker/uniter/debug/server.go:43: proc.Kill()
On 2013/07/31 02:02:07, fwereade wrote:
> What happens in all these goroutines when we run N hook windows in a row
without
> exiting the client?

At the moment, nothing much: they'll bank up on the flock, then when the client
finally does close, they'll all kill the script processes spawned above.

> On balance, I think it's good that we don't try to kill the session here,
> because staying in debug mode when the client unexpectedly disconnects is
> actually pretty cool, if you have some reason to believe that the client is
> likely to come back at some point; if so, they're likely to be pleased that we
> waited for them.

That sounds fine, but it does concern me that someone might start a debug-hooks
session, then have their connection severed (say power out), and forget that
they were in the middle of debugging when they come back. Now their unit is in
debug-mode forever. How do we deal with that? Have the unit update its state, so
users can see which units are being debugged via juju status?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server....
worker/uniter/debug/server.go:95: tmux new-session -d -s $JUJU_UNIT_NAME 2>&1 |
cat > /dev/null || true
On 2013/07/31 02:02:07, fwereade wrote:
> Is this still needed? Aren't we now using the existence of this session as a
> condition for triggering this script?

Indeed we are, thanks. Removed.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_...
File worker/uniter/debug/server_test.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_...
worker/uniter/debug/server_test.go:46: s.setenv("PATH",
s.fakebin+":"+os.Getenv("PATH"))
On 2013/07/31 02:02:07, fwereade wrote:
> This also looks awfully familiar; would you have a quick look through the
source
> for similar constructions and maybe open a bug and tag it tech-debt if it
seems
> like there are wins to be had?

Yeah I took it from ssh_test.go. I've taken a look and there are a bunch of
tests that do the same thing: change PATH and then set it back in teardown.

I don't think there's much value in breaking it out at the moment, but I logged
a bug anyway: https://bugs.launchpad.net/juju-core/+bug/1206759

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_...
worker/uniter/debug/server_test.go:109: // in exceptional situations.
On 2013/07/31 02:02:07, fwereade wrote:
> Doesn't match test name, don't think is needed.

Done.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_...
worker/uniter/debug/server_test.go:147: // exit cleanly (as if the PID were real
and no longer running).
On 2013/07/31 02:02:07, fwereade wrote:
> Hmm. Is there any way to fail a hook once we're debugging it? A `juju-fail`
hook
> command would be handy in a couple of contexts then: (1) charm authors
debugging
> hooks could fail out and retry with a clean context if they just did `juju
> resolved --retry` after `juju-fail`ing and killing the original window, and
(2)
> we could also grab those messages and put them in status, and thus make it a
> useful channel for authors to communicate charm-specific errors back to users.
> I'm worried people would abuse it to spam users with, say, python tracebacks
in
> the middle of status... but we can make it part of the charm approval
> guidelines, I guess?

Currently, the only way to fail the debug-hook is for the user to kill -9 their
ssh connection. Not very elegant, and obviously doesn't support the message
passing you're talking about.

I don't mind this suggestion, but can it be done after getting to feature parity
with pyjuju? Unless I missed something, and pyjuju has a method of failing debug
hooks?

[wallyworld, 2013-08-02 00:09:17]

LGTM, but I am not at all familiar with Juju's debug hooks. Not sure if we would
prefer another opinion. If it works, I say just land it and we can iterate.

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.go
File cmd/juju/debughooks_test.go (right):

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.g...
cmd/juju/debughooks_test.go:14: "regexp"
Please group imports

https://codereview.appspot.com/12019043/diff/20001/worker/uniter/context.go
File worker/uniter/context.go (right):

https://codereview.appspot.com/12019043/diff/20001/worker/uniter/context.go#n...
worker/uniter/context.go:164: if e := rctx.WriteSettings(); e != nil {
convention is to use err for errors

[axw, 2013-08-02 01:11:54]

Please take a look.

[axw1, 2013-08-02 01:12:52]

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.go
File cmd/juju/debughooks_test.go (right):

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.g...
cmd/juju/debughooks_test.go:14: "regexp"
On 2013/08/02 00:09:17, wallyworld wrote:
> Please group imports

Done.

https://codereview.appspot.com/12019043/diff/20001/worker/uniter/context.go
File worker/uniter/context.go (right):

https://codereview.appspot.com/12019043/diff/20001/worker/uniter/context.go#n...
worker/uniter/context.go:164: if e := rctx.WriteSettings(); e != nil {
On 2013/08/02 00:09:17, wallyworld wrote:
> convention is to use err for errors

This is existing code, I just moved it. I was going to change it to err anyway,
but there's a reason for it being named differently: see the conditional
assignment of "err = e" below.

[fwereade, 2013-08-02 09:31:30]

LGTM, just a couple of trivials.

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks.go
File cmd/juju/debughooks.go (right):

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks.go#new...
cmd/juju/debughooks.go:102: args := []string{"--", fmt.Sprintf("sudo /bin/bash
-c '%s'", fmt.Sprintf(`F=$(mktemp); echo %s | base64 -d > $F; . $F`, script))}
Could we break the inner Sprintf out please?

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.go
File cmd/juju/debughooks_test.go (right):

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.g...
cmd/juju/debughooks_test.go:60: args:   []string{"nonexistant/123"},
nonexistent

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.g...
cmd/juju/debughooks_test.go:71: m := s.makeMachines(3, c)
"machines" would be nicer than "m"

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.g...
cmd/juju/debughooks_test.go:79: c.Assert(err, IsNil)
dummy := s.AddTestingCharm(c, "dummy")

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

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/ssh.go#newcode84
cmd/juju/ssh.go:84: c.Conn, err = juju.NewConnFromName(c.EnvName)
If we're doing this, how about adding it to EnvCommandBase instead? Almost every
command that uses an env also needs to do this.

[axw1, 2013-08-02 10:06:25]

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks.go
File cmd/juju/debughooks.go (right):

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks.go#new...
cmd/juju/debughooks.go:102: args := []string{"--", fmt.Sprintf("sudo /bin/bash
-c '%s'", fmt.Sprintf(`F=$(mktemp); echo %s | base64 -d > $F; . $F`, script))}
On 2013/08/02 09:31:30, fwereade wrote:
> Could we break the inner Sprintf out please?

Done.

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.go
File cmd/juju/debughooks_test.go (right):

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.g...
cmd/juju/debughooks_test.go:60: args:   []string{"nonexistant/123"},
On 2013/08/02 09:31:30, fwereade wrote:
> nonexistent

Done.

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.g...
cmd/juju/debughooks_test.go:71: m := s.makeMachines(3, c)
On 2013/08/02 09:31:30, fwereade wrote:
> "machines" would be nicer than "m"

Done.

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.g...
cmd/juju/debughooks_test.go:79: c.Assert(err, IsNil)
On 2013/08/02 09:31:30, fwereade wrote:
> dummy := s.AddTestingCharm(c, "dummy")

Done.

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/debughooks_test.g...
cmd/juju/debughooks_test.go:79: c.Assert(err, IsNil)
On 2013/08/02 09:31:30, fwereade wrote:
> dummy := s.AddTestingCharm(c, "dummy")

Done.

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

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/ssh.go#newcode84
cmd/juju/ssh.go:84: c.Conn, err = juju.NewConnFromName(c.EnvName)
On 2013/08/02 09:31:30, fwereade wrote:
> If we're doing this, how about adding it to EnvCommandBase instead? Almost
every
> command that uses an env also needs to do this.

I don't think that it's particularly valuable. SSHCommand is special, because
it's reusing the Conn field initialised by an enclosing command.

The reason for the initConn method is to document whose responsibility it is to
close the connection, and make people aware that the conn is stored and reused.

[axw, 2013-08-02 10:08:26]

Please take a look.

[fwereade, 2013-08-02 10:56:28]

LGTM, thanks.

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

https://codereview.appspot.com/12019043/diff/20001/cmd/juju/ssh.go#newcode84
cmd/juju/ssh.go:84: c.Conn, err = juju.NewConnFromName(c.EnvName)
On 2013/08/02 10:06:26, axw1 wrote:
> On 2013/08/02 09:31:30, fwereade wrote:
> > If we're doing this, how about adding it to EnvCommandBase instead? Almost
> every
> > command that uses an env also needs to do this.
> 
> I don't think that it's particularly valuable. SSHCommand is special, because
> it's reusing the Conn field initialised by an enclosing command.
> 
> The reason for the initConn method is to document whose responsibility it is
to
> close the connection, and make people aware that the conn is stored and
reused.

Fair enough, SGTM.