Tuesday, 2014-03-18

morganfainbergdolphm, https://review.openstack.org/#/c/69162/5/keystone/trust/controllers.py this looks like a new string to me. (line 139), is this something we need to hold for Juno then?00:04
*** derek_c has quit IRC00:04
*** gokrokve has quit IRC00:20
*** derek_c has joined #openstack-keystone00:35
openstackgerritA change was merged to openstack/keystone: Sync db, db.sqlalchemy from oslo-incubator 0a3436f  https://review.openstack.org/7842900:39
openstackgerritA change was merged to openstack/keystone: Filter out nonstring environment variables before rules mapping.  https://review.openstack.org/8029300:39
jamielennoxfound another use for @positional i can remove things from the argument line and pass them through as kwargs (eg if i want to pass it through to underlying functions) without having to do arg=arg everywhere for compatability00:41
*** leseb has joined #openstack-keystone00:42
*** leseb has quit IRC00:46
*** gokrokve has joined #openstack-keystone00:47
*** jraim has quit IRC00:53
*** jraim has joined #openstack-keystone00:55
*** lbragstad has joined #openstack-keystone01:03
*** marcoemorais has quit IRC01:05
gyeejamielennox, I am blocking https://review.openstack.org/#/c/78241/ for now01:08
gyeeI don't think the bug is valid01:08
jamielennoxgyee: ok with me - you've seen the security bug that is associated with it?01:08
gyeejamielennox, which security bug, if we want to update th doc on the implication of caching then that's fine01:13
*** stevemar has joined #openstack-keystone01:14
jamielennoxgyee: oh no - it's just the linked bug it's public now01:15
gyeethat bug is an unrealistic expectation01:19
gyeesecurity is about risk management, nothing is absolute01:19
dstanekgyee: does it really make the cache useless?01:19
gyeedstanek, yes, we are essentially revalidating the token01:20
dstanekgyee: i thought it was just checking to see if it was in the revoked list cached on the client side01:20
gyeedstanek, line 836-84001:22
dstanekgyee: ah, i see. with a tiny change it could be made to work.01:24
gyeetoken_cache_time is a essentially a tradeoff between performance and security01:24
ayounggyee, you are wrong to block that bug01:25
dstanekgyee: jas, i'll show you what i mean01:26
ayoungyou can avoid the round trip if   you set the memcached timeout to shorter time than the revocation timeout01:26
ayoung"round trip" meaning fetch from the server01:26
ayoungit does not render the cache useless01:27
ayoungit makes sure that a cached token is still checked against the revocation list01:27
gyeeayoung, then there's nothing to change01:27
ayounggyee, no01:27
dstanekhttp://paste.openstack.org/show/73716/ <- gyee01:28
dstanekgyee: would that be worth having?01:28
ayoungif you validate a token with PKI or with UUID, and then cache it for a long period of time, you will miss and revocations01:28
gyeeayoung, what is cache used for?01:28
ayoungcache is used so that you do not need to go back to the server to fetch the data again for each token01:29
ayoungone rvoation list fetch every 5 minues is still preferable to a round trip per token (uuid)01:29
ayoungthe original version was just for PKI tokens, but it really should not matter01:29
ayoungso why are you blocking it?01:30
*** morganfainberg is now known as morganfainberg_Z01:30
gyeeayoung, because the current change render caching useless01:30
ayounggyee, and, to jamielennox s point, just cuz a token is valid does not mean that it is valid from anywhere.  THis patch makes sure that the bind is always checked01:31
ayoungyou are wrong01:31
ayoungthe current change checks against revocation list01:31
jamielennoxoh, he fixed the bind thing?01:31
jamielennoxi haven't looked for a few days01:31
ayoungjamielennox, yep01:31
ayounggyee, so...there is one thing that I thought you were going to say, and I think it is what you are driving at01:32
ayounga UUID token based system now depends on the revocation list01:32
ayoungbut that should be acceptable01:32
jamielennoxhmm, i'm not sure he's got what he is looking for there01:32
jamielennoxit means caching won't work on signed tokens01:33
gyeejamielennox, ayoung, what's bind part are we talking about01:33
jamielennoxand he is doing an expired check and a _cache_put that he doesn't need to do for cached tokens01:33
gyeedstanek, I think your alternative looks more reasonable01:33
ayoungjamielennox, cache.put is maintained01:33
ayoungjamielennox, I wrote that last version of the patch01:34
jamielennoxgyee: a token bind should be checked on every token usage, if it was stored in cache in wasn't being checked01:34
jamielennoxayoung: right cache.put doesn't need to be called for already cached tokens01:34
gyeejamielennox, easy, just turn off cachine!01:34
ayoungjamielennox, true.01:34
jamielennoxayoung: and memcache will invalid the cached object after expiry time so you don't need that check either01:34
gyeeif you need to check the token everytime then what's the point of caching01:34
ayoungjamielennox, only if token expiry time is the same as the memcache expiry01:35
gyeejust set the token_cache_time to -101:35
ayoungand they are not01:35
jamielennoxthey are not/01:35
jamielennoxthey are not?01:35
ayoungjamielennox, lets say you set memcahe expiry for 10 mijnutes nad token expiry is an hour01:35
ayoungtjhen at 59 minutes, you check the token01:35
ayoungwell, every 10 minutes, up until just before the token expires...01:35
jamielennoxayoung: oh wtf did they do that for01:36
ayoungso if you check at 59 minutes, you've just extended the token for 9 minutes past its expiry01:36
jamielennox_cache_put takes an expires argument so i just assume they were using that as the timeout01:36
jamielennoxit should store to cache with min(expires, self.token_cache_time)01:36
ayoungactually, I am wrong01:37
gyeejamielennox, it is, take a look at cache_get01:37
gyeeit check the expiry there01:37
jamielennoxgyee: yea, but that calls _cache_store which puts it into cache with self.token_cache_time01:37
ayoungso the puit is unnecessary01:37
ayoungcan wrap that in an if cached:01:37
jamielennoxso it's checking expires by why bother when memcache is doing that for us01:38
jamielennoxcaching in auth_token has been a known disaster for a while01:38
gyeejamielennox, but the net effect is the same01:38
gyeea token will not be valid past expiry01:40
*** david-lyle has joined #openstack-keystone01:42
gyeeanyway, dinner time, be back later01:42
jamielennoxgyee: yep - net effect is the same01:43
ayoungjamielennox, I was trying to keep the patch as straight forward as possible:  treat the cache as only unmarshalled and semarntically checked data, but all of the validity checking for roles etc needs to be confirmed with the revocation list.01:45
jamielennoxayoung: it's going to need a redo soon anyway01:46
jamielennoxby that i mean we can do the fixups later if we solve the bug01:46
ayoungsure, but a -2 blocker is not called for.01:46
ayoungWe can change the logic now to avoid the spurious put, anything else you see there>01:46
*** morganfainberg_Z is now known as morganfainberg01:49
*** morganfainberg is now known as morganfainberg_Z02:08
jamielennoxdo you think a version discovery 'status' of 'supported' is deprecated02:14
jamielennoxglance lists all it's versions in discovery02:14
jamielennoxso it has eg v2.2 as 'CURRENT', then v2.1, v2.0 as 'SUPPORTED' pointing to the same URL02:14
*** harlowja is now known as harlowja_away02:21
*** zhiyan_ is now known as zhiyan02:28
openstackgerritJamie Lennox proposed a change to openstack/python-keystoneclient: Revamp discovery  https://review.openstack.org/8114602:39
openstackgerritJamie Lennox proposed a change to openstack/python-keystoneclient: Version independent password authentication plugin  https://review.openstack.org/8114702:39
openstackgerritJamie Lennox proposed a change to openstack/python-keystoneclient: Discover should support other services  https://review.openstack.org/7287802:39
*** richm has quit IRC02:41
*** mberlin1 has joined #openstack-keystone02:59
*** mberlin has quit IRC03:00
*** amcrn has quit IRC03:01
*** harlowja_away is now known as harlowja03:06
*** daneyon has quit IRC03:37
*** daneyon has joined #openstack-keystone03:38
*** morganfainberg_Z is now known as morganfainberg03:52
ayoungmorganfainberg, I think that the revoke tree algorithm is broken.  I think that one "dailure to match" which causes short circuit logic can mask other paths that would match the tokne (and thus show it is revoked)03:58
ayoungI'm headed to bed now, but wanted to share before I did03:59
morganfainbergayoung the return out because the filter list is empty?03:59
ayoungthat is way to aggressive03:59
morganfainbergayoung, i'll take a closer look at it, but yeah03:59
ayoungthere are unpruned paths still in the tree.03:59
morganfainbergi think we need to build the whole tree then do the match03:59
ayoungI think the logic is something like this03:59
morganfainbergvs. short circuit03:59
ayoungrevoke a role03:59
ayoungthen delete a user04:00
ayoungtoken for the user with a different role is still considered valid04:00
ayoungthe deliberate match path gets taken first04:00
ayoungfor the role_id match04:00
ayoungand once it fails out, none of the wildcard paths are taken04:01
ayoungI've seen problems on both "delete user" and "deleting_project" which show this behavior04:01
ayoungtest_revoking role seems to be even more succeptable.04:02
ayoungI'm going to post my code WIP04:02
ayoungmorganfainberg, https://review.openstack.org/#/c/81166/  its a draft04:03
ayoungto see the behavior, run a server with revocation events enabled, and then run examples/scripts/generate_revocation_events.py04:04
morganfainbergi'll take a look04:04
ayoungmorganfainberg, its not "build the whole tree"  but rather "search the whole tree"04:04
ayoungmorganfainberg, I did the following04:04
ayoungput a breakpoint right before test_deleting_user04:05
ayoungthen ran from the cli04:05
ayoungmysql keystone -u keystone -e "delete from revocation_event;"04:05
ayoungand the test passed04:05
ayoungsame thing for test_deleting_project04:05
ayoungthe middle test (delete role) still failed04:05
ayoungso it is definitely masked events04:06
ayoungok, gnight04:06
*** ayoung is now known as ayoung__zzzZZZZ04:06
ayoung__zzzZZZZmorganfainberg, I think what is missing is that it should not be a return, it should be "remove this path from the bundle."  and only if the bundle is empty, with no valid paths to pursue, should be exit out04:11
ayoung__zzzZZZZok, really to bed now04:12
morganfainbergayoung__zzzZZZZ, lol nighut04:12
*** chandan_kumar has joined #openstack-keystone04:31
*** derek_c has quit IRC04:47
*** stevemar has quit IRC05:07
*** derek_c has joined #openstack-keystone05:10
*** gyee has quit IRC05:11
*** gokrokve has quit IRC05:26
*** marcoemorais has joined #openstack-keystone05:27
*** marcoemorais1 has joined #openstack-keystone05:29
*** marcoemorais has quit IRC05:32
*** harlowja is now known as harlowja_away05:40
*** derek_c has quit IRC05:56
*** gokrokve has joined #openstack-keystone05:57
*** gokrokve_ has joined #openstack-keystone06:03
openstackgerritJenkins proposed a change to openstack/keystone: Imported Translations from Transifex  https://review.openstack.org/7852506:05
*** gokrokve has quit IRC06:06
*** gokrokve_ has quit IRC06:08
openstackgerritwanghong proposed a change to openstack/keystone: remove redundant code in catalog/core.py  https://review.openstack.org/8118106:21
*** derek_c has joined #openstack-keystone06:25
openstackgerritwanghong proposed a change to openstack/keystone: support conventional domain name with one or more dot  https://review.openstack.org/7982906:27
*** gokrokve has joined #openstack-keystone06:29
*** gokrokve has quit IRC06:33
openstackgerritA change was merged to openstack/keystone: trust creation allowed with empty roles list  https://review.openstack.org/6916206:42
*** saju_m has joined #openstack-keystone06:44
*** saju_m has quit IRC06:45
*** saju_m has joined #openstack-keystone06:47
openstackgerritA change was merged to openstack/keystone: Provide option to make domain_id immutable  https://review.openstack.org/8076907:10
*** gokrokve has joined #openstack-keystone07:29
*** gokrokve has quit IRC07:34
*** derek_c has quit IRC07:35
openstackgerritMorgan Fainberg proposed a change to openstack/keystone: Uses explicit imports for _  https://review.openstack.org/5876608:24
*** gokrokve has joined #openstack-keystone08:30
*** morganfainberg is now known as morganfainberg_Z08:34
*** gokrokve has quit IRC08:35
*** saju_m has quit IRC08:50
openstackgerritJenkins proposed a change to openstack/python-keystoneclient: Updated from global requirements  https://review.openstack.org/7969508:51
*** henrynash has joined #openstack-keystone08:52
*** saju_m has joined #openstack-keystone08:56
*** andreaf has joined #openstack-keystone09:02
*** leseb has joined #openstack-keystone09:02
*** leseb has quit IRC09:16
*** leseb has joined #openstack-keystone09:17
*** leseb has quit IRC09:21
*** andreaf has quit IRC09:23
*** leseb has joined #openstack-keystone09:24
*** gokrokve has joined #openstack-keystone09:31
*** gokrokve has quit IRC09:35
*** marcoemorais1 has quit IRC09:53
*** cflmarques has joined #openstack-keystone09:54
cflmarquesHi guys.Does Keystone use oslo to send notifications events? "tenant creation/deleted...09:54
cflmarquesI manage to retrieve nova events, but I also needed keystone events. does it send notifications or it must be enabled on the conf file?09:54
cflmarquescan anyone enlighten me on this matter?09:55
chmouelcflmarques: just a quick google search leaded me to this http://docs.openstack.org/developer/keystone/event_notifications.html10:05
*** andreaf has joined #openstack-keystone10:06
cflmarqueschmouel:hi chomouel. Thank you for your reply. Does keystone send that notifications over RabbitMQ like nova?10:10
chmouelcflmarques:  it uses oslo.notify AFAIK from https://blueprints.launchpad.net/keystone/+spec/notifications10:11
cflmarqueschmouel: Thanks again chmouel.  I will dig a little more. :)10:21
openstackgerritDolph Mathews proposed a change to openstack/python-keystoneclient: Docs link to middlewarearchitecture  https://review.openstack.org/7061110:30
openstackgerritDolph Mathews proposed a change to openstack/python-keystoneclient: Docs link to middlewarearchitecture  https://review.openstack.org/7061110:30
*** bvandenh has joined #openstack-keystone10:31
*** gokrokve has joined #openstack-keystone10:32
*** gokrokve has quit IRC10:36
*** cflmarques has quit IRC11:12
*** rwsu has quit IRC11:13
*** rwsu has joined #openstack-keystone11:14
openstackgerritDolph Mathews proposed a change to openstack/keystone: Ability to turn off ldap referral chasing  https://review.openstack.org/7852111:20
openstackgerritDolph Mathews proposed a change to openstack/keystone: Ability to turn off ldap referral chasing  https://review.openstack.org/7852111:21
openstackgerritYuriy Taraday proposed a change to openstack/keystone: Fix is_revoked algorithm and add some comments to it  https://review.openstack.org/8123511:27
*** gokrokve has joined #openstack-keystone11:32
*** gokrokve has quit IRC11:37
*** leseb has quit IRC11:39
*** leseb has joined #openstack-keystone11:40
*** leseb_ has joined #openstack-keystone11:41
*** leseb has quit IRC11:45
*** david-lyle has quit IRC11:46
openstackgerritDolph Mathews proposed a change to openstack/keystone: Use oslo db.sqlalchemy.session.EngineFacade.from_config  https://review.openstack.org/7845911:48
*** cflmarques has joined #openstack-keystone11:50
cflmarqueschmouel: Hi chmouel again. I manage to get keystone events already :) Just to let you you know. Thank's11:52
dstanekthis title 'malformed endpoint URLs are destroying the API' attracked by eye on https://bugs.launchpad.net/keystone/+bug/123027911:59
dolphmdstanek: i love that title12:13
dolphmit's like the fox news of bug reports12:16
dimsLOL :)12:18
*** thiagop_ is now known as thiagop12:21
dstanekdolphm: that problem would appear to exist in v2 and v3, but the original fix was only for v312:21
dstanekdolphm: blah, scratch that. i'm not typing i'm thinking12:22
dstanekit does exist in v2 and v3, but only fixed in the sql backend - it would appear to exist in the templated backend as well12:23
dolphmdstanek: the abandoned fix?12:24
*** dims has quit IRC12:31
*** gokrokve has joined #openstack-keystone12:33
*** dims has joined #openstack-keystone12:33
*** flaper87|afk is now known as flaper8712:35
*** gokrokve has quit IRC12:38
*** cflmarques has quit IRC12:49
*** ayoung__zzzZZZZ is now known as ayoung13:02
ayoungopenstack-dev has been a wasteland since we abandoned it13:03
*** pcargnel has joined #openstack-keystone13:06
*** flaper87 is now known as flaper87|afk13:10
*** browne has joined #openstack-keystone13:15
*** flaper87|afk is now known as flaper8713:19
openstackgerritIlya Pekelny proposed a change to openstack/keystone: Sync test_migrations  https://review.openstack.org/8061813:21
openstackgerritIlya Pekelny proposed a change to openstack/keystone: Comparision of database models and migrations.  https://review.openstack.org/8063013:21
*** bknudson has joined #openstack-keystone13:31
*** gokrokve has joined #openstack-keystone13:34
*** gokrokve has quit IRC13:38
*** leseb_ has quit IRC13:51
*** leseb has joined #openstack-keystone13:51
*** nkinder has quit IRC13:52
*** gokrokve has joined #openstack-keystone13:52
*** zhiyan is now known as zhiyan_13:58
*** marekd has quit IRC13:59
*** marekd has joined #openstack-keystone14:01
*** leseb has quit IRC14:04
*** leseb has joined #openstack-keystone14:04
openstackgerritBrant Knudson proposed a change to openstack/keystone: Configurable token hash algorithm  https://review.openstack.org/8040114:05
*** lbragstad has quit IRC14:17
*** stevemar has joined #openstack-keystone14:17
*** lbragstad has joined #openstack-keystone14:17
*** wchrisj has joined #openstack-keystone14:21
*** flaper87 is now known as flaper87|afk14:25
*** derek_c has joined #openstack-keystone14:31
openstackgerritA change was merged to openstack/python-keystoneclient: Docs link to middlewarearchitecture  https://review.openstack.org/7061114:35
openstackgerritA change was merged to openstack/keystone: Removal of test .conf files  https://review.openstack.org/7952514:35
*** david-lyle has joined #openstack-keystone14:36
*** nkinder has joined #openstack-keystone14:39
openstackgerritMarek Denis proposed a change to openstack/keystone: Store groups ids objects list in the OS-FEDERATION object.  https://review.openstack.org/8127714:41
*** thedodd has joined #openstack-keystone14:41
openstackgerritIlya Pekelny proposed a change to openstack/keystone: Sync test_migrations  https://review.openstack.org/8061814:48
openstackgerritIlya Pekelny proposed a change to openstack/keystone: Comparision of database models and migrations.  https://review.openstack.org/8063014:48
dstanekdolphm: the python-memcached client library appear to be using a thread local for the Client14:56
*** raildo has joined #openstack-keystone14:58
openstackgerritDolph Mathews proposed a change to openstack/identity-api: merge OS-FEDERATION objects together  https://review.openstack.org/8102214:59
*** chandan_kumar has quit IRC15:02
*** chandan_kumar has joined #openstack-keystone15:04
*** saju_m has quit IRC15:23
*** richm has joined #openstack-keystone15:25
*** henrynash has quit IRC15:25
*** derek_c has quit IRC15:30
*** vhoward has joined #openstack-keystone15:31
*** pcargnel has quit IRC15:33
*** gokrokve_ has joined #openstack-keystone15:34
*** devlaps has joined #openstack-keystone15:37
openstackgerritFlorent Flament proposed a change to openstack/python-keystoneclient: Allow applications using keystoneclient to close connexions.  https://review.openstack.org/8129015:37
*** gokrokve has quit IRC15:37
*** gyee has joined #openstack-keystone15:38
*** saju_m has joined #openstack-keystone15:41
openstackgerritFlorent Flament proposed a change to openstack/python-keystoneclient: Allow applications using keystoneclient to close connexions.  https://review.openstack.org/8129015:41
richmhello - question about test_backend.py and the use of user_sna - where is user_sna defined?  git grep shows several places where user_sna is referenced, but not defined15:52
YorikSarayoung: Hi15:52
ayoungYorikSar, Hey15:52
YorikSarayoung: I've posted a fix for that is_revoked method15:52
ayoungYorikSar, what was the flaw you found?15:53
YorikSarayoung: Please take a look whenever you have some time.15:53
ayounglooking.  very excited15:53
YorikSarayoung: I didn't actually read backlog here...15:53
bknudsonrichm: http://git.openstack.org/cgit/openstack/keystone/tree/keystone/tests/default_fixtures.py#n7715:53
ayoungYorikSar, add me to the review, please...makes it much easier to find15:53
dolphmrichm: probably in a fixture as user name="sna"15:53
YorikSarayoung: It was the one I've posted in comment to your original change request.15:53
dolphmrichm: they're dynamically assigned as attributes of the test case during setUp15:54
YorikSarayoung: Added15:54
YorikSarhttps://review.openstack.org/81235 - my fix15:54
YorikSarhttps://review.openstack.org/55908 - where I've commented the day before (or so)15:55
*** daneyon has joined #openstack-keystone15:55
richmah - ok - I see - tests/core.py:510 - the setattr15:55
richmok - thanks15:55
YorikSarayoung: I was very wrong to abandon reviewing your CRs... And I've missed a really simple issue with the code that you posted after my load of comments.15:55
YorikSarayoung: The problem was with indentation15:56
*** rupsky has joined #openstack-keystone15:56
ayoungYorikSar, can we drop the dor...else semantics? I find it kindof hard top grok, and most of the other reviewer have, too15:56
YorikSarayoung: It's actually makes code cleaner...15:57
ayoungYorikSar, yeah, I realize that the "return" was happening at the wrong point15:57
*** saju_m has quit IRC15:57
YorikSarayoung: But I've added some comments, so it should be fine now :)15:57
ayoungyou need to clear the bundle before you return, but it waws only looking at the local set of branches15:57
ayoungyeah, its a novel15:57
*** rupsky has quit IRC15:58
ayoungif not bundle:15:58
ayoung199                            return True212                # Note that else: branch will not happen after this.15:58
ayoung213                break15:58
ayoungYorikSar, I had gotten that far last night15:58
ayoungYorikSar, I am going to add you to a draft client review15:59
ayoungI'm going to give your code a run, now16:00
*** daneyon has quit IRC16:01
ayoungah...I need to port your new code over to that review, too, though YorikSar as it does the same logic....16:01
*** daneyon has joined #openstack-keystone16:02
YorikSarayoung: Oh... To which one?16:04
ayoungYorikSar, client code16:04
ayounghttps://review.openstack.org/#/c/81166/  YorikSar16:04
YorikSarayoung: Wow.. There's revoke tree there too?..16:04
ayoungYorikSar, yeah, it will eventually replace the server code16:04
YorikSarayoung: Maybe we should isolate it in client and use the code in client from server?16:05
ayoungsince the logic needs to be executed the same in client and server, we want it in one and only one place16:05
ayoungso the goal is to get the client review done, then change the server to use the client code16:05
ayoungYorikSar, dead on accurate :)16:05
YorikSarayoung: Great :)16:06
ayoungYorikSar, OK...so there is still something wrong logic wise...I'll show you how to reproduce in a second, once I'posted the updated client code16:07
ayoungYorikSar, OK,  grab the client patch: https://review.openstack.org/#/c/81166/  and run your keystone server16:08
openstackgerritFlorent Flament proposed a change to openstack/python-keystoneclient: Allow applications using keystoneclient to close connexions.  https://review.openstack.org/8129016:08
ayoungyou need to make sure you have the revoke_api in the pipeline:16:08
ayoungpipeline = sizelimit url_normalize build_auth_context token_auth admin_token_auth xml_body json_body ec2_extension_v3 s3_extension simple_cert_extension revoke_extension service_v316:08
ayoungand you can do a simple test using CURL to confirm that16:09
ayoung curl http://localhost:35357/v3/OS-REVOKE/events16:09
ayoungonce that is done,  look in the client patch:   examples/scripts/generate_revocation_events.py16:09
ayoungyou need to chantge the password on the ADMIN_TOKEN16:10
ayoungso that it matches yours, but once you get it  running you should see output like this16:10
ayoungYorikSar, http://paste.openstack.org/show/73771/16:10
openstackgerritFlorent Flament proposed a change to openstack/python-keystoneclient: Allow applications using keystoneclient to close connexions.  https://review.openstack.org/8129016:10
*** wchrisj has quit IRC16:11
*** marcoemorais has joined #openstack-keystone16:11
*** wchrisj_ has joined #openstack-keystone16:13
dstanekdolphm: i think your pooling impl is fine - i was trying to find out why we actually need to, but i'm not having any luck16:13
dstanekdolphm: so i think maybe we just go with it16:13
*** marcoemorais has quit IRC16:15
*** henrynash has joined #openstack-keystone16:15
*** marcoemorais has joined #openstack-keystone16:15
*** marcoemorais has quit IRC16:15
YorikSarayoung: Crap... git review -d is broken...16:15
*** marcoemorais has joined #openstack-keystone16:16
ayoungYorikSar, probably nort16:18
ayoungYorikSar, you need to be in the right projct16:18
ayoungpooling impl?  dstanek link?16:18
YorikSarayoung: It tried to fetch from review.oprnstack.org16:18
ayoungYorikSar, probably a misconfig on you machine16:19
ayounglook in .git/config16:19
YorikSarayoung: Nah... .gitreview is fine and it used to work earlier...16:19
ayoung[remote "gerrit"]16:19
ayoung        url = ssh://ayoung@review.openstack.org:29418/openstack/python-keystoneclient.git16:19
ayoung        fetch = +refs/heads/*:refs/remotes/gerrit/*16:19
dstanekhttps://review.openstack.org/#/c/81078/2 <- ayoung16:19
ayoungdstanek, thanks16:19
YorikSarayoung: Oh, wow.16:20
YorikSarayoung: Yeah. I never used it in keystoneclient, I guess :)16:20
ayoungYorikSar, suspect that will be changing...16:20
YorikSarayoung: My magic git-my-clone script was very young the day I've cloned it and it installed bad push-url for gerrit remote16:21
ayoungYorikSar, let me know when you get the client script to run16:22
YorikSarayoung: I can't find admin_token in examples/scripts/generate_revocation_events.py16:23
ayoungI want to take that logic and make it into a unit test of some sort16:23
ayoungYorikSar, heh16:23
ayoungits not in there16:23
YorikSarayoung: Will it use the default one ... by default?16:23
ayoungits in the other scripts in that dir16:23
ayoungYorikSar, those are supposed to be example scripts, so I didn't make it very Object oriented16:24
YorikSarayoung: Oh, you and FreeIPA :)16:24
ayounginstead I subprocessed the setup script16:24
ayoungYorikSar, I want to pull the value out of /etc/keystone/keystone.conf eventually, but not certain if I should pull in Oslo config to do that16:25
*** derek_c has joined #openstack-keystone16:25
YorikSarayoung: It won't help me because I'm running from .tox/py27/bin/keystone-all :)16:26
ayoungYorikSar, there is that, too16:26
ayoungalthough, strangly, it still picks up  keystone-paste.api from /etc on my system16:27
YorikSarayoung: I remember some time around Essex there was such problem in Nova...16:28
ayoungYorikSar, let me confirm that the test in the middle is valid16:29
richmdolphm: ping - re: your review comments for bug/1282676 - you suggested mutating the object to test rather than have less strict checking - so something like this?  http://fpaste.org/86414/16:29
ayoungline 102: test_revoking_role16:30
*** flaper87|afk is now known as flaper8716:30
ayoungI guess if the user never had that  role...the token would still be valid.  OK, let me assert that16:31
YorikSarayoung: Will Keystone generate cert for itself if there is no cert file?16:33
ayoungNah, run pki_setup16:33
ayoungkeystone-manage pki_setu16:33
*** derek_c has quit IRC16:33
ayoungkeystone-manage pki_setup16:33
*** chandankumar_ has joined #openstack-keystone16:34
*** chandan_kumar has quit IRC16:37
*** pete5 has quit IRC16:38
YorikSarayoung: Ok... It claims smth is not setup properly...16:38
YorikSarayoung: Do I need some config for client?16:38
ayoungkeystone or the client?16:38
YorikSarayoung: Like... Where to store certs?16:38
YorikSarkeystoneclient.exceptions.CertificateConfigError: Unable to load certificate. Ensure your system is configured properly.16:39
ayoungcerts go in /etc/keystone/ssl on my machine, so something comparable16:39
ayoungmight be a permissions issue.  I suspect my was origianlly setup by devstack and done as root16:39
YorikSarayoung: Finally it worked.16:41
YorikSarayoung: No, I just don't like apps I develop to to anything with my OS :)16:41
YorikSarayoung: So I've changed paths in the script.16:42
ayoungYorikSar, ++16:42
YorikSarayoung: But... Everythin's OK16:42
YorikSarayoung: I mean, all tests are16:42
ayoungcurl works16:42
ayoungah, the script runs for you, and runs clean?16:42
YorikSarayoung: Yeah16:42
ayounginteresting...not for me16:42
YorikSarayoung: It was with py27 env16:43
YorikSarayoung: Installint py33 one..16:43
ayoungooh, maybe I am running py33 there.16:43
YorikSarayoung: You are :)16:43
YorikSaraccording to that paste16:43
YorikSarayoung: Oh, that'll take a while... Didn't update wheelhouse for py33 for some time already...16:44
ayounglet me try running py2716:44
YorikSarayoung: Can't stop you :)16:44
ayoungYorikSar, and now it is failing consistantly for me16:48
YorikSarayoung: Looks like I'm first. py33 is OK here too16:48
YorikSarayoung: Looks like smth's wrong with your environment...16:49
YorikSarayoung: Maybe recreate venvs, DB and ssl dirs?16:49
ayoungYorikSar, lets test the daylights out of this code16:49
YorikSarayoung: test the daylighs out?16:50
*** fabiomorais has joined #openstack-keystone16:50
YorikSarayoung: Ok, dictionary is my friend :)16:51
ayoungYorikSar, yeah...you might want to look up the common meaning of the word you used which I replaced wiht "Bundle"16:52
*** harlowja_away is now known as harlowja16:53
YorikSarayoung: It's actually the first translation dictionary gave me. But it sounded familiar from some movies or series, so I checked up with urbandictionary. And decided to play "no know English" :)16:54
*** pcargnel has joined #openstack-keystone16:54
YorikSarayoung: It's just so strange that a word can switch meaning like that.16:54
ayoungAlas, poor word choice, YorikSar16:55
ayoungYorikSar, I think it was due to the Yiddish word http://en.wiktionary.org/wiki/feygele16:56
*** pcargnel has quit IRC16:57
YorikSarayoung: Wow... It's not very similar actually.16:57
ayoungYorikSar, it is to the short form in common usage16:58
YorikSarayoung: Oh, yes.16:58
YorikSarayoung: Anyway. I'll try not to name a list of validated and verified tokens "gay_tokens" in code :)16:59
*** pcargnel_ has joined #openstack-keystone17:00
ayoungYorikSar, that would be imprecise anyway, as they were only added due to potential17:00
ayoungnot confirmation17:00
dolphmrichm: not quite -- don't make it conditional/dynamic -- explicitly mutate the object so there's no magic going on in the tests17:01
*** morganfainberg_Z is now known as morganfainberg17:01
YorikSarayoung: Ok, I'll switch to rootwrap work for now... Let me know if you find smth interesting with fixed algorithm17:02
*** marcoemorais has quit IRC17:02
*** marcoemorais has joined #openstack-keystone17:02
richmdolphm: so something like - user_ref.pop('description') ?17:03
richmi.e. hardcoding the property which may have a value of None?17:03
* morganfainberg forgets about daylight savings...17:03
dolphmrichm: yes, exactly17:03
morganfainbergstupid caldendar that doesn't do UTC.17:04
richmsorry, wrong window17:04
richmdolphm: and I can assume, for the purposes of testing, that the list of properties will be fixed, so I can have a hard coded list like noneprops = ['description', 'enabled'] - and that the object being tested will only contain these attributes and no others?17:05
richmbecause the actual list of properties is dynamic and mutable17:06
dolphmrichm: if you can't make that assumption, then something is wrong with the tests...17:06
ayoungYorikSar, OK, I'm running your code.  I wiped the revocation table prior to the run.  The tree looks like this:17:11
ayoungwildcards for17:11
ayoungconsumer_id, access_token_id, expires_at17:12
ayoungat domain_id, I see two options:  wildcard  and an explicit domain17:12
ayoungnow, I just delete the user, so the revocation event should only have user_id in it17:13
ayoungwhich means it would match domain_id=* here17:13
ayoungjust hit a breakpoint where name = 'domain_id'17:13
ayoungbundle gets populated with two "none" entries, due to alternatives17:14
ayoungand one that looks like the "right path down"17:14
*** arborism has joined #openstack-keystone17:15
ayoungproject_id=*, user_id=eb5..., role_id=*17:16
ayoungnope..userid is wrong there...17:16
ayoungYorikSar, I notice that the debugger shows bundle=<filter object>17:16
ayoungis it possible python33 broke something there?17:17
*** arborism is now known as amcrn17:17
ayoungtree does seem to be extracted out of bundle...so it looks correct17:18
YorikSarayoung: Yes, that filter object is from py3317:18
YorikSarayoung: But it shouldn't break anything. Only probably speedup.17:19
ayoungjust wish I could see inside it17:19
YorikSarayoung: Do list(bundle)17:19
YorikSarayoung: It shouldn't break it... I guess.17:20
YorikSarayoung: Oh, no, it might...17:20
ayoungnah, it was OK17:21
ayoungit seemed to get down to the role_id level17:21
YorikSarayoung: That object is an iterator. So if you run list(bundle) once, second list(bundle) will give you []17:21
*** pcargnel_ has quit IRC17:21
ayounglemme try that again17:22
openstackgerritDavid Chadwick proposed a change to openstack/identity-api: Adding support for SAML federated authentication (Federation Pt3)  https://review.openstack.org/6241717:22
*** devlaps1 has joined #openstack-keystone17:23
ayoungforest is empty17:24
*** devlaps has quit IRC17:24
ayoungit should be matching on user_id17:24
YorikSarI don't quite follow you...17:25
*** leseb has quit IRC17:30
ayoungYorikSar, I'm debugging.  tes first test is failing...and it seems to be OK dowen to role_id17:30
ayoungI've stopped where name=role_id17:30
*** leseb has joined #openstack-keystone17:30
ayoungactually, one up17:30
ayoungbundle has two entries17:30
*** thedodd has quit IRC17:31
ayoungone is role_id=4bcc...  the other is role_id=*17:31
ayoungits the second one that should match17:31
YorikSarayoung: And what roles are in token?17:31
*** andreaf has quit IRC17:32
ayoungYorikSar, doesn't matter, the user has been deleted17:32
ayoungdoes have one17:32
ayoungstars with 5dd17:32
ayoungOK, so stepped until names=role_id17:32
ayoungand forest is empty17:33
ayoungjumped right over...17:33
ayoungI wonder if that is a side effect of debugging17:33
ayoungdid you think list(bundle) would 0 it out ?17:33
YorikSarayoung: So if you need it, you should do smth like bundle = list(bundle)17:34
YorikSarand then check it out17:34
*** leseb has quit IRC17:34
ayoungyeah, doing list(forest)  zeros it out.17:36
ayoungcan't do that...OK17:37
marekdstevemar: https://review.openstack.org/#/c/81277/1/keystone/tests/test_v3_federation.py the point in serializer.to_xml() was -> without the fix all the tests pass. So I felt I should add something that would actually check if not the xml content can be built...17:37
YorikSarayoung: btw, how do you run it in py33?17:37
ayoungYorikSar, I'm going to temporarily replace that code with17:37
ayoungYorikSar, I'm using pycharm17:37
marekdstevemar: plus, instead of doing one dedicated test for that I added it in one helper class, so more tests (using _issue_unscoped_token()) would also test this behaviour...17:37
*** morganfainberg is now known as morganfainberg_Z17:38
ayoungbundle = [item for item in bundle if item]17:38
ayoungjust so I can see it17:38
YorikSarayoung: You could do just bundle = list(filter(None, bundle)) :)17:39
ayoungOK, userid is now caa13a445e0a46edb731033d37279e52  and I have two roles entries in the bundle17:39
ayoungYorikSar, nope, we assign that to bundle17:39
ayoungnothing left to filter at the point I needed it17:39
ayoung'salright...logic should be the same...17:40
*** jamielennox is now known as jamielennox|away17:40
YorikSarI mean, just wrap that filter(...) in text with list(...). You'll get the same effect with less modifications.17:40
ayoungtree.get(wildcard)  == None for Roleid17:40
YorikSarBut that's not the point.17:40
ayoungah..but that is the other tree17:41
YorikSarHm... Do you have some combination of tree and token that doesn't work as expected?17:42
YorikSarCan you dump them to paste so that I can check it out?17:42
ayoungYorikSar, and...it just succeeded17:44
ayoungbut the last test failed17:45
YorikSarayoung: And it failed with filter()?..17:45
ayoungoooh...don't know if that was the difference17:45
YorikSarCan you try to run tests with list comprehension?17:45
ayoungYorikSar, run the tests a bunc of times and see if any of them fail for you?17:45
YorikSarayoung: yeah...17:46
ayoungYorikSar, I suspect it has to do with ABC order and the ids17:46
YorikSarayoung: Nothing fails on my setup.17:46
YorikSarayoung: Hm... I don't see anything order-related at all in the code...17:48
ayoungYorikSar, not every test fails every time.  try running it in a loop17:48
openstackgerritMarek Denis proposed a change to openstack/keystone: Filter SAML2 assertion parameters with certain prefix.  https://review.openstack.org/8094617:49
ayoungfor i in `seq 1 3` ; do python generate_revocation_events.py ; done17:49
dstanekthis is a bummer - https://bugs.launchpad.net/python-keystoneclient/+bug/128208917:50
YorikSarayoung: Did just that...17:50
YorikSarayoung: Nothing fails...17:50
stevemarmarekd, cool, didnt rwalize that17:51
*** gokrokve_ has quit IRC17:52
*** flaper87 is now known as flaper87|afk17:53
marekdstevemar: otherwise the 'good way' for new tests would be: rewrite all the test cases with XML :-)17:53
YorikSarayoung: Started this: while true; do ../../.tox/py33/bin/python generate_revocation_events.py; done | sed 's/FAIL/FAIL\a/'17:54
ayoungYorikSar, OK,  I'm going to step through one last time and see if I can figure out where the problem is.  I wonder if it has to do with the clock17:54
YorikSarayoung: Let's see if it bangs17:54
ayoungI'm going to be a sleep 1 in there17:54
YorikSarayoung: You can try to dump tree and token on every call to is_revoked17:55
ayoungYorikSar, yeah, that is true, too17:55
*** fabiog has joined #openstack-keystone17:55
ayoungmight be a good debugging tool17:56
ayoungYorikSar, yep17:57
ayoungsleep one made it work17:58
dolphmrichm: thanks! replied to your comment on this with a follow up question https://review.openstack.org/#/c/78521/4/keystone/common/ldap/core.py17:58
*** morganfainberg_Z is now known as morganfainberg17:58
morganfainbergdolphm, hopefully this one will pass now https://review.openstack.org/#/c/58766/17:58
morganfainbergdolphm, the trust with no roles patch introduced a new string and the trust controller didn't have use of _ before, so it failed gate.17:58
YorikSarayoung: Hm... I don't see any possibility for some race or smth like that there... Do you?17:58
ayoungYorikSar, I suspec that the delete was happening too soon to the token issuing, and some rounding error is getting in there17:59
ayoungYorikSar, maybe one with microsecond one without?17:59
*** gyee has quit IRC17:59
YorikSarayoung: Oh... Maybe.17:59
*** gyee has joined #openstack-keystone17:59
dolphmmorganfainberg: hrm, so new translated strings are now bad?18:00
*** marcoemorais has quit IRC18:00
ayoungYorikSar, keystone-meeting time18:00
ayoungcare to join #openstack-meeting?18:01
*** harlowja is now known as harlowja_away18:01
YorikSarayoung: Sure :)18:01
*** marcoemorais has joined #openstack-keystone18:01
*** harlowja_away is now known as harlowja18:02
*** shakamunyi has joined #openstack-keystone18:04
*** marcoemorais has quit IRC18:08
*** marcoemorais has joined #openstack-keystone18:09
*** jamielennox|away is now known as jamielennox18:09
*** shakamunyi has quit IRC18:18
*** shakamunyi has joined #openstack-keystone18:27
*** henrynash_ has joined #openstack-keystone18:45
*** henrynash has quit IRC18:48
*** henrynash_ is now known as henrynash18:48
*** shakamunyi has quit IRC18:49
openstackgerritayoung proposed a change to openstack/python-keystoneclient: Compressed Signature and Validation  https://review.openstack.org/7118118:50
*** fabiomorais has quit IRC18:52
*** gokrokve has joined #openstack-keystone18:56
*** fabiomorais has joined #openstack-keystone18:57
*** flaper87|afk is now known as flaper8718:59
*** fabiog has quit IRC19:01
*** marekd is now known as marekd|bbl19:01
morganfainbergok ayoung , stevemar , marekd , dolphm, i'm ok with making the only way we revoke IDP tokens being by way of revocation events, if we are comfortable claiming federation is as expirimental as revocation events19:01
dolphmmorganfainberg: i think they're both in the same boat as of icehouse19:02
ayoungmorganfainberg, it is as experimental, if not more so19:02
dolphmmorganfainberg: neither are proven19:02
morganfainbergdolphm, cool.19:02
morganfainbergdolphm, that was my only concern with it having a required dep on revocation events to close the security hole19:02
stevemarmakes sense to me19:02
morganfainbergdolphm, and that answer works 100% for me19:02
morganfainbergstevemar, ayoung, can we include a line in the documenation about this requirement? for Icehouse release?19:03
marekd|bblmorganfainberg: federation is not yet fully usable so it can be claimed as 'experimental'19:03
*** marekd|bbl is now known as marekd19:03
morganfainbergmarekd|bbl, great. just wanted us to be all on that same page19:03
jamielennoxalso, i realize everyone's busy - i currently have 17 open (non-WIP) reviews for keystoneclient without votes on them - if the problem is understanding them let me know as i'm around now19:03
morganfainbergall my concerns with this approach are resolved.19:03
morganfainbergjamielennox, i've started going through them. it's slow but i'm working on it19:04
jamielennoxmorganfainberg: appreciated, thanks19:04
morganfainbergmarekd, ^ on the documentation for how revoking tokens w/ IDPs.19:04
gyeejamielennox, question on the v3 password auth plugin19:04
jamielennoxgyee: shoot19:04
morganfainberglunch time, will fix keystone error then back to keystoneclient reviews.19:04
gyeefor passwordCredentials, we don't see to support userId19:04
marekdmorganfainberg: hm?19:04
gyeejust username and password19:04
gyeethe protocol does support userId and password it seems19:05
ayoungYorikSar, so..what kind of propblem is the corrected algorithm fixing, and how do we right a test for it?19:05
jamielennoxgyee: for v3?19:05
marekddolphm: https://review.openstack.org/#/c/81277/1/keystone/tests/test_v3_federation.py does 'lazy' here mean 'bad, should not be done this way'? :-)19:06
YorikSarayoung: I'm formulating it in the bug...19:06
gyeejamielennox, for v219:06
gyeejamielennox, https://github.com/openstack/keystone/blob/master/keystone/token/controllers.py#L24319:06
YorikSarayoung: And then I can synthesise test for you19:06
*** pcargnel has joined #openstack-keystone19:06
dolphmmarekd: lazy means you could write an explicit test in the other module19:08
dolphmmarekd: and it would do fairly robust two way json -> xml -> json and xml -> json -> xml transformation testing19:09
YorikSarayoung: smth like this: https://bugs.launchpad.net/keystone/+bug/129429219:09
jamielennoxgyee: yea, you appear to be right - do you know if that was originally supported by a v2 client and i removed it or was never supported?19:09
dolphmYorikSar: can you describe the real world impact on that?19:09
YorikSarayoung: I'll go now and add a line to commit message and some test19:10
dolphmYorikSar: bugs that contain nothing but implementation details aren't very useful to read later on19:10
YorikSardolphm: This bug is really very implementation-related19:10
marekddolphm: allrighty, will add it.19:10
YorikSardolphm: I can add an example of failing check19:10
gyeejamielennox, seem like we never supported it on the client side19:11
jamielennoxgyee: yep, that's what i'm seeing: https://github.com/openstack/python-keystoneclient/blob/0.6.0/keystoneclient/v2_0/client.py#L18819:11
gyeek, I'll submit a patch to fix that19:12
gyeeshould be a trivial one19:12
jamielennoxgyee: ++19:13
ayoungYorikSar, add a test for that in test_v3_auth,19:13
YorikSarayoung: I though about adding a test in test_revoke...19:13
YorikSarayoung: Isn't it the right place?19:14
ayoungYorikSar, if you can get it in test_revoke, do so19:14
ayoungI was working with existing tokens, and it is pretty clear how to translate that to the auth API19:14
ayoungbut, yeah, it should be do-able in test_revoke19:15
thiagopGuys, quick question: the python-keystoneclient can already authenticate a user in a domain other then 'default' or I can do that only via REST API?19:17
*** marekd is now known as marekd|away19:17
gyeethiagop, for V3 yes19:17
dolphmmorganfainberg: make it go https://review.openstack.org/#/c/58766/19:18
jamielennoxthiagop: by keystoneclient do you mean the CLI or the library?19:18
*** saju_m has joined #openstack-keystone19:18
thiagopjamielennox: the library (used by Horizon, isn't it?)19:19
jamielennoxthiagop: should be yea, yes you can give a domain_id or a domain_name to the client to scope to a different domain19:19
jamielennoxthiagop: ah sorry, user_domain_id or user_domain_name to specify the domain the user belongs to19:20
gyeethiagop, may want to check with david-lyle, I think horizon is working on v3 support19:20
david-lylethiagop, Horizon has support for authenticating to domains other than the default domain, problem is once you do, you can't do anything19:22
*** gyee has quit IRC19:26
*** daneyon has quit IRC19:32
dstanekjamielennox, dolphm: what do you guys think about this issue? https://bugs.launchpad.net/python-keystoneclient/+bug/1282089/comments/419:34
*** Nathan255 has joined #openstack-keystone19:35
bknudsondstanek: would having a close() function on the client fix it?19:37
jamielennoxdstanek: what's the circular ref you refer to?19:38
dstanekbknudson: only if the close is actually killing the references19:39
jamielennoxi think the problem is the assumption of making many httpclients, i am hoping to push people to make 1 session object and then make  multiple clients based on that session19:39
bknudsonI assume the client creates a bunch of managers that refer back to it?19:39
jamielennoxthis is a problem in horizons case where we are juggling a lot of different auths though19:39
dstanekjamielennox: http://git.openstack.org/cgit/openstack/python-keystoneclient/tree/keystoneclient/v3/client.py#n95 - the client hold only the things it creates and passes in a reference to self19:40
dolphmNathan255: o/19:40
bknudsondstanek: close() could delete all the managers?19:41
jamielennoxdstanek: oh, yuk - never thought of that, that manager code has been around forever19:42
dstanekbknudson: yes, it definitely could and probably should19:42
jamielennoxi think this must be a problem for all the clients?19:43
jamielennoxor maybe having httpclient and client seperate objects breaks that19:43
dstanekbknudson: but that still leaves a developer with having to rely on explicitly closing - a __del__ impl may also be nice to have here19:43
jamielennoxdstanek: right, a close() then makes the client object unusable19:44
jamielennoxin which case you should just del client19:44
dstanekjamielennox: from my 30 seconds of looking it seemed like the oslo client stuff encourages this sort of design19:44
bknudsondstanek: then they'd have to del it?19:44
bknudsondstanek: why do the managers need the self ref?19:45
bknudsoncould they instead be passed a function?19:45
dstanekbknudson: no, i would do that in addition to close19:45
jamielennoxbknudson: i'm not advocating it - but if they have to .close() it it would be almost the same19:45
dstanekbknudson: no idea, i've just been walking through bugs the last few days19:45
jamielennoxcan we just weakref the passing of client to manager?19:46
*** david-lyle has quit IRC19:46
dstanekthe __del__ should be called automatically when the object goes out of scope; no need to explicitly call it19:47
openstackgerritYuriy Taraday proposed a change to openstack/keystone: Fix is_revoked algorithm and add some comments to it  https://review.openstack.org/8123519:47
dstanekthe challenge is that sometimes the things you want to reference in a __del__ have already been removed, but in our case i don't think that can be true because of the circular refs19:47
jamielennoxdstanek: but would the weakrefs work? i think that would be relatively simple to do?19:48
YorikSarayoung, dolphm: Added test to commit, link to bug to commit message and an example to bug description.19:49
ayoungYorikSar, thanks19:49
bknudsonweakrefs seem like not the thing to use in this situation19:49
YorikSarThe example is a bit synthetic though...19:49
dstanekjamielennox: i tried hacking kc.openstack.client.base (or something like that) and i didn't work19:50
dstanekjamielennox: i didn't follow all of the references around; i just wanted enough info to know if the proposed fix is a good one19:50
jamielennoxbknudson: oh? this would be exactly my definition of where weakrefs should be used19:51
jamielennoxdstanek: there are two different issues i think, session closing and client cleanup19:51
jamielennoxobviously they relate because if the client cleaned up correctly then you wouldn't need to close the session19:51
jamielennoxbut the close() i see very much relating to the HTTP stuff rather than the cleaning up of managers etc19:52
jamielennoxdstanek: give me today to have a look at fixing client cleanup and we can see if it helps with the HTTP closing?19:52
bknudsonmaybe weakrefs are used differently in python...19:53
dstanekjamielennox: sounds good to me :-)19:53
bknudsonevery time you use a weakref you need to check if it's still valid?19:53
dstanekbknudson: in this case i don't think you need to because if it's invalid then the managers can't be called anyway19:53
jamielennoxbknudson: yes19:54
jamielennoxbknudson: but as the standard way to call a manager is client.users.list() i don't think that's a problem19:54
jamielennoxwe would just have an error if someone grabs users = client.users and lets client fall out of scope19:54
bknudsonjamielennox: I agree that would be a pretty wacky way to use the api19:55
bknudsonI'm still wondering what the manager uses that's in the Client?19:56
bknudsoncould it be passed in instead19:56
jamielennoxbknudson: i'm not sure if it was done initially for testing or whatever but all the actual request logic is done via the client19:57
jamielennoxbknudson: so the manager develops the request data and asks the client to deliver it19:58
dstanekbknudson: this is where i tracked to to http://git.openstack.org/cgit/openstack/python-keystoneclient/tree/keystoneclient/openstack/common/apiclient/base.py#n46819:58
dstanekthere are probably other places those references and store for later use19:58
jamielennoxdstanek: erghh... are we using apiclient for this?19:58
bknudsonwould have to be fixed in oslo19:59
dstanekjamielennox: yes - http://git.openstack.org/cgit/openstack/python-keystoneclient/tree/keystoneclient/base.py#n38819:59
jamielennoxdstanek: oh, that's resource20:00
jamielennoxwhich has a dep on manager - which has a dep on client20:00
jamielennoxthat's a problem20:00
dstanekjamielennox: lol -> self.manager.delete(self)20:01
dstaneki do not want to kill me...you do it!20:01
jamielennoxbecause whilst people might not use the managers directly it means a eg User resource would hold a reference to the client20:01
jamielennoxso that you couldn't do user.update(x=x) if the client fell out of scope20:01
jamielennoxdstanek: welcome to the client20:02
ayoungYorikSar, i suspect it just as efficient, and clearer, to drop tjhe filter and to not append None valuies to the bundle20:02
*** marcoemorais has quit IRC20:02
*** marcoemorais has joined #openstack-keystone20:02
jamielennoxdstanek: i'm not sure how we fix it either, i've been working on auth and leaving that stuff alone - but i'm not a fan of that oslo apiclient stuff reenforcing all this20:03
*** marcoemorais has quit IRC20:03
*** marcoemorais has joined #openstack-keystone20:03
ayoungjamielennox, and *that* is why I submitted the patch to rollback the API client in oslo and pissedo the Oslo deities20:04
jamielennoxayoung: i remember, i +1ed it20:04
dstanekjamielennox: usually this means that the abstraction is wrong or reversed - i need to think about it a little more before i know for sure20:04
ayoungdstanek, I have a chunk of code that does  bundle.append(tree.get('role_id=%s' % role_id))  and then later20:05
ayoungbundle = filter(None, bundle)20:05
ayoungis there a neater way to say "append only if not None"20:05
jamielennoxdstanek: i would love an outside opinion, i've been messing with this stuff for so long that i've gotten used to it's stupidity20:06
morganfainbergayoung, if not none, append?20:06
ayoungmorganfainberg, then I need a temp variable20:06
dstanekmorganfainberg, ayoung: yeah, but you'll avoid the extra loop20:06
ayounga = get(x) ;  if a is not None, bundle.append(a)20:06
morganfainbergayoung, that seems like a low cost for readability20:06
morganfainbergayoung, i'd go for readability in that code20:06
jamielennoxdstanek: ideally we would wrap the session object up with some client specific context and pass that to all the managers20:06
jamielennoxthat wouldn't be too hard i think20:07
ayoungmorganfainberg, Oh, I agree, just wonder if there is a some sugar for the syntax20:07
morganfainbergayoung, use a while loop! :P20:07
morganfainbergayoung, nah, i think it's cleanest to bind to a local var and explicitly if-check20:07
dstanekayoung: if you want moar code you can turn it into a generator function20:07
jamielennoxayoung: no there isn't that i've seen unless you start doing yields20:08
morganfainbergdstanek, yeild!20:08
dstanekmorganfainberg: standing down20:08
morganfainbergdstanek, LOL :P20:08
morganfainbergdstanek, actually a generator isn't a bad idea. though i think it might be overkill20:08
jamielennoxmorganfainberg: i've always felt they are a good approach you just have to wrap them up into such a little package that it just becomes easier to do it inline20:09
dolphmYorikSar: thanks!20:09
jamielennox.... or recursively20:09
morganfainbergjamielennox, sure.20:09
openstackgerritFlorent Flament proposed a change to openstack/python-keystoneclient: Allow applications using keystoneclient to close connexions.  https://review.openstack.org/8129020:09
YorikSarayoung: Nope... It'll add a lot of if's in the code. It'll become messy with no benefit.20:10
ayoungYorikSar, disagree20:10
ayoungYorikSar, I'll post20:10
jamielennoxayoung: i also think that if you are doing a lot of var = dict.get(x) if var: then you should be doing try: dict[x] except KeyError20:11
jamielennoxbut i realize that's a preference20:12
ayoungjamielennox, yep...20:12
ayoungthat might be preferable here20:12
dstanekjamielennox: ^ that commit is tied to the bug we were just discussing20:13
ayoungYorikSar, it would end up as20:13
ayoung                try:20:13
ayoung                    bundle.append(tree[wildcard])20:13
ayoung                except KeyError:20:13
ayoung                    pass20:13
ayoungand then drop the filter20:13
ayoungmore lines of code...but clearer20:14
YorikSarayoung: It's exception-driven control flow. It's bad. And probably less effecient than filter20:14
morganfainbergoh if you keep the filter, make sure you use six.moves ayoung20:14
YorikSarI can't see how it's clearer.20:14
ayoungYorikSar, its python, we gave up on efficiency when we picked the language20:15
YorikSarmorganfainberg: No, in this case py3k filter(..) is even better for us.20:15
morganfainbergthen we need a comment20:15
YorikSarayoung: But you suggest add some mess to the code, make it longer and harder to read. For what benefit?20:15
morganfainbergjust saying it's intended to be filter() vs. six.moves20:15
dolphmmorganfainberg: what happened to filter in py3?20:16
ayoungYorikSar, nah, I'll pull out into a hlepr function20:16
morganfainbergdolphm, iterator changes20:16
YorikSardolphm: It returns magic iterator wrapper20:16
*** leseb has joined #openstack-keystone20:16
YorikSarayoung: Again... For what benefit?..20:16
bknudsonhttps://pythonhosted.org/six/#module-six.moves --   filter itertools.ifilter() filter()20:16
ayoungYorikSar, don't get too attached to the beuty of your code.  Kill your darlings!20:16
morganfainbergYorikSar, i disagree, filter is almost never easier to read especially with the density that is this code20:17
YorikSarayoung: Function calls are expensive, btw...20:17
morganfainbergor there needs to be some comments explaining the expected behavior20:17
ayoungYorikSar, heh20:17
ayoungYorikSar, premature optimization is the root of all evil20:17
YorikSarmorganfainberg: https://review.openstack.org/#/c/81235/2/keystone/contrib/revoke/model.py20:17
YorikSarmorganfainberg: Do you need more comments? :)20:17
*** david-lyle has joined #openstack-keystone20:18
morganfainbergYorikSar, that is so much better :)20:18
morganfainbergYorikSar, Thank you :)20:18
YorikSarayoung: Well... It's you optimizing smth here, not me ;)20:18
ayoungmorganfainberg, agreed20:19
ayoungYorikSar, optimizing for readability.20:19
morganfainbergYorikSar, can i add one other complaint, what is the benefit of the for/else syntax here? besides convience, if you remove the else and backdent one it is easier to read and _appears_ to do the same thing, since you're using an explicit return (break out of function) rather than a break of the loop and continuation20:19
dolphmconversations concerning optimization aren't usually productive without benchmarks... so you actually have something to discuss20:19
dolphmotherwise, err on the side of readability :)20:20
YorikSardolphm: You can't benchmark readability :)20:20
morganfainbergYorikSar, i have a personal adversion to for/else because i've had to explain it a bunch.20:20
morganfainbergand it tends to confuse people.20:20
ayoungfor else is probably not needed here, either.20:20
dolphmmorganfainberg: definitely use it sparingly, and comment it when you do!20:20
morganfainbergdolphm, ++20:20
YorikSarmorganfainberg: If comment explains it and it prevents from some mistakes... Why not use it?20:20
ayoungsince the for loop on line 220 effective gates that hole section20:21
morganfainbergYorikSar, because even python verterans have issues with for/else20:21
morganfainbergYorikSar, it should be the exception to the rule "this really makes sense" vs. "well it's pretty so why not"20:21
YorikSarmorganfainberg: Really?20:21
morganfainbergyes, for/else is an odd construct that confuses people easily20:21
*** marekd|away is now known as marekd20:22
ayoungYorikSar, I hacve to admit, I learned about it on this commit20:22
morganfainbergand if the code doesn't really benefit from it, using is adds extra readability issues.20:22
YorikSarFor me it made a lot of sense... We have a special final iteration of the loop after iterator is finished.20:22
morganfainbergit's not often used, and not "intuitive"20:22
dstanekmorganfainberg: i tend to use it when i would otherwise need a temporary variable to track what happened in a loop20:23
morganfainbergYorikSar, sure, but do you disagree that it's more readable backdented, it's not using a infrequent/obscurely used part of the language?20:23
YorikSarScroll up to line 147. Is it intuitive there?20:23
morganfainbergdstanek, and i think that is a fair usecase20:23
jamielennoxwithout looking at the code in question - an special final iteration of a loop is just something you do after the loop has finished20:23
dstanekmorganfainberg: if the for and the else don't fit in the 30 lines or so in my editor i won't use it20:23
dolphmwhat *other* languages support for/while-else?20:24
morganfainbergYorikSar, again, is it really needed?20:24
morganfainbergdstanek, also python is sloppy in scope20:24
morganfainbergdstanek, :P20:24
dolphmmorganfainberg: lazy*20:24
morganfainbergdolphm, ++20:24
morganfainbergsorry lazy is better term20:24
jamielennoxalso i really like the for/else pattern if you are actually doing breaks in there20:24
dstanekdolphm: lazy! like me!20:24
dolphmmorganfainberg: lazy is efficient20:24
dolphmmorganfainberg: efficient is performant!20:24
morganfainbergif there was a break usecase20:25
morganfainbergi'd be for for/else20:25
morganfainbergbut both of these cases aren't using break semantics20:25
dolphmdstanek: ++ the best engineers are the laziest ;)20:25
morganfainbergone is a return (bail) and the other is just "do something at the end"20:25
dstanekmorganfainberg: what code are you guys talking about?20:25
morganfainbergdstanek, https://review.openstack.org/#/c/81235/2/keystone/contrib/revoke/model.py20:25
morganfainbergline 147 and 21920:26
morganfainbergthough i guess we're using break now in 21920:26
morganfainbergso it does make more sense.20:26
morganfainbergin that review20:26
morganfainbergvs. the current code20:26
YorikSarjamielennox: Unless you want to exit from the loop with break to let other people add some code after your loop.20:26
ayoungmorganfainberg, we don't need to20:27
*** flaper87 is now known as flaper87|afk20:27
jamielennoxYorikSar: looking at the code though the only break is essentially a return False20:27
jamielennoxif you made that a return False then the final else block would just be run after20:27
YorikSarI made it a break because I had to add "return False" in the end either way...20:28
ayoung            forest = bundle20:28
ayoung            if not forest:20:28
ayoung                break20:28
jamielennoxyea, but i think it clearer to say that in this case we are just exiting out early20:28
ayoungfor leaf in forest:20:28
ayoungwill just pass over20:28
*** derek_c has joined #openstack-keystone20:29
YorikSarayoung: Yes, I though about it. But desided do leave it as is...20:29
ayoungYorikSar, I have top admit, part of the reason I am playing with it is to understand the code better20:29
*** flaper87|afk is now known as flaper8720:29
ayounggonna run the tests and submit a new version here in second20:30
*** gyee has joined #openstack-keystone20:30
YorikSarOk, I guess there's no point to argue with everyone but myself.20:30
morganfainbergYorikSar, my biggest worry with for/else (especially in this context) is that readability is very important especially in the security context. I'd rather anyone looking at this get it than needing to explain why it works like this. it's really my major complaint.20:30
morganfainbergYorikSar, not that it needs to be changed, nor that i would -1 for it or -220:31
morganfainbergYorikSar, just keep in mind readability is very important on a project of this size :)20:31
YorikSarfor-else was made for smth like search and do smth with what we found.20:31
YorikSarI'll change it though.20:31
ayoungYorikSar, ther real value is in this discussion, in that all of the Pythonistas are looking at the code and finally understanding it20:32
YorikSarI'm ready to give up for-else but I'll never give up filter! :)20:32
morganfainbergYorikSar, haha gotta pick your battles right?20:32
YorikSarYeah... Even professionals can't understand my Python. What can we say about my Russian with my girlfriend?.. :)20:34
ayoungYorikSar, morganfainberg what do you think of renaming forest to....potential_matches?20:34
YorikSarayoung: I use forest here because... Well, it's a forest in the graph theory.20:34
morganfainbergayoung, YorikSar, with the comment in YorikSar's review i don't think it matters20:35
morganfainbergayoung, sans comments, forest is less explicit20:35
ayoungI can't see the forest for the trees20:35
morganfainbergayoung, i see what you did there.20:35
* ayoung looking like the cheshire cat20:35
bknudsondoes python have a graph theory library?20:36
morganfainbergbknudson, good question20:36
*** marcoemorais has quit IRC20:36
*** daneyon has joined #openstack-keystone20:36
*** marcoemorais has joined #openstack-keystone20:36
*** flaper87 is now known as flaper87|afk20:39
ayoungYorikSar, I don't really want to use the tree/forest metaphor here.  There is only one tree, and we are searching it from top to bottom.20:42
YorikSarayoung: How about bundle and new_bundle? :)20:42
ayoungYorikSar, I'll psot what I have in a moment.  I'm just modifying some comments20:43
ayoungI'm also not sure if it is necessary to avoid the early return20:43
ayoungI think it is actually prefereable20:43
YorikSarayoung: Oh... You're modifying this commit too?20:43
ayoungYorikSar, yeah.20:43
YorikSarayoung: I've already changed it a bit. Should I not push it then?20:43
ayoungNah, let me first.20:44
dolphmOS-FEDERATION api change/cleanup required for an RC bug :-/ https://review.openstack.org/#/c/81022/20:45
openstackgerritayoung proposed a change to openstack/keystone: is_revoked check all viable subtrees  https://review.openstack.org/8123520:48
ayoungYorikSar, ^^20:48
*** ayoung is now known as ayoung_Dad_mode20:48
YorikSarayoung: I really don't see how self.append_if_not_none increases readability...20:50
morganfainbergYorikSar, vs doing the filter?20:51
morganfainbergYorikSar, yeah looking at it now.20:51
YorikSarTake a look at new lines 202-20320:51
YorikSarOr, better, 208-21020:51
morganfainbergi don't feel strongly either way20:52
morganfainbergi'm ok with going w/ filter vs a method call20:52
morganfainbergi'll let you and ayoung_Dad_mode figure that out.20:52
morganfainbergbut i wouldn't block either implementation, in fact, i thing both are perfectly valid.20:52
morganfainbergfilter does add the benefit of brevity20:53
YorikSarAnd I dont like having it another instance method. Of course premature optimization is evil but just moving it to module level saves lots of time (and for me - readability).20:53
morganfainbergYorikSar, it's a staticmethod, it should be a function if anything20:54
YorikSarOk, it's coming to 1am and I'm getting grumpy. I guess, it doesn't matter much since either way works and it's not performance-critical part.20:54
morganfainbergYorikSar, eh, i'll poke ayoung to move it back to filter20:55
morganfainbergYorikSar, this change isn't a real benefit.20:55
morganfainbergYorikSar, thanks for conversing on everything here.20:55
YorikSarI'd be against @staticmethod too because every call is another attribute search.20:55
morganfainbergno i wouldn't say make it static20:56
morganfainbergi was saying it is effectively static20:56
morganfainbergit should be a function if anything vs. a method20:56
YorikSarmorganfainberg: Oh, thanks to you I actually in process of changing my mind about use of for-else in such cases :)20:56
morganfainbergYorikSar, :)20:57
morganfainbergYorikSar, but i think filter really is better than the instance method20:57
morganfainberglooking at it more20:57
YorikSarOk, time to sleep now. See you around.20:57
*** dims has quit IRC21:12
*** pcargnel has quit IRC21:18
*** ayoung_ has joined #openstack-keystone21:21
*** ayoung_ is now known as ayoung21:22
*** ayoung_Dad_mode has quit IRC21:24
*** dims has joined #openstack-keystone21:26
*** derek_c has quit IRC21:31
openstackgerritDiane Fleming proposed a change to openstack/identity-api: Clean up naming to match  new conventions  https://review.openstack.org/8107621:34
morganfainbergdolphm, almost have a working patch for the 500 error cleanup21:34
marekdmorganfainberg: this apache/pki tokens thingy?21:35
dolphmmorganfainberg: \o/21:35
morganfainbergmarekd, https://bugs.launchpad.net/keystone/+bug/129104721:36
dolphmmarekd: no, leaking internal details on 500's21:36
morganfainbergdolphm, i made unexpected error subclass of securityerror and i also make it explicitly blank 'exception' from kwargs if not conf.debug OR exception not in kwargs21:36
morganfainbergso no replacement error(s).21:36
morganfainberg1 test needed fixing.21:36
*** daneyon has quit IRC21:36
*** leseb has quit IRC21:38
marekddolphm: BTW, this (de)serializer json->xml->json seems to be already implemented, so I am just gonna reuse this and make dedicated test: https://github.com/openstack/keystone/blob/master/keystone/tests/test_serializer.py#L2521:38
morganfainbergI'm almost of the opinion UnexpectedExceptions (globally) should be made generic 500s21:38
morganfainbergdolphm, should i "fix" the other UnexpectedExceptions to be very generic w/o debug=true?21:39
morganfainbergwe have it subclassed in 5 areas21:39
morganfainbergerm as 5 exceptions21:40
mfischwas the hash-password option deprecated?21:40
morganfainbergi'm almost leaning towards yes, it should just be "unexpected error" 500 returned from the api instead of details21:41
dolphmmorganfainberg: anything specific being exposed?21:41
dolphmmorganfainberg: or room for other error messages to be inserted?21:42
morganfainbergdolphm, group_id not found by mapping_id, malformed endpoint, certificate files not found, config file not found21:42
*** saju_m has quit IRC21:42
morganfainbergthe federated one i think should be obscured21:42
dolphmmorganfainberg: i'd buy that.. the rest could be obscured but will only be raised to deployers anyway21:43
morganfainbergi'll clean these all up21:43
*** derek_c has joined #openstack-keystone21:44
*** bknudson has quit IRC22:07
openstackgerritMorgan Fainberg proposed a change to openstack/keystone: Do not expose internal data on UnexpectedError  https://review.openstack.org/8138522:09
openstackgerritMorgan Fainberg proposed a change to openstack/keystone: Do not expose internal data on UnexpectedError  https://review.openstack.org/8138522:10
*** nkinder has quit IRC22:12
openstackgerritMorgan Fainberg proposed a change to openstack/keystone: Do not expose internal data on UnexpectedError  https://review.openstack.org/8138522:12
* morganfainberg grumbles about some extra changes sneaking in22:13
*** wchrisj_ has quit IRC22:14
*** wchrisj has joined #openstack-keystone22:14
marekdis it safe to  call assertEqual for dicts equality?  https://github.com/openstack/keystone/blob/master/keystone/tests/test_serializer.py#L2922:17
*** derek_c has quit IRC22:17
morganfainbergmarekd, i think there is a assertDictEqual22:17
*** leseb has joined #openstack-keystone22:17
morganfainbergmarekd, yeah use self.assertDictEqual22:17
marekdmorganfainberg: whoa, would be super cool.22:17
morganfainbergmfisch, sorry just saw your question, which option?22:18
mfischmorganfainberg: I found an old web reference to a config option called hash-password22:18
morganfainbergmfisch, in keystone?22:19
mfischour FreeIPA box is complaining that we're trying to create users with hashed passwords22:19
mfischmorganfainberg: yeah, but I don't see any modern references to it.22:19
*** leseb has quit IRC22:19
*** leseb has joined #openstack-keystone22:19
mfischmorganfainberg: https://www.ibm.com/developerworks/community/wikis/home?lang=en#!/wiki/OpenStack/page/keystone.conf22:19
mfischI assume it was deprecated22:19
morganfainbergmfisch, i am guessing so.22:21
marekddolphm: morganfainberg: hmm, strange, json->xml->json seems to have inconsistency, but i don't feel like it's a topic for my federation patch: http://pasteraw.com/s9eaf818sbz1udlyr43943h7airmqgg.22:21
marekddolphm: morganfainberg (see 'extras')22:21
morganfainbergmarekd, extras thing22:22
*** nkinder has joined #openstack-keystone22:22
marekdmorganfainberg: known issue?22:22
morganfainbergmarekd, i am guessing you are converting from a SQLAlchemy object based on dictbase?22:22
marekdmorganfainberg: yes, basically an unscoped token.22:23
morganfainbergmarekd, hmm.22:23
marekdmorganfainberg: what i really want to check is to see whether xml will complain about names/tags, nothing more.22:23
morganfainbergmarekd, yeah xml doesn't see the extras22:23
marekdmorganfainberg: i'd be happy to remove extras from the initial json dict.22:24
morganfainbergmarekd, invert your expected/refernce in your dicteqlas22:24
morganfainbergmarekd, xml i think smushes those out since they don't exist22:24
morganfainbergmarekd, so you either need to pop extras off the original22:24
morganfainbergmarekd, or figure out how to add it back in :P (hint, this is the wrong choice)22:25
marekdmorganfainberg: thanks!22:25
marekdhaha, 100x faster working in your tz :P22:25
marekdand get real-time consultancy :P22:25
morganfainbergmarekd, though you might run into issues w/ users et al22:25
morganfainbergmarekd, LOL22:26
morganfainbergmarekd, likely the fix is 'if not json.get(extras): json.pop(extras)'22:26
marekdmorganfainberg: no i won't in this particular case, seriously what would satisfy me to see serializer.to_xml() not failing, nothing more....22:26
morganfainbergmarekd, hehe22:26
morganfainbergmarekd, sounds good, but i would check to see if 'extras' has anything in it22:27
morganfainbergbefore removing it22:27
marekdhmm, but directly in assertSerializeToXML(self, json_body) here in test_serializer's method?22:27
marekdmorganfainberg: ^^22:27
morganfainbergmarekd, hmm22:30
morganfainbergmarekd, no you're right22:30
morganfainbergnot in the assertSerializetoXML22:30
marekdmorganfainberg: OK22:30
*** lbragstad has left #openstack-keystone22:42
*** lbragstad has joined #openstack-keystone22:51
openstackgerritMarek Denis proposed a change to openstack/keystone: Store groups ids objects list in the OS-FEDERATION object.  https://review.openstack.org/8127722:52
*** lbragstad is now known as lbragstad__22:55
*** nkinder has quit IRC22:57
marekdhttps://github.com/openstack/keystone/blob/master/keystone/tests/test_v3_federation.py#L891 - just notices one (passing) test is disabled, due to removed test_ prefix. In order to merge this trivial change do I need to go through bug->fix->approve process? I am asking about the bug espcially...23:00
*** devlaps has joined #openstack-keystone23:00
*** gokrokve has quit IRC23:01
*** browne has quit IRC23:01
*** devlaps1 has quit IRC23:02
*** leseb has quit IRC23:11
openstackgerritA change was merged to openstack/identity-api: merge OS-FEDERATION objects together  https://review.openstack.org/8102223:11
*** leseb has joined #openstack-keystone23:12
openstackgerritMarek Denis proposed a change to openstack/keystone: Rename scope_to_bad_project() to test_scope_to_bad_project().  https://review.openstack.org/8139523:13
marekdmorganfainberg: dolphm stevemar :  ^^ should not take too long for a review :-)23:14
stevemarmarekd, hehe, no, it won't23:15
*** leseb has quit IRC23:16
marekdok, i am out of here. cheers!23:19
*** marekd is now known as marekd|away23:19
*** henrynash has quit IRC23:23
stevemarsee ya marekd|away23:27
dolphmmarekd|away: good catch!23:38
dolphmmorganfainberg: marekd|away: "extras" was never part of the spec and it serves no useful purpose, hence it's not tested for json <-> xml conversion ;)23:41
morganfainbergdolphm, aha23:41
morganfainbergdolphm, https://review.openstack.org/#/c/81385/ there is an outstanding docstring stupid i left in23:42
morganfainbergdolphm, but there ya go23:42
morganfainbergline 294 of exceptions.py is ... well wrong.23:43
morganfainbergexcept if you factor in the super call23:43
dolphmmorganfainberg: why change it from message_format to __message_format just for 500's?23:43
morganfainbergdolphm, @property does the switching. I guess security error could receive a similar treatment?23:44
dolphmmorganfainberg: ah, overlooked that23:45
morganfainbergdolphm, actually no security error shouldn't get the same treatment23:45
morganfainbergbasically it squashes all the 500s down to a generic 500 unless debug is enabled23:46
morganfainbergregardless of what people pass to it / define on the subclasses (if someone defines message_format, sure it'll override, but we can stop that via reviews)23:46
dolphmmorganfainberg: don't we have a test_exception module?23:47
morganfainbergdolphm, no, we test it in test_wsgi23:47
morganfainbergas a side effect i guess23:47
morganfainbergdolphm, want me to add in an explicit test exception?23:48
morganfainbergwouldn't be hard to do23:48
dolphmmorganfainberg: https://github.com/openstack/keystone/blob/master/keystone/tests/test_exception.py#L103-L17023:48
morganfainbergoh wtf. i overlooked it23:48
* morganfainberg can't brain.23:48
morganfainbergok i'll add explicit for unknown error23:48
morganfainbergdolphm, thanks *is embarassed*23:49
dolphmmorganfainberg: exceptions are so stable you've just never had to look at the tests :P23:49
dolphmi think this is supposed to be me http://i.imgur.com/tektZzm.jpg23:55
dolphmcoming soon to a teespring near you23:56
jamielennoxdolphm: can you have another go at: https://review.openstack.org/#/c/60752/ i have people wanting it23:56
jamielennoxgyee: ^^23:56
gyeejamielennox, just a suggestion here https://review.openstack.org/#/c/60752/14/keystoneclient/httpclient.py23:58
gyeeotherwise, looks good23:58

Generated by irclog2html.py 2.14.0 by Marius Gedminas - find it at mg.pov.lt!