Merge ~ubuntu-release/britney/+git/britney2-ubuntu:sru-regression-messages into ~ubuntu-release/britney/+git/britney2-ubuntu:master

Proposed by Łukasz Zemczak
Status: Merged
Merged at revision: bb032d98d874e36dc36ab2d4285f74da5dcd1556
Proposed branch: ~ubuntu-release/britney/+git/britney2-ubuntu:sru-regression-messages
Merge into: ~ubuntu-release/britney/+git/britney2-ubuntu:master
Diff against target: 0 lines
Reviewer Review Type Date Requested Status
Iain Lane Needs Fixing
Review via email:

Commit message

First stab on SRU ADT-regression bug messaging in britney. We use the LP e-mail interface for sending the messages to avoid additional dependencies on lpapi.

Description of the change

Probably still work-in-progress, sending out for initial review. I am still testing this to some extent.

First stab on SRU ADT-regression bug messaging in britney. We use the LP e-mail interface for sending the messages to avoid additional dependencies on lpapi.

Since the e-mail SMTP functions are basically the same as we use for the policy, I didn't include any unit tests for this part of the system.

Things to do for the future:
Right now we don't list the actual regressions in the bug. This might require more work (like ripping out the test-result-gathering from the policy), so I went in a two-stage approach, having this basic thing first.

To post a comment you must log in.
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Just a random thought popped up in case launchpadlib is too controversial:

17:02 <sil2100> I'm open to any other ideas how to achieve the same effect
17:03 <sil2100> (maybe there's some comment-on-bugs-through-e-mail as well? Not
                the best option, but we basically have an e-mail handler

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

Ok, this is now ready for re-review. As per Laney's recommendation, to get rid of the troublesome LP API dependency I have started using the e-mail interface instead. This might still need some tests in some staged instance before getting merged, but a review is more than welcome.

Revision history for this message
Iain Lane (laney) wrote :

Thanks Łukasz!

I've made some comments inline.

I think when we're ready we should merge and turn it on in dry-run mode, and then watch the logs for a bit to make sure it would have done the right thing.

It's a little bit concerning to me that the state files will only ever grow. I wonder if we could try to come up with a way to stop that? One idea I had was in `initialize()` to check all the bugs in the state file to see if they are finished (released/removed) and drop them when they are. Not sure if that would slow things down too much though.

This could also be extended to actually use the LP API to check if we already commented - using the state file to account for delays in email sending - and also drop the entry in that case.

review: Needs Fixing
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!

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

Anyway, the branch should be readyish for review if you have time - or if you think the LP comment-checking won't be too slow, I can rebase and push that version ASAP.

Preview Diff



People subscribed via source and target branches