add uniter/charm package

add uniter/charm package

Contains useful types and constants related to charms, similar to the
uniter/hook and uniter/relation packages.

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

(do not edit description out of merge proposal)

[fwereade, 2012-08-17 16:41:41]

Please take a look.

[niemeyer, 2012-08-17 18:38:06]

This looks great. Trivial suggestions, with one relevant point only:

https://codereview.appspot.com/6446156/diff/1/testing/httpserver.go
File testing/httpserver.go (right):

https://codereview.appspot.com/6446156/diff/1/testing/httpserver.go#newcode10
testing/httpserver.go:10: // Server is an HTTP server that is convenient to use
in tests.
We already have a test http server in store/http_test.go that enables better
control of the requests. Can we use it/improve it instead, and have a single
suite?

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

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:19: // Status enumerates the possible states for
the charm directory.
s/the charm/a charm/

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:27: UpgradingForced Status = "upgrading-forced"
What's the difference for the charm directory if the upgrade was forced or not?

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:30: // valid returns false if the status is not
recognized.
// valid returns whether the status is recognized.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:40: type State struct {
That's nice so that we can have more fields in the future by changing just the
implementation and not the disk file, but we don't have to work with it in the
API below. We can keep it private (as "state") for now, and manipulate Status
directly in Read/Write. Note that Write actually already takes Status rather
than State.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:44: // StateFile gives access to persistent charm
state.
// A StateFile contains the description of a charm directory's state.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:55: func (f *StateFile) Read() (st State, err
error) {
s/State/Status/

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:56: data, err := ioutil.ReadFile(f.path)
st := state{}
err := trivial.ReadYaml(f.path, &st)
?

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:76: return trivial.AtomicWrite(f.path, &State{s})
I suggest calling this WriteYaml instead. Atomicity is definitely an important
property of it, of course.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:105: // download gets the supplied charm and checks
that it has the correct sha256
s/gets/fetches|retrieves|downloads/ ("get" tends to be lightweight in APIs)

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:109: defer trivial.ErrorContextf(&err, "failed to
download charm %q from %q", sch.URL(), sch.BundleURL())
Woohay trivial :)

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:110: dl := downloader.New(sch.BundleURL().String())
If we want to rename below (which sounds wise), we'll need a way to change the
temporary path used by the downloader onto something that is necessarily within
the same filsystem. os.TempDir is most certainly not.

[fwereade, 2012-08-18 00:24:01]

Good comments all; thank you. A couple of responses follow, implementation of
the suggestions will follow at some future point in time.

https://codereview.appspot.com/6446156/diff/1/testing/httpserver.go
File testing/httpserver.go (right):

https://codereview.appspot.com/6446156/diff/1/testing/httpserver.go#newcode10
testing/httpserver.go:10: // Server is an HTTP server that is convenient to use
in tests.
On 2012/08/17 18:38:06, niemeyer wrote:
> We already have a test http server in store/http_test.go that enables better
> control of the requests. Can we use it/improve it instead, and have a single
> suite?

Huh, I thought I was engaging in responsible reuse; that'll learn me :). Anyway,
sounds sensible; when I see what's involved in rationalizing the various servers
that can or should be, I might end up making it a separate CL.

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

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:27: UpgradingForced Status = "upgrading-forced"
On 2012/08/17 18:38:06, niemeyer wrote:
> What's the difference for the charm directory if the upgrade was forced or
not?

The intent is perhaps more to enumerate the states the *uniter* will care about;
for example, from the charm dir's point of view there's no distinction even
between installation and upgrade. From the uniter's point of view, though, the 3
states that indicate a charm change in in progress all indicate distinct
behaviour, and that's the perspective to which the package is tailored.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:40: type State struct {
On 2012/08/17 18:38:06, niemeyer wrote:
> That's nice so that we can have more fields in the future by changing just the
> implementation and not the disk file, but we don't have to work with it in the
> API below. We can keep it private (as "state") for now, and manipulate Status
> directly in Read/Write. Note that Write actually already takes Status rather
> than State.

SGTM, I won't even try go into the tortuous mental processes that led me
consider reading State but writing Status to be the Best Possible Solution ;/.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:56: data, err := ioutil.ReadFile(f.path)
On 2012/08/17 18:38:06, niemeyer wrote:
> st := state{}
> err := trivial.ReadYaml(f.path, &st)
> ?

SGTM

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:76: return trivial.AtomicWrite(f.path, &State{s})
On 2012/08/17 18:38:06, niemeyer wrote:
> I suggest calling this WriteYaml instead. Atomicity is definitely an important
> property of it, of course.

SGTM

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:110: dl := downloader.New(sch.BundleURL().String())
On 2012/08/17 18:38:06, niemeyer wrote:
> If we want to rename below (which sounds wise), we'll need a way to change the
> temporary path used by the downloader onto something that is necessarily
within
> the same filsystem. os.TempDir is most certainly not.

I wondered about that; I'll take a look.

[fwereade, 2012-08-18 22:48:55]

Please take a look.

https://codereview.appspot.com/6446156/diff/1/testing/httpserver.go
File testing/httpserver.go (right):

https://codereview.appspot.com/6446156/diff/1/testing/httpserver.go#newcode10
testing/httpserver.go:10: // Server is an HTTP server that is convenient to use
in tests.
On 2012/08/18 00:24:01, fwereade wrote:
> On 2012/08/17 18:38:06, niemeyer wrote:
> > We already have a test http server in store/http_test.go that enables better
> > control of the requests. Can we use it/improve it instead, and have a single
> > suite?
> 
> Huh, I thought I was engaging in responsible reuse; that'll learn me :).
Anyway,
> sounds sensible; when I see what's involved in rationalizing the various
servers
> that can or should be, I might end up making it a separate CL.

Done. Turned out very simple.

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

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:19: // Status enumerates the possible states for
the charm directory.
On 2012/08/17 18:38:06, niemeyer wrote:
> s/the charm/a charm/

Done.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:40: type State struct {
On 2012/08/18 00:24:01, fwereade wrote:
> On 2012/08/17 18:38:06, niemeyer wrote:
> > That's nice so that we can have more fields in the future by changing just
the
> > implementation and not the disk file, but we don't have to work with it in
the
> > API below. We can keep it private (as "state") for now, and manipulate
Status
> > directly in Read/Write. Note that Write actually already takes Status rather
> > than State.
> 
> SGTM, I won't even try go into the tortuous mental processes that led me
> consider reading State but writing Status to be the Best Possible Solution ;/.

Done.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:44: // StateFile gives access to persistent charm
state.
On 2012/08/17 18:38:06, niemeyer wrote:
> // A StateFile contains the description of a charm directory's state.

Done.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:55: func (f *StateFile) Read() (st State, err
error) {
On 2012/08/17 18:38:06, niemeyer wrote:
> s/State/Status/

Done.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:56: data, err := ioutil.ReadFile(f.path)
On 2012/08/18 00:24:01, fwereade wrote:
> On 2012/08/17 18:38:06, niemeyer wrote:
> > st := state{}
> > err := trivial.ReadYaml(f.path, &st)
> > ?
> 
> SGTM

Done.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:76: return trivial.AtomicWrite(f.path, &State{s})
On 2012/08/18 00:24:01, fwereade wrote:
> On 2012/08/17 18:38:06, niemeyer wrote:
> > I suggest calling this WriteYaml instead. Atomicity is definitely an
important
> > property of it, of course.
> 
> SGTM

Done.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:105: // download gets the supplied charm and checks
that it has the correct sha256
On 2012/08/17 18:38:06, niemeyer wrote:
> s/gets/fetches|retrieves|downloads/ ("get" tends to be lightweight in APIs)

Done.

https://codereview.appspot.com/6446156/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:110: dl := downloader.New(sch.BundleURL().String())
On 2012/08/18 00:24:01, fwereade wrote:
> On 2012/08/17 18:38:06, niemeyer wrote:
> > If we want to rename below (which sounds wise), we'll need a way to change
the
> > temporary path used by the downloader onto something that is necessarily
> within
> > the same filsystem. os.TempDir is most certainly not.
> 
> I wondered about that; I'll take a look.

Done.

[niemeyer, 2012-08-20 15:24:05]

This is awesome. LGTM

https://codereview.appspot.com/6446156/diff/1006/downloader/downloader.go
File downloader/downloader.go (right):

https://codereview.appspot.com/6446156/diff/1006/downloader/downloader.go#new...
downloader/downloader.go:28: // from the given URL to the given directory.
... If dir is empty, it defaults to os.TempDir().

(please reindent as well)

[fwereade, 2012-08-20 16:13:49]

*** Submitted:

add uniter/charm package

Contains useful types and constants related to charms, similar to the
uniter/hook and uniter/relation packages.

R=
CC=
https://codereview.appspot.com/6446156