| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-08-26 | |||
| 12:45:58 | bauzas | that's what I just call good habits | |
| 12:46:01 | stephenfin | yup, that's fair | |
| 12:46:47 | stephenfin | In this instance though, are you okay to leave it in the gate queue or should I pull it out and rework it? | |
| 12:46:50 | bauzas | stephenfin: on a fun side, should I *really* review https://review.opendev.org/#/c/742863/9 and all the type hints changes, given all the discussions we had in the past ? :D | |
| 12:47:04 | bauzas | stephenfin: nah, I said I can assume this being fixed automatically | |
| 12:47:12 | stephenfin | okay, cool | |
| 12:48:06 | bauzas | stephenfin: so, tbh, I guess you're doing https://review.opendev.org/#/c/742863/9/nova/compute/manager.py unrelated to the vTPM series, right? | |
| 12:48:07 | stephenfin | bauzas: Up to you :) It is really trivial, if that helps (not much hints being added, just getting mypy passing) | |
| 12:48:19 | bauzas | ahah, I see | |
| 12:48:32 | stephenfin | yeah, see my reply to lyarwood | |
| 12:48:35 | bauzas | you added type hints for the vtpm changes but this didn't work, gotcha | |
| 12:48:35 | stephenfin | I'm adding new type functions in the next change and I want to annotate them and validate those annotations. To enable validation, I need to fix a couple of issues with the file. That's what I'm doing here. | |
| 12:48:43 | stephenfin | exactly | |
| 12:49:10 | stephenfin | it felt wrong to add type hints but not validate them, and I couldn't enable validation without some additional work | |
| 12:49:20 | stephenfin | so I did that work separately since it's not totally related | |
| 12:49:22 | stephenfin | if that makes sense | |
| 12:49:33 | bauzas | stephenfin: yeah | |
| 12:50:20 | bauzas | that's not I wanna argue against type hints | |
| 12:50:34 | bauzas | if you wanna use them, this works for me | |
| 12:50:45 | bauzas | but let's not make it mandatory for all of us | |
| 12:51:06 | stephenfin | yeah, to be clear, I'm not asking it of anyone who doesn't want to add them | |
| 12:51:10 | sean-k-mooney | once a file has first been enabeld for type hints you can add them inline | |
| 12:51:15 | stephenfin | but I'd like to add them to my code. I find them helpful | |
| 12:51:15 | sean-k-mooney | instead of as a seperate patch | |
| 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 | |