| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-sdks - 2020-03-02 | |||
| 16:37:16 | openstackgerrit | Rodolfo Alonso Hernandez proposed openstack/openstacksdk master: Include "fields" to "SecurityGroup" query parameters https://review.opendev.org/710820 | |
| 16:37:58 | mordred | brtknr: if you have a sec, you could probably just cargo-cult in an if not strict_mode: add back uuid patch to keep it simple for the time being - and we can see about migrating the methods as a followup | |
| 16:39:05 | larsks | mordred: Do you know if there is a particular reason that volume.volumes(details=True) doesn't return the user_id information from the remote response? | |
| 16:41:04 | openstackgerrit | Bharat Kunwar proposed openstack/openstacksdk master: Return uuid alias for coe_cluster in non strict mode https://review.opendev.org/710823 | |
| 16:41:08 | brtknr | mordred: ^ | |
| 16:41:36 | mordred | larsks: looking | |
| 16:42:09 | mordred | brtknr: looks great. it's possible a test mock will need to get updated as a result - but let's see how it goes | |
| 16:42:31 | openstackgerrit | Rodolfo Alonso Hernandez proposed openstack/python-openstackclient master: Add "fields" parameter to ListSecurityGroup query https://review.opendev.org/710825 | |
| 16:43:34 | mordred | larsks: there is no mention of user_id in openstack/block_storage/v3/volume.py - so I think it's purely that it's missing and not anything on purpose | |
| 16:43:56 | larsks | mordred: thanks. I was just curious if the omission was intentional or not. I'll submit a change. | |
| 16:45:08 | brtknr | mordred: locally, all my tox -e py36 failures are related to cloud devstack not found as I export OS_CLOUD=devstack to my env vars | |
| 16:46:02 | mordred | brtknr: hah. nice | |
| 16:46:12 | mordred | brtknr: well - let's see what zuul tells us :) | |
| 16:46:23 | brtknr | mordred: also TIL what cargo cult means | |
| 16:46:42 | mordred | brtknr: :) | |
| 16:47:19 | brtknr | If this patch goes through, it will void this patch: https://review.opendev.org/#/c/709734/ | |
| 16:48:44 | brtknr | py36: commands succeeded | |
| 16:48:46 | brtknr | congratulations :) | |
| 16:48:57 | brtknr | after unsetting OS_CLOUD | |
| 16:49:26 | mordred | brtknr: I think we should land that patch too - it describes the current state - and somone might have those modules and _not_ latest sdk - and the id codepath you did is always going to work | |
| 16:50:00 | mordred | when the uuid codepath lands in sdk it'll improve things for people and then we can add a doc that says uuid will be an alias in the return value if sdk is new enough | |
| 16:50:07 | brtknr | hmm this seems to highlight an underlying problem, why is tox -e py36 fail when I export OS_CLOUD= | |
| 16:50:28 | brtknr | mordred: sounds good to me | |
| 16:50:37 | brtknr | I will revert the change to the doc | |
| 16:50:54 | brtknr | btw how the hell do you use ansible-collection-openstack? | |
| 16:51:16 | brtknr | Its not available on galaxy yet so cant use ansible-galaxy to install it | |
| 16:51:42 | smcginnis | brtknr: I haven't been following the conversation here, but sound like the OS_CLOUD issue might be a side effect of this: https://opendev.org/openstack/ansible-collections-openstack/src/branch/master/tox.ini#L10 | |
| 16:51:56 | mordred | brtknr: no - don't revert it - I think it's solid | |
| 16:52:01 | mordred | brtknr: it's the right thing | |
| 16:52:21 | mordred | brtknr: you need to build a tarball from source then you can install the tarball | |
| 16:52:30 | mordred | we're working on getting galaxy pub working | |
| 16:52:43 | mordred | brtknr: I recommend using tox -ebuild to make the tarball | |
| 16:52:50 | mordred | otherwise it's going to have a sad | |
| 16:52:59 | brtknr | revert line 190 here: https://review.opendev.org/#/c/709734/4/plugins/modules/os_coe_cluster.py | |
| 16:53:16 | mordred | no - don't revert that | |
| 16:53:21 | mordred | your comment there is correct | |
| 16:53:43 | brtknr | Ah okay fair enough | |
| 16:53:51 | mordred | that _is_ the situation most users will encounter - they won't see uuid unless they install an as-yet-unreleased sdk :) | |
| 16:54:17 | brtknr | mordred: is it correct that when a cluster gets *created* then uuid is returned? | |
| 16:54:35 | brtknr | shouldn't the uuid get normalised in the same way for create requests too? | |
| 16:54:38 | mordred | it shouldn't - it should be the same data structure all the time | |
| 16:54:40 | mordred | yup | |
| 16:55:07 | brtknr | ok so thats another bummer, it returns uuid at present | |
| 16:55:15 | brtknr | when cluster get created | |
| 16:55:29 | mordred | wow really? | |
| 16:56:09 | brtknr | yes sorry | |
| 16:56:20 | mordred | so that means we're not running normalize on the result of create - which is sadmaking. maybe we need to do a workaround in the ansible module itself | |
| 16:56:53 | mordred | to grab the id in both cases with a get('id', get('uuid') - then make sure both id and uuid are set in the dict before we return to teh ansible user | |
| 16:57:03 | mordred | cause otherwise this is just a mess for the user | |
| 16:58:31 | brtknr | that is the premise of the patch | |
| 16:58:50 | brtknr | its what i am already proposing we do with that patch | |
| 17:00:01 | mordred | cool. then yes - I think we should land that patch asap | |
| 17:00:13 | mordred | Shrews: ^^ feel like an easy +A? I know you like patches with no context | |
| 17:00:25 | Shrews | so much | |
| 17:00:27 | brtknr | :D | |
| 17:00:38 | mordred | brtknr: oh - actually - one more thing... | |
| 17:01:47 | mordred | brtknr: real quick ... or I can update the patch if you're packing up | |
| 17:01:54 | mordred | brtknr: (left inline comment) | |
| 17:02:10 | Shrews | mordred: let's pretend i haven't read the backscroll yet. is 709734 the thing you referred to? | |
| 17:03:08 | Shrews | that looks doubtful. /me just reads bs | |
| 17:03:12 | mordred | Shrews: yes. and the tl;dr is that normalization of cluster_template dicts is blown and we forgot to add the 'uuid' field back to the dict, but the ansible docs and module refer to uuid not id | |
| 17:04:07 | mordred | there's an sdk fix to add uuid - but of course latest module doesn't mean latest sdk - so there's also a patch to the module to work around the fact that it might not get uuid back - but a user might be expecting it to exist because of the docs saying it's there | |
| 17:07:25 | brtknr | mordred: done | |
| 17:07:26 | openstackgerrit | Bharat Kunwar proposed openstack/ansible-collections-openstack master: os_coe_cluster: Retrive id/uuid correctly https://review.opendev.org/709734 | |
| 17:08:05 | mordred | brtknr: thanks! | |
| 17:08:16 | brtknr | is cluster['id'] = cluster['uuid'] = cluster_id okay? | |
| 17:08:39 | Shrews | shouldn't the changelog fragment at least mention the module being fixed? not sure that that is clear when just reading the resulting log | |
| 17:08:39 | brtknr | mordred: or is that bad practice? | |
| 17:09:38 | brtknr | Shrews: done | |
| 17:09:39 | openstackgerrit | Bharat Kunwar proposed openstack/ansible-collections-openstack master: os_coe_cluster: Retrive id/uuid correctly https://review.opendev.org/709734 | |
| 17:09:43 | mordred | brtknr: I think it's fine in this case - not sure if pep8 will agree or not :) | |
| 17:10:31 | brtknr | I'm running pep8 locally | |
| 17:10:37 | brtknr | band it succeeds | |
| 17:10:58 | brtknr | Thanks folks | |
| 17:11:13 | openstackgerrit | Lars Kellogg-Stedman proposed openstack/openstacksdk master: Include user_id attribute in volume information https://review.opendev.org/710834 | |
| 17:11:28 | mordred | woot. thanks brtknr | |
| 17:11:53 | mordred | larsks: thanks | |
| 17:12:12 | mordred | I think when that and the coe patch above land we should cut a bugfix release | |
| 17:12:16 | brtknr | would be nice to fix the create case too on openstacksdk side for consistency? | |
| 17:13:18 | Shrews | mordred: you'll need to reapply your +2 to 709734 | |
| 17:13:30 | mordred | brtknr: yeah, it would | |
| 17:13:52 | mordred | Shrews: done. thanks | |
| 17:14:55 | mordred | brtknr: if you want to do it - it would be adding a normalize call in openstack/cloud/_coe.py in create_cluster_template (and create_cluster) - | |
| 17:15:11 | mordred | brtknr: so like return self._normalize_cluster_template(cluster_template) | |
| 17:40:26 | openstackgerrit | Adam Harwell proposed openstack/python-openstackclient master: Switch image to use SDK https://review.opendev.org/650374 | |
| 17:40:26 | openstackgerrit | Adam Harwell proposed openstack/python-openstackclient master: Complete switch from glanceclient to SDK for image service https://review.opendev.org/699416 | |
| 17:48:11 | mordred | rm_work: \o/ | |
| 17:48:34 | rm_work | ... i believe i still owe something with ospurge too <_< | |
| 17:48:44 | rm_work | my volunteer backlog with SDK/OSC is high T_T | |
| 17:49:52 | mordred | rm_work: well - https://review.opendev.org/#/c/700219/ | |
| 17:50:45 | rm_work | yes, but i believe that was superseded by the decisions we made in the meetup in Shanghai :D | |
| 17:51:00 | rm_work | (or is this something else?) | |
| 17:51:45 | mordred | I think this is still aiming at that - but it's entirely possible it's going down the wrong road | |
| 17:52:20 | rm_work | I believe what we said was "just drop-in the ospurge codebase into SDK" | |
| 17:52:33 | mordred | oh right | |
| 17:52:36 | rm_work | and then drop the mic and walk away :D | |
| 17:52:50 | mordred | and I think we keep getting busy ... | |
| 17:52:54 | rm_work | yes. | |
| 17:53:01 | mordred | stupid busy | |
| 17:53:18 | mordred | ok - let me wrap my head around that whole thing this wweek | |