| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-sdks - 2019-08-29 | |||
| 16:26:11 | gtema_ | oj, the nitpicker of dtantsur is a killer bomb ;-) | |
| 16:26:12 | efried | because there's a mental barrier to re-reviewing a 1KLOC patch. | |
| 16:26:17 | elmiko | efried: yes, i will make notes about this when i triage next week. if those patches are in good enough shape to merge i will weigh that heavily in my comments. | |
| 16:26:19 | efried | regardless of how small the inter-patch delta is. | |
| 16:26:35 | elmiko | efried: ++ | |
| 16:27:29 | efried | especially with docs, as long as the patch moves the ball forward and doesn't introduce actual inaccuracies, we should be able to be much less perfectionist about merging | |
| 16:27:46 | efried | it's not like we're going to introduce a security regression or something. | |
| 16:27:51 | elmiko | yeah | |
| 16:27:53 | efried | break CERN's deployment. | |
| 16:28:03 | elmiko | i think we've just been really nitpicky about getting the details correct | |
| 16:28:13 | elmiko | and that version discovery beast is tough to wrangle | |
| 16:28:18 | efried | no joke | |
| 16:28:45 | elmiko | especially when we stay true to our processes and ask for input from the community | |
| 16:29:48 | elmiko | efried: so that's a question i have though. we have clear guides about how we will merge changes, i don't think it's a good idea to break them /but/ we do open ourselves up to drive by -1's when we do this and that throws the whole process off. any suggestions? | |
| 16:30:39 | efried | clear guides written for different times; we should have the temerity to alter those processes. | |
| 16:30:48 | elmiko | fair | |
| 16:31:10 | dtantsur | ++ | |
| 16:31:34 | efried | A number of teams are, out of necessity, moving to single-core approvals. Not saying thats' a good idea here necessarily, just that it's a thing. | |
| 16:31:36 | elmiko | appreciate all the feedback, it is helpful | |
| 16:31:37 | dtantsur | and the version discovery one doesn't have -1's | |
| 16:31:48 | elmiko | dtantsur: ++ | |
| 16:31:50 | dtantsur | so strictly speaking we can approve it | |
| 16:31:59 | elmiko | cool, maybe we just do that | |
| 16:32:16 | elmiko | give it one more read through to make sure there are not glaring errors | |
| 16:32:25 | efried | Yeah, for something like this, if it's clear it's been thoroughly reviewed by people with at least some understanding of the content, it's reasonable to proxy their +1 as a +2 when there's a dearth of cores available. | |
| 16:32:27 | efried | IMHO | |
| 16:32:35 | elmiko | ++ | |
| 16:32:47 | efried | IIRC I went through one or more of those enormous patches and +1ed. | |
| 16:32:55 | efried | I consider myself at least somewhat up on version discovery. | |
| 16:33:19 | efried | and changes since my +1 have been minimal, so feel free to carry those forward. | |
| 16:33:27 | elmiko | ack, thanks | |
| 16:33:56 | elmiko | is it time for cake and punch? | |
| 16:34:01 | gtema_ | I was missing in the version discovery rules on what is open and what requires AUTH. This got lost in some restructuring | |
| 16:34:28 | elmiko | gtema_: definitely add comments if you can figure out where it was | |
| 16:34:35 | gtema_ | did it already | |
| 16:34:40 | gtema_ | long time ago | |
| 16:34:55 | gtema_ | but that is the point - there are way to many to see the overview | |
| 16:34:58 | elmiko | ok, cool. i will inevitably come across them ;) | |
| 16:35:14 | elmiko | yeah, i'm gonna carve out like half a day for going reviewing these | |
| 16:35:28 | dtantsur | I've seen gtema_'s comments, just never got to fixing them | |
| 16:35:36 | elmiko | ok, maybe i can help | |
| 16:35:41 | dtantsur | and again, I'd prefer to fix in a follow-up | |
| 16:35:49 | elmiko | ++ | |
| 16:36:02 | elmiko | if there are easy fixes though, i will try to just add them | |
| 16:36:15 | gtema_ | cool | |
| 16:42:43 | gtema_ | dtantsur: what do you think about https://opendev.org/openstack/openstacksdk/src/branch/master/openstack/resource.py#L1331 | |
| 16:42:56 | gtema_ | can we change it to "Accept: *"? | |
| 16:43:35 | gtema_ | empty value is somehow not RFC-nice | |
| 16:46:13 | dtantsur | gtema_: I wonder why we send *anything* if we don't expect a body | |
| 16:46:22 | dtantsur | HEAD + Accept doesn't make sense to me | |
| 16:46:30 | gtema_ | well, some APIs might return a body | |
| 16:46:37 | dtantsur | to HEAD?? Oo | |
| 16:47:16 | gtema_ | or, no | |
| 16:47:22 | gtema_ | looked to the wrong section | |
| 16:47:53 | gtema_ | I would suggest either remove it completely, or set a "*" | |
| 16:47:57 | gtema_ | not an empty value | |
| 16:48:13 | edleafe | A HEAD with a body is, well, a whole person | |
| 16:48:30 | gtema_ | yupp | |
| 16:48:40 | elmiko | lol | |
| 16:49:06 | dtantsur | :D | |
| 16:49:10 | dtantsur | I'd remove it | |
| 16:49:18 | gtema_ | ok, will do. | |
| 16:49:20 | dtantsur | and see if mordred complains | |
| 16:49:46 | gtema_ | and now PUT, which might return body, but contain no body in request (swift create object) | |
| 16:50:02 | dtantsur | Accept is related to returned body, right? | |
| 16:50:04 | elmiko | dtantsur: the ultimate test ;) | |
| 16:50:10 | gtema_ | yes | |
| 16:50:15 | dtantsur | so if you expect that a body might be returned, you should be sending Accept | |
| 16:50:25 | dtantsur | (although the exactly value depends on the call) | |
| 16:50:42 | gtema_ | yeah, but not the "Accept: ", at least "Accept: *" or "Accept: application/json" | |
| 16:50:52 | dtantsur | true | |
| 16:50:59 | dtantsur | probably the default should be Accept:* or no accept | |
| 16:51:00 | gtema_ | this empty value is what breaks it for me | |
| 16:51:12 | dtantsur | then everything except for swift should use Accept: application/json | |
| 16:51:13 | timburke | fwiw, requests adds an "Accept: */*" header by default. if i had to guess, i'd say the empty was trying to prevent that, but i don't know why | |
| 16:51:26 | dtantsur | weird | |
| 16:52:15 | gtema_ | my small test was not confirming that - after removing this empty there was nothing at all | |
| 16:52:18 | timburke | if you really want to drop the header entirely, you could try {'Accept': None} | |
| 16:52:35 | dtantsur | I'm not sure why we would want that | |
| 16:52:52 | dtantsur | I would expect that no header is equivalent to Accept:*/* | |
| 16:53:00 | gtema_ | yeah | |
| 16:53:19 | gtema_ | this is also how I interpret RFC | |
| 16:53:19 | timburke | i know that i've done that for Accept-Encoding before, where swiftclient will freak out a bit if requests goes and decompresses object data as it's getting downloaded | |
| 16:53:35 | dtantsur | gtema_: propose a fix and see if anybody complains | |
| 16:53:42 | gtema_ | cool | |
| 16:53:48 | edleafe | "Accept: None" should mean "don't return *any* body in the response" | |
| 16:53:48 | dtantsur | timburke: but it's swiftclient specific, right? not openstacksdk? | |
| 16:53:56 | gtema_ | exactly SDK | |
| 16:54:12 | elmiko | i don't want to break up the accept discussion too much, but before we end the office hour i /do/ want to thank edleafe one more time. he has been a pillar of this community and i personally have learned a great deal from him. thank you again Ed =) | |
| 16:54:29 | gtema_ | yeah, thanks Ed | |
| 16:55:03 | edleafe | I am going to miss hanging out with all of you. Even elmiko! | |
| 16:55:10 | elmiko | hehehe <3 | |
| 16:55:40 | dtantsur | thanks you edleafe! | |
| 16:55:40 | timburke | i'm not sure what you guys do when downloading object data ;-) i know swiftclient wants to do some checksumming, which'll require the raw (compressed, if it was stored compressed) stream | |
| 16:55:57 | dtantsur | good question | |
| 16:56:47 | gtema_ | there is hell amount of logic | |
| 16:57:12 | gtema_ | at least for uploading | |
| 16:58:05 | gtema_ | for downloading we also set "Accept: bytes" | |
| 17:01:17 | gtema_ | but we do not do download checksumming | |
| 17:01:52 | elmiko | i'm heading out for the weekend, take care all o/ | |