notification.failure_comment: fine-tuning

[martinpitt]

Description

Issue #1911 introduced automatic notifications for failed packit tests. This was necessary to enable cross-project testing, but now we have a somewhat inverse problem of getting too many notifications of failures which shouldn't be "escalated". Tests regularly fail for first PR submissions, including the project's own tests.

So ideally that notification would only happen when a PR is "ready" and the project's "own" tests succeed. Quotes because defining what this actually means is hard.

For an "80% good enough" approximation/mitigation, I propose the following:

• By default, only send the notifications if a PR is not in "draft" state. That may be configured, of course.
• Add an option to enable/disable notifications on labels. E.g. podman PRs use "do-not-merge/work-in-progress" which means "I know that this is still broken".

The "own tests" feels a bit hard to define and racy. I'll think about this for a bit, but I don't yet have an idea in my head how to do that.

@lsm5 I suppose you are getting a similar notification firehose for the failed COPR builds. Do you have an opinion or ideas about this?

Thank you!

Benefit

Less notification noise, and thus the remaining notifications become more prominent when they really need attention.

Importance

No response

What is the impacted category (job)?

Testing Farm tests

Workaround

• There is an existing workaround that can be used until this feature is implemented.

Participation

• I am willing to submit a pull request for this issue. (Packit team is happy to help!)

[lsm5]

@martinpitt we noticed a bunch of notifications because of failed ELN builds, but I guess that's been resolved since Friday.

I didn't really have any ideas in mind as I've mostly been only receiving the build failure messages, but now we're adding more TMT tests, so I expect to see what you're seeing.

Your 2 suggestions sound good to me from packit's side. On podman's (any packit service user's ) side I think it would be nice to have some workflow changes like initial PR submission is always a draft, if that's even enforceable.

[lbarcziova]

hi @martinpitt @lsm5 ! Thanks for the provided suggestions!

Regarding the "draft" point, both options make sense to me and could be added under failure_comment :

1. disable_for_drafts or similar that would be just a boolean

• as for the initial PR submission of always draft, that would probably be more complicated to implement and I am also not sure whether this would be a good UX (e.g. can happen the PR is ready right away when first submitting and there are no other changes to it later)

2. disable_for_labels (and if needed enable_for_labels) that would be list of labels

As for the second point about "own" tests not failing, I think this would be really hard to determine and also to implement (e.g. if the reverse dep tests finish earlier than the "own" tests etc.). But if you figure out something, we are happy to discuss it further.

[lachmanfrantisek]

Hello all,
Regarding the waiting for "own" tests to pass, this might be resolved by triggering the Packit manually by comments, but it's probably not what we want:

• Needs to be thought about.
• Won't trigger jobs on follow-up pushes.

[lachmanfrantisek]

I would start with the two points mentioned by Laura -- actually, there is a similar ask with labels for downstream jobs. Sadly, we already use the term labels for something else.

[martinpitt]

@lbarcziova @lachmanfrantisek Agreed -- the "own tests" etc. was more or less thinking aloud, but we all agree that's difficult to pin down. "initial PR submission of always draft" doesn't sound related to packit (or this issue), at least if I understand it correctly - that would be something that GitHub PRs would have to implement in their settings.

disable_for_drafts and {dis,en}able_for_labels sound like a good improvement already, and perhaps/hopefully it's already sufficient.

[lsm5]

• as for the initial PR submission of always draft, that would probably be more complicated to implement and I am also not sure whether this would be a good UX (e.g. can happen the PR is ready right away when first submitting and there are no other changes to it later)

Sorry, I meant this part for the individual project to consider, not a packit problem.

[lsm5]

disable_for_drafts and {dis,en}able_for_labels sound like a good improvement already, and perhaps/hopefully it's already sufficient.

@lbarcziova @martinpitt SGTM as well

[lachmanfrantisek]

@martinpitt @lsm5 Just a last question. Do you think this should be a notification setting (=the jobs are run but notification is not created) or a job execution setting (=the job is not run)?

I see both use cases useful but I am not sure which one is more relevant to you.

We have a similar ask to allow/block downstream Koji builds by labels (#2186) so we should be consistent when working on these...

[martinpitt]

From my POV the jobs should always run -- it makes sense to give test feedback on draft PRs. Often enough, the whole reason to send a draft PR first is to throw it at CI and see how much sticks. This is purely tweaking of the notifications.

[lsm5]

@martinpitt @lsm5 Just a last question. Do you think this should be a notification setting (=the jobs are run but notification is not created) or a job execution setting (=the job is not run)?

I see both use cases useful but I am not sure which one is more relevant to you.

We have a similar ask to allow/block downstream Koji builds by labels (#2186) so if we should be consistent when working on these...

@lachmanfrantisek i've only gotten concerns from people about the notification flood, so run but not notify would work for me. The running itself doesn't seem like a problem.

[lachmanfrantisek]

Martin, Lokesh, thanks for the confirmation! With that, this should really be a notification config option.

[martinpitt]

@lachmanfrantisek Well, not that easy I suppose -- the packit app would also have to start listening to "Draft → Ready to review" events and send the notification about a previously failed run while it was still in draft mode.

[lachmanfrantisek]

Good point, Martin, I haven't thought about this situation and you are right this won't be easy to do. (Maybe, we can leave this as a follow-up task since this needs to be handled differently and start with.)