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

Issue 6345072: state: added services watcher for added and removed services (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 1 month ago by TheMue
Modified:
7 years ago
Reviewers:
mp+113728
Visibility:
Public.

Description

state: added services watcher for added and removed services Provisioning and firewall need notifications about added and removed services. The services watcher observes the topology and returns this information. https://code.launchpad.net/~themue/juju-core/go-state-service-watcher/+merge/113728 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 14

Patch Set 2 : state: added services watcher for added and removed services #

Unified diffs Side-by-side diffs Delta from patch set Stats (+142 lines, -1 line) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M state/state.go View 1 1 chunk +8 lines, -1 line 0 comments Download
M state/state_test.go View 1 chunk +54 lines, -0 lines 0 comments Download
M state/watcher.go View 1 1 chunk +78 lines, -0 lines 0 comments Download

Messages

Total messages: 3
TheMue
Please take a look.
7 years, 1 month ago (2012-07-06 11:29:40 UTC) #1
niemeyer
LGTM.. doc suggestions only, thanks. https://codereview.appspot.com/6345072/diff/1/state/state.go File state/state.go (right): https://codereview.appspot.com/6345072/diff/1/state/state.go#newcode68 state/state.go:68: // WatchServices watches for ...
7 years, 1 month ago (2012-07-09 18:35:46 UTC) #2
TheMue
7 years, 1 month ago (2012-07-10 11:06:35 UTC) #3
*** Submitted:

state: added services watcher for added and removed services

Provisioning and firewall need notifications about added and
removed services. The services watcher observes the topology
and returns this information.

R=niemeyer
CC=
https://codereview.appspot.com/6345072

https://codereview.appspot.com/6345072/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6345072/diff/1/state/state.go#newcode68
state/state.go:68: // WatchServices watches for new Machines added or removed.
On 2012/07/09 18:35:46, niemeyer wrote:
> // WatchServices returns a watcher for observing services
> // being added or removed.

Done.

https://codereview.appspot.com/6345072/diff/1/state/state.go#newcode73
state/state.go:73: // WatchMachines watches for new Machines added or removed.
On 2012/07/09 18:35:46, niemeyer wrote:
> // WatchMachines returns a watcher for observing machines
> // being added or removed.

Done.

https://codereview.appspot.com/6345072/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6345072/diff/1/state/watcher.go#newcode68
state/watcher.go:68: // services that have been added to or removed.
On 2012/07/09 18:35:46, niemeyer wrote:
> // ServicesChange holds services that were added or removed
> // from the environment.

Done.

https://codereview.appspot.com/6345072/diff/1/state/watcher.go#newcode74
state/watcher.go:74: // ServicesWatcher observes the addition and removal
On 2012/07/09 18:35:46, niemeyer wrote:
> Please reindent (this fits in a single line easily).

Done.

https://codereview.appspot.com/6345072/diff/1/state/watcher.go#newcode84
state/watcher.go:84: // for service changes.
On 2012/07/09 18:35:46, niemeyer wrote:
> "service changes" is too vague. This comment can either go away entirely or be
> replaced with the obvious:
> 
> // newServicesWatcher returns a new ServicesWatcher.

Done.

https://codereview.appspot.com/6345072/diff/1/state/watcher.go#newcode95
state/watcher.go:95: // Changes returns a channel that will receive changes when
services
On 2012/07/09 18:35:46, niemeyer wrote:
> s/receive changes/receive a notification/

Done.

https://codereview.appspot.com/6345072/diff/1/state/watcher.go#newcode97
state/watcher.go:97: // the first event on the channel holds the initial state
as returned
On 2012/07/09 18:35:46, niemeyer wrote:
> s/as returned/as would be returned/

Done.
Sign in to reply to this message.

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