Trigger Log Detective for failed downstream Koji builds#2971
Conversation
Jany26
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new handler to trigger Log Detective for failed Koji and Copr builds. The overall approach is sound, but I've identified a few critical issues that would cause runtime failures, including syntax errors and incorrect use of return in an __init__ method. I've provided suggestions to fix these. Additionally, I've proposed a refactoring of the database interaction logic to improve its clarity and maintainability. Addressing these points will strengthen the implementation.
|
❌ pre-commit FAILURE in 1m 53s |
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
thanks for the PR! Generally the logic looks good, but let's start with handling Koji builds only for now, see the previous comments
|
Is this more in line with your suggestions? This seems much cleaner and should be easier to test. |
|
❌ pre-commit FAILURE in 1m 38s |
Jany26
changed the title
Jany26
force-pushed
the
log-detective-trigger-handler
branch
from
04e1c15 to
7794416
Compare
|
❌ pre-commit FAILURE in 1m 47s |
|
❌ pre-commit FAILURE in 1m 48s |
Jany26
force-pushed
the
log-detective-trigger-handler
branch
from
d105c5d to
9969900
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 45s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 48s |
|
This PR will focus on downstream Koji handling, I will create a separate PR for triggering LD from Copr later. |
Jany26
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new handler to trigger Log Detective for failed downstream Koji builds. The implementation is well-structured, with a new helper class LogDetectiveKojiTriggerHelper to encapsulate the logic for interacting with the Log Detective service. The changes are correctly integrated into the existing Koji task reporting flow. The error handling in the new helper is robust, and the PR includes both unit and integration tests for the new functionality. I have a few minor suggestions to improve code clarity and clean up the tests.
Jany26
force-pushed
the
log-detective-trigger-handler
branch
from
567bd72 to
ab57dc2
Compare
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 43s |
jpodivin
left a comment
jpodivin
left a comment
There was a problem hiding this comment.
I have minor suggestions for improving maintainability of the code in the long run. Other than that I think this is good to go. The tests especially are a nice touch, they cover 100% of the new helper.
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 47s |
Jany26
force-pushed
the
log-detective-trigger-handler
branch
from
7c9de82 to
08ca132
Compare
Jany26
changed the title
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 50s |
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
this looks solid, thank you! Last few notes
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 01s |
Jany26
force-pushed
the
log-detective-trigger-handler
branch
from
8b412c6 to
b8e00ad
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 53s |
Jany26
force-pushed
the
log-detective-trigger-handler
branch
from
b8e00ad to
9ddf704
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 50s |
Jany26
force-pushed
the
log-detective-trigger-handler
branch
from
9ddf704 to
fc0b76c
Compare
|
I included the requests and squashed the commits, since there was some back-and-forth in the commit history. Now I think it is ready for merging, in case no other issues are found. |
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot! I will wait a bit in case other team members want to have a look and merge later today
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 49s |
|
@Jany26 can you rebase please? |
Signed-off-by: Jan Matufka <jmatufka@redhat.com>
Jany26
force-pushed
the
log-detective-trigger-handler
branch
from
fc0b76c to
116d5d3
Compare
|
@lbarcziova done |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 51s |
|
regate |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 1m 52s |
4 participants
This is a complementary handler to the one from #2905 by @jpodivin
This handler is supposed to detect changes in (downstream) Koji tasks and if they signal that the build has failed, it creates a json payload, then sends it to a middle-man server (implementation here: https://github.com/fedora-copr/logdetective-packit, the URL itself has not yet been determined) which triggers Log Detective. Additionally, some DB entries for the Log Detective run are also created. Analysis results themselves are then handled in another class.
RELEASE NOTES BEGIN
Packit can now trigger Log Detective when it detects a failed downstream Koji task.
RELEASE NOTES END