Posted	Nick	Remark
#openstack-nova - 2020-12-15
15:17:32	bauzas	anyway, I need to get my kids
15:17:47	lyarwood	Value: {}
15:17:57	lyarwood	ack np
15:18:14	gibi	I vaguely remember other cases when we disallow {} for a server action
15:23:48	gibi	yepp, we have similar construct for lock, migrate and rescue, but they are not the same as they not require anything in {}. So it is interesting why we deviated from this pattern in case of unshelve
15:27:19	gibi	gmann: I still think we should not try to satify both W503 and W504 at the same time just for the sake of the rules as it does not promote a good code style in my eyes
15:30:11	gmann	gibi: if we make all operator in same line with bracket '(' then W503 is automatically solved. that give more consistency also.
15:30:30	gmann	I mean fixing it in that consistent way automatically solve the both
15:32:12	gibi	gmann: as far as I see you wrapped only the parts of the condition into and extra () that is over the line length
15:32:50	gibi	(amount_needed < min_unit or amount_needed > max_unit or (
15:32:51	gibi	amount_needed % step_size != 0))
15:32:54	gibi	eg ^^
15:33:10	gibi	here it is strange why we have an extra () only for the last or but not the other ors
15:34:00	gibi	I still think that we should not try to satisfy both W504 and W503 at the same time. we should decide to wrap the line before the operatr (504) or after the operator (503)
15:40:55	openstackgerrit	Matteo Sposato proposed openstack/nova master: Refactoring of functional.regression.test_bug_1702454 https://review.opendev.org/c/openstack/nova/+/765997
15:45:17	openstackgerrit	Matteo Sposato proposed openstack/nova master: Refactoring of functional.regression.test_bug_1702454 https://review.opendev.org/c/openstack/nova/+/765997
15:45:18	openstackgerrit	Matteo Sposato proposed openstack/nova master: Functional tests remmoved direct post call https://review.opendev.org/c/openstack/nova/+/766068
15:47:53	openstackgerrit	Matteo Sposato proposed openstack/nova master: Functional tests removed direct post call https://review.opendev.org/c/openstack/nova/+/766068
15:53:53	gmann	gibi: ok. so doing in nova way and ignore W504 ?
15:55:01	gibi	gmann: that would give us the consistency with nova too. So yes. If I would start a new project then I might use W504 instead and keep the binary operator at the beginnig of the line as that feels like the more imporant information.
15:55:48	gmann	gibi: yeah, i think consistency is imp for easy maintenance. will update the patch.
15:58:16	gibi	gmann: thanks, I hope I did not sound too demanding above
15:58:59	gmann	gibi: no, it would take much time, i think less things to fix may be :)
16:00:21	gibi	gmann: yeah, the above example pass W503 without the extra ()
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

Earlier   Later