uniter/charm: manifestDeployer type

uniter/charm: manifestDeployer type

unit tested, but not integrated yet

https://code.launchpad.net/~fwereade/juju-core/actual-manifest-deployer/+merge/212962

Requires: https://code.launchpad.net/~fwereade/juju-core/filetesting-package/+merge/212811

(do not edit description out of merge proposal)

[fwereade, 2014-03-27 13:38:03]

Please take a look.

[fwereade, 2014-04-02 09:19:51]

Please take a look.

[jameinel, 2014-04-03 11:14:42]

I had a few comments, but it looks pretty good to me overall. LGTM, though
you're welcome to discuss anything.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/charm_...
File worker/uniter/charm/charm_test.go (right):

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/charm_...
worker/uniter/charm/charm_test.go:40: if br.waitAbort == nil {
this looks really odd to me. I would have expected something more like:
br.SetAbortWait()
// close it immediately because nobody actually wants it.
close(b.waitAbort)

It is just one of those "function that has side effects *and* returns something"
that ends up being confusing because you are first referencing the side-effect
in the if, and then referencing the returned value in the close().

I'm also really confused about the fact that you have SetWaitAbort which creates
waitAbort and another 'abort' channel sitting around, that doesn't actually
abort but prevents aborting.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
File worker/uniter/charm/manifest_deployer.go (right):

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:17: // deployingURLPath holds the path
in the charm dir to which the manifest
holds the path in the charm dir *where* the manifest ...

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:95: if err :=
d.removeDiff(baseManifest, d.staged.manifest); err != nil {
Is it worth optimizing for when baseURL is nil then baseManifest is an empty set
and we could just skip this step? (either here or in the removeDiff)?

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:135: for _, path := range
diff.SortedValues() {
When deleting you need to go in reverse sorted order. So that you delete
"foo/bar" before you delete "foo/".

Arguably if RemoveAll doesn't fail when the target doesn't exist, then it isn't
terrible to just rm -rf the dir first, and then ignore the fact that you can't
ignore the contents later.
But if we wanted a more careful "only remove what we put there" and used
os.Remove and os.Rmdir() then we'd want to reverse the list.

It would probably be good, regardless, to have a test that has a charm that
removes a directory and a file within that directory.

http://golang.org/pkg/os/#RemoveAll does say it returns no-error when the path
doesn't exist.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:152: // one, removes all entries in the
manifest for the interrupted operation. This
It doesn't seem to remove all entries, just the different ones. probably change
it to:
removes all entries unique to the manifest for the interrupted operation.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:163: logger.Debugf("detected
interrupted deploy of charm %q", deployingURL)
maybe higher than Debugf, as this is likely a bad place to be in. Infof seems
appropriate.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:178: func (d *manifestDeployer)
storeManifest(url *charm.URL, manifest set.Strings) error {
It seems like it would be nice if the Manifest had the concept of its own URL in
the content of the file, rather than only as the Quoted URL name.
That would imply writing a nested YAML file (since you need some way to have
content that is the actual files and some content that is metadata).
Is that worth doing?
I really like things on disk to be self describing, but maybe I'm going
overboard.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
File worker/uniter/charm/manifest_deployer_test.go (right):

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer_test.go:89: wait :=
s.bundles.SetAbortWait()
I can see that you are testing that SetAbortWait does something, but it isn't
clear to me what the use case for it is. (It seems to just create another
channel like abort but that doesn't cause an error to be returned.)

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer_test.go:117:
ft.Symlink{"another-dir/some-link", "../some-file"},
maybe this is the test I was asking about having a file in a dir.

[jameinel, 2014-04-03 11:26:02]

a suggested spelling for the wait and abort code.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/charm_...
File worker/uniter/charm/charm_test.go (right):

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/charm_...
worker/uniter/charm/charm_test.go:40: if br.waitAbort == nil {
Perhaps this:

waitingForAbortChan = br.waitAbort
if waitingForAbortChan == nil {
  // We aren't waiting for an abort, so we just need an always-closed channel
  waitingForAbortChan = make(chan struct{})
  close(waitingForAbortChan)
} else {
  // EnableWaitForAbort is a one-time wait, so make sure it is gone when we are
done
  defer func() { br.WaitAbort = nil }()
}

select {
case <-abort:
  return nil, fmt.Errorf("charm read aborted")
case <-waitForAbortChan:
  // someone asked us to stop waiting, so return the bundle now
}
return bundle, nil

// EnableWaitForAbort makes it so that the next call to bundleReader.Read() can
be aborted (in testing). Otherwise the reader can finish faster than the abort
could be triggered. The returned channel can be closed when the caller doesn't
want to wait anymore. [closing the abort channel will cause Read() to fail with
an abort, closing the waiting channel will cause the Read() to succeed.]
func (br *bundleReader) EnableWaitForAbort() chan struct {
}

[fwereade, 2014-04-11 06:33:08]

On 2014/04/03 11:26:02, jameinel wrote:
> a suggested spelling for the wait and abort code.
> 
>
https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/charm_...
> File worker/uniter/charm/charm_test.go (right):
> 
>
https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/charm_...
> worker/uniter/charm/charm_test.go:40: if br.waitAbort == nil {
> Perhaps this:
> 
> waitingForAbortChan = br.waitAbort
> if waitingForAbortChan == nil {
>   // We aren't waiting for an abort, so we just need an always-closed channel
>   waitingForAbortChan = make(chan struct{})
>   close(waitingForAbortChan)
> } else {
>   // EnableWaitForAbort is a one-time wait, so make sure it is gone when we
are
> done
>   defer func() { br.WaitAbort = nil }()
> }
> 
> select {
> case <-abort:
>   return nil, fmt.Errorf("charm read aborted")
> case <-waitForAbortChan:
>   // someone asked us to stop waiting, so return the bundle now
> }
> return bundle, nil
> 
> // EnableWaitForAbort makes it so that the next call to bundleReader.Read()
can
> be aborted (in testing). Otherwise the reader can finish faster than the abort
> could be triggered. The returned channel can be closed when the caller doesn't
> want to wait anymore. [closing the abort channel will cause Read() to fail
with
> an abort, closing the waiting channel will cause the Read() to succeed.]
> func (br *bundleReader) EnableWaitForAbort() chan struct {
> }

I ended up going with something slightly different, but this helped me make it
clearer. Thanks.

[fwereade, 2014-04-11 06:39:56]

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/charm_...
File worker/uniter/charm/charm_test.go (right):

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/charm_...
worker/uniter/charm/charm_test.go:40: if br.waitAbort == nil {
On 2014/04/03 11:26:02, jameinel wrote:
> Perhaps this:
> 
> waitingForAbortChan = br.waitAbort
> if waitingForAbortChan == nil {
>   // We aren't waiting for an abort, so we just need an always-closed channel
>   waitingForAbortChan = make(chan struct{})
>   close(waitingForAbortChan)
> } else {
>   // EnableWaitForAbort is a one-time wait, so make sure it is gone when we
are
> done
>   defer func() { br.WaitAbort = nil }()
> }
> 
> select {
> case <-abort:
>   return nil, fmt.Errorf("charm read aborted")
> case <-waitForAbortChan:
>   // someone asked us to stop waiting, so return the bundle now
> }
> return bundle, nil
> 
> // EnableWaitForAbort makes it so that the next call to bundleReader.Read()
can
> be aborted (in testing). Otherwise the reader can finish faster than the abort
> could be triggered. The returned channel can be closed when the caller doesn't
> want to wait anymore. [closing the abort channel will cause Read() to fail
with
> an abort, closing the waiting channel will cause the Read() to succeed.]
> func (br *bundleReader) EnableWaitForAbort() chan struct {
> }

Done, but a bit differently.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
File worker/uniter/charm/manifest_deployer.go (right):

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:17: // deployingURLPath holds the path
in the charm dir to which the manifest
On 2014/04/03 11:14:42, jameinel wrote:
> holds the path in the charm dir *where* the manifest ...

Really? Seems wrong to me. Done regardless.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:95: if err :=
d.removeDiff(baseManifest, d.staged.manifest); err != nil {
On 2014/04/03 11:14:42, jameinel wrote:
> Is it worth optimizing for when baseURL is nil then baseManifest is an empty
set
> and we could just skip this step? (either here or in the removeDiff)?

I originally did that but it seemed to obscure the flow for no actual
improvement in behaviour. OTOH, I do now have `upgrading` in a var, so let's go
with it.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:135: for _, path := range
diff.SortedValues() {
On 2014/04/03 11:14:42, jameinel wrote:
> When deleting you need to go in reverse sorted order. So that you delete
> "foo/bar" before you delete "foo/".
> 
> Arguably if RemoveAll doesn't fail when the target doesn't exist, then it
isn't
> terrible to just rm -rf the dir first, and then ignore the fact that you can't
> ignore the contents later.
> But if we wanted a more careful "only remove what we put there" and used
> os.Remove and os.Rmdir() then we'd want to reverse the list.
> 
> It would probably be good, regardless, to have a test that has a charm that
> removes a directory and a file within that directory.
> 
> http://golang.org/pkg/os/#RemoveAll does say it returns no-error when the path
> doesn't exist.

We want to be brutal about overwrites -- using this will actually resolve
lp:1191028 as well. Charms which want to keep user files in old dirs can just
leave the dir in the charm, and user files will be preserved; if there's only
junk like .pycs, they will be trashed without mercy.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:152: // one, removes all entries in the
manifest for the interrupted operation. This
On 2014/04/03 11:14:42, jameinel wrote:
> It doesn't seem to remove all entries, just the different ones. probably
change
> it to:
> removes all entries unique to the manifest for the interrupted operation.

Done.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:163: logger.Debugf("detected
interrupted deploy of charm %q", deployingURL)
On 2014/04/03 11:14:42, jameinel wrote:
> maybe higher than Debugf, as this is likely a bad place to be in. Infof seems
> appropriate.

Done.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer.go:178: func (d *manifestDeployer)
storeManifest(url *charm.URL, manifest set.Strings) error {
On 2014/04/03 11:14:42, jameinel wrote:
> It seems like it would be nice if the Manifest had the concept of its own URL
in
> the content of the file, rather than only as the Quoted URL name.
> That would imply writing a nested YAML file (since you need some way to have
> content that is the actual files and some content that is metadata).
> Is that worth doing?
> I really like things on disk to be self describing, but maybe I'm going
> overboard.

I dunno -- I feel that the unit of self-description is the directory itself
really. Once you have a charm url in the file, you have an opportunity for
inconsistency between the real key -- the filename -- and the contents; and then
you need to think about how to handle that inconsistency... it's just more
things that could go wrong.

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
File worker/uniter/charm/manifest_deployer_test.go (right):

https://codereview.appspot.com/81110043/diff/60001/worker/uniter/charm/manife...
worker/uniter/charm/manifest_deployer_test.go:89: wait :=
s.bundles.SetAbortWait()
On 2014/04/03 11:14:42, jameinel wrote:
> I can see that you are testing that SetAbortWait does something, but it isn't
> clear to me what the use case for it is. (It seems to just create another
> channel like abort but that doesn't cause an error to be returned.)

Yeah, I have evidently not made its purpose sufficiently clear.