16:00:02 <lbragstad> #startmeeting keystone
16:00:03 <openstack> Meeting started Tue Jul 17 16:00:02 2018 UTC and is due to finish in 60 minutes.  The chair is lbragstad. Information about MeetBot at http://wiki.debian.org/MeetBot.
16:00:04 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
16:00:06 <openstack> The meeting name has been set to 'keystone'
16:00:07 <lbragstad> #link https://etherpad.openstack.org/p/keystone-weekly-meeting
16:00:10 <lbragstad> agenda ^
16:00:15 <cmurphy> o/
16:00:18 <jgrassler> Hello
16:00:20 <wxy|> o/
16:00:23 <lbragstad> ping ayoung, breton, cmurphy, dstanek, gagehugo, hrybacki, knikolla, lamt, lbragstad, lwanderley, kmalloc, rodrigods, samueldmq, spilla, aselius, dpar, jdennis, ruan_he, wxy, sonuk
16:00:24 <gagehugo> o/
16:00:29 <hrybacki> o/
16:00:30 <knikolla> o/
16:01:22 <kmalloc> o/
16:01:28 <lbragstad> we have a relatively light schedule today - so we'll give folks another minute or two to show up
16:02:51 <sonuk> \o
16:03:02 <lbragstad> #topic release status
16:03:08 <lbragstad> just a couple quick announcements
16:03:56 <lbragstad> we have non-client library freeze by the end of the weke
16:03:58 <lbragstad> week*
16:04:00 <lbragstad> #link https://releases.openstack.org/rocky/schedule.html#r-final-lib
16:04:21 <lbragstad> so if there is anything we need from an oslo/ksa perspective, we'll need to get those things squared away
16:04:32 <lbragstad> i don't think i have anything on my radar
16:04:36 <cmurphy> also ksm
16:04:47 <lbragstad> ++ yeah
16:04:50 <cmurphy> there's at least one ksm change that needs attention
16:06:14 <lbragstad> these are the open reviews
16:06:17 <lbragstad> #link https://review.openstack.org/#/q/project:openstack/keystonemiddleware+status:open
16:07:11 <lbragstad> cmurphy: which review were you referring to?
16:07:39 <cmurphy> lbragstad: well i guess there's more than one :)
16:07:49 <lbragstad> lol
16:08:03 <cmurphy> one i'm having trouble with is https://review.openstack.org/578008 would be good to have other eyes on it
16:09:21 <lbragstad> just the purpose of it?
16:09:24 <lbragstad> or something else?
16:09:35 <lbragstad> i'll be honest, i haven't looked at this one yet
16:10:24 <cmurphy> well we don't need to use the meeting to look at it, just wanted to highlight it
16:10:50 <lbragstad> i'll make a note to review it today
16:11:32 <lbragstad> any other patches we need to get into ksa, ksm, or oslo libraries before Friday?
16:12:12 <lbragstad> note that oslo.limit will be exempt from the freeze since it's not revved past 1.0 yet
16:13:19 <lbragstad> if someone does stumble across something we need to include, just say something
16:13:38 <lbragstad> along the same vein - requirements freeze is next week
16:14:00 <lbragstad> so we'll need to be mindful of versions we need, if any
16:14:06 <wxy|> https://review.openstack.org/#/c/583215/ this one in ksa, since it's a bug from s10 for a long time.
16:14:34 <lbragstad> wxy|: nice - i can review that today, too
16:15:23 <wxy|> lbragstad: thanks.
16:15:37 <lbragstad> thanks for the patch
16:16:12 <lbragstad> that's about all i had for release stuff
16:16:17 <lbragstad> #topic keystoneauth url discovery bug
16:16:25 <kmalloc> that bug sucks.
16:16:32 <lbragstad> #link: https://bugs.launchpad.net/keystoneauth/+bug/1733052
16:16:32 <openstack> Launchpad bug 1733052 in keystoneauth "Usage of internal URL in clouds.yaml causes a 404" [High,In progress] - Assigned to wangxiyuan (wangxiyuan)
16:16:38 <lbragstad> ^ that one you mean :)
16:16:50 <kmalloc> look closely at the code, we need to be sure we're not going to break anything
16:16:54 <lbragstad> i'm not entirely sure who put this on the schedule for today
16:17:13 <kmalloc> but we need to get that landed if possible
16:17:42 <kmalloc> i don't know who added it, but mordred, wxy|, me, and a few others have a vested interest in that bugfix landing
16:17:57 <kmalloc> it will unbreak some folks.
16:18:19 <lbragstad> sounds good
16:18:30 <mordred> yeah. it's important
16:18:49 <mordred> it's definitely a thing we should fix in the client library and not in the services themselves ;)
16:18:51 <lbragstad> we'll looks like we just need some reviews
16:19:28 <kmalloc> yep
16:19:38 <mordred> we should land it - because there are old clouds out there that are broken - but it's also fundamentally broken from the pov of the services
16:19:38 <lbragstad> is there anything we want to discuss about the fix?
16:19:51 <mordred> and at some point someone should actually fix all of the services
16:19:57 <ayoung> looking
16:20:29 <kmalloc> nothing besides what has already been said.
16:20:38 <mordred> kmalloc: I could rage a bit more if it's helpful
16:20:55 <kmalloc> mordred: lol
16:20:59 <ayoung> why does it not have a test?
16:21:48 <wxy|> ayoung: oh, i'll add one later.
16:22:05 <ayoung> wxy|, I won't touch it without a test
16:22:10 <ayoung> tests first.  Tests always
16:24:09 <lbragstad> ok - anything more on this topic?
16:24:16 <lbragstad> otherwise i'll open it up for discussion
16:24:56 <ayoung> has anyone looked at our test coverage numbers lately?
16:25:06 <lbragstad> yes
16:25:17 <lbragstad> we're around 92% in keystone server
16:25:21 <ayoung> how we looking?
16:26:05 <lbragstad> i would like to gate on coverage, but i'm not sure if we've tried that in the past
16:26:09 <lbragstad> #topic open discussion
16:26:39 <ayoung> yeah...it would be great if a patch was immediately rejected if the lines of codes changed were not covered by a test
16:27:10 <lbragstad> does anyone know if other projects gate on test coverage?
16:27:24 <lbragstad> i'm not aware of that done by any of the other services
16:27:47 <gagehugo> I don't think so
16:29:03 <ayoung> could we build it out of existing tools?
16:29:13 <lbragstad> what do you mean?
16:29:25 <ayoung> something like:  upon checking, run test coverage.  THen, from the git patch, get the new lines of code
16:29:29 <clarkb> one specific concern around using coverage in that way is it can legitimetely drop (say you delete a bunch of unused code that was only covered by test suite)
16:29:40 <lbragstad> we have a coverage job defined in our tox.ini
16:29:41 <clarkb> and no one has built a thing to address those concerns
16:29:43 <ayoung> and then query the test covereage output to make sure those lines were covered?
16:30:11 <ayoung> clarkb, I'm looking for "all new lines are covered by some code"
16:30:16 <ayoung> which is a way to move forward
16:30:59 <ayoung> deleting code  would only fail if you ended up with a partial line change, and that line was uncovered
16:31:21 <ayoung> sounds like a great intern projec
16:31:22 <ayoung> t
16:32:28 <ayoung> Oh, BTWE, I have some good news for people trying to develop on RHEL75 etc
16:32:48 <ayoung> http://adam.younglogic.com/2018/07/running-openstack-components-on-rhel-with-software-collections/
16:33:03 <ayoung> which can be used to run upstream code in a supported way.  ish
16:33:20 <ayoung> so for running the covereage code, I can use that
16:33:28 <ayoung> scl enable rh-python35 bash
16:33:46 <ayoung> tox -e cover
16:33:56 * ayoung running now
16:33:57 <kmalloc> i am -2 on a "if coverage goes down error" because of what clarkb said.
16:34:15 <ayoung> kmalloc, agreed
16:34:19 <lbragstad> yeah - that's a good point
16:34:21 <ayoung> I don't want to do it on percentage
16:34:34 <lbragstad> what about at least publishing it formally somewhere?
16:34:50 <lbragstad> (i don't think we do that either)
16:34:54 <ayoung> I want to do it on "if you are adding or modding  a line of code in a patch, it needs a test."
16:34:59 <kmalloc> and there are legitimate cases (e.g. flask) that testing could not have been fully written without it being a massive 3000+ line patch
16:35:09 <kmalloc> and then tests on top of it
16:35:25 <ayoung> you can mock out a lot
16:35:37 <kmalloc> ok, let me just say this is a people problem not a tech problem.
16:35:42 <ayoung> disagree
16:35:47 <ayoung> this is something we can automate
16:35:54 <kmalloc> it is on us, reviewers to say "this needs tests" and look at the coverage report
16:35:56 <ayoung> and that makes it something not part of a code review
16:35:59 <kmalloc> it is not automatable
16:36:08 <kmalloc> there are too many variables
16:36:16 <knikolla> I tend to agree with kmalloc
16:36:21 <lbragstad> at some point you have to review the code and make sure it's doing what it's suppose to
16:36:23 <ayoung> lets give it a try
16:36:47 <ayoung> lbragstad, agreed, but automated checks for "you must have a test" are like automated pep8
16:37:10 <kmalloc> mocking it all out is a terrible policy just to get past a hurdle of "well you're going to just undo these once the next patch lands"
16:37:22 <ayoung> ok, I have an idea
16:37:26 <mordred> this was first proposed at the cactus summit fwiw, and so far nobody has managed to build a thing that works enough to be usable
16:37:27 <mordred> HOWEVER
16:37:35 <ayoung> instead of making it a gerrit check
16:37:37 <mordred> it was first proposed at the cactus summit and people have wanted it ever since
16:37:46 <mordred> so I thik it would be welcome if someone can figure it out
16:37:50 <kmalloc> ayoung: if you can produce something legitimately reliable and handle the edge cases, i'll be the first to +2 it
16:37:55 <mordred> ++
16:38:00 <ayoung> lets get a tool that at least we can run, that does a cover check + "these lines are covered or not" check
16:38:04 <kmalloc> and i'm fine trialing it in keystone
16:38:13 * lbragstad would settle for publishing (not gating on) coverage reports as a way to encourage people to use it as a learning tool while filling the gaps
16:38:15 <kmalloc> but i want a well designed tool.
16:38:22 <ayoung> step one is showing it can be done
16:38:24 <mordred> ayoung: yes. agree. a tool we can run is a GREAT step 1
16:38:35 <kmalloc> lbragstad: it is published in the coverage run.
16:38:39 <kmalloc> we just need to look at it
16:38:41 <kmalloc> just like docs
16:38:46 <lbragstad> right...
16:38:50 <ayoung> step 4 is automating it.  Not sure about 2 and 3, but I am sure there are steps there
16:38:58 <gagehugo> http://logs.openstack.org/58/580258/10/check/openstack-tox-cover/9140707/cover/
16:39:09 <gagehugo> ^ example
16:39:10 <lbragstad> in addition to that i gues it would be nice to have a badge displaying the coverage of master
16:39:58 <ayoung> CLI is our worst offender, followed by LDAP (my quick scan)
16:40:21 * kmalloc also very strongly believes the it is a fallacy that 100% code coverage means anything. it results in the terrible thing you see in a lot of java projects with bazillion mocks
16:40:43 <mordred> ayoung: easy win - delete CLI :)
16:40:47 <kmalloc> mordred: ++
16:40:54 <kmalloc> mordred: didn't we already do that in keystone? ;)
16:40:59 <ayoung> Didn't we try that already?
16:41:06 <kmalloc> keystoneclient has no cli
16:41:11 <kmalloc> osc is our cli
16:41:15 <ayoung> this is keystone-manage
16:41:19 <mordred> ah.
16:41:25 <mordred> deleting that is probably not a great idea
16:41:26 <lbragstad> keystone/cmd/cli.py
16:41:41 <kmalloc> we should be writing tests to test behavior
16:41:56 <clarkb> one approach some projects have taken is to run non voting coverage jobs in check to produce coverage reports for projects separately from the main unittes run
16:41:56 <ayoung> want me to open some bugs for that?
16:42:10 <kmalloc> if we are specifying behavior rather than "did you write a test that exercises the code"
16:42:11 <clarkb> this is done because python coverage impacts timing in ways that can break things and be difficult to fix
16:42:14 <kmalloc> it is better.
16:42:40 <kmalloc> clarkb: our coverage report is voting, at least in check.
16:42:41 <kmalloc> iirc
16:42:41 <ayoung> "Mapping engine tester is untested"
16:43:16 <knikolla> is the current coverage job enough? if not, what is it missing?
16:43:26 <kmalloc> writing purely synthetic tests that shows the code behaves as it was written is a lot less useful than "here is the expected behavior, does it do that"
16:43:38 <kmalloc> and we tend to do a lot more of the former
16:43:54 <kmalloc> knikolla: i don't know what is missing from the coverage report being at least initially useful.
16:45:09 <lbragstad> i think it's initially useful - i guess i just want it to be more accessible?
16:45:47 <kmalloc> publish it alongside docs? make the coverage report a part of the dev docs?
16:46:11 <kmalloc> *coverage*->>docs.o.o/developer/keystone/coverage [not real url, but example]
16:46:32 <knikolla> sure
16:46:45 <kmalloc> i'm fine with that
16:46:51 <lbragstad> yeah - that might be a good idea
16:47:45 <kmalloc> it will make our docs job a LOT slower
16:47:59 <ayoung> so, when submitting a patch, run covereage, and make sure your new code is covered.  When reviewing a patch, do the same.
16:47:59 <kmalloc> or we would need a docs-coverage publisher
16:48:05 <lbragstad> it only have to run unit tests?
16:48:12 <kmalloc> yeah it has to run all the unit tests
16:48:16 <kmalloc> docs does not have to do that today
16:48:48 <kmalloc> ayoung: but ftr we already do that on every patch: example of one I am working on http://logs.openstack.org/24/582724/1/check/openstack-tox-cover/7263f00/cover/
16:49:09 <kmalloc> the report is already run in check [and if the job fails, the patch gets a -1]
16:49:10 <kmalloc> from zuul
16:49:33 <kmalloc> ayoung: right now, we just need to, as reviewers, look at that result :)
16:49:39 <ayoung> does not check every line is covered, or these would never get through
16:49:49 <kmalloc> that is the job of reviewers
16:49:50 <ayoung> right
16:49:58 <knikolla> kmalloc: ++
16:50:03 <kmalloc> 100% test coverage does not mean anything useful
16:50:06 <kmalloc> never has
16:50:32 <kmalloc> because you then have 1000000000 mocks. look at the bad pattern in a lot of bigger java (historically) projects
16:50:49 <kmalloc> does this test the bits of code you'd expect: yes/no, that is a human question
16:51:05 <ayoung> that gets into design issues.  All I am saying is that changed lines of code need tests.
16:51:25 <ayoung> I'll see if I can automate that
16:51:38 <kmalloc> i have to step out of this convo, i am so opposed to this line of thinking as an automatable task...
16:52:12 <kmalloc> the only proposal I recommend at this point is: Reviewers, look at the coverage report please. and add it to the factors you use to review code
16:52:13 <lbragstad> anything else we want to cover with this topic?
16:52:32 <lbragstad> kmalloc: ++
16:53:03 <knikolla> it's my failure as a reviewer if i let something in which isn't tested, and that tests don't cover edge cases or behaviors
16:53:13 <knikolla> i wouldn't trust the automation anyway because edge cases
16:54:29 <ayoung> "Never send a man to do a machines job."
16:55:17 <ayoung> "Never send a Human to do a machine's job."  is the proper quote
16:55:46 <ayoung> so to see the lines that change from a patch looks like it is scriptable...
16:55:55 <ayoung> and then pull thyose line numbers out of cover:
16:56:56 <ayoung> for example, in cover/keystone_server_flask_common_py.html
16:57:05 <kmalloc> ayoung: i can point to concrete examples of why automation is very hard on this: any abstract base class that does NotImplementedError() is NOT tested
16:57:21 <ayoung> kmalloc, as I said, first step is tooling
16:57:23 <kmalloc> ayoung: the wsgi framework loaders don't get captured in unit tests and can't be
16:57:28 <kmalloc> because of how they load
16:57:32 <knikolla> a machine's job here would be aiding the reviewer to make a decision. the binary tested or not doesn't help me too much. what would help me would be a "here's where this changed line is tested from".
16:57:39 <kmalloc> this is why functional tests, a real keystone, is useful
16:57:42 <kmalloc> and behavior
16:57:49 <lbragstad> three minute check
16:58:36 <kmalloc> i'll go on record and say i'm a hard -2 on adding jobs that do coverage level checks until we have clear examples on it working and not forcing needing to mock things for the sake of "did we test this"
16:59:09 <kmalloc> knikolla: ++ better "we tested with testcase X and Y and Z" would be good
16:59:49 <kmalloc> but that is also *hard*
17:00:15 <lbragstad> alright - wrapping this up since we're out of time
17:00:21 <lbragstad> thanks for coming all!
17:00:25 <lbragstad> #endmeeting