Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1889)

Issue 8322043: state: add environment persistency with UUID (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 12 months ago by TheMue
Modified:
10 years, 11 months ago
Reviewers:
dimitern, mp+156891, fwereade
Visibility:
Public.

Description

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)

Patch Set 1 #

Total comments: 13

Patch Set 2 : state: add environment persistency with UUID #

Total comments: 33

Patch Set 3 : state: add environment persistency with UUID #

Total comments: 16

Patch Set 4 : state: add environment persistency with UUID #

Total comments: 16

Patch Set 5 : state: add environment persistency with UUID #

Total comments: 6

Patch Set 6 : state: add environment persistency with UUID #

Patch Set 7 : state: add environment persistency with UUID #

Total comments: 7

Patch Set 8 : state: add environment persistency with UUID #

Unified diffs Side-by-side diffs Delta from patch set Stats (+175 lines, -33 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M state/environ.go View 1 2 3 4 5 6 7 2 chunks +43 lines, -8 lines 0 comments Download
M state/environ_test.go View 1 2 3 4 1 chunk +15 lines, -0 lines 0 comments Download
M state/open.go View 1 2 3 4 5 6 4 chunks +11 lines, -4 lines 0 comments Download
M state/state.go View 1 2 3 4 5 6 4 chunks +5 lines, -4 lines 0 comments Download
M state/watcher.go View 1 2 1 chunk +1 line, -1 line 0 comments Download
M trivial/trivial_test.go View 1 2 3 4 5 6 1 chunk +18 lines, -0 lines 0 comments Download
A trivial/uuid.go View 1 2 3 4 5 6 1 chunk +41 lines, -0 lines 0 comments Download
M worker/uniter/context.go View 1 2 3 4 3 chunks +7 lines, -1 line 0 comments Download
M worker/uniter/context_test.go View 1 2 3 4 5 6 8 chunks +15 lines, -7 lines 0 comments Download
M worker/uniter/uniter.go View 1 2 3 4 3 chunks +8 lines, -1 line 0 comments Download
M worker/uniter/uniter_test.go View 1 2 3 4 5 6 5 chunks +9 lines, -7 lines 0 comments Download

Messages

Total messages: 15
TheMue
Please take a look.
10 years, 12 months ago (2013-04-03 15:56:55 UTC) #1
fwereade
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): ...
10 years, 12 months ago (2013-04-04 06:34:11 UTC) #2
TheMue
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 ...
10 years, 12 months ago (2013-04-04 12:40:58 UTC) #3
fwereade
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, ...
10 years, 12 months ago (2013-04-04 13:15:40 UTC) #4
dimitern
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): ...
10 years, 12 months ago (2013-04-04 14:52:14 UTC) #5
TheMue
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 ...
10 years, 12 months ago (2013-04-05 12:05:20 UTC) #6
fwereade
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 ...
10 years, 12 months ago (2013-04-05 16:52:55 UTC) #7
TheMue
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 ...
10 years, 12 months ago (2013-04-05 19:27:40 UTC) #8
fwereade
Sorry, I had a few comments in unhelpful places that I don't think you saw. ...
10 years, 12 months ago (2013-04-05 21:03:42 UTC) #9
TheMue
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, ...
10 years, 11 months ago (2013-04-08 13:45:39 UTC) #10
TheMue
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 ...
10 years, 11 months ago (2013-04-08 14:17:43 UTC) #11
fwereade
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) { ...
10 years, 11 months ago (2013-04-08 15:38:25 UTC) #12
TheMue
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) { ...
10 years, 11 months ago (2013-04-08 16:29:19 UTC) #13
dimitern
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 ...
10 years, 11 months ago (2013-04-08 16:50:29 UTC) #14
TheMue
10 years, 11 months ago (2013-04-08 17:35:54 UTC) #15
*** 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b