Add GitPullRequestHelpHandler class#2853
Conversation
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 57s |
457d872 to
7cb22ab
Compare
|
Build failed. ❌ pre-commit FAILURE in 1m 43s |
|
Build failed. ✔️ pre-commit SUCCESS in 1m 48s |
|
/gemini review |
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 |
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.
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 |
3ca3c90 to
499c4f2
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 50s |
76f128d to
5636422
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 48s |
d9b68d5 to
f40ae8a
Compare
|
I'm thinking whether it wouldn't be better to remove the Edit: Something like this would be more relevant to this issue, which is directly about making changes to the process of parsing comments. I've left the suggestion there. |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 44s |
f40ae8a to
f8b6edc
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 45s |
majamassarini
left a comment
There was a problem hiding this comment.
I like the idea of a new Handler for reacting to the help comments! I've just left a couple of suggestions.
I don't think you need a "real singleton", but if I follow you, then yes. Injecting the proper class and relying on it instead of checking which method we need to call can be a better solution. |
|
And sorry for commenting twice, I tried to wrote the comments from inside cursor, and they showed up after a while... I thought I had lost them 😅 |
14434d3 to
91b0c02
Compare
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.
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.
This is done so that tests can be run successfully outside of containers.
`GitPullRequestHelpHandler` now reacts to issue comments as well.
The help message referencing `/packit help` has been reworded to sound better. Co-authored-by: Maja Massarini <2678400+majamassarini@users.noreply.github.com>
The code should now be clear that comments starting with "/packit-ci-stg" and "/packit-ci" lead to the use of the same Fedora CI comment parser. Co-authored-by: Maja Massarini <2678400+majamassarini@users.noreply.github.com>
Removed this inheritance due to the fact that none of PackitAPIWithDownstreamMixin's functionality is needed. Empty implementations of `clean_api` and `packit_api` have been added to avoid TypeError when instantiating `GitPullRequestHelpHandler`.
The staging instance now uses the "/packit-stg" keyword in the help message, the production instance uses "/packit". Equivalent changes have been done when it comes to Fedora CI help messages.
When parsing comments outside of the `GitPullRequestHelpHandler`, the help message parameters are unnecessary (they are only needed when generating the help message). As such, comments parsers are now instantiated without them. The values of these parameters now have the default value `None`.
b923b5e to
fc037f1
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 48s |
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