-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Message schemas: improve the notifications #3344
Conversation
@@ -184,6 +184,8 @@ def usernames(self): | |||
|
|||
|
|||
class _BuildChrootMessage(_BuildMessage): | |||
severity = message.DEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
??? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chroot messages are always paired with a build message, which means two notifications for basically the same thing in FMN. By setting the chroot severity to DEBUG (which seems the least notify-worthy of the two), it will not generate a notification in FMN by default.
By setting the severity in a message, you can hint people who subscribe to your events which events are important, which are not, and which require more attention, pretty much like log entries.
Does this answer your question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this answer your question?
It does for me but in that case, I would like to ask you if you could write something like this in a code comment because I'd probably just thought it was a typo when encountering it :-)
Thank you very much for the PR @abompard.
If I understand correctly, it fixes the following problem mentioned in the issue comment 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. |
Right, it does not improve the build messages, so I guess we'll need another PR to fix that. |
Signed-off-by: Aurélien Bompard <[email protected]>
Oh wait, the class I added the severity to is actually used for the |
Signed-off-by: Aurélien Bompard <[email protected]>
Signed-off-by: Aurélien Bompard <[email protected]>
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
That would be very much appreciated @abompard :-) |
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 |
jakub
Agreement ❤️ 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.
So this is IMO not possible short term. We should keep that old message as-is, and provide a new one aside.
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. |
Some things that need modification is the backend side, frontend side and common. |
|
||
""" | ||
Base class that all Copr messages should inherit from. | ||
""" | ||
def __str__(self): | ||
def summary(self): | ||
"""A one-line, human-readable representation of this message.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to keep __str__()
, and add summary
? It still seems like a convenient thing for experiments at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The __str__()
function already defaults to something that can be used in experiments, I think :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I already forgot the reasons for re-defining that, I think the multiline pretty prints just hurt my 👀 in the command line (though sub-class is not the place to fix this). Ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, per docs:
.. note:: Sub-classes should override this method. It is used to create
the body of email notifications and by other tools to display messages
to humans.
That's why I probably opted-in originally to decide the preferred one-line format when doing print(msg)
.
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. |
This PR contains 2 changes:
__str__
representation (which goes in the email body)Fixes: #3237