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. > >> >> 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