Fix the issue with checklist #371
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 mechanism to prevent posting duplicate checklists on GitLab merge requests. This is achieved by modifying the create_merge_request_checklist tool to first check for an existing checklist note before creating a new one. The checklist content is also now sourced from the GITLAB_MR_CHECKLIST constant. The changes are well-tested with a new unit test for the duplicate-checking logic. My review includes a suggestion to make the duplicate check more robust by dynamically determining the checklist identifier and using a more precise string comparison, which will improve maintainability.
| checklist_body = note_body.strip() | ||
| checklist_identifier = "# Jötnar MR Review Checklist" | ||
|
|
||
| for note in notes: | ||
| note_body_text = note.body.strip() | ||
| if (checklist_identifier in note_body_text or | ||
| note_body_text == checklist_body): | ||
| return True |
There was a problem hiding this comment.
This implementation has a couple of areas for improvement to make it more robust:
- The
checklist_identifieris hardcoded. This is brittle because if the title in theGITLAB_MR_CHECKLISTconstant changes, this code will fail to detect existing checklists. It's better to derive the identifier from thenote_body's first line. - Using
infor a substring check could lead to false positives. It's safer to check if the note bodystartswith()the identifier. - The case where
note_bodyis empty isn't handled, which could lead to unexpected behavior.
Here's a suggestion that addresses these points.
| checklist_body = note_body.strip() | |
| checklist_identifier = "# Jötnar MR Review Checklist" | |
| for note in notes: | |
| note_body_text = note.body.strip() | |
| if (checklist_identifier in note_body_text or | |
| note_body_text == checklist_body): | |
| return True | |
| checklist_body = note_body.strip() | |
| if not checklist_body: | |
| # Can't check for an empty checklist, assume it doesn't exist. | |
| return False | |
| # Use the first line of the checklist as the unique identifier. | |
| checklist_identifier = checklist_body.splitlines()[0] | |
| for note in notes: | |
| note_body_text = note.body.strip() | |
| if (note_body_text.startswith(checklist_identifier) or | |
| note_body_text == checklist_body): | |
| return True |
Also add a check for the duplicate to not post the checklist on multiple runs
|
Could you please also update this point? ai-workflows/common/constants.py Line 110 in 1a10192
There are probably more things to update, but this one is important. |
|
@nforro please review the latest commit. I made several changes to the message body, since there were quite a few outdated or wrong information |
common/constants.py
Outdated
| - [ ] **RHEL Build**: Successful scratch build | ||
| - [ ] **build_rpm**: Successful draft build. Once the gating is complete, it will automatically trigger a RHEL build. | ||
| - [ ] **Gitbz Check**: Commit messages correctly associated with approved Jira ticket (commit messages use "Resolves: RHEL-XXXXX" format) | ||
| - [ ] **Labels**: `target::latest` is set, exception or zstream are set only for exceptions or 0day, please consult Veronika Kabatova for that |
There was a problem hiding this comment.
What about target::zstream label?
There was a problem hiding this comment.
I can see it mentioned below, but it could be already set by the automation.
There was a problem hiding this comment.
It's under the Labels checkmark. And yes, it is set by automation. I left this note here just to ensure it is set correctly.
There was a problem hiding this comment.
But it says "target::latest is set", while it can be target::zstream.
common/constants.py
Outdated
| - [ ] **Labels**: `target::latest` is set, exception or zstream are set only for exceptions or 0day, please consult Veronika Kabatova for that | ||
| - you can utilise draft builds if you don’t want to trigger builds after merging manually, by labelling the MR with `feature::draft-builds::enabled` and clicking `Run pipeline` to rerun the pipeline | ||
| - Draft builds are now enabled by default, you should see the label `feature::draft-builds::enabled` on your MR. | ||
| - 0day is a regullar zstream build, it just needs to be added to the 0day batch at the Erratum level. |
There was a problem hiding this comment.
I'm not sure if we need this here.
There was a problem hiding this comment.
I'm okay with removing it. I left it there for people who don't often come into contact with RHEL development.
There was a problem hiding this comment.
I suppose it doesn't hurt to have it here, there is just a typo in regular.
Co-authored-by: Nikola Forró <[email protected]>
2 participants
Also add a check for the duplicate to not post the checklist on multiple runs