| ganso | vishalmanchanda: hi! have you had any chance to give another try at https://review.opendev.org/c/openstack/horizon/+/856308 ? | 13:16 |
|---|---|---|
| vishalmanchanda | ganso: sorry I forgot:(. | 13:59 |
| ganso | vishalmanchanda: I posted new findings in the LP | 14:00 |
| ganso | vishalmanchanda: I may have found why you were unable to reproduce, there are specific combinations where the problem does not happen | 14:00 |
| ganso | vishalmanchanda: unfortunately those combinations are not useful for my customer, they are still stuck | 14:01 |
| vishalmanchanda | ganso: looking at your findings in the LP. | 14:01 |
| vishalmanchanda | #startmeeting horizon | 15:00 |
| opendevmeet | Meeting started Wed Sep 28 15:00:15 2022 UTC and is due to finish in 60 minutes. The chair is vishalmanchanda. Information about MeetBot at http://wiki.debian.org/MeetBot. | 15:00 |
| opendevmeet | Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. | 15:00 |
| opendevmeet | The meeting name has been set to 'horizon' | 15:00 |
| ganso | o/ | 15:00 |
| tmazur | o/ | 15:00 |
| rdopiera | o/ | 15:00 |
| vishalmanchanda | Hello everyone | 15:01 |
| vishalmanchanda | There is no update from my side for this week. | 15:01 |
| vishalmanchanda | so moving to open-discussion | 15:02 |
| vishalmanchanda | #topic open-discussion | 15:02 |
| ganso | has anyone been able to take a look at https://review.opendev.org/c/openstack/horizon/+/856308 ? | 15:02 |
| vishalmanchanda | I was busy in some interanl work so didn't get a chance to review it. | 15:04 |
| vishalmanchanda | ganso: will review it soon. | 15:04 |
| ganso | vishalmanchanda: thanks | 15:04 |
| rdopiera | ganso: that really looks to me like the problem is in a different place -- namely with the check where it fails when the domain token exists | 15:06 |
| rdopiera | ganso: do you think it would be possible to fix it there, instead of adding such workaround to horizon? | 15:07 |
| ganso | rdopiera: hmmm what other possible place are you suggesting? I checked keystone and it doesn't seem to be there | 15:07 |
| rdopiera | ganso: well, you get an error somewhere form that api call, no? | 15:08 |
| ganso | rdopiera: given the CLI always works also hints that the fix has to be in horizon | 15:08 |
| ganso | rdopiera: there is no error per se, it is not just a different behavior | 15:08 |
| ganso | rdopiera: the API doesn't error out, and the API call and parameters are exactly the same as the CLI, except for the token | 15:09 |
| ganso | rdopiera: keystone receives the API call and derives the context from the token suplied | 15:09 |
| ganso | rdopiera: so the API is also not the issue, it is the token being sent | 15:09 |
| ganso | rdopiera: if the CLI is sending a token and getting the correct result, and Horizon is sending another token and getting a different result, seems like the problem is around selecting which token to send, which Horizon's code does | 15:10 |
| rdopiera | ganso: then maybe there should be an option added to force sending a specific kind of token? | 15:10 |
| ganso | rdopiera: that's exactly what the proposed fix is doing | 15:11 |
| ganso | rdopiera: but not as an option, but trying to match the behavior to the CLI, deemed as the "expected" behavior | 15:11 |
| rdopiera | no, the proposed fix is monkey-patching the request object to get around a check that is later in the code | 15:11 |
| ganso | rdopiera: yea, but achieving the intended result | 15:12 |
| rdopiera | and making everything harder to understand and maintain | 15:12 |
| ganso | rdopiera: but when you say "an option" it sounds user-facing or controlled | 15:13 |
| ganso | rdopiera: indeed, I don't like the approach much, but a config option or a button in the UI will be a more dramatic change, which is worth discussing here | 15:14 |
| ganso | that's part of the point of bringing it to discussion | 15:14 |
| rdopiera | I don't mean a config option, I mean an option you can pass when calling the keystone functions | 15:14 |
| ganso | rdopiera: a config option would need to be well thought out as to which workflows it would affect. It seems this is the only workflow that is being affected by this token issue | 15:15 |
| rdopiera | this way 1. it's fixed where it's actually handled, 2. other api calls can use it too | 15:15 |
| ganso | rdopiera: oh so you mean, do the same thing but in a less ugly way ? | 15:15 |
| rdopiera | not a config option | 15:15 |
| rdopiera | a parameter | 15:15 |
| rdopiera | perhaps when creating keystoneclient | 15:15 |
| ganso | rdopiera: I see, I have looked at the code, I don't have the exact line codes and filename handy right now but I know the code that performs the decision on whether to use the domain_token or the user_token (in keystoneclient), what you suggesting is a parameter when creating the keystoneclient like "force_user_token=True" which would make the solution clean, and the app cred call would just pass that to the keystoneclient | 15:18 |
| ganso | rdopiera: instead of doing that request token reshuffling | 15:18 |
| ganso | rdopiera: correct? | 15:18 |
| rdopiera | look at line 155 of that file | 15:19 |
| rdopiera | correct | 15:19 |
| ganso | rdopiera: oh yes, exactly, that's the one, I forgot the "keystoneclient()" was in the same file | 15:20 |
| ganso | ln 155 | 15:20 |
| ganso | rdopiera: it is just basically avoid line 157 with a new flag passed as parameter | 15:21 |
| rdopiera | I don't know this code too well myself, but I would think the fix should be around there somewhere | 15:21 |
| ganso | rdopiera: sounds good, I agree, looks cleaner, produces the same result | 15:22 |
| ganso | rdopiera: I will make the changes | 15:22 |
| vishalmanchanda | rdopiera: ganso :thanks for discussion. | 15:23 |
| ganso | rdopiera: BUT, the original concern I raised last week still stands whether we bug is actually intended behavior or not, but based on all the evidence I see barely any argument in favor to say that it is, mainly due to the fact that the beahvior is different than the CLI | 15:23 |
| ganso | s/we bug/the bug | 15:24 |
| rdopiera | as long as it doesn't break things for other users... | 15:24 |
| ganso | rdopiera: given the new parameter will be only used in create app cred, it won't. However, I hope it doesn't break users that are intending to create app cred WITHOUT project_id, which I don't see why anyone would be needing that | 15:25 |
| amotoki | perhaps it happens due to the fact that horizon assume the scope based on which panel is used but we may need more explicit scope. | 15:25 |
| rdopiera | adding a parameter would be a step in the right direction | 15:27 |
| ganso | +1 | 15:27 |
| ganso | great, thanks for the discussion =) | 15:27 |
| rdopiera | tmazur: you wanted to talk about tests? | 15:30 |
| tmazur | vishalmanchanda: could you please take a look at https://review.opendev.org/c/openstack/horizon/+/819725/ ? | 15:30 |
| vishalmanchanda | tmazur: sure, will test it after the meeting or tommorow morning. | 15:31 |
| tmazur | It seems changes in webdriver.py make the integration tests more stable | 15:31 |
| tmazur | So probably if we have it merged it will improve integration tests stability in general | 15:32 |
| tmazur | vishalmanchanda: thanks! | 15:32 |
| vishalmanchanda | great. | 15:33 |
| vishalmanchanda | Does anyone have any other topic to discuss? | 15:34 |
| vishalmanchanda | if nothing more to discuss, let end this meeting | 15:38 |
| vishalmanchanda | A reminder about adding topics for antelope PTG | 15:38 |
| vishalmanchanda | https://etherpad.opendev.org/p/horizon-antelope-ptg | 15:38 |
| vishalmanchanda | Thanks everyone for your contributions! | 15:38 |
| vishalmanchanda | #endmeeting | 15:39 |
| opendevmeet | Meeting ended Wed Sep 28 15:39:07 2022 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) | 15:39 |
| opendevmeet | Minutes: https://meetings.opendev.org/meetings/horizon/2022/horizon.2022-09-28-15.00.html | 15:39 |
| opendevmeet | Minutes (text): https://meetings.opendev.org/meetings/horizon/2022/horizon.2022-09-28-15.00.txt | 15:39 |
| opendevmeet | Log: https://meetings.opendev.org/meetings/horizon/2022/horizon.2022-09-28-15.00.log.html | 15:39 |
| tobias-urdin | vishalmanchanda: any chance i could get some reviews and input on https://review.opendev.org/c/openstack/horizon/+/844574 ? | 17:20 |
| opendevreview | Merged openstack/horizon master: Integration test navigation machinery for Angular pages https://review.opendev.org/c/openstack/horizon/+/819725 | 19:12 |
| opendevreview | Rodrigo Barbieri proposed openstack/horizon master: Fix app cred create without project_id for domain admins https://review.opendev.org/c/openstack/horizon/+/856308 | 21:18 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!