[openstack-dev] [oslo][oslo.versionedobjects] Is it possible to make changes to oslo repos?

Hayes, Graham graham.hayes at hpe.com
Thu Jan 28 19:17:54 UTC 2016


On 28/01/2016 18:54, 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.

This is my least favorite option - as it allows people to do the same
as me, and wonder why their UUIDField is not working.

The steps above allow for having a single Field type, and still allow
current users to keep current usage.

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

As I said in another message - I think they are quite "sticky"

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

When I broke everything - yes it was. My point is when it stopped being 
breaking, it should no longer have the -2.

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

And my point is that while you have a -2, even if 2 other cores can see
the point, they cannot merge it until your -2 is removed.

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




More information about the OpenStack-dev mailing list