Merge lp:~jtv/juju-core/create-gwacl-sessions into lp:~go-bot/juju-core/trunk
Proposed by
Jeroen T. Vermeulen
| Status: | Merged |
|---|---|
| Approved by: | John A Meinel |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 1313 |
| Proposed branch: | lp:~jtv/juju-core/create-gwacl-sessions |
| Merge into: | lp:~go-bot/juju-core/trunk |
| Diff against target: |
212 lines (+164/-1) 2 files modified
environs/azure/environ.go (+34/-1) environs/azure/environ_test.go (+130/-0) |
| To merge this branch: | bzr merge lp:~jtv/juju-core/create-gwacl-sessions |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Tim Penhey (community) | Approve | ||
|
Review via email:
|
|||
Commit message
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.
Description of the change
The pattern was discussed with Gavin, the gwacl work with Raphaël.
To post a comment you must log in.
Revision history for this message
| John A Meinel (jameinel) wrote | # |
Revision history for this message
| Данило Шеган (danilo) wrote | # |
Revision history for this message
| Jeroen T. Vermeulen (jtv) wrote | # |
Revision history for this message
| Tim Penhey (thumper) wrote | # |
Revision history for this message
| Tim Penhey (thumper) | # |
review:
Approve
Revision history for this message
| John A Meinel (jameinel) wrote | # |
Revision history for this message
| Go Bot (go-bot) wrote | # |
Revision history for this message
| Jeroen T. Vermeulen (jtv) wrote | # |
Revision history for this message
| Go Bot (go-bot) wrote | # |
Revision history for this message
| Go Bot (go-bot) wrote | # |
Revision history for this message
| Go Bot (go-bot) wrote | # |
Download full text (22.4 KiB)
Revision history for this message
| Go Bot (go-bot) wrote | # |
Download full text (22.4 KiB)
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 2013-06-18 14:05, Jeroen T. Vermeulen wrote:
> Jeroen T. Vermeulen has proposed merging
> lp:~jtv/juju-core/create-gwacl-sessions into lp:juju-core.
>
> Commit message: 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.
>
> Requested reviews: juju hackers (juju)
>
> For more details, see:
> https:/
>
>
>
The pattern was discussed with Gavin, the gwacl work with
> Raphaël.
>
> https:/
>
You could improve the pattern by ensuring goroutine 2 starts first.
You could also encapsulate all of the logic in a helper with closures.
So something like:
func CheckLocking(c *C, lock sync.Locker, baseAction func()
interface{}, concurrentAction func()) interface{} {
resultChan := make(chan interface{})
startedChan := make(chan bool)
lock.Lock()
go func() {
startedChan <- true
resultChan <- baseAction()
}()
go func() {
//Make sure the baseAction has started
<-startedChan
concurrentA
lock.Unlock()
}()
return <-resultChan
}
then your test becomes:
func (EnvironSuite) TestGetSnapshot
original := azureEnviron{name: "old-name"}
base := func () interface{} {
return original.
}
concurrent := func() {
}
result := CheckLocking(
snapshot := result.
c.Check(
}
You can make it shorter by inlining the functions:
func (EnvironSuite) TestGetSnapshot
original := azureEnviron{name: "old-name"}
snapshot := CheckLocking(
func() interface{} {return original.
func() {original.name = "new-name"
c.Check(
}
Thoughts?
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://
iEYEARECAAYFAlH
JxAAnixDGf5LQ3u
=pE6P
-----END PGP SIGNATURE-----