Retrigger bodhi update via dist-git PR comment

[nikromen]

I'm sorry it took so long. I couldn't find the time to come back to this issue recently 😞

note: OGR doesn't have has_write_access method. Not yet. I am going to implement it :)

also help needed 😁: test_bodhi_update_retrigger_via_dist_git_pr_comment won't pass because because the CreateBodhiUpdateHandler.run method is not called. Even though the task.bodhi_update task gets the signature of the Celery task. This means that the JobHandler.get_signature method is executed and according to its documentation the run_bodhi_update task should be run, but this obviously does not happen. Any idea why?

Fixes #1541

Merge after org#742 packit#1729

---

RELEASE NOTES BEGIN
Packit now allows to re-trigger Bodhi update via dist-git PR comment /packit create-update.
RELEASE NOTES END

[softwarefactory-project-zuul]

Build failed.

✔️ pre-commit SUCCESS in 3m 58s
❌ packit-service-tests FAILURE in 3m 27s
❌ packit-service-tests-openshift RETRY_LIMIT in 15m 00s

[lbarcziova]

Thanks for finding time to implement this! I will have a closer look at the PR later. But as for the question:

also help needed 😁 test_bodhi_update_retrigger_via_dist_git_pr_comment won't pass because because the CreateBodhiUpdateHandler.run method is not called. Even though the task.bodhi_update task gets the signature of the Celery task. This means that the JobHandler.get_signature method is executed and according to its documentation the run_bodhi_update task should be run, but this obviously does not happen. Any idea why?

Since there is no real Celery in the tests, we call some methods which would Celery do => call the run_.. ourselves :D yes, there is definitely a place for improvement of the tests, we already discussed splitting the integration tests to not test the whole use case at once. You can see how this should be done e.g. here.

[softwarefactory-project-zuul]

Build succeeded.

✔️ pre-commit SUCCESS in 1m 58s
✔️ packit-service-tests SUCCESS in 2m 02s
✔️ packit-service-tests-openshift SUCCESS in 12m 45s

[majamassarini]

I tried your changes locally with podman-compose.
I have this error and maybe it could be somehow related with the tests (but I am not sure)

worker              |   File "/usr/local/lib/python3.10/site-packages/packit_service/worker/handlers/bodhi.py", line 175, in pre_check
worker              |     if self.koji_build_event_data.state != KojiBuildState.complete:
worker              |   File "/usr/local/lib/python3.10/site-packages/packit_service/worker/handlers/bodhi.py", line 118, in koji_build_event_data
worker              |     self._koji_build_event_data = self._build_koji_event_data()
worker              |   File "/usr/local/lib/python3.10/site-packages/packit_service/worker/handlers/bodhi.py", line 111, in _build_koji_event_data
worker              |     return self._build_from_koji_api()
worker              |   File "/usr/local/lib/python3.10/site-packages/packit_service/worker/handlers/bodhi.py", line 96, in _build_from_koji_api
worker              |     nvr = DistGit.get_latest_build_in_tag(
worker              | TypeError: DistGit.get_latest_build_in_tag() missing 1 required positional argument: 'self'

I have not fixed it because I am not really sure that it could be helpful.
I may be wrong (and maybe I am missing something else) but I think that in the commit named Feat(bodhi hangler): Get last koji build from koji api we expect a KojiBuildEvent even if in this case we are reacting to a PullRequestCommentPagureEvent which has not the needed Koji data.
Should we take the last built Koji package from the db instead?

[nikromen]

I'll take it in points:

• I have this error and maybe it could be somehow related with the tests (but I am not sure)

  • well the reason why you get this error is that the get_latest_build_in_tag method should be static, because the self parameter is not used. Only I forgot to make a PR in the packit repository that removes the self parameter :D thanks for pointing to it

• Should we take the last built Koji package from the db instead?

  • I'm not sure if I get your point. However, I tried to recreate the KojiBuildEvent (because I didn't know that koji has python API) using data from the database. I ended up adding some new stuff to the database and code in general, and the result was a lot of extra code. On top of that, most of the tests were crashing :D

• but I think that in the commit named Feat(bodhi hangler): Get last koji build from koji api we expect a KojiBuildEvent even if in this case we are reacting to a PullRequestCommentPagureEvent which has not the needed Koji data

  • I don't think that KojiBuildEvent is relevant in this context. We only need to extract a minimal amount of information from it, so I have created a wrapper that contains the minimal amount of information we need to re-trigger a bodhi update. Basically, the user wants to re-trigger the bodhi update thus the koji build were already triggered once in the past so we are not interested in KojiBuildEvent in this case (besides the above mentioned amount of information to re-trigger the bodhi update). Compared to creating a whole KojiBuildEvent, I don't think it's worth it in the current situation. (Although it's possible that the koji API has tools to get all the information e.g. ClientSession().getBuild(). However, I haven't read that much documentation and also getBuild method doesn't return everything (I think).

• I tried your changes locally with podman-compose.

  • Wow that's cool, I could use it at least 50 times already... How? :D I didn't know we are able to run packit service locally. Do we have it documented somewhere? How you do it? :D

[jpopelka]

I didn't know we are able to run packit service locally. Do we have it documented somewhere?

https://github.com/packit/packit-service/blob/main/CONTRIBUTING.md#running-packit-service-locally is a good start

EDIT: I'm using docker-compose with podman (I've been hitting one problem recently, but there are workarounds)

[majamassarini]

Sorry, I did not realize you were trying to build a KojiBuildEvent I thought you were trying to use it.

I think you don't need to do that; here you can see that the list of Koji builds is optional.
And here you can see, that if the list is not supplied, then the latest Koji builds are searched in the same way you are doing.

As regarding the debug, I have instrumented celerizer.py with the following code (I use Visual Studio Code for debugging and I have also opened the port 5678 in docker-compose).
I have created the following event using ad hoc branches in my dist-git project python-teamcity-messages.

import debugpy
debugpy.listen(("0.0.0.0", 5678))
debugpy.wait_for_client()
import json
event = json.loads(
    '{"agent": "mmassari", "pullrequest": {"assignee": null, "branch": "test-koji-downstream", "branch_from": "test-koji-downstream-2", "cached_merge_status": "FFORWARD", "closed_at": null, "closed_by": null, "comments": [{"comment": "/packit create-update", "commit": null, "date_created": "1657783448", "edited_on": null, "editor": null, "filename": null, "id": 110401, "line": null, "notification": false, "parent": null, "reactions": {}, "tree": null, "user": {"full_url": "https://src.fedoraproject.org/user/mmassari", "fullname": "Maja Massarini", "name": "mmassari", "url_path": "user/mmassari"}}], "commit_start": "c7637e51dadbfd0f6395b8259cac51b9f98641c4", "commit_stop": "c7637e51dadbfd0f6395b8259cac51b9f98641c4", "date_created": "1657783415", "full_url": "https://src.fedoraproject.org/rpms/python-teamcity-messages/pull-request/36", "id": 36, "initial_comment": null, "last_updated": "1657783448", "project": {"access_groups": {"admin": [], "collaborator": [], "commit": [], "ticket": []}, "access_users": {"admin": ["limb"], "collaborator": [], "commit": [], "owner": ["mmassari"], "ticket": []}, "close_status": [], "custom_keys": [], "date_created": "1643654065", "date_modified": "1650542166", "description": "The python-teamcity-messages package\\n", "full_url": "https://src.fedoraproject.org/rpms/python-teamcity-messages", "fullname": "rpms/python-teamcity-messages", "id": 54766, "milestones": {}, "name": "python-teamcity-messages", "namespace": "rpms", "parent": null, "priorities": {}, "tags": [], "url_path": "rpms/python-teamcity-messages", "user": {"full_url": "https://src.fedoraproject.org/user/mmassari", "fullname": "Maja Massarini", "name": "mmassari", "url_path": "user/mmassari"}}, "remote_git": null, "repo_from": {"access_groups": {"admin": [], "collaborator": [], "commit": [], "ticket": []}, "access_users": {"admin": ["limb"], "collaborator": [], "commit": [], "owner": ["mmassari"], "ticket": []}, "close_status": [], "custom_keys": [], "date_created": "1643654065", "date_modified": "1650542166", "description": "The python-teamcity-messages package\\n", "full_url": "https://src.fedoraproject.org/rpms/python-teamcity-messages", "fullname": "rpms/python-teamcity-messages", "id": 54766, "milestones": {}, "name": "python-teamcity-messages", "namespace": "rpms", "parent": null, "priorities": {}, "tags": [], "url_path": "rpms/python-teamcity-messages", "user": {"full_url": "https://src.fedoraproject.org/user/mmassari", "fullname": "Maja Massarini", "name": "mmassari", "url_path": "user/mmassari"}}, "status": "Open", "tags": [], "threshold_reached": null, "title": "Not usefull commit", "uid": "d8b1dd625c674cb9ad8010985bd98351", "updated_on": "1657783448", "user": {"full_url": "https://src.fedoraproject.org/user/mmassari", "fullname": "Maja Massarini", "name": "mmassari", "url_path": "user/mmassari"}}, "topic": "org.fedoraproject.prod.pagure.pull-request.comment.added", "timestamp": 1657776351.055628}'
)

celery_app.send_task(
    name="task.steve_jobs.process_message",
    args=[],
    kwargs={
        "event": event,
    },
)

[nikromen]

I think you don't need to do that; here you can see that the list of Koji builds is optional.

Yes, I know that if I'll set the koji_builds parameter to None it gets the latest build. However the problem is that we need some data for logging, creating error messages and generally in the pre-check method. So this means we still have to get the data somehow.

then the latest Koji builds are searched in the same way you are doing

Unfortunately yes. But at least I am trying to use at least get_latest_build_in_tag method from packit. As I described above - we need this data not only for create_update but for logs, etc...

But if you have an idea how to get around it without searching for the data in the way I do it then I am open to suggestions 😇

And thanks for sharing your debugging workflow! 👍

[lbarcziova]

thanks for the implementation! will you be able to finish this (I only added 2 notes) or should we help you somehow? Also, we do not have any reporting in case pre-check fails (e.g.when the person who commented doesn't have permissions), I am wondering whether we should solve this here or do it together for retriggering Koji builds by comment where this is missing as well

[lbarcziova]

I think that we could refactor the packit get_latest_build_in_tag method to return the whole build dictionary and not only nvr here and then we would not need to call the Koji API here the second time

[lbarcziova]

we just discussed with Franta and Tomas that here we should check both whether the user is a packager and has write access to the repo, so here we need to call the is_packager(

packit-service/packit_service/worker/handlers/distgit.py

Line 443 in 85b02fc

def is_packager(self, user):

)

[lachmanfrantisek]

will you be able to finish this (I only added 2 notes) or should we help you somehow?

We have discussed with Jirka that we will probably meet next Thursday to finish this. (And that we can help if needed.)

[TomasTomecek]

I am wondering whether we should solve this here or do it together for retriggering Koji builds by comment where this is missing as well

I would open a followup to that :) so we can get this merged finally