| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-02-17 | |||
| 21:08:50 | prometheanfire | sean-k-mooney: good point, but that's on the projects themselves :P | |
| 21:09:15 | sean-k-mooney | sure i was asking more for nova | |
| 21:09:16 | efried | sean-k-mooney: I don't even know how we would do that. Can you propose a patch? | |
| 21:09:27 | efried | sean-k-mooney: Like, how would we prevent picking up transitive deps etc? | |
| 21:09:55 | sean-k-mooney | ya so i dont know if we could prevent it | |
| 21:10:04 | sean-k-mooney | but we could just stop listing it in our test requirements | |
| 21:10:24 | sean-k-mooney | eventurally if we got all the project to do that we would stop using it | |
| 21:10:40 | efried | prometheanfire: At a quick look, there are on the order of a hundred uses of mock spec in nova. I would rather find a better way to fix this than hacking dir() into each of them (and it *is* a hack). | |
| 21:11:45 | efried | sean-k-mooney: since it's in the test env, hopefully projects only have it in their test-requirements, which *should* mean the only transitive deps we would have to worry about would be things like oslotest. But... no idea. | |
| 21:12:12 | prometheanfire | efried: if upstream considers it a bug (aka, they plan on fixing something) we can blacklist 4.0.0/4.0.1 | |
| 21:12:24 | prometheanfire | otherwise we are in denial | |
| 21:12:27 | efried | prometheanfire: okay, that was going to be my next question: also broken in 4.0.0? | |
| 21:12:43 | prometheanfire | efried: not sure, this jumped over it, but I imagine it has the same issue | |
| 21:12:51 | sean-k-mooney | ya so os-vif does not use it as far as i know. ill remove it if it does. os-brick might but other then that the only other poject nova really depends on is oslo | |
| 21:13:03 | sean-k-mooney | i guess hacking also | |
| 21:13:55 | efried | prometheanfire: okay, so we need to make sure an upstream issue is opened. They may kill it, but at least the path would be clear. If you don't know of one already, or know of someone who would know, I can go digging... | |
| 21:14:08 | efried | johnsom: ^ ? | |
| 21:14:40 | johnsom | Reading scroll back | |
| 21:14:52 | efried | meanwhile I'll try a local build with mock removed from reqs and see if the venv still installs it... | |
| 21:15:12 | sean-k-mooney | ya so bunch of oslo libs use it | |
| 21:15:14 | sean-k-mooney | http://codesearch.openstack.org/?q=mock&i=nope&files=test-requirements.txt&repos=openstack/oslo-cookiecutter,openstack/oslo-specs,openstack/oslo.cache,openstack/oslo.concurrency,openstack/oslo.config,openstack/oslo.context,openstack/oslo.db,openstack/oslo.i18n,openstack/oslo.limit,openstack/oslo.log,openstack/oslo.messaging,openstack/oslo.middleware,openstack/oslo.policy,openstack/oslo.privsep,opensta | |
| 21:15:16 | sean-k-mooney | ck/oslo.reports,openstack/oslo.rootwrap,openstack/oslo.serialization,openstack/oslo.service,openstack/oslo.tools,openstack/oslo.upgradecheck,openstack/oslo.utils,openstack/oslo.versionedobjects,openstack/oslo.vmware,openstack/oslotest | |
| 21:15:22 | efried | f | |
| 21:15:38 | johnsom | backport candidate is a tool the Octavia cores are using to track patches for backport to stable branches. We added that for our repos. | |
| 21:16:02 | johnsom | Since.... Storyboard...... Enough said | |
| 21:16:29 | efried | johnsom: I was poking you about whether there's a known bug opened against mock for this | |
| 21:16:33 | sean-k-mooney | efried: what we could do is update the requriements file only install mock on python2 | |
| 21:17:17 | johnsom | efried I burned my morning trying to fix a cause and or fix for this. No luck. I'm also not sure this is the same issue your project has with the UC changes. | |
| 21:17:21 | sean-k-mooney | we would then need to update all the projects. | |
| 21:17:25 | efried | mock;python!=3.xx | |
| 21:17:25 | efried | It might be possible to say | |
| 21:17:25 | efried | But taking that idea... | |
| 21:17:25 | efried | sean-k-mooney: not sure that helps, | |
| 21:17:46 | efried | johnsom: yeah, valid point, I didn't actually look. | |
| 21:17:53 | johnsom | We are having the same discussion, whether we should be using this package or reverting to the built-in now. | |
| 21:17:54 | sean-k-mooney | ya so we have a way to match based on python version in the requeriments file already | |
| 21:18:35 | efried | well, fwiw, simply removing mock from test-requirements didn't stop it from installing in the venv. Imagine we would have to yank it from oslotest and oslo.vmware at least. | |
| 21:18:37 | sean-k-mooney | efried: i can ask the requirements folk about it tomorrow seperate form the current disucssion | |
| 21:18:52 | sean-k-mooney | efried: yes we would | |
| 21:19:08 | sean-k-mooney | efried: if you mannually remove it form the enve does it resovel the issue | |
| 21:19:18 | efried | right -- prometheanfire fyi ^ unless you know a way we can explicitly prevent mock from installing, we'll still get it from transitive if we just remove it from our own test-requirements. | |
| 21:19:41 | johnsom | Agreed, it is a total hack, but it was only in "legacy" code for us, so we were like.... good enough, it's going to die anyway. | |
| 21:20:06 | prometheanfire | heh, that's a good point | |
| 21:20:35 | prometheanfire | efried: I don't know of one yet | |
| 21:21:25 | efried | johnsom: I totally wasn't judging you btw :P | |
| 21:21:33 | sean-k-mooney | prometheanfire: have you confirmed if removing the lib version resolved the issue | |
| 21:21:40 | sean-k-mooney | also what is the issue? | |
| 21:22:14 | efried | https://ad2781ccb869c0ef3e99-0fce708e89812c28d28e2be1213f1c9b.ssl.cf5.rackcdn.com/707064/10/check/cross-nova-py36/d1c7fa6/testr_results.html | |
| 21:22:21 | prometheanfire | I don't have a link to an issue yet, this is bigger than I first though that it was | |
| 21:22:52 | johnsom | Yeah, your logs don't look like the same issue we had | |
| 21:23:35 | sean-k-mooney | "ValueError: Sentinels must not start with _" ? | |
| 21:23:40 | johnsom | Ah, ok, some are kind of similar | |
| 21:23:50 | johnsom | Yeah, that appears to be on a "spec" mock | |
| 21:23:52 | prometheanfire | heh https://github.com/testing-cabal/mock/pull/482 | |
| 21:25:29 | sean-k-mooney | right so we could just replace all uses of import mock with import unittest.mock? | |
| 21:25:45 | sean-k-mooney | to get rid of the dep on mock the lib | |
| 21:25:53 | efried | "just" | |
| 21:25:56 | sean-k-mooney | if we wanted too in the futrue and add a hacking check | |
| 21:26:24 | sean-k-mooney | efried: its hopefully 1 sed command but yes | |
| 21:26:29 | sean-k-mooney | it could explode | |
| 21:26:43 | efried | 384 occurrences. | |
| 21:27:38 | prometheanfire | well, there's a way to find out I guess | |
| 21:28:06 | efried | yeah, working on it. | |
| 21:28:57 | sean-k-mooney | there has been a long standing issue in nova where we cant use self.assertRaises as a context manager in a with statement which im pretty sure is caused by mock the lib by the way whic this might also fix | |
| 21:29:07 | efried | prometheanfire, sean-k-mooney, johnsom: so yeah, that fixes it, at least for the one instance I tried. | |
| 21:29:21 | efried | s/^import mock$/from unittest import mock/ | |
| 21:29:25 | prometheanfire | heh | |
| 21:29:36 | johnsom | Hmmm, so tempting..... grin | |
| 21:29:55 | sean-k-mooney | can you do that for all nova and see if the tests still run | |
| 21:29:59 | johnsom | Yeah, I think I will try that too | |
| 21:30:04 | efried | sean-k-mooney: yeah but I'm not gonna run locally. | |
| 21:30:11 | efried | My laptop will burn a hole to the center of the earth. | |
| 21:30:14 | efried | I'll let zuul do it. | |
| 21:30:19 | sean-k-mooney | ok | |
| 21:30:22 | johnsom | It's a mock revolt. lol | |
| 21:31:01 | sean-k-mooney | if it does have issue i guess we could just do it selectivly to solve the issues we currently have until either mock is fixt or we fix the issues we find | |
| 21:32:01 | efried | sean-k-mooney: the sed command works fine... except for the pep8 requirement to sort imports :P | |
| 21:32:28 | sean-k-mooney | autopep8 might fix that for you | |
| 21:32:50 | sean-k-mooney | unlike black it only fixes the minium amount of stuff to pass pep8 | |
| 21:33:00 | sean-k-mooney | so it does not break the worlds and passes our gate | |
| 21:33:47 | sean-k-mooney | anyway we coudl cross that bridge when we see if it even passes the zuul run | |
| 21:37:22 | efried | prometheanfire: having proven this is a bug in the mock library, are you more convinced that we should blacklist the mock versions? | |
| 21:38:13 | sean-k-mooney | got to love that commit message https://github.com/testing-cabal/mock/commit/9e5e0380626fd3c540aa4799df0e794cf24d16aa | |
| 21:38:34 | prometheanfire | efried: ya, kinda sucks, but ok | |
| 21:38:39 | openstackgerrit | Eric Fried proposed openstack/nova master: WIP: Use stock mock https://review.opendev.org/708262 | |
| 21:38:56 | efried | prometheanfire: if you want to try making your patch dep on ^ ... | |
| 21:39:13 | efried | sean-k-mooney: --^ | |
| 21:39:14 | johnsom | Yeah, octavia seems to pass fine with the native mock | |
| 21:39:39 | efried | o queso we need a bug against mock | |
| 21:40:23 | efried | am I reading this right that there are zero open issues? | |
| 21:40:28 | efried | am I looking at the right repo? | |
| 21:40:44 | sean-k-mooney | and 404 closed | |
| 21:40:58 | johnsom | Zero for that repo | |
| 21:41:13 | johnsom | Though they pull in from the native bugs too | |
| 21:41:40 | sean-k-mooney | i guess they just fix them quickly or close them as wont fix | |
| 21:41:42 | prometheanfire | efried: well, for me I'm blacklisting on the reqs side, not nova side | |
| 21:41:51 | sean-k-mooney | e.g. they dont leave them open for log | |
| 21:41:54 | sean-k-mooney | *long | |
| 21:43:42 | efried | prometheanfire: I assume we would like to be able to continue following upgrades here, given that trying to fix the transitive deps through all of openstack would be hard and take long. So we should still open an issue for this. Agree? | |
| 21:44:15 | sean-k-mooney | efried: if it broke us it will proably break others | |