copr icon indicating copy to clipboard operation
copr copied to clipboard

Message schemas: improve the notifications

Open abompard opened this issue 1 year ago • 6 comments

This PR contains 2 changes:

  • set the one-line description to be the summary instead of the __str__ representation (which goes in the email body)
  • set the chroot message severity to DEBUG, which will not generate a notification in FMN by default. Those are always paired with a build message, so it makes more sense to notify on that one.

Fixes: #3237

abompard avatar Jul 29 '24 08:07 abompard

Thank you very much for the PR @abompard.

Fixes: https://github.com/fedora-copr/copr/issues/3237

If I understand correctly, it fixes the following problem mentioned in the issue comment https://github.com/fedora-copr/copr/issues/3237#issuecomment-2170959693 but not the issue itself?

Meaning, that people won't get "duplicate" messages with missing information, which is an improvement, but we still won't send per-build messages correctly.

FrostyX avatar Aug 07 '24 11:08 FrostyX

Right, it does not improve the build messages, so I guess we'll need another PR to fix that. I'll add a comment.

abompard avatar Aug 07 '24 15:08 abompard

Oh wait, the class I added the severity to is actually used for the copr.build.start messages as well! I got tricked by the name too ;-)

abompard avatar Aug 07 '24 16:08 abompard

If I understand correctly, no message is triggered when a package's builds are done, right? Only when each build is done. Should I try to add that? Would you be open to a PR that does it?

abompard avatar Aug 07 '24 16:08 abompard

If I understand correctly, no message is triggered when a package's builds are done, right? Only when each build is done. Should I try to add that? Would you be open to a PR that does it?

I think the situation is even worse. Or maybe we are just describing the same thing with different words.

Anyway, when a build is submitted for multiple chroots, when every chroot is started it sends both build.start and chroot.start and when every chroot ends, it sends build.end. There are many things wrong here.

  1. When any of the chroots starts building, we should send chroot.start, that is fine. But we should send build.start only when the very first chroot from that build starts
  2. We should not send build.end for every ended chroot, we should send chroot.end there
  3. The build.end message should be sent only once when the last chroot from that build finishes

Should I try to add that? Would you be open to a PR that does it?

That would be very much appreciated @abompard :-)

FrostyX avatar Aug 09 '24 11:08 FrostyX

Thanks for the explanation @FrostyX .

I've looked at the code, and what I understand is that all messages are sent by the backend, but I'm not sure the backend is aware of a Package Build being created, only of the "chroots" it has to build. I'm not sure it's aware of other chroots belonging to the same Build either.

I could make the frontend send the build.started (or build.created) message when a build is created, but at the moment the message bus abstraction is in copr-backend. I can move it to copr-common and instantiate it in copr-frontend when a package build is created (or rebuilt), and when it's done or cancelled for the build.ended message. But this is looking like a bigger change. Does it sound OK to you from a code structure perspective? Do you see a better solution?

abompard avatar Aug 22 '24 09:08 abompard

jakub

Anyway, when a build is submitted for multiple chroots, when every chroot is started it sends both build.start and chroot.start and when every chroot ends, it sends build.end. There are many things wrong here.

Agreement :heart: I've been thinking many times about changing the status quo, but we should provide some "migration time" before we do a cut for the old message API. And I haven't found enough time to think this through.

But we should send build.start only when the very first chroot from that build starts

So this is IMO not possible short term. We should keep that old message as-is, and provide a new one aside.

@abompard

I could make the frontend send the build.started (or build.created) message when a build is created, but at the moment the message bus abstraction is in copr-backend.

While things are better nowadays, we still choke from time to time (the overall build is blocked on sending messages). We can not afford to send messages directly by web UI and but we need to send them async (and check it has been delivered).

That said; the easiest thing for external contributors would be to create new backend "action type", and let backend send the message.

praiskup avatar Sep 23 '24 08:09 praiskup

Some things that need modification is the backend side, frontend side and common.

praiskup avatar Sep 23 '24 08:09 praiskup

We talked about this at a meeting and agreed that we like this PR as it is now and the improvements that I suggested should be done in a separate PR because it won't be a straightforward thing to do.

Thank you again for this PR @abompard and sorry it took so long to merge.

FrostyX avatar Sep 23 '24 20:09 FrostyX