Remove bodhi comment code
ClosedPublic

Authored by mkrizek on Dec 15 2015, 1:17 PM.

Diff Detail

Repository
rLTRN libtaskotron
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mkrizek retitled this revision from to Remove bodhi comment code.Dec 15 2015, 1:17 PM
mkrizek updated this object.
mkrizek edited the test plan for this revision. (Show Details)
mkrizek added reviewers: tflink, kparal, jskladan, lbrabec.

I'm very much in favor of this, but as I commented in T556#7818, I think we should make sure there is some simple way for maintainers to receive notifications of check failures. The easiest solution would be for Bodhi to start sending emails for these changes (just direct emails, as they do with Bodhi comments). They have some code to detect this already, since they display task results on Bodhi pages. The RFE is linked in our ticket.

conf/taskotron.yaml.example
51–52

Please add

## [default: True for production, False for development]
148–149

If you think it's a good idea, I think we can throw this out as well. We used FAS credentials only for "write" operations into Bodhi - sending comments. Now that it is gone, I don't think we have any use for it, so it would be less code. Unless we know about something that would use FAS credentials in the future.

In D691#13184, @kparal wrote:

I'm very much in favor of this, but as I commented in T556#7818, I think we should make sure there is some simple way for maintainers to receive notifications of check failures. The easiest solution would be for Bodhi to start sending emails for these changes (just direct emails, as they do with Bodhi comments). They have some code to detect this already, since they display task results on Bodhi pages. The RFE is linked in our ticket.

Why aren't fedmsgs and fmn enough? Is there some feature I'm not aware of?

tflink accepted this revision.Dec 15 2015, 10:44 PM

Other than @kparal's comments, looks good to me. Assuming that fedmsg emission and fmn are working as designed, I don't see a reason to keep this code around.

Macro fireworks: This code looks so much better when it's being blown up

conf/taskotron.yaml.example
148–149

I can't think of anything else that would need FAS credentials, removing this bit sounds good to me

This revision is now accepted and ready to land.Dec 15 2015, 10:44 PM
mkrizek updated this revision to Diff 1795.Dec 16 2015, 12:45 PM
  • Remove not needed fas_* config options

Why aren't fedmsgs and fmn enough? Is there some feature I'm not aware of?

I thought we agreed in the past that core taskotron messages were suboptimal for direct package maintainer consumption. Maybe I'm misremembering it. Anyway, my concerns:

  1. You'll receive all outcomes, not just failures. I guess we could create a new fmn rule to be able to filter based on outcome. But with this, people will be able to receive failures only, not failures and failure->passed transitions (as we do with bodhi comments). I don't think we can put this advanced logic into FMN.
  2. You'll receive results from all checks executed, not just "important checks". We have a fmn rule to filter based on check name, but each person will need to maintain the "important checks" list manually. We could create a new fmn rule with hardcoded list of "important checks" to fix this. This concern is somewhat moot at the moment, because we don't have not that many "not important" checks - just rpmlint, and that can be excluded with current fmn rules (if people know how to do it properly). But it will be more visible if we add some more "not important" checks.
  3. There's no way of grouping results. So you can't wait for results of all archs and then send a summary message. You can't wait for results of all checks and then send a summary message. You can't have a nice logic which sends all the failures individually, but if there are no failures, waits until *everything* is completed and then send an overall pass result. The result is increase in number of emails received compared to current state.
  4. If/When FAS nickname matching is implemented correctly, the update creator would receive results for the whole update (1 message per check per arch), and package owners and people with commit rights would receive results for individual builds (1 message per your build in the update per check per arch). I've already mentioned increase in number of emails, but this also increases the number of people notified. I'm not saying this is wrong, but it's definitely a change in behavior. Bodhi used to inform only the update author.

I don't consider all of this game-breaking, but we should be aware what's going to happen (unless I'm wrong in some or all points).

We have spent today some time with @mkrizek to test whether FMN integration works at all, at present. It seems it doesn't. Martin used default FMN filters and created a fedmsg with depcheck results, but received no email for his Bodhi update. We will probably need to change these things:
a) Add packages: ['foo'] keyval to taskotron fedmsgs to allow FMN to be able to match package owners with our results. This is based on our notion how things work, we haven't seen the code and we don't even know where it is implemented.
b) Create a new filter named Taskotron results for packages I own (ideally enabled by default), which will do the matching. IIUIC people can create such a filter themselves, but they need to understand FMN concepts and it wouldn't be enabled for everyone by default (which we want, I believe), so we should make sure it is there, and it's possible to enable/disable it with one click.
c) The changes above should fix build notifications (e.g. depcheck passed for build X), but not update notifications. For bodhi_update type of result, we can't easily deduce packages: ['foo', 'bar', 'baz'] only from the provided updateid on our resultsdb server. We would have to query Bodhi from the resultsdb server, which would probably have to be done asynchronously. Or we could offload that job to our checks (most of them have this information already), and transfer it to resultdb server in the resultsyaml output, but it would have to get mandatory. Both approaches seem subpar.

When we fix this, I think it should start working as described in the concerns section above. My last concern is that we wanted to get rid of dealing with bodhi updates (to eliminate bodhi querying and the delays and troubles related to it), and only test and publish build-based results. However, if we do that, even more things will break (for example we won't be able to notify update authors anymore, just build owners).

If Bodhi received Taskotron fedmsgs and used its own logic and a local database access to process the results and publish its own fedmsgs, it would eliminate most of the issues. It would be possible to have advanced notification logic, it would be possible to group results, it would not spam package owners but just update authors, taskotron could publish only build-based results. I realize it's probably a lot of work, and it might take some time. As recently discussed in the github ticket, they most probably just display live results and haven't implemented any storage or ways to take actions on it in the background. So it's not going to be a simple patch, as I hoped for. We can certainly go forward with all of this right now and improve things gradually, but I'm afraid some maintainers will not be happy about the changes and I also don't see it as an improvement compared to taskotron bodhi comments (from their POV).

This revision was automatically updated to reflect the committed changes.
kparal added a subscriber: ralph.Dec 17 2015, 4:55 PM
In D691#13273, @kparal wrote:

a) Add packages: ['foo'] keyval to taskotron fedmsgs to allow FMN to be able to match package owners with our results. This is based on our notion how things work, we haven't seen the code and we don't even know where it is implemented.
c) The changes above should fix build notifications (e.g. depcheck passed for build X), but not update notifications. For bodhi_update type of result, we can't easily deduce packages: ['foo', 'bar', 'baz'] only from the provided updateid on our resultsdb server. We would have to query Bodhi from the resultsdb server, which would probably have to be done asynchronously. Or we could offload that job to our checks (most of them have this information already), and transfer it to resultdb server in the resultsyaml output, but it would have to get mandatory. Both approaches seem subpar.

Both of these have been ninja-fixed by @ralph on FMN side at https://github.com/fedora-infra/fedmsg_meta_fedora_infrastructure/pull/349 .

b) Create a new filter named Taskotron results for packages I own (ideally enabled by default), which will do the matching. IIUIC people can create such a filter themselves, but they need to understand FMN concepts and it wouldn't be enabled for everyone by default (which we want, I believe), so we should make sure it is there, and it's possible to enable/disable it with one click.

I have filed a number of tickets and added them as blocking to T627. We can follow up there. There's probably more work to be done, but I think it's a good start.