state: add environment persistency with UUID

state: add environment persistency with UUID

Changed environment to be stored using an own document
in an own collection. Currently it only stores the name
and the UUID of the environment. The latter is generated
during the initializing of the state. Both, name and
UUID cannot be changed. The UUID will be passed as the
environment variable $JUJU_ENV_UUID to the charm hooks.

https://code.launchpad.net/~themue/juju-core/018-juju-env-uuid/+merge/156891

(do not edit description out of merge proposal)

[TheMue, 2013-04-03 15:56:55]

Please take a look.

[fwereade, 2013-04-04 06:34:11]

A few thoughts; we should maybe chat live about key choice.

https://codereview.appspot.com/8322043/diff/1/state/environ.go
File state/environ.go (left):

https://codereview.appspot.com/8322043/diff/1/state/environ.go#oldcode37
state/environ.go:37: return "e#" + e.name
FWIW, if we have a UUID, we should probably be using it here (rather than the
name, which will not necessarily be unique across environments).

https://codereview.appspot.com/8322043/diff/1/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/1/state/environ.go#newcode67
state/environ.go:67: Id:   "e",
...but I think something's backwards here regardless. What's the thinking behind
calling this "e", which is totally undifferentiated?

https://codereview.appspot.com/8322043/diff/1/state/environ_test.go
File state/environ_test.go (right):

https://codereview.appspot.com/8322043/diff/1/state/environ_test.go#newcode32
state/environ_test.go:32: c.Assert(env.UUID().String(), Matches,
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}")
FitsTypeOf instead? or maybe get the Environment to return a string directly?

https://codereview.appspot.com/8322043/diff/1/trivial/uuid.go
File trivial/uuid.go (right):

https://codereview.appspot.com/8322043/diff/1/trivial/uuid.go#newcode19
trivial/uuid.go:19: // Set version (4) and variant (2) according to RfC 4122.
This really rings a bell. Do we not have this code somewhere already?

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

https://codereview.appspot.com/8322043/diff/1/worker/uniter/context_test.go#n...
worker/uniter/context_test.go:103: c.Errorf("expected to find %v matching %q",
key, match)
Not sure this is much of a win when we could create a UUID at test init time and
always check that exactly.

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

https://codereview.appspot.com/8322043/diff/1/worker/uniter/uniter.go#newcode267
worker/uniter/uniter.go:267: hctx := NewHookContext(u.unit, hctxId, u.uuid,
relationId, hi.RemoteUnit, ctxRelations)
This is not tested.

[TheMue, 2013-04-04 12:40:58]

Please take a look.

https://codereview.appspot.com/8322043/diff/1/state/environ.go
File state/environ.go (left):

https://codereview.appspot.com/8322043/diff/1/state/environ.go#oldcode37
state/environ.go:37: return "e#" + e.name
On 2013/04/04 06:34:11, fwereade wrote:
> FWIW, if we have a UUID, we should probably be using it here (rather than the
> name, which will not necessarily be unique across environments).

Done.

https://codereview.appspot.com/8322043/diff/1/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/1/state/environ.go#newcode67
state/environ.go:67: Id:   "e",
On 2013/04/04 06:34:11, fwereade wrote:
> ...but I think something's backwards here regardless. What's the thinking
behind
> calling this "e", which is totally undifferentiated?

It's a constant identifier for this document, inspired by the "e" for the
environment settings and used in FindId(). But changed it to the more speaking
"environment" now.

https://codereview.appspot.com/8322043/diff/1/state/environ_test.go
File state/environ_test.go (right):

https://codereview.appspot.com/8322043/diff/1/state/environ_test.go#newcode32
state/environ_test.go:32: c.Assert(env.UUID().String(), Matches,
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}")
On 2013/04/04 06:34:11, fwereade wrote:
> FitsTypeOf instead? or maybe get the Environment to return a string directly?

Doing both tests now. I prefer to return the correct type, which implements the
Stringer interface.

https://codereview.appspot.com/8322043/diff/1/trivial/uuid.go
File trivial/uuid.go (right):

https://codereview.appspot.com/8322043/diff/1/trivial/uuid.go#newcode19
trivial/uuid.go:19: // Set version (4) and variant (2) according to RfC 4122.
On 2013/04/04 06:34:11, fwereade wrote:
> This really rings a bell. Do we not have this code somewhere already?

So far I only found the OpenStack provider fetching the instance UUID string.

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

https://codereview.appspot.com/8322043/diff/1/worker/uniter/context_test.go#n...
worker/uniter/context_test.go:103: c.Errorf("expected to find %v matching %q",
key, match)
On 2013/04/04 06:34:11, fwereade wrote:
> Not sure this is much of a win when we could create a UUID at test init time
and
> always check that exactly.

Testing it now by comparing it against a value generated externally and passed
to GetHookContext().

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

https://codereview.appspot.com/8322043/diff/1/worker/uniter/uniter.go#newcode267
worker/uniter/uniter.go:267: hctx := NewHookContext(u.unit, hctxId, u.uuid,
relationId, hi.RemoteUnit, ctxRelations)
On 2013/04/04 06:34:11, fwereade wrote:
> This is not tested.

Testing it now by passing a UUID and compare it later exactly against
JUJU_ENV_UUID.

[fwereade, 2013-04-04 13:15:40]

Few more comments:

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

https://codereview.appspot.com/8322043/diff/1/worker/uniter/uniter.go#newcode267
worker/uniter/uniter.go:267: hctx := NewHookContext(u.unit, hctxId, u.uuid,
relationId, hi.RemoteUnit, ctxRelations)
On 2013/04/04 12:40:58, TheMue wrote:
> On 2013/04/04 06:34:11, fwereade wrote:
> > This is not tested.
> 
> Testing it now by passing a UUID and compare it later exactly against
> JUJU_ENV_UUID.

Still not tested AFAICT..?

https://codereview.appspot.com/8322043/diff/9001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode20
state/environ.go:20: Id       string `bson:"_id"`
Please just use the UUID here.

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode23
state/environ.go:23: TxnRevno int64 `bson:"txn-revno"`
What's this used for?

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode29
state/environ.go:29: err := st.environments.FindId("environment").One(&doc)
st.environments.Find().One(&doc) is fine here.

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode60
state/environ.go:60: return e.uuid.String()
Please use (const) environGlobalKey (= "e") throughout.

https://codereview.appspot.com/8322043/diff/9001/state/environ_test.go
File state/environ_test.go (right):

https://codereview.appspot.com/8322043/diff/9001/state/environ_test.go#newcode35
state/environ_test.go:35: c.Assert(uuid.String(), Matches,
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}")
I still don't think it's very useful to put a test for the UUID type in the
state package.

https://codereview.appspot.com/8322043/diff/9001/trivial/uuid.go
File trivial/uuid.go (right):

https://codereview.appspot.com/8322043/diff/9001/trivial/uuid.go#newcode10
trivial/uuid.go:10: type UUID []byte
[16] perhaps, then? Or does that complicate matters?

https://codereview.appspot.com/8322043/diff/9001/worker/uniter/context_test.go
File worker/uniter/context_test.go (right):

https://codereview.appspot.com/8322043/diff/9001/worker/uniter/context_test.g...
worker/uniter/context_test.go:95: func AssertEnv(c *C, outPath string, charmDir
string, env map[string]string, uuid trivial.UUID) {
ISTM that it would be easier to create one UUID and assert it everywhere, just
as we do with "/path/to/socket". But as you like.

https://codereview.appspot.com/8322043/diff/9001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/8322043/diff/9001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:59: panic(err)
Why is this a panic?

[dimitern, 2013-04-04 14:52:14]

Looks good, but I have a number of mostly trivial suggestions.

https://codereview.appspot.com/8322043/diff/9001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode53
state/environ.go:53: // UUID returns universally unique identifier of the
environment.
s/returns/returns an/

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode63
state/environ.go:63: // createEnvironmentOp creates the transaction operation to
insert the
// createEnvironmentOp returns the operation needed to create
// an environment document with the given name and UUID.

https://codereview.appspot.com/8322043/diff/9001/state/environ_test.go
File state/environ_test.go (right):

https://codereview.appspot.com/8322043/diff/9001/state/environ_test.go#newcode35
state/environ_test.go:35: c.Assert(uuid.String(), Matches,
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}")
This regexp should be put in a ValidUUID() call in trivial, so it can be reused
potentially. Then you can have NewUUID() test in trivial use that and skip this
test here.

https://codereview.appspot.com/8322043/diff/9001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/8322043/diff/9001/state/open.go#newcode145
state/open.go:145: createEnvironmentOp(st, cfg.Name(), trivial.NewUUID()),
please put this first and the settings and constraints ops after it.

https://codereview.appspot.com/8322043/diff/9001/trivial/trivial_test.go
File trivial/trivial_test.go (right):

https://codereview.appspot.com/8322043/diff/9001/trivial/trivial_test.go#newc...
trivial/trivial_test.go:117: c.Assert(uuidStr, Matches,
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}")
See other comment about ValidUUID().

https://codereview.appspot.com/8322043/diff/9001/trivial/uuid.go
File trivial/uuid.go (right):

https://codereview.appspot.com/8322043/diff/9001/trivial/uuid.go#newcode10
trivial/uuid.go:10: type UUID []byte
On 2013/04/04 13:15:40, fwereade wrote:
> [16] perhaps, then? Or does that complicate matters?

+1, it's fixed anyway.

https://codereview.appspot.com/8322043/diff/9001/trivial/uuid.go#newcode43
trivial/uuid.go:43: base := hex.EncodeToString(uuid.Raw())
just use:
fmt.Sprintf("%x-%x-%x-%x-%x", uuid[0:4], uuid[4:6], uuid[6:8], uuid[8:10],
uuid[10:])
(see goose:testservices/novaservice/service_http.go)

[TheMue, 2013-04-05 12:05:20]

Please take a look.

https://codereview.appspot.com/8322043/diff/9001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode20
state/environ.go:20: Id       string `bson:"_id"`
On 2013/04/04 13:15:40, fwereade wrote:
> Please just use the UUID here.

Tried, bad Find() needs a query. With an empty one or nil it returns a query
where One() writes no values into the passed doc. So still using Id.

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode23
state/environ.go:23: TxnRevno int64 `bson:"txn-revno"`
On 2013/04/04 13:15:40, fwereade wrote:
> What's this used for?

Environment is written inside a transaction. But as it is never changed I
removed it.

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode29
state/environ.go:29: err := st.environments.FindId("environment").One(&doc)
On 2013/04/04 13:15:40, fwereade wrote:
> st.environments.Find().One(&doc) is fine here.

See above, Find() doesn't work.

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode53
state/environ.go:53: // UUID returns universally unique identifier of the
environment.
On 2013/04/04 14:52:14, dimitern wrote:
> s/returns/returns an/

Done.

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode60
state/environ.go:60: return e.uuid.String()
On 2013/04/04 13:15:40, fwereade wrote:
> Please use (const) environGlobalKey (= "e") throughout.

Done.

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode63
state/environ.go:63: // createEnvironmentOp creates the transaction operation to
insert the
On 2013/04/04 14:52:14, dimitern wrote:
> // createEnvironmentOp returns the operation needed to create
> // an environment document with the given name and UUID.

Done.

https://codereview.appspot.com/8322043/diff/9001/state/environ_test.go
File state/environ_test.go (right):

https://codereview.appspot.com/8322043/diff/9001/state/environ_test.go#newcode35
state/environ_test.go:35: c.Assert(uuid.String(), Matches,
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}")
On 2013/04/04 13:15:40, fwereade wrote:
> I still don't think it's very useful to put a test for the UUID type in the
> state package.

Yes, after FitsTypeOf it's not needed anymore. You're right.

https://codereview.appspot.com/8322043/diff/9001/state/environ_test.go#newcode35
state/environ_test.go:35: c.Assert(uuid.String(), Matches,
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}")
On 2013/04/04 14:52:14, dimitern wrote:
> This regexp should be put in a ValidUUID() call in trivial, so it can be
reused
> potentially. Then you can have NewUUID() test in trivial use that and skip
this
> test here.

It's removed. ;)

https://codereview.appspot.com/8322043/diff/9001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/8322043/diff/9001/state/open.go#newcode145
state/open.go:145: createEnvironmentOp(st, cfg.Name(), trivial.NewUUID()),
On 2013/04/04 14:52:14, dimitern wrote:
> please put this first and the settings and constraints ops after it.

Done.

https://codereview.appspot.com/8322043/diff/9001/trivial/trivial_test.go
File trivial/trivial_test.go (right):

https://codereview.appspot.com/8322043/diff/9001/trivial/trivial_test.go#newc...
trivial/trivial_test.go:117: c.Assert(uuidStr, Matches,
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}")
On 2013/04/04 14:52:14, dimitern wrote:
> See other comment about ValidUUID().

This is now the only UUID test. A test for a valid UUID by productive code is
not intended. Where do you would like to use ValidUUID() and what would you test
(e.g. version, variant)?

https://codereview.appspot.com/8322043/diff/9001/trivial/uuid.go
File trivial/uuid.go (right):

https://codereview.appspot.com/8322043/diff/9001/trivial/uuid.go#newcode10
trivial/uuid.go:10: type UUID []byte
On 2013/04/04 13:15:40, fwereade wrote:
> [16] perhaps, then? Or does that complicate matters?

Looks a bit more complicated and ugly, but works. ;) And it's more safe.

https://codereview.appspot.com/8322043/diff/9001/trivial/uuid.go#newcode43
trivial/uuid.go:43: base := hex.EncodeToString(uuid.Raw())
On 2013/04/04 14:52:14, dimitern wrote:
> just use:
> fmt.Sprintf("%x-%x-%x-%x-%x", uuid[0:4], uuid[4:6], uuid[6:8], uuid[8:10],
> uuid[10:])
> (see goose:testservices/novaservice/service_http.go)

Done.

https://codereview.appspot.com/8322043/diff/9001/worker/uniter/context_test.go
File worker/uniter/context_test.go (right):

https://codereview.appspot.com/8322043/diff/9001/worker/uniter/context_test.g...
worker/uniter/context_test.go:95: func AssertEnv(c *C, outPath string, charmDir
string, env map[string]string, uuid trivial.UUID) {
On 2013/04/04 13:15:40, fwereade wrote:
> ISTM that it would be easier to create one UUID and assert it everywhere, just
> as we do with "/path/to/socket". But as you like.

"/path/to/socket" is used as argument multiple times as it is needed, but it is
only asserted here in AssertEnv() which is only called in one test case. The
UUID is created dynamically only once in the same test and the passed for
assertion here. So it's almost similar.

https://codereview.appspot.com/8322043/diff/9001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/8322043/diff/9001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:59: panic(err)
On 2013/04/04 13:15:40, fwereade wrote:
> Why is this a panic?

If err is not nil we have no valid environment. The current implementation does
not access the environment and so doesn't has to check this error. 

Two possibilities: (a) don't check the error <iirks> or (b) pass it to the
caller, so that the API changes.

[fwereade, 2013-04-05 16:52:55]

Another round, I'm afraid...

https://codereview.appspot.com/8322043/diff/9001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode20
state/environ.go:20: Id       string `bson:"_id"`
On 2013/04/05 12:05:20, TheMue wrote:
> On 2013/04/04 13:15:40, fwereade wrote:
> > Please just use the UUID here.
> 
> Tried, bad Find() needs a query. With an empty one or nil it returns a query
> where One() writes no values into the passed doc. So still using Id.

As discussed live, we don't want or need *another* sort of environment "key":
name/uuid/"e" is bad enough, and using "e" here is unhelpfully unconventional
(entity keys do not usually match global keys). Please just use the UUID.

https://codereview.appspot.com/8322043/diff/9001/state/environ.go#newcode23
state/environ.go:23: TxnRevno int64 `bson:"txn-revno"`
On 2013/04/05 12:05:20, TheMue wrote:
> On 2013/04/04 13:15:40, fwereade wrote:
> > What's this used for?
> 
> Environment is written inside a transaction. But as it is never changed I
> removed it.

It should never ever be *changed* anywhere in our code; and it shouldn't even be
included unless there's a really compelling reason to do so -- ie, basically,
watching. (*Some* transactions *may* require an assert on it, but those should
be a last resort. I don't think we have any now.)

https://codereview.appspot.com/8322043/diff/9001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/8322043/diff/9001/state/open.go#newcode145
state/open.go:145: createEnvironmentOp(st, cfg.Name(), trivial.NewUUID()),
On 2013/04/05 12:05:20, TheMue wrote:
> On 2013/04/04 14:52:14, dimitern wrote:
> > please put this first and the settings and constraints ops after it.
> 
> Done.

Hmm. Shouldn't the existence of an environment guarantee its config and
constraints' existence, not vice versa?

https://codereview.appspot.com/8322043/diff/9001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/8322043/diff/9001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:59: panic(err)
On 2013/04/05 12:05:20, TheMue wrote:
> On 2013/04/04 13:15:40, fwereade wrote:
> > Why is this a panic?
> 
> If err is not nil we have no valid environment. The current implementation
does
> not access the environment and so doesn't has to check this error. 
> 
> Two possibilities: (a) don't check the error <iirks> or (b) pass it to the
> caller, so that the API changes.

Why not just do things like in the rest of the uniter? There's a perfectly good
init() method for exactly this reason...

https://codereview.appspot.com/8322043/diff/20001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/20001/state/environ.go#newcode56
state/environ.go:56: // UUID returns an universally unique identifier of the
environment.
s/an/the/

https://codereview.appspot.com/8322043/diff/20001/state/environ_test.go
File state/environ_test.go (right):

https://codereview.appspot.com/8322043/diff/20001/state/environ_test.go#newco...
state/environ_test.go:32: c.Assert(uuid, FitsTypeOf, trivial.UUID{})
Please also test that initializing a new environment generates a new UUID.

https://codereview.appspot.com/8322043/diff/20001/trivial/trivial_test.go
File trivial/trivial_test.go (right):

https://codereview.appspot.com/8322043/diff/20001/trivial/trivial_test.go#new...
trivial/trivial_test.go:121: }
Please add a quick test that two UUIDs are not the same.

https://codereview.appspot.com/8322043/diff/20001/trivial/uuid.go
File trivial/uuid.go (right):

https://codereview.appspot.com/8322043/diff/20001/trivial/uuid.go#newcode17
trivial/uuid.go:17: panic(err)
Why panic? How is this an unambiguous programmer error?

https://codereview.appspot.com/8322043/diff/20001/trivial/uuid.go#newcode38
trivial/uuid.go:38: }
Why doesn't copy() work here?

https://codereview.appspot.com/8322043/diff/20001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/8322043/diff/20001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:107: uuid          string
Any particular reason it's not a UUID?

https://codereview.appspot.com/8322043/diff/20001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:162: `: UniterSuite-%d`+ // test badge; prevents
cross-pollution
Please drop the UniterSuite-%d stuff, and drop id, as I asked; or explain why
you couldn't.

https://codereview.appspot.com/8322043/diff/20001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:836: defer s.Reset(c)
Why add a path that skips the Reset?

[TheMue, 2013-04-05 19:27:40]

Please take a look.

https://codereview.appspot.com/8322043/diff/20001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/20001/state/environ.go#newcode56
state/environ.go:56: // UUID returns an universally unique identifier of the
environment.
On 2013/04/05 16:52:55, fwereade wrote:
> s/an/the/

Done.

https://codereview.appspot.com/8322043/diff/20001/state/environ_test.go
File state/environ_test.go (right):

https://codereview.appspot.com/8322043/diff/20001/state/environ_test.go#newco...
state/environ_test.go:32: c.Assert(uuid, FitsTypeOf, trivial.UUID{})
On 2013/04/05 16:52:55, fwereade wrote:
> Please also test that initializing a new environment generates a new UUID.

Done.

https://codereview.appspot.com/8322043/diff/20001/trivial/trivial_test.go
File trivial/trivial_test.go (right):

https://codereview.appspot.com/8322043/diff/20001/trivial/trivial_test.go#new...
trivial/trivial_test.go:121: }
On 2013/04/05 16:52:55, fwereade wrote:
> Please add a quick test that two UUIDs are not the same.

Done.

https://codereview.appspot.com/8322043/diff/20001/trivial/uuid.go
File trivial/uuid.go (right):

https://codereview.appspot.com/8322043/diff/20001/trivial/uuid.go#newcode17
trivial/uuid.go:17: panic(err)
On 2013/04/05 16:52:55, fwereade wrote:
> Why panic? How is this an unambiguous programmer error?

Done.

https://codereview.appspot.com/8322043/diff/20001/trivial/uuid.go#newcode38
trivial/uuid.go:38: }
On 2013/04/05 16:52:55, fwereade wrote:
> Why doesn't copy() work here?

Done.

https://codereview.appspot.com/8322043/diff/20001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/8322043/diff/20001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:107: uuid          string
On 2013/04/05 16:52:55, fwereade wrote:
> Any particular reason it's not a UUID?

Done.

https://codereview.appspot.com/8322043/diff/20001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:162: `: UniterSuite-%d`+ // test badge; prevents
cross-pollution
On 2013/04/05 16:52:55, fwereade wrote:
> Please drop the UniterSuite-%d stuff, and drop id, as I asked; or explain why
> you couldn't.

Done.

https://codereview.appspot.com/8322043/diff/20001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:836: defer s.Reset(c)
On 2013/04/05 16:52:55, fwereade wrote:
> Why add a path that skips the Reset?

Done.

[fwereade, 2013-04-05 21:03:42]

Sorry, I had a few comments in unhelpful places that I don't think you saw. But
apart from those, please consider carefully what you're doing with the various
environment keys.

https://codereview.appspot.com/8322043/diff/36002/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/36002/state/environ.go#newcode18
state/environ.go:18: uuid trivial.UUID
What advantages does this approach have over the usual .doc field? Sorry, I only
just spotted this...

https://codereview.appspot.com/8322043/diff/36002/state/environ.go#newcode24
state/environ.go:24: UUID trivial.UUID
`bson:"_id"`?

https://codereview.appspot.com/8322043/diff/36002/state/environ.go#newcode31
state/environ.go:31: err := st.environments.Find(D{{"name", D{{"$ne",
""}}}}).One(&doc)
This bit is implemented as if you'd set the UUID to the _id above... but further
down, you use the global key. Which is it to be, and why?

https://codereview.appspot.com/8322043/diff/36002/state/environ.go#newcode74
state/environ.go:74: Id:     environGlobalKey,
If you *really* don't want to use the UUID (why?) please at least use a constant
that is *not* the global key, such as "environment" (as it originally was).

This is not to say that those keys *shouldn't* be the same -- they probably
should -- but while they're not, please stay consistent :).

https://codereview.appspot.com/8322043/diff/36002/state/environ_test.go
File state/environ_test.go (right):

https://codereview.appspot.com/8322043/diff/36002/state/environ_test.go#newco...
state/environ_test.go:42: c.Assert(uuidA, Not(Equals), uuidB)
Not(DeepEquals), surely? Might even be clearer to test that their String()s are
not equal...

https://codereview.appspot.com/8322043/diff/36002/state/open.go
File state/open.go (right):

https://codereview.appspot.com/8322043/diff/36002/state/open.go#newcode149
state/open.go:149: createConstraintsOp(st, environGlobalKey,
constraints.Value{}),
The operation order is still bad here. I think you might have missed my
follow-up comments on patch set 2?

https://codereview.appspot.com/8322043/diff/36002/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/8322043/diff/36002/worker/uniter/uniter.go#new...
worker/uniter/uniter.go:59: panic(err)
Sorry, I really should have made sure my comments were all on the latest patch
set. I thought I was being clearer by continuing the conversations, but I guess
not. Sorry :(

https://codereview.appspot.com/8322043/diff/36002/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/8322043/diff/36002/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:168: `: %s`+ // JUJU_ENV_UUID
// JUJU_ENV_UUID (context badge, prevents cross-test pollution)

[TheMue, 2013-04-08 13:45:39]

Please take a look.

https://codereview.appspot.com/8322043/diff/36002/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/36002/state/environ.go#newcode18
state/environ.go:18: uuid trivial.UUID
On 2013/04/05 21:03:42, fwereade wrote:
> What advantages does this approach have over the usual .doc field? Sorry, I
only
> just spotted this...

Ouch, missed that convention. Changed it.

https://codereview.appspot.com/8322043/diff/36002/state/environ.go#newcode24
state/environ.go:24: UUID trivial.UUID
On 2013/04/05 21:03:42, fwereade wrote:
> `bson:"_id"`?

Didn't work for UUID as array ("not hashable"), but now with the UUID as string
I added it again.

https://codereview.appspot.com/8322043/diff/36002/state/environ.go#newcode31
state/environ.go:31: err := st.environments.Find(D{{"name", D{{"$ne",
""}}}}).One(&doc)
On 2013/04/05 21:03:42, fwereade wrote:
> This bit is implemented as if you'd set the UUID to the _id above... but
further
> down, you use the global key. Which is it to be, and why?

Sorry, can't follow. That's how we discussed it. But now I've changed it to a
non-empty UUID field.

https://codereview.appspot.com/8322043/diff/36002/state/environ.go#newcode74
state/environ.go:74: Id:     environGlobalKey,
On 2013/04/05 21:03:42, fwereade wrote:
> If you *really* don't want to use the UUID (why?) please at least use a
constant
> that is *not* the global key, such as "environment" (as it originally was).
> 
> This is not to say that those keys *shouldn't* be the same -- they probably
> should -- but while they're not, please stay consistent :).

The UUID hasn't been the _id before, due to mgo reasons. But still I could have
used the UUID here, yes. 

In the beginning we talked about a collection ("environments") with only one
document. That's why I used a constant Id and FindId() before.

https://codereview.appspot.com/8322043/diff/36002/state/environ_test.go
File state/environ_test.go (right):

https://codereview.appspot.com/8322043/diff/36002/state/environ_test.go#newco...
state/environ_test.go:42: c.Assert(uuidA, Not(Equals), uuidB)
On 2013/04/05 21:03:42, fwereade wrote:
> Not(DeepEquals), surely? Might even be clearer to test that their String()s
are
> not equal...

Ouch, shure, see trivial_test.go. But now as string it's ok again. ;)

https://codereview.appspot.com/8322043/diff/36002/state/open.go
File state/open.go (right):

https://codereview.appspot.com/8322043/diff/36002/state/open.go#newcode149
state/open.go:149: createConstraintsOp(st, environGlobalKey,
constraints.Value{}),
On 2013/04/05 21:03:42, fwereade wrote:
> The operation order is still bad here. I think you might have missed my
> follow-up comments on patch set 2?

Hmm, can't follow. To me it's like Dimiter suggested a natural order creating
the environment, its settings and its constraints. This transaction works. So
why do your think it is a wrong order? Which failure do you expect? And which
test should have catched this wrong order?

https://codereview.appspot.com/8322043/diff/36002/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/8322043/diff/36002/worker/uniter/uniter.go#new...
worker/uniter/uniter.go:59: panic(err)
On 2013/04/05 21:03:42, fwereade wrote:
> Sorry, I really should have made sure my comments were all on the latest patch
> set. I thought I was being clearer by continuing the conversations, but I
guess
> not. Sorry :(

Ah, thanks, found it.

[TheMue, 2013-04-08 14:17:43]

Please take a look.

https://codereview.appspot.com/8322043/diff/36002/state/open.go
File state/open.go (right):

https://codereview.appspot.com/8322043/diff/36002/state/open.go#newcode149
state/open.go:149: createConstraintsOp(st, environGlobalKey,
constraints.Value{}),
On 2013/04/08 13:45:39, TheMue wrote:
> On 2013/04/05 21:03:42, fwereade wrote:
> > The operation order is still bad here. I think you might have missed my
> > follow-up comments on patch set 2?
> 
> Hmm, can't follow. To me it's like Dimiter suggested a natural order creating
> the environment, its settings and its constraints. This transaction works. So
> why do your think it is a wrong order? Which failure do you expect? And which
> test should have catched this wrong order?

Has been discussed online, so ok.

[fwereade, 2013-04-08 15:38:25]

LGTM with the below

https://codereview.appspot.com/8322043/diff/46001/trivial/uuid.go
File trivial/uuid.go (right):

https://codereview.appspot.com/8322043/diff/46001/trivial/uuid.go#newcode13
trivial/uuid.go:13: func GenerateUUID() (UUID, error) {
NewUUID

https://codereview.appspot.com/8322043/diff/46001/trivial/uuid.go#newcode44
trivial/uuid.go:44: func NewUUID() (string, error) {
NewUUIDString

...and please either test it or just drop this func and use .String() at the
call site.

https://codereview.appspot.com/8322043/diff/46001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/8322043/diff/46001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:167: `: %s`+ // JUJU_ENV_UUID
I still think it's worth explaining that this is here to stop tests polluting
one another.

[TheMue, 2013-04-08 16:29:19]

Please take a look.

https://codereview.appspot.com/8322043/diff/46001/trivial/uuid.go
File trivial/uuid.go (right):

https://codereview.appspot.com/8322043/diff/46001/trivial/uuid.go#newcode13
trivial/uuid.go:13: func GenerateUUID() (UUID, error) {
On 2013/04/08 15:38:25, fwereade wrote:
> NewUUID

Done.

https://codereview.appspot.com/8322043/diff/46001/trivial/uuid.go#newcode44
trivial/uuid.go:44: func NewUUID() (string, error) {
On 2013/04/08 15:38:25, fwereade wrote:
> NewUUIDString
> 
> ...and please either test it or just drop this func and use .String() at the
> call site.

Done.

https://codereview.appspot.com/8322043/diff/46001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/8322043/diff/46001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:167: `: %s`+ // JUJU_ENV_UUID
On 2013/04/08 15:38:25, fwereade wrote:
> I still think it's worth explaining that this is here to stop tests polluting
> one another.

Done.

[dimitern, 2013-04-08 16:50:29]

LGTM, much nicer - just a few trivals.

https://codereview.appspot.com/8322043/diff/59001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/59001/state/environ.go#newcode66
state/environ.go:66: doc := &environmentDoc{
just &environmentDoc{uuid, name} ?

https://codereview.appspot.com/8322043/diff/59001/state/environ_test.go
File state/environ_test.go (right):

https://codereview.appspot.com/8322043/diff/59001/state/environ_test.go#newco...
state/environ_test.go:35: s.MgoSuite.TearDownTest(c)
smart and cheeky :)

https://codereview.appspot.com/8322043/diff/59001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/8322043/diff/59001/state/open.go#newcode149
state/open.go:149: createEnvironmentOp(st, cfg.Name(), uuid.String()),
sorry, i was wrong before, please move the env creation first, makes a lot more
sense.

https://codereview.appspot.com/8322043/diff/59001/trivial/trivial_test.go
File trivial/trivial_test.go (right):

https://codereview.appspot.com/8322043/diff/59001/trivial/trivial_test.go#new...
trivial/trivial_test.go:118: c.Assert(uuidStr, Matches,
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}")
I still think this ought to move to a const ValidUUID.

[TheMue, 2013-04-08 17:35:54]

*** Submitted:

state: add environment persistency with UUID

Changed environment to be stored using an own document
in an own collection. Currently it only stores the name
and the UUID of the environment. The latter is generated
during the initializing of the state. Both, name and
UUID cannot be changed. The UUID will be passed as the
environment variable $JUJU_ENV_UUID to the charm hooks.

R=fwereade, dimitern
CC=
https://codereview.appspot.com/8322043

https://codereview.appspot.com/8322043/diff/59001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/8322043/diff/59001/state/environ.go#newcode66
state/environ.go:66: doc := &environmentDoc{
On 2013/04/08 16:50:29, dimitern wrote:
> just &environmentDoc{uuid, name} ?

Done.

https://codereview.appspot.com/8322043/diff/59001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/8322043/diff/59001/state/open.go#newcode149
state/open.go:149: createEnvironmentOp(st, cfg.Name(), uuid.String()),
On 2013/04/08 16:50:29, dimitern wrote:
> sorry, i was wrong before, please move the env creation first, makes a lot
more
> sense.

See discussion with William, this order has to do with mgo/txn and how it works
in case of a failing transaction.

https://codereview.appspot.com/8322043/diff/59001/trivial/trivial_test.go
File trivial/trivial_test.go (right):

https://codereview.appspot.com/8322043/diff/59001/trivial/trivial_test.go#new...
trivial/trivial_test.go:118: c.Assert(uuidStr, Matches,
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}")
On 2013/04/08 16:50:29, dimitern wrote:
> I still think this ought to move to a const ValidUUID.

As we only test it here I would like to keep it as it is. In case we have to
validate external UUIDs (and maybe convert it into the type) it gets more than
just one regexp.  That function would have to validate the binary form as well
as string representations and here the different versions and variants. It's not
very complex, but we don't need it yet.