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

Issue 6488062: better-arranged charm manager

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

Description

better-arranged charm manager I think it's looking nicer now; I'm not convinced that adding a 3rd uniter state will make things any simpler or clearer, and I feel the cbSuccess mechanism in Manager.Update actually works pretty well. Thoughts? https://code.launchpad.net/~fwereade/juju-core/working-charm-manager/+merge/122281 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+700 lines, -129 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M worker/uniter/charm/charm.go View 5 chunks +163 lines, -49 lines 3 comments Download
M worker/uniter/charm/charm_test.go View 1 chunk +0 lines, -1 line 0 comments Download
M worker/uniter/modes.go View 6 chunks +124 lines, -12 lines 0 comments Download
M worker/uniter/uniter.go View 5 chunks +41 lines, -30 lines 0 comments Download
M worker/uniter/uniter_test.go View 18 chunks +370 lines, -37 lines 0 comments Download

Messages

Total messages: 1
niemeyer
12 years, 10 months ago (2012-08-31 14:29:11 UTC) #1
https://codereview.appspot.com/6488062/diff/1/worker/uniter/charm/charm.go
File worker/uniter/charm/charm.go (right):

https://codereview.appspot.com/6488062/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:52: updatesPath := filepath.Join(stateDir,
"charm-updates")
As I was reading this it occurred to me: instead of having stateDir, could we
possibly put things under .git? It's not a big deal, but it might simplify a bit
the interface and make the directory self-contained.

https://codereview.appspot.com/6488062/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:62: // DeployPath returns the path to the directory
in which the charm is deployed.
Why do we call it charmDir on one end and DeployPath on the other?

https://codereview.appspot.com/6488062/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:147: if err = cbSuccess(); err != nil {
I don't get the idea here. Update is managing its own state. We ask it to
Update, it updates, and tracks the fact it updates. Success means this method
returned without errors. The fact we're hooking into this feels like a hack.

I still think the previous commentary made around the status logic is sensible,
and is why you've been fighting with the current API. If you disagree, I'd
appreciate understanding your point of view.
Sign in to reply to this message.

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