Monday, 2018-02-12

*** k0da has quit IRC00:30
*** mdrabe has quit IRC00:35
*** mdrabe has joined #openstack-powervm00:38
*** mdrabe has quit IRC00:45
*** mdrabe has joined #openstack-powervm00:48
*** mdrabe has quit IRC01:07
*** mdrabe has joined #openstack-powervm01:10
*** mdrabe has quit IRC01:22
*** mdrabe has joined #openstack-powervm01:26
*** mdrabe_ has joined #openstack-powervm02:03
*** mdrabe has quit IRC02:05
*** edmondsw has joined #openstack-powervm03:07
*** edmondsw has quit IRC03:11
*** mdrabe_ has quit IRC03:24
*** mdrabe has joined #openstack-powervm03:25
*** mdrabe has quit IRC03:32
*** mdrabe has joined #openstack-powervm03:34
*** mdrabe has quit IRC03:43
*** mdrabe has joined #openstack-powervm03:51
*** mdrabe has quit IRC04:05
*** mdrabe has joined #openstack-powervm04:07
*** mdrabe has quit IRC04:32
*** mdrabe has joined #openstack-powervm04:34
*** mdrabe has quit IRC04:44
*** mdrabe has joined #openstack-powervm04:50
*** mdrabe_ has joined #openstack-powervm05:17
*** mdrabe has quit IRC05:17
*** mdrabe_ has quit IRC05:22
*** mdrabe has joined #openstack-powervm05:22
*** mdrabe has quit IRC05:58
*** mdrabe has joined #openstack-powervm06:00
*** AlexeyAbashkin has joined #openstack-powervm07:51
*** k0da has joined #openstack-powervm08:28
*** catmando has quit IRC11:25
*** catmando has joined #openstack-powervm11:26
*** svenkat has joined #openstack-powervm13:13
*** edmondsw has joined #openstack-powervm13:15
*** edmondsw has quit IRC13:15
*** edmondsw_ has joined #openstack-powervm13:16
*** svenkat has quit IRC13:18
*** svenkat has joined #openstack-powervm13:28
*** edmondsw_ is now known as edmondsw13:37
*** apearson_ has joined #openstack-powervm13:44
*** mdrabe_ has joined #openstack-powervm14:17
*** mdrabe has quit IRC14:20
*** mdrabe_ is now known as mdrabe14:23
*** edmondsw has quit IRC14:38
*** svenkat has quit IRC14:39
*** edmondsw has joined #openstack-powervm14:52
*** svenkat has joined #openstack-powervm14:53
*** esberglu has joined #openstack-powervm14:59
*** tjakobs has joined #openstack-powervm15:01
openstackgerritEric Berglund proposed openstack/nova-powervm master: Update UPPER_CONSTRAINTS_FILE for stable/queens
openstackgerritEric Berglund proposed openstack/networking-powervm master: Update UPPER_CONSTRAINTS_FILE for stable/queens
efriedesberglu: What are you doing?15:15
esbergluefried: I'm not sure why, I have stable/queens checked out locally, but the git command pushed the tox changes to master15:16
efriedesberglu: looking...15:16
esbergluefried: I'm sure I just messed something up15:17
efriedesberglu: Yeah, no, I see 543482 tracked against master.15:17
efriedoh, hm, now I don't, after a fetch.15:17
efriedesberglu: When you push, are you saying `git review stable/queens` ?15:18
esbergluefried: I need to do the .gitreview change first probably15:21
edmondswyeah, make the .gitreview change.15:22
esberglu^ need that in nova/net-powervm first15:22
efriedWhen have we ever done that before?15:23
efriedI don't see it in pike or ocata15:23
efriedor newton or mitaka...15:23
edmondswhmmm... we *should* have done that...15:23
efriedShrug, I see Nova does it.15:24
edmondswit's just a convenience, though15:24
efriedI guess just for conv... yeah15:24 that esberglu doesn't have to remember to say stable/queens when he does his git review command.15:24
esbergluApparently I don't work on the stable branches much :)15:28
edmondswesberglu do you have any idea where stable-ocata_a comes from vs. stable-ocata? here:
edmondswI noticed neither were public. I'm going to mark one public so it can be seen... I assume stable-ocata15:32
esbergluedmondsw: Not sure what you're looking at?15:33
edmondswedmondsw scroll down to inactive versions15:33
edmondswesberglu ^15:34
esbergluNot on my page15:34
edmondswdid you login?15:34
esbergluOh lol15:34
edmondswI'm also thinking we probably need to deactivate stable-newton and older and instead active *-eol15:37
edmondswesberglu which brings another intesteresting question... there are a couple different liberty-eol there15:37
edmondswoh, one uses underscore... I assume liberty-eol is the right one?15:38
esbergluedmondsw: No idea on the _a question15:40
edmondswesberglu k, I'll just ignore them and activate the ones without the _a, hope they're the latest15:40
esbergluedmondsw: And yeah we should deactivate newton and earlier15:41
edmondswesberglu doing that now15:41
esbergluedmondsw: And activate stable-ocata15:41
edmondswesberglu there is a "wipe" link on these... I may try that on the _a and see if it removes them. I don't think we need them15:42
edmondswesberglu whatever is creating the _a must still be there with queens... see
edmondswI guess I'll just leave them, they're not hurting anything15:48
*** k0da has quit IRC16:40
esbergluedmondsw: I think I finally figured out what is going wrong with that update_server_name tempest test17:19
esbergluWe shouldn't be passing instance into that init17:20
esbergluHaven't tested the fix yet, but I'm pretty sure that's where the issue is coming from17:21
efriedesberglu: OOT doesn't have that17:23
esbergluefried: Yep17:24
esbergluI'm pretty sure that the all of the taskflow work happened while the Config Drive patch was sitting as WIP17:24
efriedesberglu: While you're at it, name the freaking kwarg if you please.17:24
esbergluSo it slipped through the cracks17:24
esbergluI'll check and make sure it wasn't missed anywhere else17:24
efriedHad we been doing that all along as a habit, that never would have happened.17:24
efriedAgain, the OOT version is doing that properly.17:25
efried...since 2015.  Not sure how that slipped through.17:25
*** AlexeyAbashkin has quit IRC17:36
edmondswesberglul why would update_server_name involve DeleteVOpt?17:58
esbergluedmondsw: The test isn't actually failing, it's deleting the server afterwards17:58
edmondswthat's a good catch17:58
esbergluthat is failing17:58
edmondswah, ok17:58
esbergluSo we're passing in instance as the task name17:59
esbergluWhich has some non-ascii stuff in it from the test17:59
edmondswopen a LP bug17:59
edmondswand we can maybe get this fixed for rc217:59
efriedThat's backport-worthy.18:00
efriedDid it just land in Q?18:00
esbergluefried: Yeah18:00
edmondswefried yep, if they don't take it for rc2 we can backport18:00
edmondswesberglu so does the server delete work but leave the VOPt behind?18:01
edmondswcuz surely there are tests to make sure server delete works...18:02
edmondswhow did we not have a UT fail?18:02
esbergluedmondsw: This only fails because of the non-ascii rename18:02
edmondswthat will need to be part of the fix18:02
edmondswwhy would it only be an issue for non-ascii?18:02
edmondswI'm missing something18:03
esbergluDeleteVopt is passing in the full instance as the task name. Which is weird but doesn't actually break anything18:03
esbergluIt's just a string representation of the instance18:03
esbergluBut when there is non-ascii in that string it breaks the taskflow compilation18:04
esbergluYou can see what I mean there18:05
edmondswyeah, I got it18:05
edmondswesberglu I assume you'll fix the other places as well, e.g.
esbergluedmondsw: Yep18:06
edmondswgood catch18:07
esbergluefried: edmondsw: Remind me how backports work right now. Is the fix supposed to merge to master before being propsed to queens?18:18
edmondswonce it merges to master, you cherry-pick it to stable/queens18:20
esbergluedmondsw: Yeah just wasn't sure if I had to wait to cherry-pick or not. Tx18:20
edmondswI don't know that you have to wait to propose it, but you do have to wait to merge it18:21
edmondswso may make sense to wait on proposal... not sure on etiquette there18:21
edmondswI usually wait18:21
*** k0da has joined #openstack-powervm18:33
*** k0da has quit IRC18:58
*** apearson_ has quit IRC18:58
esbergluedmondsw: We aren't mocking taskflow, we are executing the tasks, not sure how autospec would help here19:20
*** k0da has joined #openstack-powervm19:32
edmondswesberglu looking19:34
edmondswesberglu yeah looks like we've got tests that mock tasks, but at the execute method level instead of the class level19:35
*** k0da has quit IRC19:39
efriedautospec wouldn't help.19:54
efriedI mean, you could add a bug-specific test by creating an instance with deliberately non-ascii chars in the name and making sure you can delete its VOpt.19:54
efriedBut that seems really narrow.19:55
*** apearson_ has joined #openstack-powervm20:05
*** svenkat_ has joined #openstack-powervm20:09
*** svenkat has quit IRC20:10
*** svenkat_ has quit IRC20:13
esbergluedmondsw: Tried a couple variations on what you sent me in slack, haven't got them working yet20:17
esbergluI've got to relocate quick, will pick back up after20:18
*** esberglu has quit IRC20:19
*** esberglu has joined #openstack-powervm20:30
edmondswefried this is what I was thinking and had suggested to esberglu... but apparently he hasn't gotten it to work:
efriededmondsw: How would that help to validate anything?20:38
efriedTask.__init__ takes a bunch of arguments.  If you're not specifying the kwarg keys, and they (usually) cast correctly, it'll happily take it.20:38
edmondswefried yeah, true20:39
efriedIn order to make this test just the bug (which, again, I think is kind of narrow), we should *not* mock Task, deliberately create an instance with non-ascii, and then validate that our task passes.20:39
efriedBreak it first to validate that it can be broken thusly.20:39
efriedThen apply the fix and make sure it fixes it.20:40
efriedYou know, The Gibi Process.20:40
edmondswI suppose we could just test that the task name is what it's supposed to be20:41
efriedYeah.  Which is also a little goofy.20:41
esbergluefried: I don't think trying to catch the non-ascii bug is what we should be doing here though. The problem was that we were passing bad args20:41
efriedSo you could mock Task.__init__ and make sure the first arg is instanceof str.20:41
edmondswis there a way to combine the autospec test with enforcement of using kwargs instead of passing as args?20:42
efriedand/or not instanceof Instance.20:42
efriedBoth of which are goofy.20:42
efriededmondsw: Enforcing kwargs can be done with @positional20:42
efriedBut people are shying away from that20:42
efriedin the community20:42
efriedit seems20:42
edmondswany idea why?20:42
efriedeven though IMO it's a pretty good idea.20:42
efriednot sure, let me dig up some history from ksa...20:42
edmondswmaybe doesn't make as much sense in some places as it does here?20:42
edmondswI'd think that was generally more of a hacking test than a UT, but in this case it's needed in UT20:43
efried    The positional decorator results in poorly maintainable code in20:43
efried    a misguided effort to emulate python3's key-word-arg only notation20:43
efried    and functionality. This patch removes keysteonauth's dependance20:43
efried    on the positional decorator.20:43
efriededmondsw: I guess you could ask Morgan about it.20:43
edmondswwill do20:44
efriededmondsw: However, I think in this case the decorator would have to be on Task.__init__, so wouldn't help us.20:44
efriedWe can assert that we called it with kwargs, though.20:44
edmondswwell, and positional was removed from g-r...
efriedCause if you assert called_with('name') it'll fail if you actually called it with name='name' and vice versa.20:45
edmondswoh, there you go20:45
efriedSo yeah, we can mock Task.__init__ and make sure we're calling it with a single arg with the key specified.  But are we really gonna do that friggin everywhere?20:46
edmondswefried kinda pointless unless you do it everywhere20:50
edmondswwe have what, about a dozen tasks?20:50
edmondswlooks like 13 in nova today20:50
efriedIt's more than just TaskFlow, really.20:51
efriedI mean, we could bite off TaskFlow for one.20:51
edmondswI would hope that we're mocking out most things besides TaskFlow20:51
efriedHow does that help our cause?20:52
edmondswe.g. pypowervm20:52
efriedAre we really going to assert every constructor call we make to anything we mock?20:52
edmondswyeah, at some point things just need to work20:53
edmondswUT can't catch everything20:53
efriedThere's a move in Nova to do much more functional testing.20:56
efriedWe would need to add some serious fixture framework to make that happen for us.20:57
esbergluefried: edmondsw: So you guys thinking we add tests for the task constructor calls?21:02
*** ChanServ has quit IRC21:02
efriedesberglu: IMO add one for this specific one and put a low-priority TODO to add it for every init of every constructor of every mocked class everywhere.21:03
efriedFor the sake of this patch, we don't want to extend the scope too far.21:03
*** ChanServ has joined #openstack-powervm21:10
*** sets mode: +o ChanServ21:10
esbergluefried: edmondsw:
esbergluAny idea why B is failing? Only diff is adding autospec=True21:14
efriedUgh, could it be because Task inherits its .name attr from *its* parent class?21:16
*** k0da has joined #openstack-powervm21:17
edmondswit actually says CreateAndConnectCfgDrive doesn't have name, not Task... so may indeed be a parent/child thing but the other direction21:17
edmondswmock doesn't seem to deal with __init__ well in general :(21:18
efriedYou can try addressing your mock just to Task, not Task.__init__.21:19
efriedThat's and add instance=True21:19
efrieds/That's a/A/21:19
efriedThat's how I normally do constructor mocks, I think.21:19
esbergluIdk that mriedem is going to be cool with just fixing the specific one and not adding tests for the rest21:36
esbergluWe shall see21:37
*** edmondsw has quit IRC21:39
efriedesberglu: He'll be cooler with that than extending the tests to unrelated code paths in this same patch.21:40
efriedesberglu: If you wanted to be super stringent and proper about it, you would first propose the patch that does it for everything, with the failing test assertions commented out with a bug number; then propose the fix on top and include uncommenting those assertions in that fix.21:41
efriedI seriously doubt he'd demand that, but he would probably be more impressed if you did it that way.21:41
esbergluefried: Would that 1st patch be allowed through at this point in the release? Not sure if patches have to be directly fixing bugs21:46
esbergluefried: Also, tried your instance=True suggestion, still getting the same21:48
*** apearson_ has quit IRC22:26
*** tjakobs has quit IRC23:06
*** k0da has quit IRC23:19
*** AlexeyAbashkin has joined #openstack-powervm23:21
*** AlexeyAbashkin has quit IRC23:26

Generated by 2.15.3 by Marius Gedminas - find it at!