Add GitPullRequestHelpHandler class #2853
Conversation
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 57s |
betulependule
force-pushed
the
help-pr-comment-command
branch
from
457d872 to
7cb22ab
Compare
|
Build failed. ❌ pre-commit FAILURE in 1m 43s |
|
Build failed. ✔️ pre-commit SUCCESS in 1m 48s |
|
/gemini review |
![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 PR introduces a new handler for the help command in PR comments, which is a great addition for user experience. The implementation is straightforward and follows existing patterns in the codebase. The changes are well-contained and include corresponding tests.
I've left a few comments with suggestions for improvement, mainly around code clarity, maintainability, and a potential thread-safety issue in the planned implementation. Once the dependent PR is merged and the commented-out code is enabled, the suggestion regarding thread-safety will be particularly important to address.
Overall, this is a solid contribution.
|
Build failed. ✔️ pre-commit SUCCESS in 1m 48s |
|
Build failed. ❌ pre-commit FAILURE in 1m 49s |
betulependule
force-pushed
the
help-pr-comment-command
branch
from
6e25b11 to
efb691b
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 46s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 13s |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new GitPullRequestHelpHandler to respond to /packit help commands in pull request comments. The implementation is well-structured, adding the necessary handler, task, and event routing. The changes also include integration tests for the new functionality on GitHub and Pagure.
My review focuses on improving the robustness of the command detection logic. I've identified a couple of areas where handling of different command prefixes (especially for staging environments) could be improved to prevent issues in the future. One of these is in the active code and could cause bugs in staging, while the other is in commented-out code that is planned for a future update. Addressing these will make the new feature more reliable across all environments.
packit_service/worker/jobs.py
Outdated
| packit_comment_command_prefix = ( | ||
| "/packit-ci" if comment.startswith("/packit-ci") else "/packit" | ||
| ) | ||
|
|
||
| command = get_packit_commands_from_comment( | ||
| comment, | ||
| packit_comment_command_prefix, | ||
| ) | ||
|
|
||
| return bool(command and command[0] == PACKIT_HELP_COMMAND) |
There was a problem hiding this comment.
The logic to detect the help command is not robust enough. It doesn't handle staging prefixes like /packit-stg or /packit-ci-stg, and it can fail if the comment has leading whitespace since comment.startswith is used on the raw comment string.
A better approach would be to iterate through all possible prefixes and use get_packit_commands_from_comment to check for the help command. This would also make the logic more resilient to comment formatting.
Consider improving the test cases in tests/integration/test_pr_comment.py to cover staging prefixes to catch such issues in the future.
| packit_comment_command_prefix = ( | |
| "/packit-ci" if comment.startswith("/packit-ci") else "/packit" | |
| ) | |
| command = get_packit_commands_from_comment( | |
| comment, | |
| packit_comment_command_prefix, | |
| ) | |
| return bool(command and command[0] == PACKIT_HELP_COMMAND) | |
| prefixes = ("/packit-ci-stg", "/packit-ci", "/packit-stg", "/packit") | |
| for prefix in prefixes: | |
| command = get_packit_commands_from_comment( | |
| comment, | |
| prefix, | |
| ) | |
| if command and command[0] == PACKIT_HELP_COMMAND: | |
| return True | |
| return False |
There was a problem hiding this comment.
The "-stg" suffixes definitely need to be taken into account. However, I don't think I like the trial and error for loop. I'll try a different solution.
betulependule
force-pushed
the
help-pr-comment-command
branch
from
1df9cdb to
a4ae68a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new GitPullRequestHelpHandler to respond to /packit help commands in pull request comments across GitHub, GitLab, and Pagure. This is a valuable feature for improving user experience. The implementation is well-structured, adding a new handler, a Celery task, and updating the job processing logic. Integration tests for GitHub and Pagure are included. My review focuses on a minor typo, improving test coverage by adding a GitLab test, and correcting type hints for better code quality.
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 46s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 41s |
The previously used variable seemed to always be initialized with the "/packit" value, which would result in incorrect behaviour in Fedora CI as the expected value in this case would be "/packit-ci" instead. Correct value should now be in use.
betulependule
force-pushed
the
help-pr-comment-command
branch
from
3ca3c90 to
499c4f2
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 50s |
There is now a reference to /packit-ci when user uses /packit and the other way around. This is in case the user uses /packit when they should be using /packit-ci and vice versa.
betulependule
force-pushed
the
help-pr-comment-command
branch
from
76f128d to
5636422
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 48s |
1 participant
A new job handler has been created so that
packit-servicenow has the ability to respond to thehelpcommand in PR comments. The bot then should automatically generate a comment, which contains a help message that includes the usage, link to documentation and contacts. A test for Github PR was made to test this new functionality.TODO:
packit/packit.dev./packit-ciwhen user uses/packitand the other way around. This is in case the user uses/packitwhen they should be using/packit-ciand vice versa.Fixes #1668
Related to
Merge after #2850
RELEASE NOTES BEGIN
Packit-service now supports the
helpcommand in PR comments.RELEASE NOTES END