Post a comment per dist-git PR about transition#3012
Post a comment per dist-git PR about transition#3012centosinfra-prod-github-app[bot] merged 2 commits intopackit:mainfrom
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 one-time informational comment on Fedora dist-git pull requests to notify users about the transition to Packit as the default CI. The implementation correctly checks for existing comments to avoid duplication and handles potential errors gracefully. The changes are well-contained and include TODOs for future removal of this temporary feature.
I've added a couple of suggestions to improve code clarity and test coverage. Overall, this is a good change.
| packit_user = ( | ||
| "packit" if self.service_config.deployment == Deployment.prod else "packit-stg" | ||
| ) |
There was a problem hiding this comment.
For improved readability and to make the logic more explicit for all deployment types, consider expanding this ternary operator. This will make it clearer how dev and stg deployments are handled.
if self.service_config.deployment == Deployment.prod:
packit_user = "packit"
else:
# Both stg and dev deployments use packit-stg
packit_user = "packit-stg"| # Mock for CI transition comment | ||
| .should_receive("get_comments") | ||
| .and_return([]) | ||
| .once() | ||
| .mock() | ||
| .should_receive("comment") | ||
| .with_args(str) | ||
| .once() | ||
| .mock() |
There was a problem hiding this comment.
The current tests cover the scenario where the transition comment is posted. To ensure complete coverage of the new logic, please consider adding a test case for when the comment already exists. This would involve mocking get_comments() to return a comment matching FEDORA_CI_TRANSITION_COMMENT and asserting that pr.comment() is not called.
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 52s |
majamassarini
left a comment
majamassarini
left a comment
There was a problem hiding this comment.
LGTM thanks!
I have just two nitpicks that Claude suggested, but I think you can also ignore them.
| "- Tests: `/packit-ci test`\n\n" | ||
| "- See more in the " | ||
| "[retriggering documentation](https://packit.dev/fedora-ci/retriggering).\n\n" | ||
| "Questions? Reach us at [#packit:fedora.im](https://matrix.to/#/#packit:fedora.im)\n\n" |
There was a problem hiding this comment.
| "Questions? Reach us at [#packit:fedora.im](https://matrix.to/#/#packit:fedora.im)\n\n" | |
| "Questions? Reach out to us at [#packit:fedora.im](https://matrix.to/#/#packit:fedora.im)\n\n" |
lachmanfrantisek
left a comment
lachmanfrantisek
left a comment
There was a problem hiding this comment.
👍 Short but contains all the info needed. Thanks!
Co-authored-by: Maja Massarini <2678400+majamassarini@users.noreply.github.com>
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 49s |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 1m 46s |
4 participants
Fixes #3008
Should look like:
