fix(gitlab): check permission according to RememberOkToTest setting #2318
Conversation
Summary of ChangesHello @zakisk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a bug in the GitLab ACL check mechanism by refining how the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔍 PR Lint Feedback
|
![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 refactors the GitLab ACL check to correctly handle the RememberOKToTest setting, which is a good optimization to avoid unnecessary API calls. The changes look mostly correct, but I've identified a couple of issues. There's an inconsistency in how the event sender is updated when an /ok-to-test comment is found, which depends on the RememberOKToTest setting. Additionally, I've found an issue in the test setup that causes the tests for the /ok-to-test feature to pass for the wrong reason, not actually testing the intended logic. My review includes detailed comments and suggestions to address these points.
| client, mux, tearDown := thelp.Setup(t) | ||
| v.gitlabClient = client | ||
| if tt.allowMemberID != 0 { | ||
| v.userID = tt.allowMemberID |
There was a problem hiding this comment.
This line seems to introduce a bug in the test logic for /ok-to-test scenarios. By setting v.userID = tt.allowMemberID, you are overwriting the ID of the user who triggered the event (the non-member) with the ID of the authorized user.
As a result, the initial v.checkMembership(ctx, event, v.userID) check in IsAllowed will pass, and the test will succeed without ever exercising the checkOkToTestCommentFromApprovedMember logic, which is what these tests are supposed to validate.
The v.userID on the provider should represent the user who triggered the event. The allowMemberID is correctly used to mock the API response for when the commenter's membership is checked inside checkOkToTestCommentFromApprovedMember. This line should be removed to correctly test the intended code path.
Refactor ACL check to properly handle RememberOKToTest setting: - When RememberOKToTest is disabled for MergeEvent, skip checking all discussion notes and return false early - When RememberOKToTest is disabled for MergeCommentEvent, check only the current comment instead of all discussion history - Add aclAllowedOkToTestCurrentComment function to validate the specific comment that triggered the event This avoids checking all comments for /ok-to-test regardless of RememberOkToTest setting and optimizes the ACL check by avoiding unnecessary API calls to fetch all discussion notes when RememberOKToTest is disabled. https://issues.redhat.com/browse/SRVKP-9200 Signed-off-by: Zaki Shaikh <[email protected]>
zakisk
force-pushed
the
SRVKP-9200-fix-remember-ok-to-test-for-gitlab
branch
from
94397af to
2f0871e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the GitLab ACL checks to correctly handle the RememberOKToTest setting, which avoids unnecessary API calls. The implementation is logical and adds corresponding unit tests.
I have two main points of feedback:
- In
pkg/provider/gitlab/acl.go, I've suggested a small refactoring to make the creation of a temporaryinfo.Eventmore robust and less prone to future errors. - In
pkg/provider/gitlab/acl_test.go, I've identified a logic issue in the test setup that causes some tests to pass for incorrect reasons. Fixing this is important to ensure the new functionality is properly tested.
Once these points are addressed, the PR will be in great shape.
| client, mux, tearDown := thelp.Setup(t) | ||
| v.gitlabClient = client | ||
| if tt.allowMemberID != 0 { | ||
| v.userID = tt.allowMemberID |
There was a problem hiding this comment.
This line v.userID = tt.allowMemberID appears to be a bug in the test logic. It overwrites the userID of the provider with the ID of the allowed member.
For tests checking the /ok-to-test functionality, v.userID should be the ID of the user who triggered the event (e.g., the PR author), who is typically not a project member. The initial membership check in IsAllowed should fail, and the logic should then proceed to check for /ok-to-test comments.
By setting v.userID to tt.allowMemberID, the initial v.checkMembership(ctx, event, v.userID) in IsAllowed will pass, and the test will succeed without ever exercising the checkOkToTestCommentFromApprovedMember code path. This means the tests for /ok-to-test are not actually testing the intended functionality.
Please remove this line to ensure the tests are correctly validating the ACL logic for /ok-to-test comments.
| commenterEvent := info.NewEvent() | ||
| commenterEvent.Event = event.Event | ||
| commenterEvent.Sender = comment.Author.Username | ||
| commenterEvent.BaseBranch = event.BaseBranch | ||
| commenterEvent.HeadBranch = event.HeadBranch | ||
| commenterEvent.DefaultBranch = event.DefaultBranch |
There was a problem hiding this comment.
Creating a new info.Event and manually copying only a few fields is brittle. If checkMembership or its callees start depending on other fields from the event struct in the future, this could lead to subtle bugs because those fields would have their zero value.
It's safer to create a shallow copy of the original event and then modify only the fields that need to be changed. This ensures that all other context from the original event is preserved.
| commenterEvent := info.NewEvent() | |
| commenterEvent.Event = event.Event | |
| commenterEvent.Sender = comment.Author.Username | |
| commenterEvent.BaseBranch = event.BaseBranch | |
| commenterEvent.HeadBranch = event.HeadBranch | |
| commenterEvent.DefaultBranch = event.DefaultBranch | |
| commenterEvent := new(info.Event) | |
| event.DeepCopyInto(commenterEvent) | |
| commenterEvent.Sender = comment.Author.Username |
|
/test |
|
/retest |
1 participant
Refactor ACL check to properly handle RememberOKToTest setting:
This avoids checking all comments for /ok-to-test regardless of RememberOkToTest setting and optimizes the ACL check by avoiding unnecessary API calls to fetch all discussion notes when RememberOKToTest is disabled.
📝 Description of the Change
👨🏻 Linked Jira
https://issues.redhat.com/browse/SRVKP-9200
🔗 Linked GitHub Issue
Fixes #
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini [email protected]
Co-authored-by: ChatGPT [email protected]
Co-authored-by: Claude [email protected]
Co-authored-by: Cursor [email protected]
Co-authored-by: Copilot [email protected]
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.