Code review - Issue 8561045: finish nonced provisioninghttps://codereview.appspot.com/2013-04-10T12:36:40+00:00rietveld
Message from unknown
2013-04-09T13:27:36+00:00dimiternurn:md5:ec607037ceacd95f76e5c4927bc1f83d
Message from dimiter.naydenov@canonical.com
2013-04-09T13:27:42+00:00dimiternurn:md5:15c6ee7f0ce5de20f5057a9f657a73de
Please take a look.
Message from themue@gmail.com
2013-04-09T15:12:22+00:00TheMueurn:md5:a04bde5cb92215899e3ac50621ae1a8d
Mostly OK, but I'm not happy with the term ValidUUID, see comment.
https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go
File trivial/uuid.go (right):
https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go#newcode12
trivial/uuid.go:12: // ValidUUID specifies the regular expression that matches an UUID.
As told before, that's not correct. A UUID is a 16 octet binary value. The regexp is only matching the *string representation* of a UUID, and here especially version 4 variant 1. There are others that a valid too but would not be covered by this regexp.
Message from dimiter.naydenov@canonical.com
2013-04-09T15:14:32+00:00dimiternurn:md5:cfbd9190a46a160b76e56495ef195058
On 2013/04/09 15:12:22, TheMue wrote:
> Mostly OK, but I'm not happy with the term ValidUUID, see comment.
>
> https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go
> File trivial/uuid.go (right):
>
> https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go#newcode12
> trivial/uuid.go:12: // ValidUUID specifies the regular expression that matches
> an UUID.
> As told before, that's not correct. A UUID is a 16 octet binary value. The
> regexp is only matching the *string representation* of a UUID, and here
> especially version 4 variant 1. There are others that a valid too but would not
> be covered by this regexp.
Then it's worth mentioning this in the comment, like: "ValidUUID specifies the regular expression to match a UUID version 4, variant 1. "? I still think it's worth having it as an exported const, just for reasons like this.
Message from themue@gmail.com
2013-04-09T15:19:26+00:00TheMueurn:md5:aeac75fd77147604068191d39bf7ec2d
On 2013/04/09 15:14:32, dimitern wrote:
> On 2013/04/09 15:12:22, TheMue wrote:
> > Mostly OK, but I'm not happy with the term ValidUUID, see comment.
> >
> > https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go
> > File trivial/uuid.go (right):
> >
> > https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go#newcode12
> > trivial/uuid.go:12: // ValidUUID specifies the regular expression that matches
> > an UUID.
> > As told before, that's not correct. A UUID is a 16 octet binary value. The
> > regexp is only matching the *string representation* of a UUID, and here
> > especially version 4 variant 1. There are others that a valid too but would
> not
> > be covered by this regexp.
>
> Then it's worth mentioning this in the comment, like: "ValidUUID specifies the
> regular expression to match a UUID version 4, variant 1. "? I still think it's
> worth having it as an exported const, just for reasons like this.
Having a const is ok (I btw tested elsewhere simply with HasLen 36). But the naming is wrong. Sadly a correct name would be long: ValidVer4Var1UUIDString. ;) But ValidUUIDString should be fine.
Message from dimiter.naydenov@canonical.com
2013-04-09T15:30:31+00:00dimiternurn:md5:e9e0af4c19b11bf52ed0ac044b9f5cb6
On 2013/04/09 15:19:26, TheMue wrote:
> On 2013/04/09 15:14:32, dimitern wrote:
> > On 2013/04/09 15:12:22, TheMue wrote:
> > > Mostly OK, but I'm not happy with the term ValidUUID, see comment.
> > >
> > > https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go
> > > File trivial/uuid.go (right):
> > >
> > > https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go#newcode12
> > > trivial/uuid.go:12: // ValidUUID specifies the regular expression that
> matches
> > > an UUID.
> > > As told before, that's not correct. A UUID is a 16 octet binary value. The
> > > regexp is only matching the *string representation* of a UUID, and here
> > > especially version 4 variant 1. There are others that a valid too but would
> > not
> > > be covered by this regexp.
> >
> > Then it's worth mentioning this in the comment, like: "ValidUUID specifies the
> > regular expression to match a UUID version 4, variant 1. "? I still think it's
> > worth having it as an exported const, just for reasons like this.
>
> Having a const is ok (I btw tested elsewhere simply with HasLen 36). But the
> naming is wrong. Sadly a correct name would be long: ValidVer4Var1UUIDString. ;)
> But ValidUUIDString should be fine.
It is a string, there's no doubt about it. If you really insist on the suffix, ok, although I think is making the name longer for no good reason.
Message from themue@gmail.com
2013-04-09T15:48:19+00:00TheMueurn:md5:fa2ed1412df61a073245daac91e4e619
On 2013/04/09 15:30:31, dimitern wrote:
> On 2013/04/09 15:19:26, TheMue wrote:
> > On 2013/04/09 15:14:32, dimitern wrote:
> > > On 2013/04/09 15:12:22, TheMue wrote:
> > > > Mostly OK, but I'm not happy with the term ValidUUID, see comment.
> > > >
> > > > https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go
> > > > File trivial/uuid.go (right):
> > > >
> > > > https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go#newcode12
> > > > trivial/uuid.go:12: // ValidUUID specifies the regular expression that
> > matches
> > > > an UUID.
> > > > As told before, that's not correct. A UUID is a 16 octet binary value. The
> > > > regexp is only matching the *string representation* of a UUID, and here
> > > > especially version 4 variant 1. There are others that a valid too but
> would
> > > not
> > > > be covered by this regexp.
> > >
> > > Then it's worth mentioning this in the comment, like: "ValidUUID specifies
> the
> > > regular expression to match a UUID version 4, variant 1. "? I still think
> it's
> > > worth having it as an exported const, just for reasons like this.
> >
> > Having a const is ok (I btw tested elsewhere simply with HasLen 36). But the
> > naming is wrong. Sadly a correct name would be long: ValidVer4Var1UUIDString.
> ;)
> > But ValidUUIDString should be fine.
>
> It is a string, there's no doubt about it. If you really insist on the suffix,
> ok, although I think is making the name longer for no good reason.
As it is used in the direct context of the type UUID I would like ti have it named correctly.
Message from fwereade@gmail.com
2013-04-09T21:27:21+00:00fwereadeurn:md5:fac58729c35e9a0d16573e199c0e616b
LGTM
https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):
https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go#newcode135
cmd/jujud/machine.go:135: if m.CheckProvisioned(a.Conf.MachineNonce) {
I'd prefer to invert this and have the error return inside the if -- it just feels a bit less mentally costly.
https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go#newcode138
cmd/jujud/machine.go:138: // Not provisioned -> no need to run the agent further.
// The agent is running on a different machine to the one it
// should be according to state. It must stop immediately.
https://codereview.appspot.com/8561045/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):
https://codereview.appspot.com/8561045/diff/1/worker/provisioner/provisioner.go#newcode278
worker/provisioner/provisioner.go:278: uniqueNonce := uuid.String()
Please badge this with the tag of the machine running the provisioner, to match BootstrapNonce's style.
Message from dimiter.naydenov@canonical.com
2013-04-10T12:36:40+00:00dimiternurn:md5:52530af3f17859e40ca223f1cd979956
I screwed up the branch by interrupting lbox propose (remembered to change the description to include the changes to NewProvisioner) - it was too late, and now the branch in LP needs to be deleted and pushed again. I'll recreate this CL afterwards, so I'm closing this one.
https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):
https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go#newcode135
cmd/jujud/machine.go:135: if m.CheckProvisioned(a.Conf.MachineNonce) {
On 2013/04/09 21:27:21, fwereade wrote:
> I'd prefer to invert this and have the error return inside the if -- it just
> feels a bit less mentally costly.
Done.
https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go#newcode138
cmd/jujud/machine.go:138: // Not provisioned -> no need to run the agent further.
On 2013/04/09 21:27:21, fwereade wrote:
> // The agent is running on a different machine to the one it
> // should be according to state. It must stop immediately.
Done.
https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go
File trivial/uuid.go (right):
https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go#newcode12
trivial/uuid.go:12: // ValidUUID specifies the regular expression that matches an UUID.
On 2013/04/09 15:12:22, TheMue wrote:
> As told before, that's not correct. A UUID is a 16 octet binary value. The
> regexp is only matching the *string representation* of a UUID, and here
> especially version 4 variant 1. There are others that a valid too but would not
> be covered by this regexp.
The only thing this regexp validates is the UUID generated by NewUUID - it doesn't generate all the possible range of versions and variants, right? I updated the comment as discussed to include (version 4, variant 2) and changed the name to ValidUUIDString. I also split the test for UUID in its own module.
https://codereview.appspot.com/8561045/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):
https://codereview.appspot.com/8561045/diff/1/worker/provisioner/provisioner.go#newcode278
worker/provisioner/provisioner.go:278: uniqueNonce := uuid.String()
On 2013/04/09 21:27:21, fwereade wrote:
> Please badge this with the tag of the machine running the provisioner, to match
> BootstrapNonce's style.
As discussed, by passing the machine id to NewProvisioner, we have this information here and it's used to get the tag for the badge.