Open Stack

> On Jan 28, 2016, at 1:10 PM, Mike Bayer <mbayer at redhat.com> wrote:
> 
> 
> 
> On 01/28/2016 01:52 PM, Doug Hellmann wrote:
>> 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.
> 
> I'm +1 on a new class, -1 on consuming projects implementing this
> themselves (e.g. more cut-and-paste of key functionality).   Normally
> I'd be +1 on the "validates=True" flag approach as well but that makes
> it impossible to ever change the default to True someday.  Better to
> deprecate UUIDField in favor of a new class.

As the-guy-in-nova-that-does-this-a-bunch, I’ve pushed a bunch of changes to nova because we didn’t have them yet in o.vo, and once they get released, I “re-sync” them back to nova to use the o.vo version, see:

https://review.openstack.org/#/c/272641/
https://github.com/openstack/oslo.versionedobjects/commit/442ddcaeab0184268ff987d4ff1bf0f95dd87f2e
https://github.com/openstack/oslo.versionedobjects/commit/b8818720862490c31a412e6cf6abf1962fd7a2b0
https://github.com/openstack/oslo.versionedobjects/commit/cb898f382e9a22dc9dcbc2de753d37b1b107176d

I don’t find it all that terrible, but maybe that’s because I’m only watching it in one project.

> 
>> 
>>> 
>>> 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
>>> 
>> 
>> __________________________________________________________________________
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>> 
> 
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


-----
Thanks,

Ryan Rossiter (rlrossit)