Bodhi comment directive module
AbandonedPublic

Authored by jskladan on Mar 26 2014, 1:33 PM.

Details

Summary

This is an initial version of the Bodhi comment directive.

/libtaskotron/bodhi_comment_utils.py are the bits'n'pieces taken directly from AutoQA with minimal changes
/libtaskotron/check.py adds the ability to load TAPv13 output into a list of CheckDetails
/libtaskotron/rpm_utils.py as per tflink's comment on D10 this is the place for rpm-related methods

Note that this code is dependent on some pieces of code that are under development right now:

  • bodhi_utils from pschindl's T47
  • config from kparal's feature/config branch in the GIT

There are also some points that require attention and/or further action

  1. How to get the URL's of results/logs (/libtaskotron/directives/bodhi_directive.py at line #74)
  2. What exactly does BodhiUpdateState in /libtaskotron/bodhi_comment_utils.py do, and how to sensibly remove the hard-coded references to depcheck & upgradepath
Test Plan

None so far

Diff Detail

Branch
feature/T46-bodhi-comment-directive
Lint
No Linters Available
Unit
No Unit Test Coverage
tflink requested changes to this revision.Mar 27 2014, 7:30 AM

Overall, it's not bad. As mentioned, it's missing quite a few bits (config, bodhi_utils).

I'm going to be a bit of a broken record here, but you really need to add some test coverage. Reporting and notifications are one of those areas where errors will be very visible and problematic. Has this code been tested locally yet?

What exactly does BodhiUpdateState in /libtaskotron/bodhi_comment_utils.py do, and how to sensibly remove the hard-coded references to depcheck & upgradepath

If you don't have an answer for this I have to wonder why it's a part of this review request. The AutoQA code was designed It's a piece of code to determine the current state of an update - whether it needs a comment, whether that comment should send an email from bodhi etc.

libtaskotron/bodhi_comment_utils.py
31

I realize that this was originally stuff I wrote but I'd like to see us drop the dep on peak.util.symbols. At the absolute very least, it needs to be declared as a dep

153

This should be a log statement

373

this needs to be a logging statement

libtaskotron/check.py
305

This should at least attempt to output some more details on why the parsing failed

libtaskotron/directives/bodhi_directive.py
21

This import is not needed

57

Shouldn't this be TaskotronError?

ad: "If you don't have an answer for this I have to wonder why it's a part of this review request. The AutoQA code was designed It's a piece of code to determine the current state of an update - whether it needs a comment, whether that comment should send an email from bodhi etc."
I, of course, do know, what is the code supposed to do. As per the initial comment, these are the pieces (of most probably yours) code taken directly from AutoQA. What I was trying to say is, that especially the BodhiUpdateState is quite a mess (and I found no tests for it), and I'm looking for the reasonings behind its existence, as it is there just to provide some extra logic for some specific tests.

My goal here is to find out whether the test-specific behaviour (especially for upgradepath, as the depcheck-parts seem to be quite generic and widely applicable), should be somehow included in general, which would mean adding an option for "acting in a way the code actually does for upgradepath, so any test can use it", or whether this is something the test (upgradepath) should take care of, or even if that is something we want to do in general (refering especially to "# since upgradepath is only run on updates-pending, set it to passed if we're working on updates-testing").

Other than that, I do fully agree with the comments. The test suite is being worked on :)

A few more nitpicks from me, by skimming the source code in 1 minute ;-)

It might be worth the discussion whether we wouldn't want to get rid of BodhiUpdateState completely and simply send a new bodhi email for each check update (except for honoring the minimum delay between failed reports). Because it was way too hardcoded before and that won't fit into our future endeavors. On top of that, I believe we will be trying to get rid of bodhi emails completely soon.

libtaskotron/bodhi_comment_utils.py
20–21

Missing module docstring.

libtaskotron/check.py
305

log.exception() is very good for that. And you can even make the ValueError value a part of the raised error.

libtaskotron/directives/bodhi_directive.py
19

Missing module docstring.

32–33

Or at least the class docstring.

libtaskotron/rpm_utils.py
20–21

Missing module docstring.

I, of course, do know, what is the code supposed to do. As per the initial comment, these are the pieces (of most probably yours) code taken directly from AutoQA. What I was trying to say is, that especially the BodhiUpdateState is quite a mess (and I found no tests for it), and I'm looking for the reasonings behind its existence, as it is there just to provide some extra logic for some specific tests

Actually, BodhiUpdateState is one of the few parts of the autoqa codebase that does have unit tests:
https://git.fedorahosted.org/cgit/autoqa.git/tree/lib/autoqa/tests/test_bodhi_update_state.py

BodhiUpdateState was created to address the frustration that packagers were expressing about the amount of email that they were receiving from bodhi as a result of AutoQA comments. It exists to ensure that emails are only sent on test failure or state change. The biggest change is that email is not sent if all the AutoQA tests pass for a given update. In order to do this, we needed to have a definition of "complete" and that led to the hard coding of specific test names. I still think it was an appropriate solution at the time but I agree with @kparal that it may not be appropriate for porting now since any definition of "complete" will be difficult to determine.

It might be worth the discussion whether we wouldn't want to get rid of BodhiUpdateState completely and simply send a new bodhi email for each check update (except for honoring the minimum delay between failed reports). Because it was way too hardcoded before and that won't fit into our future endeavors. On top of that, I believe we will be trying to get rid of bodhi emails completely soon.

I think that we do want to remove it but I vehemently disagree that we should just send emails on comments - that was extremely unpopular in the past and I'd rather not go forward with "hey, here's this new system that brings back some of the annoyances that we did away with a while back" but it might be worth querying devel@ to see what packagers think.

We're going to have to address notifications and general results in the near future but for now, let's keep things as simple as we can without blasting folks with email.

kparal added a comment.Apr 1 2014, 8:52 AM

We're going to have to address notifications and general results in the near future but for now, let's keep things as simple as we can without blasting folks with email.

OK, in that case I have a completely opposite suggestion - just don't send any Bodhi email at all. That means we can nuke a lot of code. And anyone can set up Bodhi notifications using FMN:
https://apps.fedoraproject.org/notifications
(Bodhi comments on my packages filter)

You can have email or IRC notifications, whatever you prefer. I think this is a good opt-in approach. If we document it, the packagers might be happy about it.

jskladan abandoned this revision.Apr 23 2014, 11:05 AM