Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-12-15
15:12:35 bauzas you need to pass a specific AZ
15:12:39 bauzas when unshelving
15:13:34 lyarwood https://github.com/openstack/nova/blob/240ee3091c5ec458753983afa90e3a0cc11dc322/nova/api/openstack/compute/shelve.py#L95-L98
15:13:37 gibi bauzas: is it an api ref doc bug or a code bug?
15:13:45 lyarwood so wouldn't {'unshelve': null} still work?
15:14:15 bauzas lyarwood: no
15:14:22 bauzas see https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/schemas/shelve.py#L31
15:14:35 bauzas gibi: well, the doc says it's an optional field
15:14:53 bauzas and honestly, I don't know why it should be a mandatory field
15:15:21 bauzas I'm a bit afraid we ask AZs everytime people want to unshelve
15:15:25 gibi bauzas: would it be defaulted to 'nova' if not provided?
15:15:26 lyarwood I think {'unshelve': null} still works looking at the API code at least
15:15:41 bauzas lyarwood: no because of the schema
15:15:51 bauzas lyarwood: and I tested it
15:16:24 lyarwood huh it lists null as an acceptable type above
15:16:31 gibi hm, maybe we made the same too strict then, the code works without az provided
15:16:32 lyarwood kk if you've tested it then
15:16:52 gibi ohh, null is special cased
15:16:57 bauzas gibi: lyarwood: http://paste.openstack.org/show/801061/
15:17:22 gibi so unshelve: null OK but unshelve:{} is not
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 gibi I was lazy today to brew coffee, trying to survive with black tea
16:25:19 bauzas ?
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

Earlier   Later