| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-08-26 | |||
| 12:51:34 | sean-k-mooney | but when your ading a new file you often have to update unrelated code to get it to pass | |
| 12:51:49 | sean-k-mooney | well semingly unrelated | |
| 12:52:49 | bauzas | there are two ways of seeing type hints | |
| 12:53:03 | stephenfin | for expected backports, melwitt and others have asked I do them separately to keep changes to the backport to a minimum, which I can deal with :) | |
| 12:53:03 | stephenfin | sean-k-mooney: yeah, that's what I'm doing for new code at least | |
| 12:53:10 | bauzas | either as a terrible waste of time for reviewers, or as an opportunity for code readability and static checks | |
| 12:53:29 | sean-k-mooney | bauzas: yep i am stongly in teh latter camp | |
| 12:53:31 | stephenfin | s/type hints/tests/ | |
| 12:53:33 | bauzas | I'm not opiniated on any of both, I just try to be pragmatic | |
| 12:54:04 | sean-k-mooney | lack of type hints is why i hated python for the first 2-3 years of working on openstack | |
| 12:54:07 | stephenfin | same argument can be made, though the value of tests is definitely more significant | |
| 12:54:22 | bauzas | my biggest concern being this a py3 feature, but given we now have a good consensus on things we can vs we can't do, I'm okay | |
| 12:54:23 | stephenfin | but it's also dead code from the perspective of production | |
| 12:54:23 | sean-k-mooney | wehn we started adding the parms comment it helped a bit but not as much as type hints | |
| 12:54:56 | bauzas | adding only type hints on new code seems reasonable... but... | |
| 12:55:06 | stephenfin | the current situation works for me (add them as you're going, and only if you want to) | |
| 12:55:16 | stephenfin | bauzas: to be clear, new code or modified code | |
| 12:55:20 | bauzas | there is a trade-off, the more we're using them, the higher chances we can have merge conflicts with backporting | |
| 12:55:35 | sean-k-mooney | yes which i think is ok | |
| 12:55:52 | sean-k-mooney | we will have merge conflitcs anyway | |
| 12:56:08 | sean-k-mooney | due to python3 vs python 2 | |
| 12:56:15 | lyarwood | ha if you think this is going to cause merge conflicts wait until you see stephenfin's func test series :D | |
| 12:56:36 | sean-k-mooney | which one there is one every cycle :P | |
| 12:56:40 | lyarwood | it's all worthwhile however :) | |
| 12:57:00 | sean-k-mooney | yep stephenfin has been making the func tests more and more useful | |
| 12:57:03 | bauzas | well, I don't know for you folks but I'm working in a company that promises 5-year support for OpenStack | |
| 12:57:08 | stephenfin | as do I. I think we add a little _too_ much weight to preserving backportability, at the expense of making master worse | |
| 12:57:09 | stephenfin | gibi: good spot on the chown thing | |
| 12:57:09 | stephenfin | i.e. don't move these lines around because it'll make the backport harder, even though doing so would make things significantly easier to grok on master | |
| 12:57:37 | lyarwood | bauzas: you missed a downstream discussion around stable/train while you were out FWIW | |
| 12:57:58 | bauzas | damn, it's a cabal ! | |
| 12:58:02 | stephenfin | gibi: Functional tests should have caught that. I don't know why it didn't for sure but I suspect the dumb mocking of that cleanup routine is the culprit though | |
| 12:58:20 | sean-k-mooney | lyarwood: did i also miss that i dont rememebr it | |
| 12:58:45 | gibi | stephenfin: if you can catch those things in func test then I can ease on my tempest requirements | |
| 12:58:47 | bauzas | lyarwood: I feel like I was going to be caught in Varennes | |
| 12:58:47 | stephenfin | lyarwood: you mean the question of backporting functional test rework changes | |
| 12:58:57 | bauzas | that's a coup ! | |
| 12:59:09 | lyarwood | sean-k-mooney: I think you were there | |
| 12:59:11 | lyarwood | stephenfin: yeah | |
| 12:59:14 | sean-k-mooney | oh that | |
| 12:59:16 | stephenfin | what does a Frenchman know of coups | |
| 12:59:21 | sean-k-mooney | dont we always backport them | |
| 12:59:28 | sean-k-mooney | to keep as close to master as possible | |
| 12:59:37 | sean-k-mooney | to make future backports simpler | |
| 12:59:38 | lyarwood | not always | |
| 12:59:40 | stephenfin | sometimes yes, sometimes no | |
| 12:59:52 | sean-k-mooney | ok i guess it depend on how invasive it is | |
| 13:00:00 | stephenfin | lyarwood's suggesting we always do, even if there are quite a few, since it will ultimately make our lives easier | |
| 13:00:04 | sean-k-mooney | just being pargmatic about it | |
| 13:00:18 | stephenfin | and it's test only so impact on stability should be non-existent | |
| 13:00:26 | lyarwood | yup | |
| 13:00:32 | bauzas | I see | |
| 13:00:43 | bauzas | well, for functional test only changes, this could work | |
| 13:01:23 | sean-k-mooney | bauzas: i think it came up in the context of one of melwitt patches where she would have had to rewrite the tests if she did not backport a refactor patch | |
| 13:01:35 | sean-k-mooney | and i suggested she shoudl backport the refactor | |
| 13:01:44 | sean-k-mooney | to avoid that and keep the backport clean | |
| 13:01:48 | stephenfin | gibi: Also, it's always been broken, i.e. blame efried, not me :P https://review.opendev.org/#/c/639934/30/nova/virt/libvirt/utils.py@691 | |
| 13:02:07 | bauzas | sean-k-mooney: which makes sense from a pragmatic pov | |
| 13:02:13 | efried | I blame reviewers | |
| 13:02:36 | bauzas | but again, we all sign-off for maintaining code for half a decade, refrain your style changes | |
| 13:02:58 | sean-k-mooney | bauzas: or backport them | |
| 13:03:25 | bauzas | we all = those who wear some head coverage with a color (that is less red and more blue) | |
| 13:03:27 | sean-k-mooney | the more consitent we are the eaiser it is to grock the code | |
| 13:03:36 | efried | I don't actually recognize that code; I may get to blame mdbooth | |
| 13:03:55 | bauzas | sean-k-mooney: I'd say that providing style changes in an implementation patch is terrible and we should avoid this | |
| 13:04:02 | efried | or cfriesen | |
| 13:04:06 | gibi | stephenfin, efried: I can take the blame found the issue :) | |
| 13:04:12 | gibi | finding even | |
| 13:04:15 | sean-k-mooney | right but type hints are not a style change | |
| 13:04:20 | sean-k-mooney | they are a type of testing | |
| 13:04:46 | lyarwood | have to say I disagree bauzas, we'd never be able to move the codebase forward otherwise | |
| 13:04:55 | sean-k-mooney | its technically a liniting enhancement | |
| 13:05:11 | stephenfin | as someone who's paid to work on the libvirt module, so do I | |
| 13:05:14 | bauzas | gibi: the story of my life https://i.pinimg.com/564x/c5/de/f4/c5def4bd565b44f4d27e1b7478dfa528.jpg | |
| 13:05:17 | stephenfin | so. much. cruft. | |
| 13:05:28 | sean-k-mooney | bauzas: hehe | |
| 13:05:33 | lyarwood | awww ./me hugs nova.virt.libvirt | |
| 13:05:45 | stephenfin | heh | |
| 13:05:47 | bauzas | lyarwood: stephenfin: I don't say we shouldn't modify code | |
| 13:06:08 | sean-k-mooney | bauzas: worst part of git blame. who wrote this crap.. damb it was was me... | |
| 13:06:21 | stephenfin | bauzas: well, take things like this https://review.opendev.org/#/c/631363/61/nova/tests/unit/virt/libvirt/fakelibvirt.py@135 | |
| 13:06:32 | efried | Hah! Yes! I get to blame Chris! https://review.opendev.org/#/c/639934/7..8/nova/virt/libvirt/utils.py@630 | |
| 13:06:35 | stephenfin | personally, I think that's a good thing | |
| 13:07:24 | gibi | efried: good, he is not around to pass it forward :) | |
| 13:07:32 | efried | Perfect | |
| 13:07:56 | sean-k-mooney | oh recursive=true | |
| 13:08:03 | stephenfin | I mean, I'm touching that code and the additional change makes the whole thing better | |
| 13:08:13 | bauzas | stephenfin: again, no disagreement, just saying that the more we can decouple style changes from implementation changes, the better it would be as we could pull those style changes down to the stable branches without also pulling fancy new features | |
| 13:08:35 | sean-k-mooney | stephenfin: did you see if this is need for livemiration by the way. i think we dont have too copy for live migration but we never validated that | |
| 13:08:49 | sean-k-mooney | stephenfin: so currently we do just to be safe | |
| 13:09:04 | stephenfin | sean-k-mooney: Currently I have live migration blocked because I haven't validated it | |
| 13:09:20 | sean-k-mooney | ok well when you do can you try commenting out the tpm data copy | |
| 13:09:22 | stephenfin | I do have a DevStack env on your cluster that I'll use to validate it once the rest has merged though | |
| 13:09:23 | sean-k-mooney | and see if it works | |
| 13:09:28 | bauzas | stephenfin: fortunately, git is smart enough to handle conflicts on blank lines | |
| 13:09:39 | stephenfin | sean-k-mooney: If we're talking about the same thing, then that's for cold migration | |
| 13:09:43 | bauzas | (or it should) | |
| 13:09:51 | sean-k-mooney | https://review.opendev.org/#/c/639934/30/nova/virt/libvirt/utils.py@691 | |
| 13:09:56 | sean-k-mooney | the save and restore | |
| 13:10:16 | stephenfin | Yes, that is necessary. I checked the libvirt docs and included the relevant snippet in the commit message | |