Code review comment for ~ubuntu-release/britney/+git/britney2-ubuntu:sru-regression-messages

Revision history for this message
Ɓukasz Zemczak (sil2100) wrote :

Thanks for the review Laney! I have addressed some of your inline concerns, with a few comments/exceptions below. Anyway, the statefile should be now getting cleaned (I left a TODO for that and forgot about it last time, sorry!).

1) So I actually also wanted to do LP checks for packages instead of using a statefile at first when working on the initial branch, but then I noticed how many LP calls this would result in. I might be wrong here as I don't know the britney code as well as you, but without the statefile it felt to me that we'd have to do a lot LP calls and matching operations. Every time we'd be checking if a package considered by britney has bugs, then parsing the bugs, then fetching the comments (messages), then searching through them for our comments. And this would be happening for every package with autopkgtest regressions, even if we already commented. Sure, it's probably not *that* many packages as I originally though, but it's still a lot of calls and busywork.

Where with a statefile, no browsing through LP bug comments is needed, and the only additional calls we need to do is on britney initialization to drop the old packages if they're gone from -proposed (guess maybe we could do that somehow with the info from britney even?). It shouldn't slow things much down as it only happens once per run. Minus of this approach is that if we lose the statefile, all hell's going down and we send duplicate comments.

Anyway, thinking about it now I guess we could do it with checking bug comments for comments (or use some tags for tagging the bugs? But that sounds error-prone). I was a bit concerned it might be a bit suboptimal though. If you feel we should anyway try it this way, I do have an older branch with the code for that - might just need to rebase it.

2) The strange state[distro][series][package] thing - well, I know it doesn't make much sense in case when britney's running once per distro-series (sorry, forgot about that when I was writing it!), but it's actually somewhat convenient. The info regarding which distro and series we're running on is useful during old package cleanup (this state info is needed!) and it seemed much nicer to fetch it this way than adding 2 separate distro and series dictionary items to the statefile that would be 'updated' with the same contents on each policy run.
Or maybe there is a way to fetch that info from the britney object? If yes, I guess I could re-consider.

3) Apologies for the ExitStack() pieces! It's just very convenient for me to use that, a bit more than the regular @patch pieces. It's something we use a lot in ubuntu-image (Barry infected me with this pythonism). For now I left those alone (since ETOOMANYTHINGS), but if you really think it's super-unreadable, I can try rewriting the tests.

Sorry for not addressing all of the issues, I guess there's an overload of things to do recently!

« Back to merge proposal