add charm.Deployer; extend charm.GitDir

add charm.Deployer; extend charm.GitDir

Deployer makes heavy use of GitDirs to implement the charm install/upgrade
procedures.

https://code.launchpad.net/~fwereade/juju-core/uniter-charm-deployer/+merge/122807

(do not edit description out of merge proposal)

[fwereade, 2012-09-05 08:02:44]

Please take a look.

[fwereade, 2012-09-05 09:19:34]

Please take a look.

[TheMue, 2012-09-05 11:57:37]

LGTM, only one little note.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer.go
File worker/uniter/charm/deployer.go (right):

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:13: // Deployer maintains a git repository
tracking a series of charm versions,
Haven't followed the discussion, but IMHO a Deployer would be an interface while
this is its implementation as GitDeployer. So later maybe a BazaarDeployer or
MercurialDeployer could be added.

[niemeyer, 2012-09-05 22:47:21]

Very nice. LGTM.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer.go
File worker/uniter/charm/deployer.go (right):

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:13: // Deployer maintains a git repository
tracking a series of charm versions,
On 2012/09/05 11:57:38, TheMue wrote:
> Haven't followed the discussion, but IMHO a Deployer would be an interface
while
> this is its implementation as GitDeployer. So later maybe a BazaarDeployer or
> MercurialDeployer could be added.

This is over-generalizing too early, IMO. We have no intention of ever having
parallel implementations of this logic, and this is more an implementation
detail of how to do proper upgrading than a real feature we're offering people.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:30: func (d *Deployer) SetCharm(bun
*charm.Bundle, url *charm.URL) error {
// Read present state of current.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:47: }
// Prepare a temporary update repository, using current's
// history if it exists.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:61: }
// Write down the desired new state and commit.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:70: }
// Atomically rename temporary update repository to current.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:75: return os.Rename(tmplink, d.current.Path())
If we're keeping the repository directory around, it'd be nice to have something
more human than something like "update-KJhkjwhek" as a name. Perhaps
"update-<nice timestamp>"

It's also probably misnamed, in the sense that the directory is not actually
"temporary", but the real content that will stay around and we'll depend on.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:80: defer func() {
I suggest something like:

if err == ErrConflict {
    log.Printf("charm deployment completed with conflicts")
} else if err != nil {
    log.Printf("charm deployment failed: %v", err)
} else {
    log.Printf("charm deployment succeeded")
}

[fwereade, 2012-09-06 08:18:04]

Please take a look.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer.go
File worker/uniter/charm/deployer.go (right):

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:30: func (d *Deployer) SetCharm(bun
*charm.Bundle, url *charm.URL) error {
On 2012/09/05 22:47:21, niemeyer wrote:
> // Read present state of current.

Done.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:47: }
On 2012/09/05 22:47:21, niemeyer wrote:
> // Prepare a temporary update repository, using current's
> // history if it exists.

Done.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:61: }
On 2012/09/05 22:47:21, niemeyer wrote:
> // Write down the desired new state and commit.

Done.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:70: }
On 2012/09/05 22:47:21, niemeyer wrote:
> // Atomically rename temporary update repository to current. 

Done.

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:75: return os.Rename(tmplink, d.current.Path())
On 2012/09/05 22:47:21, niemeyer wrote:
> If we're keeping the repository directory around, it'd be nice to have
something
> more human than something like "update-KJhkjwhek" as a name. Perhaps
> "update-<nice timestamp>"
> 
> It's also probably misnamed, in the sense that the directory is not actually
> "temporary", but the real content that will stay around and we'll depend on.

Definitely agree on the misnaming; not really sure the value of
human-readability actually outweighs the simplicity/clarity of the original
approach. Does newDir look sane to you?

https://codereview.appspot.com/6488084/diff/2001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:80: defer func() {
On 2012/09/05 22:47:21, niemeyer wrote:
> I suggest something like:
> 
> if err == ErrConflict {
>     log.Printf("charm deployment completed with conflicts")
> } else if err != nil {
>     log.Printf("charm deployment failed: %v", err)
> } else {
>     log.Printf("charm deployment succeeded")
> }

Nice, thanks. Done.

[niemeyer, 2012-09-06 13:33:57]

Thanks. Some additional details, and LGTM.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/deployer.go
File worker/uniter/charm/deployer.go (right):

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:78: // Atomically rename temporary update
repository to current.
// Atomically rename fresh repository to current.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:189: prefix = prefix +
time.Now().Format("-%Y%m%d-%H%M%S")
var err error
var path string

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:192: if err := os.Mkdir(path, 0755); err != nil
{
if err = os.Mkdir(path, 0755); err == nil {
        return path, nil
} else if !os.IsExist(err) {
        break
}

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:198: return "", fmt.Errorf("failed to create
deployer directory after 10 tries")
return "", fmt.Errorf("failed to create %q: %v", path, err)

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go
File worker/uniter/charm/git.go (right):

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:43: return false, fmt.Errorf("%s is not a directory",
d.path)
s/%s/%q/

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:67: return d.cmd("reset", "--soft")
If this is strictly necessary after removing the lock, there's a race condition
above: someone else wins the race, deletes the file, dies == remove without
reset.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:210: cmd := exec.Command("git", "status",
"--porcelain")
I love this flag name. :-)

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:214: return nil, fmt.Errorf("git status failed: %s",
err)
I'm not sure if it makes a difference today, since there were changes pre-Go1,
but we still use "%v" for err conventionally, I think.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:216: log.Printf(string(out))
Git's output as a *format*?

I guess this is even a left-over? It should go away, either way, as we don't
want to the output of git on successes every time.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:224: cmd = exec.Command("git", "status")
Again!? Leftover?

[fwereade, 2012-09-06 21:15:48]

*** Submitted:

add charm.Deployer; extend charm.GitDir

Deployer makes heavy use of GitDirs to implement the charm install/upgrade
procedures.

R=TheMue, niemeyer
CC=
https://codereview.appspot.com/6488084

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/deployer.go
File worker/uniter/charm/deployer.go (right):

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:78: // Atomically rename temporary update
repository to current.
On 2012/09/06 13:33:57, niemeyer wrote:
> // Atomically rename fresh repository to current.

Done.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:189: prefix = prefix +
time.Now().Format("-%Y%m%d-%H%M%S")
On 2012/09/06 13:33:57, niemeyer wrote:
> var err error
> var path string

Done.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:192: if err := os.Mkdir(path, 0755); err != nil
{
On 2012/09/06 13:33:57, niemeyer wrote:
> if err = os.Mkdir(path, 0755); err == nil {
>         return path, nil
> } else if !os.IsExist(err) {
>         break
> }

Done.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/deployer...
worker/uniter/charm/deployer.go:198: return "", fmt.Errorf("failed to create
deployer directory after 10 tries")
On 2012/09/06 13:33:57, niemeyer wrote:
> return "", fmt.Errorf("failed to create %q: %v", path, err)

Done.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go
File worker/uniter/charm/git.go (right):

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:43: return false, fmt.Errorf("%s is not a directory",
d.path)
On 2012/09/06 13:33:57, niemeyer wrote:
> s/%s/%q/

Done.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:67: return d.cmd("reset", "--soft")
On 2012/09/06 13:33:57, niemeyer wrote:
> If this is strictly necessary after removing the lock, there's a race
condition
> above: someone else wins the race, deletes the file, dies == remove without
> reset.

Good point. Happily, it turns out reset --soft should be fine to run at any time
for our purposes, and it doesn't care about index.lock, so I now always reset
and then delete the lock.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:214: return nil, fmt.Errorf("git status failed: %s",
err)
On 2012/09/06 13:33:57, niemeyer wrote:
> I'm not sure if it makes a difference today, since there were changes pre-Go1,
> but we still use "%v" for err conventionally, I think.

Done.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:216: log.Printf(string(out))
On 2012/09/06 13:33:57, niemeyer wrote:
> Git's output as a *format*?
> 
> I guess this is even a left-over? It should go away, either way, as we don't
> want to the output of git on successes every time.

Crackhead nonsense -- it was useful for something at one point, and then the
tests started pasisng and I stopped seeing the logs :/. Sorry.

https://codereview.appspot.com/6488084/diff/8001/worker/uniter/charm/git.go#n...
worker/uniter/charm/git.go:224: cmd = exec.Command("git", "status")
On 2012/09/06 13:33:57, niemeyer wrote:
> Again!? Leftover?
Yep :(.