| opendevreview | OpenStack Proposal Bot proposed openstack/glance master: Imported Translations from Zanata https://review.opendev.org/c/openstack/glance/+/846845 | 02:37 |
|---|---|---|
| *** abhishekk is now known as akekane|home | 04:48 | |
| *** akekane|home is now known as abhishekk | 04:48 | |
| opendevreview | Abhishek Kekane proposed openstack/glance master: [APIImpact] Correct API response code for DELETE cache APIs https://review.opendev.org/c/openstack/glance/+/847877 | 05:46 |
| abhishekk | dansmith, croelandt, jokke_ ^^ | 05:49 |
| abhishekk | croelandt, dansmith jokke_ if you are around kind look at new glance patch, https://review.opendev.org/c/openstack/glance/+/847877 | 13:36 |
| dansmith | that was back in xena? | 13:38 |
| abhishekk | yoga | 13:39 |
| dansmith | oh okay the spec you linked said xena | 13:41 |
| abhishekk | spec was in xena but it is implemented in yoga | 13:41 |
| dansmith | ack | 13:44 |
| dansmith | so just thinking about this a sec.. 200->204 seems kinda "meh" in terms of usefulness, | 13:45 |
| dansmith | but also, this is a similar case to the PUT right? you ask for it to be deleted, but we don't do it until later right? | 13:45 |
| abhishekk | yes | 13:45 |
| dansmith | just saying, 200->202 for the PUT is pretty important to say "no, we didn't already do that, we're going to do it sometime later", but delete is less important anyway, but also 200->204 doesn't tell us anything else really useful | 13:47 |
| dansmith | so I assume the same justification for the PUT change still applies, but the reasoning is weaker for this I think. | 13:47 |
| abhishekk | So can we amend the spec and api reference doc and keep it 200 only? | 13:47 |
| dansmith | maybe we see what others think? too bad rosmaita is on vacation, I'd be interested in his opinion | 13:48 |
| dansmith | yeah, could do that, let's see what jokke_ and croelandt think | 13:48 |
| abhishekk | hmm, because our api reference is also saying 204 is valid response code for these APIs | 13:48 |
| dansmith | meaning the apiref, code and spec all disagree? | 13:49 |
| abhishekk | spec and api reference is 204 and code is 200 | 13:49 |
| dansmith | er, right | 13:49 |
| dansmith | well, what's in the code is what will break people, but yeah | 13:50 |
| dansmith | good that you noticed the disparity regardless | 13:50 |
| dansmith | looks like the other DELETEs in the glance api also specify 204 | 13:51 |
| abhishekk | yes | 13:51 |
| abhishekk | it wasn't me, it was pointed out by martin kopec on my tempest patch | 13:52 |
| dansmith | ah, heh | 13:53 |
| abhishekk | Brian will not be here until next Wednesday, i guess jokke_ is also not around | 13:54 |
| dansmith | yeah I know rosmaita is out | 13:57 |
| dansmith | I think making sure jokke_ is okay with this is a good idea, | 13:57 |
| dansmith | because like last time, it's an API change, but the justification is much weaker for this, IMHO | 13:57 |
| dansmith | so I commented with some of the discussion above, I assume he'll be around at some point | 13:58 |
| dansmith | is that cool? | 13:58 |
| abhishekk | ack, only thing is we need to be consistent | 13:58 |
| abhishekk | no worries | 13:58 |
| dansmith | what's the answer to the version question? do we consider the existing bump for PUT to cover this? | 13:59 |
| dansmith | and I assume you'll backport this as well? | 13:59 |
| abhishekk | yes | 13:59 |
| abhishekk | IMO we can consider existing version bump to cover this | 14:00 |
| dansmith | okay | 14:00 |
| abhishekk | because we haven't released anything yet | 14:00 |
| dansmith | ack | 14:01 |
| croelandt | yeah I'd kind of like to see a version bump and a release note | 15:03 |
| croelandt | even though I doubt this will break anything | 15:04 |
| dansmith | well, abhishekk says we're covered under the other bump we just made for the PUT | 15:05 |
| dansmith | but a reno, yeah | 15:05 |
| abhishekk | I will add a reno once we decided to go ahead with this | 15:06 |
| croelandt | abhishekk: can you +W https://review.opendev.org/c/openstack/glance-specs/+/734683 ? | 15:06 |
| abhishekk | looking | 15:06 |
| abhishekk | yep | 15:06 |
| opendevreview | Merged openstack/glance-specs master: Update proposal for duplication image download https://review.opendev.org/c/openstack/glance-specs/+/734683 | 15:13 |
| jokke_ | abhishekk: dansmith: I'm almost leaning towards altering the doc instead. 200 with "Deleted." would be equally right response [0]. I really do like consistency 'though which is IMO the only excuse why we should do that change https://www.rfc-editor.org/rfc/rfc9110.html#name-delete | 17:19 |
| jokke_ | Or rather "Invalidated" | 17:19 |
| dansmith | yes, I agree, 204 is no more correct than 200 for this, although I think 202 *is* since we queue the delete (right?) | 17:20 |
| dansmith | consistency is important, and this delete is pretty limited use and scope (like we said for PUT) which also makes fixing it not a big deal I think | 17:20 |
| dansmith | so I think I'd lean towards fixing it since ALL the other deletes are 204, but not super opinionated | 17:21 |
| jokke_ | The cache deletes are actually synchronous so 202 would be wrong | 17:21 |
| dansmith | ah okay | 17:21 |
| dansmith | I guess the other argument for fixing it, | 17:22 |
| jokke_ | that's why I'm bit torn between consistency vs API stability for consistency :P | 17:22 |
| dansmith | is we already fixed the PUT, so one version that covers the messed-up return values for two related calls is reasonable symmetry | 17:22 |
| dansmith | it's not like we'd need two versions, or that the two corrections are wildly unrelated | 17:22 |
| dansmith | they're two calls for the same API, same feature, same kinda original oversight | 17:23 |
| jokke_ | there is that | 17:23 |
| jokke_ | well like abhishekk said ... the previous one is not released so if someone is relying on that already, they would need to change their caching code anyways | 17:24 |
| dansmith | yeah | 17:24 |
| dansmith | I suspect brian would be pro (maybe strongly) for just fixing it.. abhishekk clearly is, and I'm mildly for fixing it... are you opposed? | 17:25 |
| * jokke_ wishes HTTP had just one 200 "got ya" | 17:25 | |
| abhishekk | right | 17:25 |
| dansmith | well, in reality, most things return 200 regardless, so there kinda is :P | 17:25 |
| abhishekk | I am pro for fixing it | 17:26 |
| jokke_ | mabe we should too ... little sed and everything 2XX -> 200 | 17:26 |
| jokke_ | :P | 17:26 |
| jokke_ | but no I'm not really opposed. Kind of feeling that we shat the bed already so we might as well :D | 17:27 |
| dansmith | there seems to be more weight on the side of fixing it while we have this chance, so I say we go for it | 17:27 |
| dansmith | hah, right, my feeling exactly :) | 17:27 |
| abhishekk | ++ | 17:27 |
| jokke_ | abhishekk: could you please just make sure that these are all then ;) | 17:27 |
| abhishekk | let me add quick release notes | 17:27 |
| dansmith | abhishekk: also please reno | 17:27 |
| dansmith | yeah | 17:27 |
| abhishekk | yep PUT is already fixed | 17:27 |
| abhishekk | and list one is correct | 17:27 |
| abhishekk | list --> GET is 200 | 17:28 |
| jokke_ | good, we don't return 202 with the list there "Listing these, but there might be more, you'll never know" | 17:28 |
| dansmith | return 200 + randint(0, 99) | 17:29 |
| jokke_ | ++ | 17:29 |
| jokke_ | on everything | 17:29 |
| abhishekk | :D | 17:33 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: [APIImpact] Correct API response code for DELETE cache APIs https://review.opendev.org/c/openstack/glance/+/847877 | 17:35 |
| abhishekk | dansmith, jokke_ ^ | 17:35 |
| jokke_ | We should also add default body to all our 200s that doesn't have one specified 200 "Or something there abouts, it'll be grand anyways" | 17:37 |
| jokke_ | even better 200 "Ish, you're grand" | 17:38 |
| abhishekk | :D | 17:39 |
| abhishekk | jokke_, dansmith if patch gets merged today/tonight can either of you please propose backport to stable/yoga | 17:41 |
| dansmith | abhishekk: we can propose it now, right? | 17:42 |
| dansmith | assuming no rebase needs to happen, the hash will be the same | 17:42 |
| abhishekk | we can as there is no patch in queue to be merged | 17:42 |
| opendevreview | Abhishek Kekane proposed openstack/glance stable/yoga: [APIImpact] Correct API response code for DELETE cache APIs https://review.opendev.org/c/openstack/glance/+/848001 | 17:43 |
| abhishekk | ^^ | 17:43 |
| * dansmith was git reviewing already | 17:43 | |
| dansmith | I interrupted it ;) | 17:43 |
| abhishekk | ahh | 17:43 |
| dansmith | abhishekk: note that I can't +2 that, so I guess my work here is done :P | 17:44 |
| abhishekk | sorry :D | 17:44 |
| dansmith | no complaints from me :) | 17:44 |
| abhishekk | I will abandon mine, you can propose backport | 17:44 |
| dansmith | nonono | 17:44 |
| abhishekk | ok | 17:44 |
| dansmith | oh, if you want me to so you can vote? | 17:44 |
| abhishekk | yes | 17:44 |
| opendevreview | Dan Smith proposed openstack/glance stable/yoga: [APIImpact] Correct API response code for DELETE cache APIs https://review.opendev.org/c/openstack/glance/+/848001 | 17:44 |
| abhishekk | cool, abandoned mine | 17:45 |
| * abhishekk signing out for the day | 18:00 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!