| *** ysandeep|out is now known as ysandeep | 03:57 | |
| *** ysandeep is now known as ysandeep|afk | 07:11 | |
| opendevreview | Merged openstack/ansible-collections-openstack master: Move identity domain to use proxy layer https://review.opendev.org/c/openstack/ansible-collections-openstack/+/827267 | 08:36 |
|---|---|---|
| *** ysandeep|afk is now known as ysandeep | 08:45 | |
| opendevreview | Shnaidman Sagi (Sergey) proposed openstack/ansible-collections-openstack master: Add CentOS 9 tripleo job https://review.opendev.org/c/openstack/ansible-collections-openstack/+/827066 | 09:29 |
| *** dviroel|out is now known as dviroel|ruck | 10:05 | |
| sshnaidm | gtema, can you comment on question please? https://review.opendev.org/c/openstack/ansible-collections-openstack/+/825291/10/plugins/modules/network.py | 11:58 |
| *** ysandeep is now known as ysandeep|mtg | 12:01 | |
| opendevreview | Shnaidman Sagi (Sergey) proposed openstack/ansible-collections-openstack master: Move compute flavor info to use proxy layer https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828108 | 12:13 |
| opendevreview | Shnaidman Sagi (Sergey) proposed openstack/ansible-collections-openstack master: DNM test flavor changes https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828290 | 12:13 |
| opendevreview | Ananya proposed openstack/ansible-collections-openstack master: Moves image_info from cloud to proxy object https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828158 | 12:14 |
| sshnaidm | frenzy_friday, can you please add test for image_info in a different patch like that? https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828290 | 12:16 |
| frenzy_friday | sshnaidm, ack, thanks | 12:16 |
| opendevreview | Ananya proposed openstack/ansible-collections-openstack master: DNM testing image info changes https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828294 | 12:21 |
| frenzy_friday | sshnaidm, is this the right syntax https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828294 ? | 12:21 |
| sshnaidm | frenzy_friday, you can't depend on same repo patch, need to push testing patch "above" yours | 12:22 |
| sshnaidm | just add another commit and answer "yes" for git review | 12:23 |
| opendevreview | Ananya proposed openstack/ansible-collections-openstack master: DNM testing image info changes https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828294 | 12:23 |
| opendevreview | Ananya proposed openstack/ansible-collections-openstack master: DNM testing image info changes https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828294 | 12:23 |
| frenzy_friday | sshnaidm, oh right! done | 12:23 |
| sshnaidm | frenzy_friday, exactly, thanks! | 12:23 |
| *** ysandeep|mtg is now known as ysandeep | 12:55 | |
| opendevreview | Arx Cruz proposed openstack/ansible-collections-openstack master: Move compute flavor info to use proxy layer https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828108 | 13:04 |
| *** ysandeep is now known as ysandeep|coffee | 13:04 | |
| opendevreview | Shnaidman Sagi (Sergey) proposed openstack/ansible-collections-openstack master: DNM test flavor changes https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828290 | 13:07 |
| opendevreview | Ananya proposed openstack/ansible-collections-openstack master: DNM testing image info changes https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828294 | 13:35 |
| *** ysandeep|coffee is now known as ysandeep | 13:39 | |
| sshnaidm | gtema, is "properties" in project processed somehow? I do conn.identity.create_project(name="someone", properties={"test": "testvalue"}) with a new sdk and I don't see any "test" in outputs | 13:41 |
| gtema | properties is not a real stuff in project | 13:43 |
| gtema | do we have some custom stuff there that is not currently supported by resource? | 13:44 |
| opendevreview | Shnaidman Sagi (Sergey) proposed openstack/ansible-collections-openstack master: WIP fix collection for new SDK https://review.opendev.org/c/openstack/ansible-collections-openstack/+/825291 | 13:44 |
| sshnaidm | gtema, I see it here: https://github.com/openstack/ansible-collections-openstack/blob/master/plugins/modules/project.py#L36-L39 | 13:45 |
| sshnaidm | gtema, interesting why it's accepted then when creating project | 13:46 |
| gtema | well, actually project in keystone allows adding some custom props (as options) | 13:47 |
| gtema | https://docs.openstack.org/api-ref/identity/v3/index.html?expanded=token-authentication-with-scoped-authorization-detail,create-project-detail#create-project | 13:47 |
| gtema | yeah, so you might want to pass it into the create as options (which is supported) and not "properties", which was an old name in times of v2 I guess | 13:48 |
| sshnaidm | gtema, seems like it's different from properties: https://paste.opendev.org/show/812584/ | 13:49 |
| sshnaidm | frenzy_friday, depends-on on same repo patch doesn't work really | 13:49 |
| sshnaidm | frenzy_friday, wrt https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828294/ | 13:50 |
| gtema | if I know it correctly the backend keystone need to be configured to allow specific extensions | 13:50 |
| gtema | mean specific attributes | 13:50 |
| frenzy_friday | sshnaidm, sorry, forgot to pull the rebase before I updated the patch.. | 13:50 |
| opendevreview | Ananya proposed openstack/ansible-collections-openstack master: DNM testing image info changes https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828294 | 13:51 |
| opendevreview | Ananya proposed openstack/ansible-collections-openstack master: DNM testing image info changes https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828294 | 13:51 |
| sshnaidm | gtema, ack.. | 13:52 |
| sshnaidm | so with default devstack it won't work | 13:52 |
| gtema | I guess this is also something that nobody ever tested properly | 13:53 |
| sshnaidm | gtema, well, with old SDK it works and return properties, our collection tests always tested it and fail on new sdk only: https://github.com/openstack/ansible-collections-openstack/blob/master/ci/roles/project_properties/tasks/main.yml#L2 | 13:54 |
| sshnaidm | actually there is a whole role dedicated just to test project properties | 13:54 |
| gtema | hmm | 13:55 |
| sshnaidm | gtema, for history: https://github.com/openstack/ansible-collections-openstack/commit/c1a2496e0ff3e8b3d9b70cb8058834adb9b35099 | 13:56 |
| sshnaidm | there is a dependency on SDK | 13:56 |
| sshnaidm | https://review.opendev.org/c/openstack/openstacksdk/+/715255/ | 13:56 |
| sshnaidm | hmm.. seems like it's for update_project only | 13:57 |
| gtema | it is really weird that the test only verifies dummy_key in the create/update operation, but I do not see check for reading it | 13:58 |
| sshnaidm | gtema, there are "assert" after each task | 13:59 |
| gtema | but those are not reading. I mean if the code was just leaving this attr and giving it back - it would be passing | 13:59 |
| gtema | should not happen actually | 14:00 |
| sshnaidm | gtema, it's returned by conn.update_project | 14:00 |
| gtema | I am honestly confused how this is/supposed_to work | 14:01 |
| gtema | this is not something that I can assume from keystone api docs | 14:01 |
| gtema | as if keystone would support additional (not specified) attributes (like image does) but doesn't shed a single word on that | 14:03 |
| sshnaidm | gtema, old and new sdk outputs: https://paste.opendev.org/show/812588/ | 14:05 |
| sshnaidm | seems like old returns Much in properties | 14:05 |
| sshnaidm | s/Much/Munch/ | 14:06 |
| sshnaidm | and it's cloud function "update_project" | 14:06 |
| gtema | https://opendev.org/openstack/openstacksdk/src/commit/60d04799a776efb31e32a8e356c91a635632fc54/openstack/cloud/_normalize.py#L687 - this is where the conversion logic was happening | 14:07 |
| sshnaidm | the proxy one doesn't doesn't return much | 14:07 |
| gtema | mean whatever is non-standard returned by API is reported as "properties" | 14:07 |
| gtema | and whatever you pass to the SDK call would be passed without thinking further to API | 14:07 |
| gtema | that is not really right. I hate such undocumented behaviors | 14:08 |
| gtema | I can enable this type of behavior for project (same as for image and most of network resources), but I would really be interested to hear from Keystone whether this is a bug or a undocumented feature | 14:09 |
| sshnaidm | gtema, so how to explain lack of "properties" in a new sdk? | 14:09 |
| gtema | do you have somebody from keystone to check? | 14:10 |
| sshnaidm | gtema, nope.. but I use the same keystone, it seems like SDK processing it..? | 14:11 |
| gtema | I mean I am not a fan if implementing undocumented behavior | 14:11 |
| gtema | previously it was not thinking of the input. Now it does | 14:12 |
| sshnaidm | yeah, can't see it in docs as well | 14:16 |
| gtema | bad | 14:21 |
| sshnaidm | and seems like people use it a lot.. | 14:21 |
| gtema | really? | 14:22 |
| gtema | in this case I can enable handling of "undocumented" props in project. But I would really like to have Keystone folks to comment on that | 14:23 |
| sshnaidm | let's ask them, where are they? #openstack-keystone? | 14:24 |
| gtema | don't know, maybe | 14:25 |
| sshnaidm | asking there.. | 14:26 |
| sshnaidm | arxcruz, commented https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828108 - please add a few module executions in tests | 14:30 |
| sshnaidm | knikolla> sshnaidm: it is supported in v3, though we try to encourage people not to use it. | 14:38 |
| sshnaidm | gtema, ^^ | 14:38 |
| gtema | hehe, as I expected | 14:39 |
| sshnaidm | well, seems like I'm gonna leave it as is, who want the old behavior - use the old sdk.. | 14:40 |
| gtema | yeah, I would also go this way. If we can "help" service creators to discourage bad behavior - let's do this. | 14:43 |
| sshnaidm | damn, we had really a whole role to test it.. | 14:50 |
| sshnaidm | such an investment | 14:51 |
| opendevreview | Shnaidman Sagi (Sergey) proposed openstack/ansible-collections-openstack master: WIP fix collection for new SDK https://review.opendev.org/c/openstack/ansible-collections-openstack/+/825291 | 14:56 |
| gtema | I know I am not making friends by making such defensive decisions, but somebody need to be a janitor ;-) | 14:57 |
| sshnaidm | gtema, sure, no problem with that :) | 14:59 |
| sshnaidm | time to break! | 14:59 |
| gtema | yuhui | 15:04 |
| gtema | yuppi yey | 15:04 |
| *** dviroel|ruck is now known as dviroel|ruck|lunch | 15:11 | |
| *** ysandeep is now known as ysandeep|dinner | 15:14 | |
| arxcruz | sshnaidm ack, i'll do it | 15:31 |
| sshnaidm | gtema, probably worth to make a release before any of breaking changes are going in | 15:34 |
| sshnaidm | release of collection I mean | 15:34 |
| sshnaidm | and then finally to cut stable-1.0.0 | 15:34 |
| gtema | yeah, makes sense | 15:35 |
| sshnaidm | jm1, so new sdk don't pass some deprecated keys in output, and we need to decide if we want backward compatibility (and then using to_dict(original_names=True) or don't want and then just using new names only | 15:44 |
| *** ysandeep|dinner is now known as ysandeep | 15:44 | |
| sshnaidm | I was inclined into merging both of them together, but if we are going to break anyway collection, then we'd better just to use the new output | 15:45 |
| sshnaidm | if we can't keep backward compatibility in everything, not sure if we need to try at all.. | 15:45 |
| sshnaidm | but still open to ideas/suggestions of course | 15:45 |
| sshnaidm | dmsimard, wdyt about? ^^ | 15:46 |
| sshnaidm | as voice of ansible :) | 15:46 |
| jm1 | sshnaidm: new SDK will return new keys, e.g. openstack.network.ips() will return a new tenant_id entry which is a copy of project_id. What do we do about these new-but-somehow-deprecated fields? | 15:47 |
| sshnaidm | jm1, tenant_id is deprecated I think | 15:48 |
| sshnaidm | project_id is the "new" one | 15:48 |
| jm1 | sshnaidm: do we want to return this new field "tenant_id" from floating_ip_info although it is deprecated? | 15:51 |
| sshnaidm | jm1, that's exactly the dilemma I talked above :) | 15:55 |
| sshnaidm | I know gtema doesn't want to return "old" values | 15:55 |
| gtema | yeah, he doesn't | 15:56 |
| gtema | sooner or later even neutron will stop having it, so why don't we use this breaking time to get rid of things that will definitely go away | 15:57 |
| jm1 | sshnaidm: next question: entries from openstack.network.ips() which have been to_dict'ed contain Munch objects, e.g. see key location and project. The floating_ip_info module drops the location entry but not project, the latter gets dropped by Ansible. What to do? | 15:58 |
| sshnaidm | jm1, dropped by Ansible? | 15:59 |
| sshnaidm | how is it dropped there? | 16:01 |
| jm1 | sdk returns this project Munch, floating_ip_info module does not drop it but Ansible wont show it. so who else than Ansible should drop it? | 16:03 |
| sshnaidm | jm1, can you paste please the output of ansible run in https://paste.opendev.org/ ? | 16:04 |
| *** dviroel|ruck|lunch is now known as dviroel|ruck | 16:06 | |
| jm1 | new sdk output: https://paste.opendev.org/show/812595/ | 16:09 |
| jm1 | ansible output: https://paste.opendev.org/show/812594/ | 16:09 |
| jm1 | old sdk output: https://paste.opendev.org/show/812597/ | 16:10 |
| jm1 | sshnaidm: both have 'project' Munch. Only new sdk has tenant_id | 16:11 |
| sshnaidm | jm1, well, "project" is part of "location" dictionary | 16:18 |
| sshnaidm | so it's removed when "location" is removed | 16:18 |
| sshnaidm | and that's good, we don't need to return Munches | 16:19 |
| jm1 | Munch objects are not returned by Ansible (at least in v2.12). If we drop "dt.pop('location')" this changes nothing | 16:26 |
| jm1 | ..at least it is not outputted | 16:27 |
| jm1 | so what to do? drop "dt.pop" statements? | 16:28 |
| jm1 | i would drop them and add a assertion to the Ansible fip role which checks the output parameters | 16:28 |
| sshnaidm | jm1, I think ansible will return if it has them | 16:29 |
| jm1 | no, ansible will not return them. example role: https://paste.opendev.org/show/812600/ | 16:31 |
| jm1 | example output: https://paste.opendev.org/show/812601/ | 16:32 |
| sshnaidm | jm1, yeah, because "location" is popped | 16:32 |
| jm1 | nope, i dropped the dt.pop from the floating_ip_info module | 16:33 |
| sshnaidm | jm1, try to drop "dns_name" and see if it disappears from output | 16:34 |
| sshnaidm | jm1, example of routers output, when location is not dropped: https://paste.opendev.org/show/812602/ | 16:34 |
| jm1 | sorry, you are right. i should stop watching all-hands meetings while debugging.. | 16:37 |
| jm1 | 'project' Munch is a subkey of 'location' so of course it is dropped as well 🤦 | 16:40 |
| sshnaidm | gtema, I see "interfaces_info" is completely gone from routers output, is it intentional? | 17:07 |
| gtema | what was there before? | 17:08 |
| gtema | this is not a real property | 17:08 |
| jm1 | sshnaidm: should we add assertions to catch output changes? Example: https://paste.opendev.org/show/812604/ | 17:10 |
| sshnaidm | gtema, ha, you're right, we calculate this.. using "network:router_gateway" https://github.com/openstack/ansible-collections-openstack/blob/master/plugins/modules/routers_info.py#L172-L182 | 17:11 |
| sshnaidm | jm1, well, I think it's probably an overhead | 17:11 |
| jm1 | ok | 17:11 |
| gtema | hmm, again those nifty things | 17:12 |
| *** sshnaidm is now known as sshnaidm|afk | 17:18 | |
| *** ysandeep is now known as ysandeep|out | 17:21 | |
| opendevreview | Jakob Meng proposed openstack/ansible-collections-openstack master: Drop deprecated fields in floating_ip_info and assert remaining fields https://review.opendev.org/c/openstack/ansible-collections-openstack/+/828384 | 17:41 |
| *** dviroel|ruck is now known as dviroel|ruck|afk | 22:02 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!