Earlier  
Later
New Search
Posted Nick Remark
#openstack-sdks - 2018-03-20
15:23:43 dtroyer mnaser: in theory this should have been caught by the -tips jobs, did it get missed because OSc functional tests don't touch that command?
15:24:14 elmiko dtantsur: ack, and yeah, it will be a team effort to provide what the community might want. in general i agree with the notion of promoting async comms
15:24:14 mnaser dtroyer: well, im just writing this command now (list image members)
15:24:36 mnaser but i think this is probably broken with any command over all
15:26:42 dtroyer ok, I'm finally catching up on the exact problem
15:27:42 EmilienM hello, can someone review https://review.openstack.org/#/c/553374/ please?
15:27:44 dtroyer that's a gap in the functional tests, ie no negative tests there
15:28:06 EmilienM I'm ok to send free mapple sirup
15:29:16 dtroyer mnaser: I'd probably lean toward restoring the compatibility in the SDK for that exception, we're going to have to go through a lot of code to change all those over someday anyway but really can't do that yet
15:30:07 dtroyer and yes, that potentially affects any command that needs auth
15:32:35 openstackgerrit Mohammed Naser proposed openstack/python-openstackclient master: Add support to list image members https://review.openstack.org/554590
15:32:56 mnaser ^ my first osc change so it probably is mising a whole lot of stuff (and couldn't find a sample to iterate on)
15:33:26 mnaser dtroyer: so what would be the best direction (aka, can i help with that?)
15:35:01 dtroyer mnaser: my gut says to put in a compat exception back into the SDK since that is what changed. It appears, though, that osc-lib _may_ be the only casualty, but it'll be messier to fix there until we rip out the os-client-config imports
15:35:26 dtroyer which is not going to happen until we get an SDK 1.0 release
15:35:33 mnaser dtroyer: and the concern is how do you make sure that gets removed eventually and doesn't sit ther forever, but i cant imagine that being very harmful
15:36:11 mnaser unless, well, people start relying on it.
15:36:30 mordred mnaser: sorry - on phone, will help in just a few
15:36:33 mnaser which they probably will if thats what is bubbling up the excpetion in osc_lib
15:36:36 mnaser np :)
15:36:46 dtroyer I would expect it to get caught when cleaning up the o-c-c stuff
15:37:13 mordred in the shade layer we made the old exception classes aliases for the new ones - we should defintely do that here
15:37:37 dtroyer I'm planning to rip out much of that module for OSC 4 anyway so we'll default to doing basically the same thing that all other SDK consumers do WRT auth and special cases
15:38:34 mordred mnaser: if we just put a OpenStackConfigException = ConfigException in openstack/config/exceptions.py it should be happier
15:38:50 edleafe cdent: it *can* be ok to do that; depends on what you're doing with the members of the set
15:39:23 edleafe cdent: generally I use a 'to_remove' var to hold the ones to remove, and then remove 'em after the iteration
15:39:37 mnaser mordred: ok cool, i'd nominate myself to push up a patch for that but i think it would be useful to have the context of that in a note/commit message which i'm probably not the best at writing given i dont know the whole relationship between all these clients :)
15:39:42 cdent edleafe: I'll put it up for people to think about. I'm pretty sure it works for the case I've got, but we'll see
15:40:46 cdent edleafe: it's basically this: http://paste.openstack.org/show/706227/
15:42:58 cdent (untested)
15:44:21 edleafe cdent: did you run that?
15:44:27 cdent untested
15:44:35 cdent I'm getting ready to test it
15:44:59 cdent but I hadn't gotten that far because I realized it may very well be to dirty, so I thought I'd ask the dirty guy before I kept moving
15:45:43 dtroyer mnaser: regarding the new command, stevemar points out a prior attempt and the objection then that "image member" is not currently a resource in OSC. I don't like that name, the only place OSC uses "member" at all is in the image set command options —accept, —reject and —pending, there phrased as "project membership status". So we treat this as an image property already, not as a top-level resource
15:46:02 edleafe cdent: Generally you can't change the size of a set during an iteration over it
15:46:57 dtroyer Adding a —member (or —membership or —project) option to image show seems more natural, except that the nested structure of a list of data inside a field is always messy
15:47:19 cdent edleafe: ooops, I misread the docs for set.pop() for one thing
15:48:13 edleafe cdent: Since they're sets, I'd just grab the set of forbidden, and then do difference_update() to remove the forbidden traits
15:48:22 cdent yeah, I'll do that
15:48:41 mnaser dtroyer: i see.. so what is it that you'd recommend, i think ideally image member should be a resource and things like 'image member add' and 'image member remove' should be how things are
15:48:48 mnaser (or they should have been from the start, at least imho)
15:49:03 mnaser as members are image resources, i guess we can compare them to metadata
15:49:28 cdent edleafe: my confusion originated from conflating behaviors in a while loop with a for loop. but yeah, will do something that's less clever and actually works
15:50:21 dtroyer mnaser: we have image add/remove project already for that
15:50:31 dtroyer again, we didn't use the 'member' word
15:50:42 mnaser yeah but then image list project is weird
15:50:48 mnaser or doesnt even technically work
15:50:51 dtroyer there is a parallel to this with security groups and security group rules
15:51:08 mnaser so what was the solution from that parallel
15:51:54 dtroyer security group rules _are_ a top-level resource and treated as that everywhere.
15:52:29 dtroyer they have their own properties/attributes and require the full set of create/delete/list/show commands
15:52:38 dtroyer the member list isn't the same thing
15:52:53 dtroyer so that's not the best example of a one-to-many relationship after all :(
15:54:10 dtroyer the project list attached to an image is just another property that can have multiple values
15:55:46 mnaser dtroyer: well, its an http resource so its not like it comes along a 'image show' http request
15:56:47 dtroyer right, but frankly I don't care what the REST API looks like, it is about the model presented to the user. which is one reason not automatically pulling the data is a consideration, via adding an option to image show
15:57:18 mnaser i mean if it was a flat list (ex: projects only), it wouldn't be that bad to add it to the show
15:57:31 mnaser the issue is that it presents other things like .. member id and status (ex: accepted, pending, etc)
15:57:53 mnaser i dont mind pushing it up to show but the thing that would make me sad is that the user will have a very hard time parsing it using the tools available in osc
15:58:03 mnaser ex cant use --columns and so on
16:00:14 dtroyer right, that's a general problem with a number of our data structures, and is one reason we played with adding a jq interface to make dealing with nested data via JSON easier. turns out to still be rather messy to do in a CLI
16:01:34 openstackgerrit Jens Harbott (frickler) proposed openstack/keystoneauth master: Be more verbose when version discovery fails https://review.openstack.org/554044
16:02:48 dtroyer mnaser: so since image memebr has an ID, that improves the case for it being a top-level resource. however, I'm not aware of any place to actually use that ID. It is the status that makes this messy.
16:06:22 dtroyer I still don't like "image member" but it is better than "image project". unless we come up with something obviously better, lets add "image member" as a top-level resource, which means adding it to the resource list in doc/source/cli/commands.rst. It should be in its own module but I don't plan on adding other commands so leaving it in image.py is probably fine.
16:07:41 dtroyer I don't know what the API looks like offhand, but adding a —project option to image member list might be nice at some point to only list images that a particular project has access to. future.
16:08:40 dtroyer and now lunch. I'll summarize this in the review after lunch. thanks for the command and for letting me think through this out loud to refresh my mind.
16:09:03 dtroyer EmilienM: I'll get to your review after lunch too
16:10:29 mordred dtroyer, mnaser: ok - back ...
16:10:50 EmilienM dtroyer: thanks!
16:15:50 openstackgerrit Monty Taylor proposed openstack/python-openstacksdk master: Provide OpenStackConfigException backwards compat https://review.openstack.org/554615
16:16:32 mordred mnaser, dtroyer: ^^ that should unstick mnaser's issue ... although now that we can make osc-lib depend on openstacksdk>=0.12 - we could make a pass through and update it to remove the occ paths... I'll do that as a followup
16:17:36 mordred perhaps land that patch, cut an sdk point release, bump the min in global-requirements, then push up osc-lib patches to remove occ fallback paths?
16:17:44 mordred dtroyer: that sound like an ok plan?
16:37:57 mordred frickler: left a comment on your ksa patch
16:40:42 frickler mordred: I tend to agree with your comment, would you want to reword the original message down on L199 accordingly? would seem sensible to keep those in sync
16:49:06 mordred frickler: I'd love to - it's my least favorite error message ... however, ...
16:49:21 mordred kmalloc: ^^ do we consider error message text part of keystoneauth's api?
16:49:31 kmalloc hm
16:49:45 kmalloc mordred: i hope we don't. but honestly, haven't thought about that
16:49:53 kmalloc mordred: maybe we should?
16:49:55 mordred kmalloc: me either
16:50:09 kmalloc i'm willing to make a call on that right here conferring with you ... and....
16:50:14 kmalloc i'm inclined to say no
16:50:19 kmalloc but the exception class would be.
16:50:25 mordred kmalloc: specifically, "Could not determine a suitable URL or the plugin" - which almost never actually means what it says
16:50:33 kmalloc yeah
16:50:37 mordred kmalloc: yah - exception class == part of aPI for sure
16:51:10 kmalloc so, i think the message shouldn't be because it might change to be "better" in the future
16:51:20 kmalloc unless it's structured data.
16:51:30 kmalloc whichcase it would be part of the api
16:51:47 kmalloc if it's just a "message", it's not part of the api (in my mind, but i could be convinced otherwise)
16:51:49 mordred kmalloc: yah - but in that case I'd expect us to put that data on the execeptoin instance
16:51:52 kmalloc right
16:52:05 kmalloc so, you're good with saying it's not part of the API?
16:52:17 mordred kmalloc: so maybe let's say "no" - and if people are finding places in which they need to look at exception text, then we nee more specific exception classes
16:52:23 kmalloc wfm
16:52:25 kmalloc ^5
16:52:34 mordred frickler: ^^
16:52:46 kmalloc exception text is useful for humans

Earlier   Later