Create gwacl session objects for management & API.

Create gwacl session objects for management & API.

I commented out one of the new functions at the last moment (as well as its
test), even though the test passes.  This was done because we're still working
out a detail in gwacl that may result in an incompatible change.

For some of the code in here, including Config(), the only interesting aspect
to test really is the locking.  It's the one thing that can go wrong.  None of
the other providers seem to test for this, and it's a bit of a losing game in
that whenever you forget to lock, you're likely to forget to test for locking
as well.  But I wrote up a simple, reusable pattern so that at least we can
guard against regressions.

The pattern was discussed with Gavin, the gwacl work with Raphaël.

https://code.launchpad.net/~jtv/juju-core/create-gwacl-sessions/+merge/170032

(do not edit description out of merge proposal)

[jtv.canonical, 2013-06-18 10:03:59]

Please take a look.

[Danilo, 2013-06-19 17:03:25]

This generally LGTM. I have further suggestions on the lock testing pattern, but
it may end up being too much work that you can't commit to right now.

Also, I am not sure of the value of having an unused method commented out. I
know that you plan to change the implementation of it RSN, but why is it better
to keep it commented out instead of having it in there. If there's desire for it
to not be accidentally used by anyone, I'd remove it completely from code.

https://codereview.appspot.com/10367045/diff/1/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/10367045/diff/1/environs/azure/environ_test.go...
environs/azure/environ_test.go:57: // The pattern is still not a full guarantee
that the lock is obeyed.  The
If you are going to go to this lengths to test the locking (which I applaud),
you might as well go the full distance :)

How about you simply introduce a separate lock (or you might be able to use
select with channels) which ensures that goroutine 2 only runs after goroutine 1
has started executing.  If you make it into a generic functionality that'd
perhaps live in juju-core/testing/ package, it'd suddenly become even more
useful.  If you feel that generalizing this would take you too far away from
your immediate goals (though I know you love generalizing code :), feel free to
ignore this advice.

[jtv.canonical, 2013-06-19 22:46:37]

On 2013/06/19 17:03:25, Danilo wrote:

> Also, I am not sure of the value of having an unused method commented out. I
> know that you plan to change the implementation of it RSN, but why is it
better
> to keep it commented out instead of having it in there. If there's desire for
it
> to not be accidentally used by anyone, I'd remove it completely from code.

Thanks.  Accidental use is not an issue here, since the whole provider is
nowhere near being operational yet anyway.  The thing I wanted to prevent is
unnecessary compile errors in that function, before it was even usable, because
of an incompatible change to gwacl that looked like it was coming down the line.
 Developers might end up with some branches on their system that only compiled
against the older gwacl and other branches that only compiled against the newer
gwacl, both relying on the same shared gwacl in their $GOPATH.  Pointless pain.

Since then we came up with another way to do it that does not involve any gwacl
changes.  But I didn't want to re-un-comment the method after proposing the
merge.  It might mislead the reviewer, plus I don't actually know yet whether
the diff in Rietveld would update.


>
https://codereview.appspot.com/10367045/diff/1/environs/azure/environ_test.go...
> environs/azure/environ_test.go:57: // The pattern is still not a full
guarantee
> that the lock is obeyed.  The
> If you are going to go to this lengths to test the locking (which I applaud),
> you might as well go the full distance :)
> 
> How about you simply introduce a separate lock (or you might be able to use
> select with channels) which ensures that goroutine 2 only runs after goroutine
1
> has started executing.  If you make it into a generic functionality that'd
> perhaps live in juju-core/testing/ package, it'd suddenly become even more
> useful.  If you feel that generalizing this would take you too far away from
> your immediate goals (though I know you love generalizing code :), feel free
to
> ignore this advice.

The extra synchronization would be good to have, I agree.  I only left it out
because the pattern needed to be simple enough that we could afford to use it. 
Until we have it in a generic, reusable form like you suggest.  I only looked
into that briefly at the time, but it looked a bit painful ("it's work") because
of the lack of generics.  Maybe later!

[jtv.canonical, 2013-06-19 23:06:06]

...And it turns out that John Meinel already worked out a generic form for us! 
See the Launchpad version of this merge proposal.  (We seem to be in some kind
of limbo now between Rietveld reviews that get partially synchronized to
Launchpad, and native Launchpad reviews.)

[jtv.canonical, 2013-06-20 02:24:00]

I came up with a much simpler and more generic form for the helper:
https://codereview.appspot.com/10433043/

[thumper, 2013-06-20 02:39:21]

On 2013/06/20 02:24:00, jtv.canonical wrote:
> I came up with a much simpler and more generic form for the helper:
> https://codereview.appspot.com/10433043/

LGTM for what I could follow.