Open Stack

Excerpts from Hayes, Graham's message of 2016-01-28 19:17:54 +0000:
> 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.

I'm not sure what's involved in rolling out the changes in all projects
that would eventually let us deprecate the argument. How many projects
are we talking about, and what sort of time frame?

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

Yes, I deliberately chose to use -2 for that very reason.

Although I am certainly not the only contributor, I have been the
most recent principal developer on oslo.log. I feel quite strongly
that the change you have proposed is a bad idea, and not in the
best interests of the future direction for that library (as you say
above "out of line for the project"), if for no other reason than
it seems trivial to modify the timezone used for logging of a given
process without changing oslo.log code at all. I have indicated my
opinion using the review system in the way it is meant to be used.

If at some point a significant portion of the core team is convinced
I'm wrong, or I become convinced that the patch is a better idea
than it seems right now, I'll remove the -2. In the mean time, maybe
we should talk about why you think the change is important and
useful.

Doug