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

Issue 6501072: worker: new package to factor out some common code from workers

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 10 months ago by rog
Modified:
12 years, 10 months ago
Reviewers:
mp+122300
Visibility:
Public.

Description

worker: new package to factor out some common code from workers Also simplify workers a little. https://code.launchpad.net/~rogpeppe/juju-core/045-worker-wait-for-environ/+merge/122300 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : worker: new package to factor out some common code from workers #

Total comments: 15

Patch Set 3 : worker: new package to factor out some common code from workers #

Unified diffs Side-by-side diffs Delta from patch set Stats (+177 lines, -86 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M state/watcher/watcher.go View 1 2 2 chunks +8 lines, -4 lines 0 comments Download
A worker/environ.go View 1 2 1 chunk +34 lines, -0 lines 0 comments Download
A worker/environ_test.go View 1 2 1 chunk +72 lines, -0 lines 0 comments Download
A worker/export_test.go View 1 chunk +9 lines, -0 lines 0 comments Download
M worker/firewaller/firewaller.go View 1 2 11 chunks +33 lines, -47 lines 0 comments Download
M worker/provisioner/provisioner.go View 1 2 4 chunks +19 lines, -35 lines 0 comments Download

Messages

Total messages: 3
rog
Please take a look.
12 years, 10 months ago (2012-08-31 15:19:11 UTC) #1
niemeyer
Several great improvement here. Thanks. LGTM assuming you find these sensible enough: https://codereview.appspot.com/6501072/diff/2001/state/watcher/watcher.go File state/watcher/watcher.go ...
12 years, 10 months ago (2012-08-31 15:57:17 UTC) #2
rog
12 years, 10 months ago (2012-08-31 16:17:10 UTC) #3
*** Submitted:

worker: new package to factor out some common code from workers

Also simplify workers a little.

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

https://codereview.appspot.com/6501072/diff/2001/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6501072/diff/2001/state/watcher/watcher.go#new...
state/watcher/watcher.go:15: type Errorer interface {
On 2012/08/31 15:57:17, niemeyer wrote:
> Errer feels a lot better as a name for this.

Done.

https://codereview.appspot.com/6501072/diff/2001/worker/environ.go
File worker/environ.go (right):

https://codereview.appspot.com/6501072/diff/2001/worker/environ.go#newcode18
worker/environ.go:18: case <-stop:
On 2012/08/31 15:57:17, niemeyer wrote:
> s/stop/dying/, so we can use ErrDying below correctly.

Done.

https://codereview.appspot.com/6501072/diff/2001/worker/environ.go#newcode22
worker/environ.go:22: return nil, w.Err()
On 2012/08/31 15:57:17, niemeyer wrote:
> MustErr, I suppose?

Done.

https://codereview.appspot.com/6501072/diff/2001/worker/environ.go#newcode29
worker/environ.go:29: log.Printf("firewaller loaded invalid environment
configuration: %v", err)
On 2012/08/31 15:57:17, niemeyer wrote:
> s/firewaller //

Done.

https://codereview.appspot.com/6501072/diff/2001/worker/environ_test.go
File worker/environ_test.go (right):

https://codereview.appspot.com/6501072/diff/2001/worker/environ_test.go#newco...
worker/environ_test.go:54: c.Assert(err, IsNil)
On 2012/08/31 15:57:17, niemeyer wrote:
> Can't Assert within a goroutine. It will not stop the test and the done below
> will never be executed.

Done.

https://codereview.appspot.com/6501072/diff/2001/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6501072/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:60: return nil
On 2012/08/31 15:57:17, niemeyer wrote:
> tomb.ErrDying

Done.

https://codereview.appspot.com/6501072/diff/2001/worker/provisioner/provision...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/6501072/diff/2001/worker/provisioner/provision...
worker/provisioner/provisioner.go:69: return nil
On 2012/08/31 15:57:17, niemeyer wrote:
> tomb.ErrDying

Done.
Sign in to reply to this message.

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