| *** chhagarw has joined #openstack-powervm | 04:42 | |
| *** chhagarw has quit IRC | 04:56 | |
| *** chhagarw has joined #openstack-powervm | 05:30 | |
| *** chhagarw has quit IRC | 11:19 | |
| openstackgerrit | Matthew Edmonds proposed openstack/nova-powervm master: Use tempfile for powervm config drive https://review.openstack.org/613342 | 14:11 |
|---|---|---|
| openstackgerrit | Matthew Edmonds proposed openstack/nova-powervm master: Use tempfile for powervm config drive https://review.openstack.org/613342 | 14:21 |
| openstackgerrit | Matthew Edmonds proposed openstack/nova-powervm master: Use tempfile for powervm config drive https://review.openstack.org/613342 | 14:25 |
| edmondsw | efried mdrabe I think that's ready now ^ | 14:26 |
| efried | ack | 14:26 |
| efried | edmondsw: Could you have left the unit tests alone? | 14:28 |
| efried | I don't want to go crazy anal about this, but we're now missing coverage for the new method :( | 14:29 |
| edmondsw | efried I could have left test_driver alone, but this is definitely an improvement there | 14:32 |
| efried | edmondsw: Improvement in factoring out the name gen method, yes. But a regression in test coverage. | 14:32 |
| edmondsw | I thought about adding a UT for the new method, but I decided it wasn't really necessary... I'll look at it again | 14:33 |
| edmondsw | I don't think there's any regression in test coverage... but looking | 14:33 |
| edmondsw | efried you don't think the tests in test_media are sufficient for the new method? | 14:34 |
| efried | Previously the test boundary was at pypowervm; we were mocking the sanitize method. Now it's at the get_cfg_drv_name, and we have code (that method itself) that's not covered. | 14:35 |
| efried | Rephrase: now we have new code in nova-powervm that's not covered. | 14:36 |
| edmondsw | the new code is covered in test_crt_cfg_drv_vopt | 14:38 |
| edmondsw | I can refactor so that its in a separate method | 14:38 |
| efried | What I would actually like is to remove the mocking entirely, but that would be outside the scope of this patch. | 14:39 |
| edmondsw | efried what mocking are you referring to? | 14:41 |
| efried | mocking the sanitize method at all | 14:41 |
| efried | I know we used to have a policy of "mock anything at the boundary of nova-powervm" | 14:42 |
| efried | but I've since been convinced that the policy should be "mock as little as possible" | 14:42 |
| efried | like I said, outside the scope of this patch. | 14:42 |
| efried | When I revert the test_driver.py delta, the second test fails. Haven't dug in, but it seems like it shouldn't. | 14:43 |
| efried | ah | 14:44 |
| efried | because you've actually changed the algorithm | 14:44 |
| efried | you're no longer using the instance name; now you're using the UUID. | 14:44 |
| edmondsw | I think I did remove the sanitize mocking | 14:44 |
| edmondsw | yes, I'm using the UUID | 14:45 |
| efried | you replaced it with mocking the name gen method. My point is, I would like to remove all of that. | 14:45 |
| edmondsw | that is a necessary part of the fix | 14:45 |
| efried | but again, unrelated, let's not get distracted | 14:45 |
| edmondsw | see the bug report for why we changed from name to uuid | 14:45 |
| efried | right, that sounds familiar now. | 14:45 |
| edmondsw | will have a patch up in a minute to refactor the UT to explicitly cover the new method | 14:45 |
| edmondsw | I do agree with mocking as little as possible _outside_ of nova-powervm | 14:46 |
| openstackgerrit | Matthew Edmonds proposed openstack/nova-powervm master: Use tempfile for powervm config drive https://review.openstack.org/613342 | 14:51 |
| edmondsw | efried ^ | 14:51 |
| efried | edmondsw: +2. We need to add tests (in a separate patch) that walk the CONF.powervm.remove_vopt_media_on_boot condition. | 14:54 |
| edmondsw | efried test_spawn_with_cfg does that | 14:55 |
| edmondsw | well, and it tests the True case and test_spawn_ops tests the False case | 14:56 |
| efried | edmondsw: Hm, then why didn't that test fail at PS3? | 14:57 |
| edmondsw | because it was hardcoding 'cfg_' and '.iso' instead of using the constants | 14:58 |
| edmondsw | I assume you're referring to the const issue | 14:58 |
| edmondsw | oh, wait... hmmm | 14:58 |
| efried | yeah | 14:58 |
| efried | the test was hardcoding, which is the right thing. But the source was referencing the consts. So we must not have been hitting that source line somehow. | 14:59 |
| edmondsw | efried actually it did fail at PS3 | 14:59 |
| edmondsw | http://logs.openstack.org/42/613342/3/check/openstack-tox-py27/ce95354/testr_results.html.gz | 14:59 |
| edmondsw | the odd thing to me is that zuul never ran on PS1... but I guess that happens sometimes | 15:01 |
| edmondsw | must have proposed while there was a zuul issue | 15:01 |
| efried | okay, so we would have actually failed the gate on the +W'd patch set. That's good. | 15:03 |
| edmondsw | yep | 15:05 |
| efried | mdrabe: I'll wait for you to do the pvc sniff before approving. | 15:10 |
| mdrabe | Yep I'll do that today | 15:11 |
| efried | thank you sir | 15:17 |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!