Auto approve access request to Hellosource and Helloaggregate - #87
Auto approve access request to Hellosource and Helloaggregate#87iambibhas wants to merge 3 commits into
Conversation
Summary of ChangesHello, 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 introduces a new automated validation rule for access requests in 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 the 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 counterproductive. 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. A rule for the access is set, To check all the YAMLs we get. If names match the path, We avoid manual wrath, And approve with no need to fret. Footnotes
|
Signed-off-by: Bibhas Debnath <bdebnath@redhat.com>
a0cb1f6 to
d71fd84
Compare
![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 the hello_access_request rule to the Naysayer validation engine, enabling auto-approval for specific access request YAML files associated with the helloaggregate and hellosource data products. The changes include the core rule logic, registry integration, configuration updates in rules.yaml, a comprehensive suite of E2E test scenarios, and a new Cursor skill for rule creation. Feedback from the review highlights a security concern regarding file renames, suggesting that both source and destination paths be validated. Other recommendations include using case-insensitive comparisons for usernames, optimizing performance by caching MR context checks to avoid
| for _, change := range mrCtx.Changes { | ||
| path := change.NewPath | ||
| if path == "" { | ||
| path = change.OldPath | ||
| } | ||
| if path == "" { | ||
| continue | ||
| } | ||
| if !r.isAccessRequestFile(path) { | ||
| return "MR contains files outside allowed access-request paths: " + path | ||
| } | ||
| } |
There was a problem hiding this comment.
The current logic only checks either NewPath or OldPath (preferring NewPath). In the case of a file rename or move, this could allow a sensitive file to be moved into an access-request path and thus be deleted from its original location without manual review. To ensure the MR contains only access requests, both the source and destination paths of every change must be validated. Additionally, validation functions should collect all errors instead of returning early on the first failure.
| for _, change := range mrCtx.Changes { | |
| path := change.NewPath | |
| if path == "" { | |
| path = change.OldPath | |
| } | |
| if path == "" { | |
| continue | |
| } | |
| if !r.isAccessRequestFile(path) { | |
| return "MR contains files outside allowed access-request paths: " + path | |
| } | |
| } | |
| var errs []string | |
| for _, change := range mrCtx.Changes { | |
| if change.OldPath != "" && !r.isAccessRequestFile(change.OldPath) { | |
| errs = append(errs, "MR contains files outside allowed access-request paths: "+change.OldPath) | |
| } | |
| if change.NewPath != "" && !r.isAccessRequestFile(change.NewPath) { | |
| errs = append(errs, "MR contains files outside allowed access-request paths: "+change.NewPath) | |
| } | |
| } | |
| if len(errs) > 0 { | |
| return strings.Join(errs, ", ") | |
| } |
References
- Validation functions should run all checks and collect all errors instead of returning early on the first failure.
| return r.CreateManualReviewResult("Could not derive expected name from filename") | ||
| } | ||
|
|
||
| if doc.Name != expectedName { |
There was a problem hiding this comment.
The check doc.Name != expectedName is case-sensitive. If the filename is User.yaml but the YAML content is name: user, validation will fail. If the system treats usernames as case-insensitive, consider using strings.EqualFold to avoid unnecessary manual reviews for casing mismatches.
| if doc.Name != expectedName { | |
| if !strings.EqualFold(doc.Name, expectedName) { |
| return access_request.NewRule() | ||
| }, | ||
| Enabled: true, | ||
| Category: "hello_access_request", |
There was a problem hiding this comment.
The category hello_access_request is very specific compared to other categories in the registry (e.g., auto_approval, service_account). Consider using a more generic category like access_request to group similar future rules and maintain consistency with the existing category naming convention.
| Category: "hello_access_request", | |
| Category: "access_request", |
| } | ||
| } | ||
|
|
||
| func (r *Rule) validateMRContainsOnlyAccessRequests() string { |
There was a problem hiding this comment.
validateMRContainsOnlyAccessRequests iterates over all changes in the MR every time ValidateLines is called for a file. For MRs with many files, this results in Rule struct after the first execution.
Signed-off-by: Bibhas Debnath <bdebnath@redhat.com>
7129e1c to
b854c9a
Compare
Signed-off-by: Bibhas Debnath <bdebnath@redhat.com>
34d99f3 to
9198b42
Compare
1 participant
Description
Brief description of what this PR does.
Type of Change
Related Issues
Fixes #(issue number)
Testing
make test)make lint)