-
Notifications
You must be signed in to change notification settings - Fork 47
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
Correctly report results from Testing Farm #1565
Correctly report results from Testing Farm #1565
Conversation
packit_service/worker/jobs.py
Outdated
if ( | ||
isinstance(self.event, TestingFarmResultsEvent) | ||
and self.event.identifier != job.identifier |
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 make sense to run this in the pre_check
method of the TestingFarmResultsHandler
? otherwise, LGTM
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.
Good idea, something like this is what I was looking for. I will move it there + adjust the test coverage
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.
So testing turned out to be more complicated than I expected due to where pre_check
is called -- it is necessary to test create_tasks
which uses a lot of utility functions (dumping package config, etc) + celery stuff. I tried to remove most of this through mocks and focus on the number of tasks created. Could someone please re-review the changes?
Build failed. ✔️ pre-commit SUCCESS in 2m 08s |
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.
👏🏻
54af58c
to
05e2e12
Compare
Build failed. ✔️ pre-commit SUCCESS in 1m 45s |
recheck |
Build failed. ✔️ pre-commit SUCCESS in 1m 58s |
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.
👍 Looks good to me!
recheck |
Build failed. ✔️ pre-commit SUCCESS in 2m 50s |
recheck |
Build failed. ✔️ pre-commit SUCCESS in 2m 15s |
recheck |
Previously, if there were multiple TF jobs differentiated by their identifier, Steve would create a celery task for each of the jobs, regardless of their identifier for the single results event. To overcome this, save the identifier in the DB and obtain it when a result arrives. Check for identifier match when creating celery tasks based on jobs. Signed-off-by: František Nečas <fnecas@redhat.com>
05e2e12
to
c3768ef
Compare
Build succeeded. ✔️ pre-commit SUCCESS in 2m 04s |
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 2m 01s |
And it works https://github.com/FrNecas/packit-reproducer/pull/1 🍾 |
Previously, if there were multiple TF jobs differentiated by their
identifier, Steve would create a celery task for each of the jobs,
regardless of their identifier for the single results event. To overcome
this, save the identifier in the DB and obtain it when a result arrives.
Check for identifier match when creating celery tasks based on jobs.
TODO:
Fixes #1535 (or at least I hope, will need to be verified on stg, I have a simple reproducer https://github.com/FrNecas/packit-reproducer/pull/1 )
RELEASE NOTES BEGIN
Results from Testing Farm are now correctly reported when multiple jobs with different identifier are defined.
RELEASE NOTES END