Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-12-15
16:00:26 gibi at least I think
16:00:30 gibi it does
16:01:08 gmann yeah, there are only few with W503 i think.
16:20:04 openstackgerrit Ghanshyam proposed openstack/placement master: Fix l-c job and move to latest hacking 4.0.0 https://review.opendev.org/c/openstack/placement/+/767182
16:21:47 openstackgerrit Ghanshyam proposed openstack/placement master: Fix l-c job and move to latest hacking 4.0.0 https://review.opendev.org/c/openstack/placement/+/766994
16:22:45 gibi gmann: do you wanted to push two separate patches for this ^^ ?
16:23:13 gmann gibi: i meshed up the commit id initially. abandon the new commit and kept in old one
16:23:26 gmann gibi: this is all good now https://review.opendev.org/c/openstack/placement/+/766994
16:23:53 gibi looking...
16:24:01 gmann need to have strong coffee i think :)
16:25:18 bauzas gibi: lyarwood: I'm back, should I write then some bug asking to support {} for the unshelve action
16:25:19 bauzas ?
16:25:19 gibi I was lazy today to brew coffee, trying to survive with black tea
16:25:37 gibi bauzas: I think that is a reasonable think to ask for
16:25:54 gibi I don't see why we forbid the unshelve: {} format
16:26:04 gibi brinzhang: do you happen to rememeber the reasoning here ^^
16:27:10 gibi brinzhang: we are trying to figure out why your patch made the az mandatory in https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/schemas/shelve.py#L31
16:28:05 bauzas gibi: thanks, let's wait for brinzhang
16:28:10 sean-k-mooney isnt it an instance action
16:28:18 sean-k-mooney dont we need the action in the payload
16:28:32 gibi sean-k-mooney: the action is unshelve
16:28:36 gibi so that is alway in the payload
16:28:43 sean-k-mooney https://docs.openstack.org/api-ref/compute/?expanded=unshelve-restore-shelved-server-unshelve-action-detail#unshelve-restore-shelved-server-unshelve-action
16:28:44 gibi but unshelve has an optional parameter the az
16:28:58 sean-k-mooney ya so {
16:29:00 sean-k-mooney "unshelve": null
16:29:02 sean-k-mooney }
16:29:04 sean-k-mooney Example Unshelve server (unshelve
16:29:06 sean-k-mooney is allowed
16:29:13 sean-k-mooney you are just suggesting {} instead of null
16:29:22 sean-k-mooney if so sure that seams fine
16:29:28 gibi sean-k-mooney: yeah we could allow both as far as I see
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

Earlier   Later