Open Stack

Excerpts from Hayes, Graham's message of 2016-01-28 17:01:09 +0000:
> Recently I tried started to use oslo.versionedobjects for a project.
> 
> After playing around with it for a while, I noticed I could set "this is
> not a uuid" as the value of a UUIDField.
> 
> After making sure I made no mistakes - I looked at the underlying code,
> and found:[0]
> 
> class UUID(FieldType):
>      @staticmethod
>      def coerce(obj, attr, value):
>          # FIXME(danms): We should actually verify the UUIDness here
>          return str(value)
> 
> So, I went to implement this. [1]
> 
> it quickly got -2'd as it would break Nova - so I went and implemented 2
> steps of a 4 step process to get this field working as it should.
> 
> In the review I was told: [2]
> 
> "... I think that if a project wants that level of enforcement it
> needs to land the project, not in the library. Libraries ideally should
> support all supported branches of OpenStack."
> 
> Basically - if a project wants the UUIDField to act like a UUIDField,
> and not a field that str()'s all input, they should copy and paste code
> around.

That's not actually the only option, as you point out below.

> 
> This is being blocked by the -2 until Nova's unit tests are fixed (just
> Nova's - we have no way of knowing how many projects assumed it was
> testing UUIDness and will break)
> 
> The steps I had looked at doing was this:
> 
> 1. Allow a "validate" flag on the Field __init__() defaulting to False.
> 1.1. This would allow current projects to continue as is, and projects
>       starting for the first time to do the right thing.
> 2. Deprecate the default value - issue a FutureWarning that it is
>     changing to True
> 3. Deprecate the option entirely.
> 4. Remove the option, and always validate.
> 
> 3 & 4 are even optional if some projects want to keep using UUIDFields
> like StringFields.
> 
> Currently the -2 still stands as the reviewer does not like the idea of
> a flag.
> 
> What are the options for this now? If we are supposed to support all
> stable branches of all projects, this is the only option if it is going
> to merge in the next 2 years.
> 
> Or we can create a ActuallyValidatingUUIDField?

I like the idea of adding a new class, though maybe not the name
you've proposed here. Projects that want enforcement could use that
instead of the UUIDField. Then, as we're able to "fix" UUIDs in
other projects, the existing UUIDField class can be deprecated in
favor of the new one.

> 
> Also, olso seem to be very -2 heavy. This means that alternative views
> on the review are very unlikely. My question is what is the difference
> between a -1 and a -2 for oslo?

I'm not sure the Oslo review team's patterns are the same as in some
other projects. We do tend to discuss things that have negative reviews.

[1] https://review.openstack.org/270178

> 
> In designate we reserve -2 for things that will completely break our
> code, or is completely out of line for the project. (I would hope
> implementing a FIXME is not out of line for the project)

No, but fixing it in a way that is known to break other projects
is. In this case, the change is known to break at least one project.
We have to be extremely careful with things that look like breaking
changes, since we can break *everyone* with a release. So I think
in this case the -2 is warranted.

The other case you've pointed out on IRC, of the logging timezone
thing [1], is my -2. It was originally implemented as a breaking
change.  That has been fixed, but it still needs some discussion
on the mailing list, at least in part because I don't see the point
of the change.

Doug

> 
> Thanks,
> 
> Graham
> 
> 0 - 
> https://git.openstack.org/cgit/openstack/oslo.versionedobjects/tree/oslo_versionedobjects/fields.py#n305
> 
> 1 - https://review.openstack.org/#/c/250493/
> 
> 2 - 
> https://review.openstack.org/#/c/250493/9/oslo_versionedobjects/fields.py
>