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

Issue 6561063: cmd/jujud: implement uniter upgrade

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 6 months ago by rog
Modified:
11 years, 6 months ago
Reviewers:
mp+126714
Visibility:
Public.

Description

cmd/jujud: implement uniter upgrade https://code.launchpad.net/~rogpeppe/juju-core/064-uniter-upgrade/+merge/126714 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : cmd/jujud: implement uniter upgrade #

Patch Set 3 : cmd/jujud: implement uniter upgrade #

Total comments: 16

Patch Set 4 : cmd/jujud: implement uniter upgrade #

Patch Set 5 : cmd/jujud: implement uniter upgrade #

Total comments: 2

Patch Set 6 : cmd/jujud: implement uniter upgrade #

Patch Set 7 : cmd/jujud: implement uniter upgrade #

Patch Set 8 : cmd/jujud: implement uniter upgrade #

Total comments: 14

Patch Set 9 : cmd/jujud: implement uniter upgrade #

Patch Set 10 : cmd/jujud: implement uniter upgrade #

Unified diffs Side-by-side diffs Delta from patch set Stats (+198 lines, -108 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/jujud/agent.go View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M cmd/jujud/agent_test.go View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -4 lines 0 comments Download
M cmd/jujud/machine.go View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -8 lines 0 comments Download
M cmd/jujud/unit.go View 1 2 3 4 5 6 7 8 2 chunks +17 lines, -3 lines 0 comments Download
M cmd/jujud/upgrade.go View 1 2 3 4 5 6 7 8 6 chunks +34 lines, -11 lines 0 comments Download
M cmd/jujud/upgrade_test.go View 1 2 3 4 5 6 7 8 5 chunks +31 lines, -11 lines 0 comments Download
M environs/cloudinit/cloudinit.go View 1 2 3 4 5 6 3 chunks +6 lines, -4 lines 0 comments Download
M environs/jujutest/livetests.go View 3 chunks +28 lines, -9 lines 0 comments Download
M state/machine.go View 1 2 3 4 5 6 7 8 1 chunk +7 lines, -0 lines 0 comments Download
M state/machine_test.go View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M state/unit.go View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M worker/uniter/uniter.go View 4 5 6 2 chunks +52 lines, -43 lines 0 comments Download
M worker/uniter/uniter_test.go View 2 chunks +6 lines, -13 lines 0 comments Download

Messages

Total messages: 9
rog
Please take a look.
11 years, 6 months ago (2012-09-27 16:06:12 UTC) #1
niemeyer
LGTM with trivials, but please wait for William. https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go File cmd/jujud/unit.go (right): https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode57 cmd/jujud/unit.go:57: log.Printf("exiting ...
11 years, 6 months ago (2012-09-27 17:07:53 UTC) #2
fwereade
Record of live discussion, plus a couple of unrelated points. https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go File cmd/jujud/unit.go (right): https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode53 ...
11 years, 6 months ago (2012-09-28 08:32:19 UTC) #3
rog
Please take a look. https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go File cmd/jujud/unit.go (right): https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode53 cmd/jujud/unit.go:53: unit, err := a.runOnce() On ...
11 years, 6 months ago (2012-09-28 12:06:32 UTC) #4
fwereade
LGTM https://codereview.appspot.com/6561063/diff/28/cmd/jujud/machine.go File cmd/jujud/machine.go (right): https://codereview.appspot.com/6561063/diff/28/cmd/jujud/machine.go#newcode65 cmd/jujud/machine.go:65: log.Printf("restarting machiner after error: %v", err) Please make ...
11 years, 6 months ago (2012-09-28 12:40:51 UTC) #5
rog
Please take a look. https://codereview.appspot.com/6561063/diff/28/cmd/jujud/machine.go File cmd/jujud/machine.go (right): https://codereview.appspot.com/6561063/diff/28/cmd/jujud/machine.go#newcode65 cmd/jujud/machine.go:65: log.Printf("restarting machiner after error: %v", ...
11 years, 6 months ago (2012-09-28 12:53:51 UTC) #6
rog
Please take a look.
11 years, 6 months ago (2012-09-28 14:20:15 UTC) #7
niemeyer
Thank you very much for rolling back the additions. LGTM, with trivials: https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/unit.go File cmd/jujud/unit.go ...
11 years, 6 months ago (2012-09-28 14:39:34 UTC) #8
rog
11 years, 6 months ago (2012-09-28 15:28:58 UTC) #9
*** Submitted:

cmd/jujud: implement uniter upgrade

R=niemeyer, fwereade
CC=
https://codereview.appspot.com/6561063

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/unit.go
File cmd/jujud/unit.go (right):

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/unit.go#newcode60
cmd/jujud/unit.go:60: log.Printf("uniter: %v", err)
On 2012/09/28 14:39:34, niemeyer wrote:
> Let's please preserve the original:
> 
>     log.Printf("uniter error: %v")
> 
> "uniter: nil" doesn't mean much.

Changed to show a better error when it's nil, as discussed on line.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode35
cmd/jujud/upgrade.go:35: Tools     *state.Tools
On 2012/09/28 14:39:34, niemeyer wrote:
> NewTools would fit nicely here.

Done.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode40
cmd/jujud/upgrade.go:40: return "must restart: agent has been upgraded"
On 2012/09/28 14:39:34, niemeyer wrote:
> 
> "an agent upgrade is available"
> 
> Strictly speaking, by the time we get the error the agent hasn't been upgraded
> yet.

Done.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode44
cmd/jujud/upgrade.go:44: func (e *UpgradeReadyError) Upgrade() error {
On 2012/09/28 14:39:34, niemeyer wrote:
> This is great. I just suggest naming it as "ChangeAgentTools" so that we can
> mentally link the call sites to the underlying implementation.

Done.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode178
cmd/jujud/upgrade.go:178: return u.newError(currentTools, tools)
On 2012/09/28 14:39:34, niemeyer wrote:
> s/newError/upgradeReady/?

Done.

https://codereview.appspot.com/6561063/diff/5008/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6561063/diff/5008/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:138: fmt.Sprintf("machine-%d", cfg.MachineId),
On 2012/09/28 14:39:34, niemeyer wrote:
> How's that related to the other changes in the branch?
> 
> It doesn't feel like we want this since we have a single one of these per
> machine, and this means people have to use wildcards to work with the init
> script.

left alone, after discussion.

https://codereview.appspot.com/6561063/diff/5008/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6561063/diff/5008/state/machine.go#newcode46
state/machine.go:46: // PathKeys returned by any other entities from the same
state.
On 2012/09/28 14:39:34, niemeyer wrote:
> s/PathKeys/PathKey values/
> 
> As a good convention, when we name methods/functions/types, they should be the
> real ones.

done. i changed the one it was copied from too.
Sign in to reply to this message.

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