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

[Ryan Rossiter]

> On Jan 28, 2016, at 5:01 PM, Dan Smith  wrote:
> 
>> I know we have some projects (heat, I think?) that don't have UUIDs at
>> all. Are they using oslo.versionedobjects? I suppose we could move them
>> to a string field instead of a UUID field, instead of flipping the
>> enforcement flag on. Alternately, if we add a new class we wouldn't have
>> to ever actually deprecate the UUIDField class, though we might add a
>> warning to the docs that it isn't doing any validation and the new class
>> is preferred.
> 
> If a project isn't using UUIDs then they have no reason to use a
> UUIDField and thus aren't affected. If they are, then they're doing
> something wrong.
> 
>> I'll be curious to see what Dan and Sean have to say when they catch up
>> on this thread.
> 
> I think Ryan summed it up very well earlier, but I will echo and
> elaborate here for clarity.
> 
> To be honest, I think the right thing to do is deprecate the
> non-validating behavior and have projects use in-project validating
> fields for UUIDs (i.e. their own UUIDField implementation). When we can,
> release a major version with the validating behavior turned on.
> 
> I don't like the validate=False flag because it's hard (or at least
> ugly) to deprecate. Even allowing the call signature to tolerate it for
> compatibility but still doing the validation is terrible, IMHO. If
> people feel it is absolutely critical to have an implementation in o.vo
> right now, then we can do the parallel class option, but we basically
> just have to alias the old one to the new one when we "drop" the
> non-validating functionality, IMHO, which is just more baggage. To quote
> Ryan, "if you know you're going to break people, just don't do it."
> 
> This is a really minor issue in my opinion -- the amount of code a
> project needs to replicate is exceedingly small, especially if they just
> subclass the existing field and override the one method required to
> ensure coercion. For a point of reference, nova has a lot of these
> fields which are too nova-specific to put into o.vo; adding one more for
> this is immeasurably small:
> 
> https://github.com/openstack/nova/blob/master/nova/objects/fields.py#L76-L621
> 
> Cinder also has some already:
> 
> https://github.com/openstack/cinder/blob/master/cinder/objects/fields.py

You’re welcome for the extra Cinder evidence, Dan ;).

> 
> And to again echo Ryan's comments, we have always landed things in nova,
> sync'd them to o.vo and removed our local copy once we can depend on a
> new-enough o.vo release. This is effectively the same behavior for this
> change and really just isn't that big of a deal. Please, let's do the
> safest thing for the projects that consume our library, and for the
> people who have to use the same gate as all the rest of us.

For anyone who cares how this works, here’s a typical process for doing custom 
fields:

1) Put the field in Nova [1]
2) Put the new field in o.vo [2]
3) After o.vo is released, re-sync [3]

[1] 
https://github.com/openstack/nova/commit/b9247f52d17e18d075b995ac8a438ec5e65eacbf
[2] 
https://github.com/openstack/oslo.versionedobjects/commit/2e083bce6e4b325cb89baea4b1d6173d58c8f5bd
[3] 
https://github.com/openstack/nova/commit/3c83a47bb70ad9db6c7900e6c752f08777fa0787

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

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

[Matt Riedemann]

On 1/28/2016 11:01 PM, Dan Smith wrote:

I know we have some projects (heat, I think?) that don't have UUIDs at
all. Are they using oslo.versionedobjects? I suppose we could move them
to a string field instead of a UUID field, instead of flipping the
enforcement flag on. Alternately, if we add a new class we wouldn't have
to ever actually deprecate the UUIDField class, though we might add a
warning to the docs that it isn't doing any validation and the new class
is preferred.


If a project isn't using UUIDs then they have no reason to use a
UUIDField and thus aren't affected. If they are, then they're doing
something wrong.


I'll be curious to see what Dan and Sean have to say when they catch up
on this thread.


I think Ryan summed it up very well earlier, but I will echo and
elaborate here for clarity.

To be honest, I think the right thing to do is deprecate the
non-validating behavior and have projects use in-project validating
fields for UUIDs (i.e. their own UUIDField implementation). When we can,
release a major version with the validating behavior turned on.

I don't like the validate=False flag because it's hard (or at least
ugly) to deprecate. Even allowing the call signature to tolerate it for
compatibility but still doing the validation is terrible, IMHO. If
people feel it is absolutely critical to have an implementation in o.vo
right now, then we can do the parallel class option, but we basically
just have to alias the old one to the new one when we "drop" the
non-validating functionality, IMHO, which is just more baggage. To quote
Ryan, "if you know you're going to break people, just don't do it."

This is a really minor issue in my opinion -- the amount of code a
project needs to replicate is exceedingly small, especially if they just
subclass the existing field and override the one method required to
ensure coercion. For a point of reference, nova has a lot of these
fields which are too nova-specific to put into o.vo; adding one more for
this is immeasurably small:

https://github.com/openstack/nova/blob/master/nova/objects/fields.py#L76-L621

Cinder also has some already:

https://github.com/openstack/cinder/blob/master/cinder/objects/fields.py

And to again echo Ryan's comments, we have always landed things in nova,
sync'd them to o.vo and removed our local copy once we can depend on a
new-enough o.vo release. This is effectively the same behavior for this
change and really just isn't that big of a deal. Please, let's do the
safest thing for the projects that consume our library, and for the
people who have to use the same gate as all the rest of us.

--Dan

__
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



I also agree with deprecating the non-validation behavior and releasing 
with a major version bump (in theory) *but* because of the backward 
compat stance taken since liberty, we can't just drop this since it 
would break unit test jobs in several projects on stable/liberty, at 
least nova and cinder. And that's because (1) nova unit test jobs on 
liberty aren't using upper-constraints and (2) we aren't capping 
libraries in g-r in stable/liberty, because of said upper-constraints. I 
have my own issues with that, but that's the plan that everyone is 
working toward right now, so knowingly dropping the non-uuid support 
would break liberty and we can't allow that until it's gone from all of 
the projects, so we could be looking at mitaka-eol or newton-eol 
(assuming all projects were using UUIDField with real uuid's by the time 
stable/newton is created).  That sucks, I know, but that's the backward 
compat stance with upper-constraints (which I'm not a fan of) and right 
now everyone is having to deal with it until that changes, if that changes.


--

Thanks,

Matt Riedemann


__
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-dev] [oslo][oslo.versionedobjects] Is it possible to make changes to oslo repos?

[Hayes, Graham]

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.

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?

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?

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)

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

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

[Davanum Srinivas]

Graham,

Quite unfair characterization of Oslo being -2 heavy. Please compare
stats before making such an assertion:

http://russellbryant.net/openstack-stats/oslo-reviewers-365.txt
http://russellbryant.net/openstack-stats/nova-reviewers-365.txt
http://russellbryant.net/openstack-stats/neutron-reviewers-365.txt

On the one single specific review you had a -2, you should be talking
to the reviewer on IRC or bring it to the next Oslo meeting.

Thanks,
Dims


On Thu, Jan 28, 2016 at 12:29 PM, Joshua Harlow  wrote:
> Hayes, Graham wrote:
>>
>> 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?
>
>
> If this is the case I am sorry about it, and I'd also like to think that we
> (as the oslo team) need not do this (or think about it more before we do
> it), because it usually isn't really needed (a -1 suffices IMHO for most
> things, especially things that are being actively discussed).
>
> But I'm also in the camp that thinks the whole -1 or -2 is sorta dumb and
> IMHO just leaving comments and talking to people on IRC to work through
> issues/discussion/code is better...
>
> But then that requires people<->people interaction and I guess not everyone
> likes to do that(?)
>
> In general I hope it's not all of oslo u are grouping here because, if its
> just a few cases, hopefully we can work with the person that is -2ing stuff
> to not do it willy nilly...
>
> My 2 cents,
>
> -Josh
>
>
>
>
> __
> 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



-- 
Davanum Srinivas :: https://twitter.com/dims

__
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

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

[Doug Hellmann]

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

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

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

[Joshua Harlow]

Hayes, Graham wrote:

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?


If this is the case I am sorry about it, and I'd also like to think that 
we (as the oslo team) need not do this (or think about it more before we 
do it), because it usually isn't really needed (a -1 suffices IMHO for 
most things, especially things that are being actively discussed).


But I'm also in the camp that thinks the whole -1 or -2 is sorta dumb 
and IMHO just leaving comments and talking to people on IRC to work 
through issues/discussion/code is better...


But then that requires people<->people interaction and I guess not 
everyone likes to do that(?)


In general I hope it's not all of oslo u are grouping here because, if 
its just a few cases, hopefully we can work with the person that is 
-2ing stuff to not do it willy nilly...


My 2 cents,

-Josh



__
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

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

[Hayes, Graham]

On 28/01/2016 18:44, Davanum Srinivas wrote:
> Graham,
>
> Quite unfair characterization of Oslo being -2 heavy. Please compare
> stats before making such an assertion:

OK - that was probably my frustration boiling over at the time. I should
not have made that generalization. I had been involved in 3 patches to
olso repos in the last week, and 2 got -2'd, and that came out unfairly
in this message.

The initial -2s where not as annoying as the stickyness of them. When
the issues that led to the -2's were fixed, they were not moved to -1.

I see -2's as vetos - if I am mistaken please tell me, but that is how
most of the openstack projectsm and other projects using gerrit seem
to have treated them.

> http://russellbryant.net/openstack-stats/oslo-reviewers-365.txt
> http://russellbryant.net/openstack-stats/nova-reviewers-365.txt
> http://russellbryant.net/openstack-stats/neutron-reviewers-365.txt
>
> On the one single specific review you had a -2, you should be talking
> to the reviewer on IRC or bring it to the next Oslo meeting.

Did that. And on the review itself.

> Thanks,
> Dims
>
>
> On Thu, Jan 28, 2016 at 12:29 PM, Joshua Harlow  wrote:
>> Hayes, Graham wrote:
>>>
>>> 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?
>>
>>
>> If this is the case I am sorry about it, and I'd also like to think that we
>> (as the oslo team) need not do this (or think about it more before we do
>> it), because it usually isn't really needed (a -1 suffices IMHO for most
>> things, especially things that are being actively discussed).
>>
>> But I'm also in the camp that thinks the whole -1 or -2 is sorta dumb and
>> IMHO just leaving comments and talking to people on IRC to work through
>> issues/discussion/code is better...
>>
>> But then that requires people<->people interaction and I guess not everyone
>> likes to do that(?)
>>
>> In general I hope it's not all of oslo u are grouping here because, if its
>> just a few cases, hopefully we can work with the person that is -2ing stuff
>> to not do it willy nilly...
>>
>> My 2 cents,
>>
>> -Josh
>>
>>
>>
>>
>> __
>> 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

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

[Mike Bayer]

On 01/28/2016 01:52 PM, Doug Hellmann wrote:
> Excerpts from Hayes, Graham's message of 2016-01-28 17:01:09 +:
>> 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

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

[Hayes, Graham]

On 28/01/2016 19:13, Mike Bayer wrote:
>
>
> On 01/28/2016 01:52 PM, Doug Hellmann wrote:
>> Excerpts from Hayes, Graham's message of 2016-01-28 17:01:09 +:
 >>>
>>> 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.

I actually ended up writing this

http://docs.openstack.org/developer/debtcollector/examples.html#changing-the-default-value-of-a-keyword-argument
 


to allow us to deprecate the default value in the future.

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


__
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

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

[Hayes, Graham]

On 28/01/2016 18:54, Doug Hellmann wrote:
> Excerpts from Hayes, Graham's message of 2016-01-28 17:01:09 +:
>> 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-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>


__
OpenStack 

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

[Doug Hellmann]

Excerpts from Hayes, Graham's message of 2016-01-28 19:02:57 +:
> On 28/01/2016 18:44, Davanum Srinivas wrote:
> > Graham,
> >
> > Quite unfair characterization of Oslo being -2 heavy. Please compare
> > stats before making such an assertion:
> 
> OK - that was probably my frustration boiling over at the time. I should
> not have made that generalization. I had been involved in 3 patches to
> olso repos in the last week, and 2 got -2'd, and that came out unfairly
> in this message.

As Dims pointed out, the ratio of -2s on your recent patches is an
aberration. It just happens that you've picked some tricky things
to work on, and those things need more communication than they might
in other projects because of the nature of Oslo. Please don't
interpret them as shutting down discussion.

> 
> The initial -2s where not as annoying as the stickyness of them. When
> the issues that led to the -2's were fixed, they were not moved to -1.
> 
> I see -2's as vetos - if I am mistaken please tell me, but that is how
> most of the openstack projectsm and other projects using gerrit seem
> to have treated them.

Veto has a lot of connotations. In my case, the -2 is an indication
that I think the requested change is a bad idea that needs more
justification.  I hadn't seen some of the recent comments on that
patch because I've been working on something else today. I've
suggested moving the conversation to the mailing list. After that
discussion, we'll know the path forward.

I believe Dan is at the Nova mid-cycle this week, so it's likely
his -2 will be pretty sticky for now, too, since I expect he's
otherwise occupied as well.

Doug

> 
> > http://russellbryant.net/openstack-stats/oslo-reviewers-365.txt
> > http://russellbryant.net/openstack-stats/nova-reviewers-365.txt
> > http://russellbryant.net/openstack-stats/neutron-reviewers-365.txt
> >
> > On the one single specific review you had a -2, you should be talking
> > to the reviewer on IRC or bring it to the next Oslo meeting.
> 
> Did that. And on the review itself.
> 
> > Thanks,
> > Dims
> >
> >
> > On Thu, Jan 28, 2016 at 12:29 PM, Joshua Harlow  
> > wrote:
> >> Hayes, Graham wrote:
> >>>
> >>> 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?
> >>
> >>
> >> If this is the case I am sorry about it, and I'd also like to think that we
> >> (as the oslo team) need not do this (or think about it more before we do
> >> it), because it usually isn't really needed (a -1 suffices IMHO for most
> >> things, especially things that are being actively discussed).
> >>
> >> But I'm also in the camp that thinks the whole -1 or -2 is sorta dumb and
> >> IMHO just leaving comments and talking to people on IRC to work through
> >> issues/discussion/code is better...
> >>
> >> But then that requires people<->people interaction and I guess not everyone
> >> likes to do that(?)
> >>
> >> In general I hope it's not all of oslo u are grouping here because, if its
> >> just a few cases, hopefully we can work with the person that is -2ing stuff
> >> to not do it willy nilly...
> >>
> >> My 2 cents,
> >>
> >> -Josh
> >>
> >>
> >>
> >>
> >> __
> >> 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

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

[Joshua Harlow]

Ryan Rossiter wrote:


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.


Just out of curiosity but is this due to o.vo not merging reviews fast 
enough? Not releasing fast enough? A desire to experiment/prove out the 
additions in nova before moving to o.vo? Or something else?


From my point of view it feels odd to have a upstream (o.vo) and a 
downstream (nova) where the downstream in a way carries 'patches' on-top 
of o.vo when both o.vo and nova are in the same ecosystem (openstack).



-
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


__
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

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

[Ryan Rossiter]

> On Jan 28, 2016, at 4:04 PM, Joshua Harlow  wrote:
> 
> Ryan Rossiter wrote:
>> 
>> 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.
> 
> Just out of curiosity but is this due to o.vo not merging reviews fast 
> enough? Not releasing fast enough? A desire to experiment/prove out the 
> additions in nova before moving to o.vo? Or something else?

Even though I am an impatient person, it’s not because of #1 or #2. Part of it 
is to prove it works in nova. But I think the main reasoning behind most of 
them are because nova already has most of the duplicated code, and in order to 
let us use o.vo, there needs to be a slight change to it. So my steps in 
committing to both are 1) fix it in nova’s duplicated code, 2) fix it in o.vo 
3) get the change merged/released in o.vo 4) eliminate all duped code from 
nova, cut over completely to o.vo.

So the main reasons are forks and to answer the question “why does o.vo need 
this?”. The change this thread discusses doesn’t fall under this realm, we know 
why we need/want it. But my situations are just a data point to add towards 
carrying it in both places isn’t the end of the world (though I’m not saying 
this approach is the best way to do it).

> 
> From my point of view it feels odd to have a upstream (o.vo) and a downstream 
> (nova) where the downstream in a way carries 'patches' on-top of o.vo when 
> both o.vo and nova are in the same ecosystem (openstack).

I would totally agree, if o.vo was purely upstream of nova. Unfortunately, nova 
is still carrying a lot of leftovers, and because of the “forks” I need to push 
something to both of them so I can bring them together in the future. I’m doing 
my best at bringing these two closer together.

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

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

[Dan Smith]

> I know we have some projects (heat, I think?) that don't have UUIDs at
> all. Are they using oslo.versionedobjects? I suppose we could move them
> to a string field instead of a UUID field, instead of flipping the
> enforcement flag on. Alternately, if we add a new class we wouldn't have
> to ever actually deprecate the UUIDField class, though we might add a
> warning to the docs that it isn't doing any validation and the new class
> is preferred.

If a project isn't using UUIDs then they have no reason to use a
UUIDField and thus aren't affected. If they are, then they're doing
something wrong.

> I'll be curious to see what Dan and Sean have to say when they catch up
> on this thread.

I think Ryan summed it up very well earlier, but I will echo and
elaborate here for clarity.

To be honest, I think the right thing to do is deprecate the
non-validating behavior and have projects use in-project validating
fields for UUIDs (i.e. their own UUIDField implementation). When we can,
release a major version with the validating behavior turned on.

I don't like the validate=False flag because it's hard (or at least
ugly) to deprecate. Even allowing the call signature to tolerate it for
compatibility but still doing the validation is terrible, IMHO. If
people feel it is absolutely critical to have an implementation in o.vo
right now, then we can do the parallel class option, but we basically
just have to alias the old one to the new one when we "drop" the
non-validating functionality, IMHO, which is just more baggage. To quote
Ryan, "if you know you're going to break people, just don't do it."

This is a really minor issue in my opinion -- the amount of code a
project needs to replicate is exceedingly small, especially if they just
subclass the existing field and override the one method required to
ensure coercion. For a point of reference, nova has a lot of these
fields which are too nova-specific to put into o.vo; adding one more for
this is immeasurably small:

https://github.com/openstack/nova/blob/master/nova/objects/fields.py#L76-L621

Cinder also has some already:

https://github.com/openstack/cinder/blob/master/cinder/objects/fields.py

And to again echo Ryan's comments, we have always landed things in nova,
sync'd them to o.vo and removed our local copy once we can depend on a
new-enough o.vo release. This is effectively the same behavior for this
change and really just isn't that big of a deal. Please, let's do the
safest thing for the projects that consume our library, and for the
people who have to use the same gate as all the rest of us.

--Dan

__
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

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

[Doug Hellmann]

Excerpts from Hayes, Graham's message of 2016-01-28 19:17:54 +:
> On 28/01/2016 18:54, Doug Hellmann wrote:
> > Excerpts from Hayes, Graham's message of 2016-01-28 17:01:09 +:
> >> 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

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

[Doug Hellmann]

Excerpts from Hayes, Graham's message of 2016-01-28 20:42:39 +:
> On 28/01/2016 20:16, Doug Hellmann wrote:
> > Excerpts from Hayes, Graham's message of 2016-01-28 19:17:54 +:
> >> On 28/01/2016 18:54, Doug Hellmann wrote:
> >>> Excerpts from Hayes, Graham's message of 2016-01-28 17:01:09 +:
>  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?
> 
> I was assuming we would follow the standard deprecation model, so 1
> cycle for the default, and then another cycle for the option removal
> (which as I said in the initial email could be optional if enough
> people shout about it.)
> 
> It would be the same amount of projects that would have to switch to
> whatever new "ValidatingUUIDField" (or whatever it is called) after
> UUIDField is deprecated.
> 
> This all started when I saw nova or cinder using a UUIDField, and
> thought "that would be handy", implementing it locally, and then
> deciding that it might be useful in the lib so other projects could
> ensure they had valid UUIDs, and not assume that they did, based on
> the name of the class. That is the main reason for my objection to the
> new class.

OK, that all sounds reasonable.  Given that we have a way to deprecate
the old behavior with either approach, it seems like either would
work.

I know we have some projects (heat, I think?) that don't have UUIDs at
all. Are they using oslo.versionedobjects? I suppose we could move them
to a string field instead of a UUID field, instead of flipping the
enforcement flag on. Alternately, if we add a new class we wouldn't have
to ever actually deprecate the UUIDField class, though we might add a
warning to the docs that it isn't doing any validation and the new class
is preferred.

I'll be curious to see what Dan and Sean have to say when they catch up
on this thread.

> 
> >
> >>
> 
>  Also, olso 

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

[Hayes, Graham]

On 28/01/2016 20:16, Doug Hellmann wrote:
> Excerpts from Hayes, Graham's message of 2016-01-28 19:17:54 +:
>> On 28/01/2016 18:54, Doug Hellmann wrote:
>>> Excerpts from Hayes, Graham's message of 2016-01-28 17:01:09 +:
 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?

I was assuming we would follow the standard deprecation model, so 1
cycle for the default, and then another cycle for the option removal
(which as I said in the initial email could be optional if enough
people shout about it.)

It would be the same amount of projects that would have to switch to
whatever new "ValidatingUUIDField" (or whatever it is called) after
UUIDField is deprecated.

This all started when I saw nova or cinder using a UUIDField, and
thought "that would be handy", implementing it locally, and then
deciding that it might be useful in the lib so other projects could
ensure they had valid UUIDs, and not assume that they did, based on
the name of the class. That is the main reason for my objection to the
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?
>>
>> 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. 

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

[Ryan Rossiter]

> On Jan 28, 2016, at 1:10 PM, Mike Bayer  wrote:
> 
> 
> 
> On 01/28/2016 01:52 PM, Doug Hellmann wrote:
>> Excerpts from Hayes, Graham's message of 2016-01-28 17:01:09 +:
>>> 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,
>>> 

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

[Ryan Rossiter]

> On Jan 28, 2016, at 1:32 PM, Doug Hellmann  wrote:
> 
> Excerpts from Hayes, Graham's message of 2016-01-28 19:02:57 +:
>> On 28/01/2016 18:44, Davanum Srinivas wrote:
>>> Graham,
>>> 
>>> Quite unfair characterization of Oslo being -2 heavy. Please compare
>>> stats before making such an assertion:
>> 
>> OK - that was probably my frustration boiling over at the time. I should
>> not have made that generalization. I had been involved in 3 patches to
>> olso repos in the last week, and 2 got -2'd, and that came out unfairly
>> in this message.
> 
> As Dims pointed out, the ratio of -2s on your recent patches is an
> aberration. It just happens that you've picked some tricky things
> to work on, and those things need more communication than they might
> in other projects because of the nature of Oslo. Please don't
> interpret them as shutting down discussion.
> 
>> 
>> The initial -2s where not as annoying as the stickyness of them. When
>> the issues that led to the -2's were fixed, they were not moved to -1.
>> 
>> I see -2's as vetos - if I am mistaken please tell me, but that is how
>> most of the openstack projectsm and other projects using gerrit seem
>> to have treated them.
> 
> Veto has a lot of connotations. In my case, the -2 is an indication
> that I think the requested change is a bad idea that needs more
> justification.  I hadn't seen some of the recent comments on that
> patch because I've been working on something else today. I've
> suggested moving the conversation to the mailing list. After that
> discussion, we'll know the path forward.

Oslo is a special case where I think -2’s are more necessary than other 
projects. Because libraries are used by many projects, you don’t want to 
suddenly change something over that will break a project. Sometimes, yeah it 
happens, but if you know you’re going to break someone, don’t do it.

I like the stickiness of -2’s. As a person that isn’t core in anything, I get 
really frustrated in some situations where I put down a -1, someone updated the 
patch, and then it got merged before I could come back to it, but I still find 
something wrong. Now the onus is on me to get that fixed up. Granted my 
situations have been much less drastic (i.e. not a breaking change). Oslo is 
comprised of a lot of diverse knowledge. If someone knows of a breakage from a 
change, you don’t want that to get covered up by a new patch that “fixes” it. A 
-2 is (or should) never be a “I hate this patch”, it should be a “we need to 
discuss this further”.

> 
> I believe Dan is at the Nova mid-cycle this week, so it's likely
> his -2 will be pretty sticky for now, too, since I expect he's
> otherwise occupied as well.

As a guy who tries to get Dan’s attention a lot, yeah he’s a busy guy. But as a 
benefit of annoying him so much, I think (or at least hope) I’ve learned quite 
a bit under him. One of the benefits of me not being core on anything is that 
I’m usually more available for questions :). I might have to play the “I don’t 
know” card pretty frequently, but it doesn’t hurt to ask :)

> 
> Doug
> 
>> 
>>> http://russellbryant.net/openstack-stats/oslo-reviewers-365.txt
>>> http://russellbryant.net/openstack-stats/nova-reviewers-365.txt
>>> http://russellbryant.net/openstack-stats/neutron-reviewers-365.txt
>>> 
>>> On the one single specific review you had a -2, you should be talking
>>> to the reviewer on IRC or bring it to the next Oslo meeting.
>> 
>> Did that. And on the review itself.
>> 
>>> Thanks,
>>> Dims
>>> 
>>> 
>>> On Thu, Jan 28, 2016 at 12:29 PM, Joshua Harlow  
>>> wrote:
 Hayes, Graham wrote:
> 
> 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?
 
 
 If this is the case I am sorry about it, and I'd also like to think that we
 (as the oslo team) need not do this (or think about it more before we do
 it), because it usually isn't really needed (a -1 suffices IMHO for most
 things, especially things that are being actively discussed).
 
 But I'm also in the camp that thinks the whole -1 or -2 is sorta dumb and
 IMHO just leaving comments and talking to people on IRC to work through
 issues/discussion/code is better...
 
 But then that requires people<->people interaction and I guess not everyone
 likes to do that(?)
 
 In general I hope it's not all of oslo u are grouping here because, if its
 just a few cases, hopefully we can work with the person that is -2ing stuff
 to not do it willy nilly...
 
 My 2 cents,
 
 -Josh
 
 
 
 
 __
 OpenStack Development Mailing List (not for usage questions)
 Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe