Conversation
![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 dist-git onboarding feature, which is a valuable addition. The implementation is well-structured, touching upon the API, event handling, and worker tasks as necessary. The code generally follows the existing patterns and conventions of the project. I have a couple of suggestions to improve robustness and code clarity.
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Nikola Forró <nforro@redhat.com>
Signed-off-by: Nikola Forró <nforro@redhat.com>
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Nikola Forró <nforro@redhat.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for onboarding packages in dist-git. The implementation includes a new API endpoint, a Celery task, and corresponding event handlers and parsers. The code is well-structured and follows the existing design patterns of the service. The addition of an integration test is also a great practice.
I've identified a security concern regarding secret token comparison and a minor style guide violation. My detailed feedback is in the review comments.
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 53s |
|
Here is an example PR: https://src.fedoraproject.org/rpms/scipy/pull-request/58 |
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
this is neat!
just few comments
| initializer.package_config_dict | {"downstream_package_name": package}, | ||
| ) | ||
|
|
||
| self.perform_onboarding( |
There was a problem hiding this comment.
do we need some exception handling here?
There was a problem hiding this comment.
I'm not sure... it should be retried automatically on common exceptions, but perhaps it would be better to be explicit about it.
| * [`koji_build`](https://packit.dev/docs/configuration/downstream/koji_build): | ||
| Submit a Koji build as reaction to a merged pull request. | ||
| * [`bodhi_update`](https://packit.dev/docs/configuration/downstream/bodhi_update): | ||
| Create a Bodhi update as a reaction to a succesful Koji build. |
There was a problem hiding this comment.
| Create a Bodhi update as a reaction to a succesful Koji build. | |
| Create a Bodhi update as a reaction to a successful Koji build. |
| should be run for. | ||
|
|
||
| You can also further tweak the process. A few handy options are prepared for you | ||
| in the confguration file to uncomment. Rest can be found in |
There was a problem hiding this comment.
| in the confguration file to uncomment. Rest can be found in | |
| in the configuration file to uncomment. Rest can be found in |
| [how to configure multi-package updates] | ||
| (https://packit.dev/docs/fedora-releases-guide/releasing-multiple-packages). |
There was a problem hiding this comment.
| [how to configure multi-package updates] | |
| (https://packit.dev/docs/fedora-releases-guide/releasing-multiple-packages). | |
| [how to configure multi-package updates](https://packit.dev/docs/fedora-releases-guide/releasing-multiple-packages). |
| * Be aware that there are other packages and packagers and that you might break someone else's work | ||
| by using Packit in a wrong way. (E.g. be careful about dependent packages since there is |
There was a problem hiding this comment.
| * Be aware that there are other packages and packagers and that you might break someone else's work | |
| by using Packit in a wrong way. (E.g. be careful about dependent packages since there is | |
| * Be aware that there are other packages and packagers and automated updates by using Packit may affect them. | |
| (E.g. be careful about dependent packages since there is |
I would just use a bit milder tone here.
| In case you don't want to receive these pull-requests in the future, you can use | ||
| the `--onboard-packit no` option when running `fedpkg request-repo`. |
There was a problem hiding this comment.
maybe we could also mention that they can easily disable packit by either removing particular jobs or the file itself (emphasis on rawhide branch)
| the `--onboard-packit no` option when running `fedpkg request-repo`. | ||
|
|
||
|
|
||
| I hope you will be happy with the automation! |
There was a problem hiding this comment.
| I hope you will be happy with the automation! | |
| We hope you will be happy with the automation! |
| path_or_url="", | ||
| upstream_git_url=None, | ||
| ) | ||
| self.package_config = self.job_config = PackageConfig.get_from_dict( |
There was a problem hiding this comment.
PackitAPI requires loaded config, similar thing is done in dist-git init:
https://github.com/packit/packit/blob/45486eb89fa614d35f03e5ac9129c73696dc509b/packit/cli/dist_git_init.py#L502-L511
2 participants
Related to #2506.