| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-20 | |||
| 15:44:37 | efried | ++ | |
| 15:45:02 | efried | Doesn't have to be shared utils per se, just a separate module that tests trait-y things but not libvirt-specific. | |
| 15:45:08 | efried | Then the libvirty one can inherit from that. | |
| 15:45:08 | aspiers | right | |
| 15:45:29 | efried | Do the refactor in a new patch underneath, if you please. | |
| 15:45:49 | efried | If it's straightforward enough, I might be able to ninja approve it. | |
| 15:46:37 | aspiers | OK | |
| 15:47:30 | efried | yeah, test_servers is nearly 7kloc. That's too much | |
| 15:47:35 | aspiers | agreed | |
| 15:48:32 | aspiers | efried: should I first just extract TraitsTrackingTests to a separate file? | |
| 15:49:00 | efried | yeah, and TraitsBasedSchedulingTest with it | |
| 15:49:01 | aspiers | or that and ProviderTreeTests? | |
| 15:49:20 | aspiers | something like test_server_traits.py? | |
| 15:49:33 | bnemec | efried: Worth noting that the save_and_reraise context was to work around a bug in eventlet that I believe has since been fixed. | |
| 15:49:37 | aspiers | or test_server_provider_tree.py | |
| 15:49:47 | bnemec | I'm not sure you still have to worry about log calls and such messing with the exception context. | |
| 15:50:06 | efried | bnemec: oh, I didn't know that. It makes sense to me that intervening code could still muck with the exception context. | |
| 15:50:40 | efried | bnemec: or is well-behaved code supposed to not do that | |
| 15:51:16 | bnemec | It was _probably_ eventlet doing something naughty. | |
| 15:51:55 | bnemec | I'm sure it could still happen, but it's probably not common anymore. | |
| 15:53:14 | bnemec | (my overuse of probably is pretty representative of my confidence in this ;-) | |
| 15:53:36 | efried | adrianc: in any case, 'raise' rather than 'raise e' is appropriate | |
| 15:53:36 | efried | bnemec: So you're not yet recommending that we go through and rip out all our uses of that context manager? | |
| 15:53:38 | bnemec | I guess I'm not aware of anyone running into a bug that required save_and_reraise since the eventlet fix though. | |
| 15:54:15 | bnemec | efried: No, I don't think it hurts anything to have it there. | |
| 15:54:17 | efried | aspiers: test_servers_provider_tree seems like a winner. | |
| 15:54:24 | aspiers | ok | |
| 15:54:31 | bnemec | And +1 to the raise vs. raise e point. | |
| 15:58:13 | efried | Recent guidance is just to include the license. But legally, since you're porting code from A to B, you should preserve A's copyright and license header. Sorry. | |
| 15:59:43 | efried | It is stupid. I've actually spent hours with lawyers on this. | |
| 16:00:21 | aspiers | Haha my sympathies | |
| 16:03:41 | cdent | FIGHT THE POWER | |
| 16:10:12 | aspiers | :) | |
| 16:10:20 | openstackgerrit | Adam Spiers proposed openstack/nova master: Extract provider tree functional tests into new file. https://review.opendev.org/660157 | |
| 16:11:00 | aspiers | efried: ^^^ | |
| 16:11:04 | efried | on it | |
| 16:13:37 | aspiers | Why the hell did I finish the commit title with a period | |
| 16:13:56 | aspiers | I never do that | |
| 16:16:08 | efried | arght, /me retracts +2! | |
| 16:16:18 | efried | whatever | |
| 16:16:39 | aspiers | ;) | |
| 16:16:43 | efried | fix it or not. It's only your reputation at stake, permanently and indelibly etched into git | |
| 16:16:50 | aspiers | ROFL | |
| 16:17:22 | aspiers | I see plenty of other trailing '.' in the commit history, thankfully | |
| 16:17:38 | aspiers | I'll just be joining a distinguished group of disgraced engineers | |
| 16:17:56 | aspiers | Hmm, "Disgraced Engineer" sounds like a job title | |
| 16:18:05 | aspiers | Maybe that's in my career track | |
| 16:18:58 | aspiers | Crap, now I have no way to re-submit the SEV trait change without rebasing all the other SEV stuff on top of this | |
| 16:19:10 | aspiers | or without rebasing this on top of the other SEV stuff | |
| 16:19:18 | aspiers | This appears to be a flaw in the Gerrit model | |
| 16:20:03 | efried | aspiers: Rebasing the whole series should be as easy as rebasing one patch. | |
| 16:20:32 | efried | aspiers: except it looks like you may not have been done rebasing the series on itself. | |
| 16:20:54 | aspiers | From the CLI perspective maybe, but it creates a load of CI churn and also resets existing +1 / +2 | |
| 16:21:05 | aspiers | And it's hard enough to get those precious votes already | |
| 16:21:18 | efried | rebasing doesn't reset votes. | |
| 16:21:31 | efried | unless it involves actual code changes | |
| 16:21:41 | aspiers | Doesn't that depend on the patch-id remaining constant? ... right | |
| 16:21:58 | efried | Change-Id has to stay constant. | |
| 16:22:08 | aspiers | I mean the git patch-id | |
| 16:22:22 | efried | I don't know what that is. You mean the commit hash? | |
| 16:22:26 | aspiers | nope | |
| 16:22:28 | aspiers | https://git-scm.com/docs/git-patch-id | |
| 16:22:42 | efried | ah | |
| 16:22:50 | aspiers | It's how git does clever stuff during rebase, for instance | |
| 16:22:52 | sean-k-mooney | aspiers if its a commit message only change i thik it keeps previous votes | |
| 16:22:52 | efried | yes, in spirit. | |
| 16:23:06 | efried | sean-k-mooney: Pretty sure not ^ | |
| 16:23:08 | aspiers | sean-k-mooney: it's not commit message only | |
| 16:23:12 | aspiers | in this case | |
| 16:23:28 | aspiers | (We moved on from the trailing "." discussion to something more serious) | |
| 16:23:39 | efried | In this case you don't have any votes yet on the patch that's consuming the refactor, right? | |
| 16:23:50 | efried | because you're having to address some annoying core's -1 | |
| 16:24:30 | sean-k-mooney | aspiers: we dont actully enforce the no trail . in the hacking checks if i rememebr | |
| 16:24:40 | aspiers | https://review.opendev.org/#/c/638680/ depends on https://review.opendev.org/#/c/633855/ which depends on https://review.opendev.org/#/c/655268/ | |
| 16:24:56 | sean-k-mooney | so sinc its not enforced by the ci or atleast was not always enforced it is likely not consitent | |
| 16:25:04 | aspiers | Now I need https://review.opendev.org/#/c/638680/ to *also* depend on this test_servers.py refactoring | |
| 16:25:30 | efried | oh, I see, you want to make a downward-facing tree | |
| 16:25:32 | aspiers | sean-k-mooney: like I said there are tons of trailing "." already in the history. That was just a joke ... | |
| 16:25:36 | efried | yeah, don't bother with that | |
| 16:25:42 | efried | just put the refactor on the bottom of the pile. | |
| 16:25:51 | aspiers | The top of the pile? | |
| 16:26:20 | aspiers | 655268 already has lots of + votes | |
| 16:26:21 | efried | "bottom" from the perspective of the 'Related changes' list on the top right of the gerrit panel. | |
| 16:26:44 | aspiers | Is that list ordered? | |
| 16:26:45 | efried | right, and if you rebase that onto the refactor, you'll preserve those votes. | |
| 16:26:54 | efried | aspiers: Yes, "first" is at the bottom | |
| 16:27:08 | efried | etc | |
| 16:28:27 | aspiers | Hrm | |
| 16:28:42 | aspiers | Yeah, this is *definitely* a flaw in the Gerrit model if it doesn't support doing this properly | |
| 16:29:04 | aspiers | A good model should definitely not force artificial dependencies between changes like this | |
| 16:30:19 | aspiers | We're assuming here that Gerrit can only handle a linear sequence of dependencies | |
| 16:30:30 | aspiers | Or maybe you know that for a fact | |
| 16:30:39 | aspiers | Either way, it should be able to handle trees | |
| 16:30:54 | efried | say wha? | |
| 16:30:57 | efried | oh | |
| 16:31:12 | efried | you can do upward-facing trees (again, upward from the perspective of Related Changes) | |
| 16:31:28 | efried | That is, you can have both B and C depend on A but not on each other. | |
| 16:31:38 | efried | gerrit will show B and C in arbitrary order, but both will always be above A | |
| 16:31:46 | aspiers | Yes, but this is the opposite way up | |
| 16:31:54 | aspiers | A depending on both B and C | |