store: save charm config and meta

This turns CharmInfo into a charm.Charm!

https://code.launchpad.net/~niemeyer/juju/go-store-metadata/+merge/87962

(do not edit description out of merge proposal)

[niemeyer, 2012-01-09 17:12:25]

Please take a look.

[niemeyer, 2012-01-09 17:13:15]

Please take a look.

[rog, 2012-01-10 10:27:54]

looking good.

https://codereview.appspot.com/5532049/diff/1008/store/store.go
File store/store.go (right):

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode4
store/store.go:4: // the VCS revision the charm was loaded from and the urls in
which the
the last bit of the sentence reads quite awkwardly to me. how about 
"... loaded from and the URLs for the charm"?

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode7
store/store.go:7: // The following MongoDB collections are currently used:
i'm not sure this should be attached to the package-level comment - isn't it an
internal implementation detail?

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode30
store/store.go:30: UpdateIsCurrent = errors.New("charm is already up-to-date")
the hyphens aren't necessary, i think.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode37
store/store.go:37: // The Store type encapsulates communication with MongoDB to
to.... what?

not sure it's necessary to mention mongodb here,
perhaps the comment could be simpler, say:
"Store holds a connection to a charm store."?

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode42
store/store.go:42: // Open creates a new session with the store for adding or
i think it's worth saying what mongoAddr is here.
given that adding and retrieving is documented below,
perhaps this could be:

"Open creates a new session with the store. It connects to the MongoDB server at
the given address (as expected by the Mongo function in the launchpad.net/mgo
package)."

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode52
store/store.go:52: store = &Store{session: storeSession{session}}
given there's only one member, perhaps could drop "session:" member specifier.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode65
store/store.go:65: // Close terminates the session with store.
This reads like store is an identifier.
Either keep the the same and rename the
receiver to "store", or, maybe better:

"Close terminates the store session".

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode73
store/store.go:73: for i := range norev {
i think this would read better ranging over urls rather than norev:

for i, u := range urls {
    norev[i] = u.WithRevision(-1)
}

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode81
store/store.go:81: // revision identifier that represents the current charm
content.
i struggled a little to work out what this requires.
might be worth saying what form VCS revision identifier is expected to take. is
it just a revision number e.g. "23"?

if so, maybe vcsRevision might be a better name (to distinguish it from the
revision numbers), and the sentence might be "The vcsRevision parameter
specifies the VCS revision holding the charm's content"

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode85
store/store.go:85: // store, and once wc is closed successfully the content will
be
perhaps say what format the data written to wc should take? (e.g.
"conventionally a zip archive containing the charm's files")

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode88
store/store.go:88: // store, and it's equal to the maximum current revision from
all of
s/and it's//

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode111
store/store.go:111: for i := range urls {
for_, url := range urls {

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode112
store/store.go:112: urlStr := urls[i].String()
s/urls[i]/url/

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode138
store/store.go:138: return &writer{session, nil, nil, charm, urls, lock,
revision, revisionKey}, revision, nil
i think it would be worth putting keys on this initialisation.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode141
store/store.go:141: type writer struct {
i find the comment on Write below reads quite awkwardly.

perhaps a comment on this type might work better:
// writer represents a stream of data for a charm
// being written to the database.

then no comment necessary on Write.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode265
store/store.go:265: // rc must necessarily be closed after dealing with it or
resources
s/necessarily//

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode294
store/store.go:294: // Close closes the opened charm, and frees associated
resources.
s/,//

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode313
store/store.go:313: type storeSession struct {
do the methods on this type need to be public?

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode341
store/store.go:341: for i := range urls {
for i, url := range urls {

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode342
store/store.go:342: keys[i] = urls[i].String()
s/urls[i]/url/

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode372
store/store.go:372: // can lock all of them in order. The keys should be
pre-sorted so that
s/pre-//

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode373
store/store.go:373: // two-way conflicts can't happen. In case any of the keys
fail to be
s/In case/If/

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode413
store/store.go:413: // maybeConflict returns an UpdateConflict in case err is a
mgo
s/in case/when/

https://codereview.appspot.com/5532049/diff/1008/store/store_test.go
File store/store_test.go (right):

https://codereview.appspot.com/5532049/diff/1008/store/store_test.go#newcode36
store/store_test.go:36: dir, err := charm.ReadDir(repoDir("dummy"))
might be more obvious as: filepath.FromSlash("../charm/testrepo/series/dummy")
and lose the repoDir function.

https://codereview.appspot.com/5532049/diff/1008/store/store_test.go#newcode75
store/store_test.go:75: // full charm data..
s/../.../

(i thought it was an extra full stop)

https://codereview.appspot.com/5532049/diff/1008/store/store_test.go#newcode79
store/store_test.go:79: // .. and the queriable details.
s/../.../
s/queriable/queryable/

https://codereview.appspot.com/5532049/diff/1008/store/store_test.go#newcode167
store/store_test.go:167: var tests = []struct {
s/var tests/tests :=/

[niemeyer, 2012-01-12 08:22:34]

https://codereview.appspot.com/5532049/diff/1008/store/store.go
File store/store.go (right):

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode4
store/store.go:4: // the VCS revision the charm was loaded from and the urls in
which the
On 2012/01/10 10:27:54, rog wrote:
> the last bit of the sentence reads quite awkwardly to me. how about 
> "... loaded from and the URLs for the charm"?

Done.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode7
store/store.go:7: // The following MongoDB collections are currently used:
On 2012/01/10 10:27:54, rog wrote:
> i'm not sure this should be attached to the package-level comment - isn't it
an
> internal implementation detail?

Moved to an internal one.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode30
store/store.go:30: UpdateIsCurrent = errors.New("charm is already up-to-date")
On 2012/01/10 10:27:54, rog wrote:
> the hyphens aren't necessary, i think.

Reference? As far as I know it is:

http://www.thefreedictionary.com/up-to-date

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode37
store/store.go:37: // The Store type encapsulates communication with MongoDB to
Given that we already mention the fact we're using MongoDB in the package
comment, sounds good.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode42
store/store.go:42: // Open creates a new session with the store for adding or
On 2012/01/10 10:27:54, rog wrote:
> i think it's worth saying what mongoAddr is here.
> given that adding and retrieving is documented below,
> perhaps this could be:
> 
> "Open creates a new session with the store. It connects to the MongoDB server
at
> the given address (as expected by the Mongo function in the launchpad.net/mgo
> package)."

That's nice.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode52
store/store.go:52: store = &Store{session: storeSession{session}}
On 2012/01/10 10:27:54, rog wrote:
> given there's only one member, perhaps could drop "session:" member specifier.

Done.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode65
store/store.go:65: // Close terminates the session with store.
On 2012/01/10 10:27:54, rog wrote:
> This reads like store is an identifier.
> Either keep the the same and rename the
> receiver to "store", or, maybe better:
> 
> "Close terminates the store session".

Improved in a slightly different way.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode73
store/store.go:73: for i := range norev {
On 2012/01/10 10:27:54, rog wrote:
> i think this would read better ranging over urls rather than norev:
> 
> for i, u := range urls {
>     norev[i] = u.WithRevision(-1)
> }

Indeed.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode81
store/store.go:81: // revision identifier that represents the current charm
content.
On 2012/01/10 10:27:54, rog wrote:
> i struggled a little to work out what this requires.
> might be worth saying what form VCS revision identifier is expected to take.
is
> it just a revision number e.g. "23"?

It's not a revision number. I've improved the comment.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode85
store/store.go:85: // store, and once wc is closed successfully the content will
be
On 2012/01/10 10:27:54, rog wrote:
> perhaps say what format the data written to wc should take? (e.g.
> "conventionally a zip archive containing the charm's files")

Changed with s/charm content/charm bundle/

The charm bundle has a well defined format. The fact it's a zip is not relevant
here.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode88
store/store.go:88: // store, and it's equal to the maximum current revision from
all of
On 2012/01/10 10:27:54, rog wrote:
> s/and it's//

That won't read correctly either. Changed in another way.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode111
store/store.go:111: for i := range urls {
On 2012/01/10 10:27:54, rog wrote:
> for_, url := range urls {

Current version feels nicer to me.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode138
store/store.go:138: return &writer{session, nil, nil, charm, urls, lock,
revision, revisionKey}, revision, nil
On 2012/01/10 10:27:54, rog wrote:
> i think it would be worth putting keys on this initialisation.

Indeed.. grew up too much.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode141
store/store.go:141: type writer struct {
On 2012/01/10 10:27:54, rog wrote:
> i find the comment on Write below reads quite awkwardly.
> 
> perhaps a comment on this type might work better:
> // writer represents a stream of data

I don't find the suggestion great either. I've reworded the comment in Write.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode265
store/store.go:265: // rc must necessarily be closed after dealing with it or
resources
On 2012/01/10 10:27:54, rog wrote:
> s/necessarily//

Done.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode294
store/store.go:294: // Close closes the opened charm, and frees associated
resources.
On 2012/01/10 10:27:54, rog wrote:
> s/,//

Done.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode313
store/store.go:313: type storeSession struct {
On 2012/01/10 10:27:54, rog wrote:
> do the methods on this type need to be public?

No, but it looks nicer given that it's being embeded, and it also prevents the
underlying Copy from mgo.Session from being called by mistake.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode341
store/store.go:341: for i := range urls {
On 2012/01/10 10:27:54, rog wrote:
> for i, url := range urls {

Current version feels a lot nicer to me.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode372
store/store.go:372: // can lock all of them in order. The keys should be
pre-sorted so that
On 2012/01/10 10:27:54, rog wrote:
> s/pre-//

The redundancy here doesn't hurt. The comment begins with a description of what
tryLock does, and this is about the pre-conditions expected.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode373
store/store.go:373: // two-way conflicts can't happen. In case any of the keys
fail to be
On 2012/01/10 10:27:54, rog wrote:
> s/In case/If/

Done.

https://codereview.appspot.com/5532049/diff/1008/store/store.go#newcode413
store/store.go:413: // maybeConflict returns an UpdateConflict in case err is a
mgo
On 2012/01/10 10:27:54, rog wrote:
> s/in case/when/

Done.

https://codereview.appspot.com/5532049/diff/1008/store/store_test.go
File store/store_test.go (right):

https://codereview.appspot.com/5532049/diff/1008/store/store_test.go#newcode36
store/store_test.go:36: dir, err := charm.ReadDir(repoDir("dummy"))
On 2012/01/10 10:27:54, rog wrote:
> might be more obvious as: filepath.FromSlash("../charm/testrepo/series/dummy")
> and lose the repoDir function.

This convention is already in use in other files.

https://codereview.appspot.com/5532049/diff/1008/store/store_test.go#newcode79
store/store_test.go:79: // .. and the queriable details.
On 2012/01/10 10:27:54, rog wrote:
> s/queriable/queryable/

Both forms are valid.

https://codereview.appspot.com/5532049/diff/1008/store/store_test.go#newcode167
store/store_test.go:167: var tests = []struct {
On 2012/01/10 10:27:54, rog wrote:
> s/var tests/tests :=/

Done.

[niemeyer, 2012-01-12 08:23:21]

Please take a look.

[niemeyer, 2012-01-13 16:04:35]

*** Submitted:

store: save charm config and meta

This turns CharmInfo into a charm.Charm!

R=rog
CC=
https://codereview.appspot.com/5532049