git-backed charm dir and upgrades

git-backed charm dir and upgrades

Still WIP; am vacillating over design.

https://code.launchpad.net/~fwereade/juju-core/git-charm-upgrades/+merge/121878

(do not edit description out of merge proposal)

[niemeyer, 2012-08-29 15:56:16]

This coming along well. A few ideas to help the brainstorming:

https://codereview.appspot.com/6488051/diff/1/worker/uniter/charm/charm.go
File worker/uniter/charm/charm.go (right):

https://codereview.appspot.com/6488051/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:39: repoDir    *GitDir
We need better names here.

https://codereview.appspot.com/6488051/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:50: repoDir:    NewGitDir(filepath.Join(stateDir,
"repo")),
And better path names too.

https://codereview.appspot.com/6488051/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:294: // InitHistory deletes d's .git directory and
replaces it with a copy of
This seems a bit of a wild operation. We can do this more politely with stock
git using something like:

// Clone clones d into path and returns the GitDir for it.
// If checkout is false, the repository is created without
// a working tree.
func (d *GitDir) Clone(path string, checkout bool) (*GitDir, error) {
    if checkout {
        return d.cmd("git", "clone", ".", path)
    }
    return d.cmd("git", "clone", "--no-checkout", ".", path)
}

https://codereview.appspot.com/6488051/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:323: // The new revision will hold the current
contents of the dir, including empty
People won't expect Commitf to mean "add whatever is in the working tree". I
suggest splitting AddAll from Commitf.

https://codereview.appspot.com/6488051/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:363: "%s command failed: %s\npath: %s\nargs:
%#v\n%s",
Breaking this kind of line is more commonly done as:

log.Printf("%s command failed: %s\npath: %s\nargs: %#v\n%s",
        name, err, d.path, args, out)

https://codereview.appspot.com/6488051/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6488051/diff/1/worker/uniter/uniter.go#newcode34
worker/uniter/uniter.go:34: charmMgr *charm.Manager
The fact we have both charm and charmMgr here pointing to the same thing is
probably one of the reasons you're feeling bad about the current arrangement. If
we have something responsible for managing that directory, let's delegate all
the responsibilities for its management to it. I suggest preserving its name as
"charm", but as a *charm.Manager.

https://codereview.appspot.com/6488051/diff/1/worker/uniter/uniter.go#newcode49
worker/uniter/uniter.go:49: if err = charmDir.Init(); err != nil {
This should be done by the manager. The manager must also watch out for
unfinished operations and bring the directory back into sanity, so that the
uniter has something good to work with.

https://codereview.appspot.com/6488051/diff/1/worker/uniter/uniter.go#newcode115
worker/uniter/uniter.go:115: if err := u.charmMgr.WriteState(st, sch.URL()); err
!= nil {
I see another reason why this is feeling a bit icky. The manager is tracking
status that is the *uniter* status. We shouldn't have to call the manager to
tell it that we're going to call it, and shouldn't have to call it again to tell
it that we're done calling it (at the end of this function).

We're doing this today because the status we're saving here is the status of the
*uniter* that we want to track, but note that the URL shouldn't really have to
be sent separately to the uniter. It knows it's getting asked to update to this
URL because it has the whole charm being handed in below.

This is going to feel a lot better if we separate these concerns out, and have
the manager managing its own state only (has it updated? has it died in the
middle of a git merge? etc), and the uniter having its state saved elsewhere.