> On Jan 28, 2016, at 1:10 PM, Mike Bayer <mba...@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-requ...@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-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ----- Thanks, Ryan Rossiter (rlrossit) __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev