Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-12-15
16:29:44 gibi sean-k-mooney: and lock, migrate and rescue allows it
16:31:19 sean-k-mooney yep anyway got to go to a doctors apointment ill be back online tomorrow
16:31:34 gibi sean-k-mooney: o/
16:32:41 gibi bauzas: you can send https://review.opendev.org/c/openstack/placement/+/766994 thorugh if you wish
16:32:49 gmann yeah for most of server action request body are like 'allowed anything but API ignore those'
16:33:07 bauzas gibi: looking
16:33:34 bauzas all good
16:34:18 gibi thanks bauzas
16:35:02 bauzas sec
16:41:33 bauzas gibi: gmann: I'm unclear, https://review.opendev.org/c/openstack/placement/+/766994/9/placement/conf/database.py changes aren't related to neither W503 nor W504
16:42:13 bauzas and while we don't accept single lines in modules, we accept in nova indentations like L58
16:42:56 gmann bauzas: that is E128
16:43:14 bauzas yup, I was looking at https://www.flake8rules.com/rules/E128.html
16:43:30 bauzas but E128 anti-pattern is accepted by nova IIRC
16:44:15 bauzas and tbc, my concern is that you're changing for supporting E128 without explaining why
16:44:27 gmann yeah in nova we ignored E128 https://github.com/openstack/nova/blob/master/tox.ini#L259
16:44:55 gmann bauzas: placement was using too old hacking checks.
16:45:14 gmann moving to new one end up with these multiple updates
16:47:08 bauzas gmann: okay, but then why not ignoring E128 and then not modify all the placement code ?
16:47:42 gmann gibi: bauzas we can. even we can keep the same ignore list as in nova https://github.com/openstack/nova/blob/master/tox.ini#L259
16:47:55 bauzas gmann: I'd indeed prefer this way
16:47:59 bauzas given we did cut from nova
16:48:05 gmann so that we can keep future consistency also
16:48:06 bauzas when branching placement
16:48:14 gmann bauzas: ok, let me update that.
16:48:15 bauzas gibi: thoughts on it ?
16:49:11 gmann k, waiting for gibi.
16:49:15 gibi I'm meh on this. I think the new rules are meaningful and pretty cheap to introduce in placement so I'm happy to take them in
16:49:29 gibi introducing them to nova would make a lot of code change I guess
16:49:35 stephenfin I'd rather we kept the default list too
16:49:53 gibi stephenfin: default, meansh what is in nova's tox.ini?
16:49:55 stephenfin probably not worth the effort for nova, but the placement changes are done already
16:50:06 stephenfin no, what's in placement's tox.ini now
16:50:17 gibi yeah, I tend to agree with stephenfin
16:50:32 gmann yeah there are no other changes need in placement for those checks
16:50:35 bauzas my point is that we're changing placement code for the sake of a new hacking revision
16:50:52 bauzas I'd rather prefer to make nova and placement consistent in terms of style
16:51:16 bauzas besides the fact this would allow us to merge this change without needing to touch at the code
16:51:30 bauzas and later, we could just unignore rules one by one in placement if we really wish
16:51:47 bauzas baby steps, here, baby steps
16:52:00 bauzas 1/ upgrade hacking and ignore same list
16:52:16 bauzas 2/ unignore a specific rule and modify the code accordingly
16:52:27 bauzas 3/ iterate over every ignored rule
16:52:33 gibi bauzas: if you are after to split the hacking version change and the introdcution of new rules into separate patches then fine, but the current patch is not too big too
16:52:50 gmann bauzas: but this change with not-ignoring does not change much code
16:52:51 stephenfin if there were a significant amount of changes then I'd agree, but it doesn't seem worth the effort here
16:53:06 bauzas gibi: the problem is that we are silently changing placement without really explaining which hacking errors we wanted to fixc
16:53:29 gibi bauzas: sure, we can add a list of new rules applied to the commit message
16:53:40 bauzas and I don't think it's hard to just split this change in twice if you really want to unignore E128 and all the other rules
16:54:41 gibi OK, I'm fine in both ways: a) add a bit more text the commit message about the new rules that required the code change OR b) split the current patch into two
16:54:49 gmann giving it unblock the gate I preferred to move the code to latest hacking which is easy than doing one by one.
16:55:02 gibi gmann: agree, new hacknig is better
16:55:27 gmann I can add checks fix in commit msg.
16:55:28 stephenfin I don't think E128 is a big issue. W503/W504 seem to be the bulk of it. We could drop one of those, seeing as they conflict
16:55:49 gmann yeah W504 is ignored now
16:56:12 stephenfin sweet, then I don't care :)
16:56:39 gmann https://review.opendev.org/c/openstack/placement/+/766994/9/tox.ini
16:57:15 bauzas phew ok
16:57:25 bauzas I just feel we're overcomplicating
16:57:52 bauzas bumping hacking and providing the right ignore list will allow the gate to pass without needing to touch code
16:58:13 bauzas unless I'm missing something obvious
16:58:46 bauzas my take is, gmann had to touch code instead of amending the ignore list in order to fix the gate, I just feel it was unnecessary
16:58:51 stephenfin bauzas: not quite; a couple of the checks have been improved in newer versions of flake8 etc. To ignore those you'd have to disable the check and actually regress
16:59:13 bauzas stephenfin: why then nova isn't a problem ?
16:59:22 stephenfin because we fixed them there
16:59:27 stephenfin example: https://review.opendev.org/c/openstack/placement/+/766994/9/placement/conf/database.py
16:59:49 stephenfin that extra newline was (mistakenly) not required in an old version of flake8
17:00:27 bauzas for the extra newline, OK
17:00:39 bauzas for the continuous indentation, that's still there
17:00:55 bauzas so, ok, I mixed things
17:00:56 gmann nova is on 3.1.0. i could move placement too to that but 4.0.0 is already out to moving to it is easy.
17:01:26 bauzas gmann has to fix single lines within modules, but can just continue to leave uncontinuous indentation if he wishes
17:01:49 gmann anyways code is up and green so merging it is faster than splitting the patch now
17:02:53 bauzas well, I'm OK to remove my -1 for the sake of merging
17:03:01 bauzas but honestly, this is confisuing
17:04:12 gmann bauzas: how about if i explain those check fixes in cmt msg ?
17:04:43 bauzas I'm just saying we're again touching code for the beauty of our eyes and we're overcomplicating things, exactly like we did a couple of times in nova
17:04:50 bauzas but meh, I bail out
17:05:01 bauzas so, stephenfin can look at the change, I removed my -1
17:05:18 bauzas the +2 from gibi is there, so...
17:05:24 gibi yeah
17:05:34 gibi honestly I don't feel this as such a big issue
17:05:41 gibi but that jsutme
17:05:46 gibi just me
17:05:55 gmann bauzas: you seems do not like hacking checks much :)
17:06:19 bauzas it's more or less a matter of touching production code for the sake of ourselves being pedantic :)
17:06:28 bauzas but okay
17:07:15 bauzas gmann: well, I'm just pragmatic about things that work
17:07:26 bauzas but fair enough
17:08:18 gmann I see your point.
17:20:32 stephenfin ta, done
17:33:27 stephenfin gmann: Could you look at https://review.opendev.org/c/openstack/nova/+/765798/ at some point?
17:33:30 openstackgerrit Stephen Finucane proposed openstack/nova master: tests: Merge 'test_hypervisor_status' into 'test_hypervisors' https://review.opendev.org/c/openstack/nova/+/764039
17:33:31 openstackgerrit Stephen Finucane proposed openstack/nova master: WIP: Change default policy for '/os-hypervisors' https://review.opendev.org/c/openstack/nova/+/765798
17:33:31 openstackgerrit Stephen Finucane proposed openstack/nova master: api: Drop statistics-style fields from os-hypervisors https://review.opendev.org/c/openstack/nova/+/764040
17:33:33 stephenfin I'm not sure if I'm tackling that correctly
17:33:43 stephenfin Also, resolved your comment on https://review.opendev.org/c/openstack/nova/+/764039
17:33:52 gmann stephenfin: sure, checking
17:36:29 openstack Launchpad bug 1906266 in OpenStack Compute (nova) "After upgrade: "libvirt.libvirtError: Requested operation is not valid: format of backing image %s of image %s was not specified"" [Undecided,New]
17:36:29 stephenfin lyarwood: you'd probably be interested in this at some point https://bugs.launchpad.net/nova/+bug/1906266

Earlier   Later