Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(47)

Issue 6229046: environ: add SetConfig (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years ago by dave
Modified:
13 years ago
Reviewers:
Visibility:
Public.

Description

environ: add SetConfig Discussion: This proposal adds the ability for Environs to update their configuration. This facility replaces the previously proposed proxy Environ and provides more explicit control over when an Environs configuration is updated. As a side effect, several mutexes had to be added to control the visibility of variables that could be affected by SetConfig. Sorry Roger. Additionally, in the dummy provider, the zookeeper value is still parsed as a configuration item but is not used. It wasn't used before either, so I've removed it from dummy.environConfig. https://code.launchpad.net/~dave-cheney/juju/go-environs-setconfig/+merge/106930 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : environ: add SetConfig #

Patch Set 3 : environ: add SetConfig #

Total comments: 39

Patch Set 4 : environ: add SetConfig #

Total comments: 6

Patch Set 5 : environ: add SetConfig #

Total comments: 2

Patch Set 6 : environ: add SetConfig #

Patch Set 7 : environ: add SetConfig #

Patch Set 8 : environ: add SetConfig #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+116 lines, -72 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M environs/dummy/environs.go View 1 2 3 4 8 chunks +26 lines, -12 lines 0 comments Download
M environs/ec2/config_test.go View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M environs/ec2/ec2.go View 1 2 3 4 5 6 7 14 chunks +67 lines, -28 lines 6 comments Download
M environs/ec2/export_test.go View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M environs/ec2/storage.go View 1 2 3 4 5 6 2 chunks +16 lines, -30 lines 2 comments Download
M environs/interface.go View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 16
dave_cheney.net
Please take a look.
13 years ago (2012-05-23 03:18:48 UTC) #1
rog
i realise this is a WIP, but i thought i'd make some comments anyway... i ...
13 years ago (2012-05-23 08:27:34 UTC) #2
niemeyer
Thanks Dave, I quite appreciate this. Lots of comments, but almost nothing important. https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go File ...
13 years ago (2012-05-23 19:35:54 UTC) #3
dave_cheney.net
On 2012/05/23 08:27:34, rog wrote: > i realise this is a WIP, but i thought ...
13 years ago (2012-05-23 21:26:31 UTC) #4
dave_cheney.net
Please take a look. https://codereview.appspot.com/6229046/diff/4001/environs/dummy/environs.go File environs/dummy/environs.go (right): https://codereview.appspot.com/6229046/diff/4001/environs/dummy/environs.go#newcode91 environs/dummy/environs.go:91: broken bool On 2012/05/23 08:27:34, ...
13 years ago (2012-05-23 21:57:06 UTC) #5
niemeyer
LGTM assuming at least the panic fixed. https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go File environs/ec2/ec2.go (right): https://codereview.appspot.com/6229046/diff/4001/environs/ec2/ec2.go#newcode525 environs/ec2/ec2.go:525: e.configLock.Lock() On ...
13 years ago (2012-05-23 23:27:06 UTC) #6
dave_cheney.net
Please take a look. https://codereview.appspot.com/6229046/diff/12001/environs/ec2/ec2.go File environs/ec2/ec2.go (right): https://codereview.appspot.com/6229046/diff/12001/environs/ec2/ec2.go#newcode181 environs/ec2/ec2.go:181: panic(fmt.Sprintf("configuration was %T, expected %T", ...
13 years ago (2012-05-24 00:08:50 UTC) #7
niemeyer
Putting it back to WIP due to the introduced bug. https://codereview.appspot.com/6229046/diff/12001/environs/interface.go File environs/interface.go (right): https://codereview.appspot.com/6229046/diff/12001/environs/interface.go#newcode111 ...
13 years ago (2012-05-24 21:41:05 UTC) #8
dave_cheney.net
Patch set 8 incorporates https://codereview.appspot.com/6244053.
13 years ago (2012-05-25 00:38:56 UTC) #9
dave_cheney.net
Please take a look.
13 years ago (2012-05-25 00:39:58 UTC) #10
rog
https://codereview.appspot.com/6229046/diff/20001/environs/ec2/ec2.go File environs/ec2/ec2.go (right): https://codereview.appspot.com/6229046/diff/20001/environs/ec2/ec2.go#newcode184 environs/ec2/ec2.go:184: e.storage.setBucket(e.s3().Bucket(config.bucket)) On 2012/05/24 21:41:05, niemeyer wrote: > This seems ...
13 years ago (2012-05-25 08:07:22 UTC) #11
dave_cheney.net
Roger, do you have bandwidth implement makeBucket properly. I've put it on my backlog but ...
13 years ago (2012-05-25 08:09:33 UTC) #12
rog
np. i'll do it now. On 25 May 2012 09:09, Dave Cheney <dave@cheney.net> wrote: > ...
13 years ago (2012-05-25 08:21:05 UTC) #13
niemeyer
https://codereview.appspot.com/6229046/diff/18004/environs/ec2/ec2.go File environs/ec2/ec2.go (right): https://codereview.appspot.com/6229046/diff/18004/environs/ec2/ec2.go#newcode179 environs/ec2/ec2.go:179: func (e *environ) storage() *storage { Do we need ...
13 years ago (2012-05-25 19:08:02 UTC) #14
niemeyer
I went to grab the branch to help out and noticed that you actually already ...
13 years ago (2012-05-25 19:11:40 UTC) #15
niemeyer
13 years ago (2012-05-25 19:24:52 UTC) #16
Dave has replaced this proposal with:

https://codereview.appspot.com/6256050/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b