-
Notifications
You must be signed in to change notification settings - Fork 22
add review checklist inside newly created MRs as an internal note #350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a helpful feature to automatically add a review checklist as an internal note to new merge requests. The implementation correctly distinguishes between new and existing MRs. My review focuses on several small but important formatting and content issues within the new checklist template that could affect its clarity and rendering. I've also pointed out a maintainability concern regarding the use of a private attribute to interact with the GitLab API, which could be made more robust.
2daa607 to
4195263
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a feature to add a review checklist as an internal note to newly created GitLab merge requests. The implementation is straightforward, using a flag to distinguish between new and existing MRs. My review includes suggestions to improve maintainability by externalizing the large checklist string, a caution about using private library attributes, and a recommendation to enhance test coverage to verify the note is not added to existing MRs. Overall, the changes are good and address the issue, but these improvements would make the code more robust and easier to maintain.
| # this is an active API call via PR's setter method | ||
| pr.description = description | ||
| pr.title = title | ||
| is_brand_new_mr = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather see this be returned from the tool, so that the agent code can detect a brand new MR was opened and call a generic comment-adding tool with the message. Implementing this here converts the tool from universal to single-task-specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make the add_blocking_merge_request_comment tool more universal, accepting arguments such as internal or thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too late, already created a separate tool and task for that 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that's fine, since we have _get_merge_request_from_url() there is not that much code duplication.
02dbb87 to
a1ddc02
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new feature to add a review checklist as an internal note to newly created merge requests. The changes are implemented across the backport and rebase agents, introducing a new workflow step and a corresponding task. The implementation is logical, but I've found a critical issue and a medium-severity issue related to incorrect type hints that need to be addressed. The most critical issue will cause a runtime error during a dry run due to an incorrect return value. I've left detailed comments with suggestions on how to fix these issues. Overall, this is a useful addition. Once the identified issues are resolved, the code should be ready to merge.
Signed-off-by: Tomas Tomecek <[email protected]> Assisted-by: Cursor(Claude) Assisted-by: gemini-code-assist Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
e63b4ed to
908f471
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully implements the feature to add a review checklist as an internal note to newly created Merge Requests. The changes are well-integrated into the existing backport and rebase workflows, ensuring that the checklist is added only when an MR is newly created. The updates to function signatures, return types, and the PackageUpdateState model are consistent across the affected files. Unit tests have also been updated to reflect the new behavior. Overall, the changes are clean and effectively address the stated objective.
You can verify you can't see the note: https://gitlab.com/TomasTomecek/centos_rpms_git-lfs/-/merge_requests/1
This is how it looks:

Fixes https://github.com/packit/jotnar/issues/188