Earlier  
Later
New Search
Posted Nick Remark
#openstack-sdks - 2018-03-20
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
16:52:51 kmalloc not for programatic use.
16:52:56 openstackgerrit Merged openstack/python-openstacksdk master: Fix 'block_store' aliases define error https://review.openstack.org/554403
16:53:48 mordred frickler: also - fwiw, maybe toss in an 'if isinstance(e, exceptions.DiscoveryFailure): raise else: raise exceptions.DiscoveryFailure(...) so that we don't lose the original discovery context just because we made an attempt to guess a versioned endpoint
17:28:16 openstackgerrit Chris Dent proposed openstack/microversion-parse master: Improve Version.matches() method https://review.openstack.org/550251
17:28:17 openstackgerrit Chris Dent proposed openstack/microversion-parse master: Add MicroversionMiddleware https://review.openstack.org/550252
17:28:17 openstackgerrit Chris Dent proposed openstack/microversion-parse master: Allow passing a json_error_formatter to the middleware https://review.openstack.org/550263
17:28:18 openstackgerrit Chris Dent proposed openstack/microversion-parse master: Clean up package version and python version support https://review.openstack.org/550264
17:28:18 openstackgerrit Chris Dent proposed openstack/microversion-parse master: Do not use redundant attr check for keys in fold_headers https://review.openstack.org/554547
20:07:17 openstackgerrit Merged openstack/python-openstacksdk master: Use defined version instead of service.version parameter https://review.openstack.org/554223
21:06:11 openstackgerrit Dean Troyer proposed openstack/python-openstackclient master: Fix additional output encoding issues https://review.openstack.org/554698
21:55:13 dhellmann dtroyer : regarding https://review.openstack.org/#/c/554698/1 I wonder if cliff should grow support for progress bars?
21:56:23 dhellmann https://pypi.python.org/pypi/progressbar was pretty easy to use in downpour
21:57:03 dhellmann oh, although I wonder if that's actively supported any more if it's on code.google.com
21:57:16 dhellmann ah, redirects to https://github.com/niltonvolpato/python-progressbar
21:59:18 dhellmann aha, I actually used https://pypi.python.org/pypi/progressbar2
21:59:56 dhellmann anyway, let me know if you think that would be a useful thing to be able to do via cliff
22:00:11 dtroyer dhellmann: maybe? I really didn't know what to do with that _show_progress() function. Putting it where I did made calling easy to get cliff's stdout but made testing harder than I wanted to finesse to get a ppinter to the nested function.
22:00:33 dhellmann yeah
22:01:08 dhellmann oh, you could have made it a method instead of a closure
22:01:24 dhellmann that would make it easier to mock it out or whatever
22:01:39 dtroyer but then it would require passing self around and changing the signature of the callback

Earlier   Later