-
Notifications
You must be signed in to change notification settings - Fork 148
Add ignored-roles field to rate-limit configuration with defaults #15025
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
Changes from all commits
1209653
e797da3
a7d3081
d1c655d
fc0c190
311277c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,10 @@ describe("check_rate_limit", () => { | |
| delete process.env.GH_AW_RATE_LIMIT_MAX; | ||
| delete process.env.GH_AW_RATE_LIMIT_WINDOW; | ||
| delete process.env.GH_AW_RATE_LIMIT_EVENTS; | ||
| delete process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES; | ||
|
|
||
| // Reset repos mock | ||
| mockGithub.rest.repos = undefined; | ||
|
|
||
| // Reload the module to get fresh instance | ||
| vi.resetModules(); | ||
|
|
@@ -558,4 +562,161 @@ describe("check_rate_limit", () => { | |
|
|
||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Using workflow name: test-workflow (fallback")); | ||
| }); | ||
|
|
||
| it("should use default ignored roles (admin, maintain, write) when not specified", async () => { | ||
| // Don't set GH_AW_RATE_LIMIT_IGNORED_ROLES, so it uses default | ||
|
|
||
|
Comment on lines
+566
to
+568
|
||
| // Mock the permission check to return write | ||
| mockGithub.rest.repos = { | ||
| getCollaboratorPermissionLevel: vi.fn().mockResolvedValue({ | ||
| data: { | ||
| permission: "write", | ||
| }, | ||
| }), | ||
| }; | ||
|
|
||
| await checkRateLimit.main(); | ||
|
|
||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Ignored roles: admin, maintain, write")); | ||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("User 'test-user' has permission level: write")); | ||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("User 'test-user' has ignored role 'write'; skipping rate limit check")); | ||
| expect(mockCore.setOutput).toHaveBeenCalledWith("rate_limit_ok", "true"); | ||
| expect(mockGithub.rest.actions.listWorkflowRuns).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should apply rate limiting to triage users by default", async () => { | ||
| // Don't set GH_AW_RATE_LIMIT_IGNORED_ROLES, so it uses default (admin, maintain, write) | ||
|
|
||
| mockGithub.rest.repos = { | ||
| getCollaboratorPermissionLevel: vi.fn().mockResolvedValue({ | ||
| data: { | ||
| permission: "triage", | ||
| }, | ||
| }), | ||
| }; | ||
|
|
||
| mockGithub.rest.actions = { | ||
| listWorkflowRuns: vi.fn().mockResolvedValue({ | ||
| data: { | ||
| workflow_runs: [], | ||
| }, | ||
| }), | ||
| cancelWorkflowRun: vi.fn(), | ||
| }; | ||
|
|
||
| await checkRateLimit.main(); | ||
|
|
||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Ignored roles: admin, maintain, write")); | ||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("User 'test-user' has permission level: triage")); | ||
| expect(mockGithub.rest.actions.listWorkflowRuns).toHaveBeenCalled(); | ||
| expect(mockCore.setOutput).toHaveBeenCalledWith("rate_limit_ok", "true"); | ||
| }); | ||
|
|
||
| it("should skip rate limiting for users with ignored roles", async () => { | ||
| process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES = "admin,maintain"; | ||
|
|
||
| // Mock the permission check to return admin | ||
| mockGithub.rest.repos = { | ||
| getCollaboratorPermissionLevel: vi.fn().mockResolvedValue({ | ||
| data: { | ||
| permission: "admin", | ||
| }, | ||
| }), | ||
| }; | ||
|
|
||
| await checkRateLimit.main(); | ||
|
|
||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Ignored roles: admin, maintain")); | ||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("User 'test-user' has permission level: admin")); | ||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("User 'test-user' has ignored role 'admin'; skipping rate limit check")); | ||
| expect(mockCore.setOutput).toHaveBeenCalledWith("rate_limit_ok", "true"); | ||
| expect(mockGithub.rest.actions.listWorkflowRuns).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should skip rate limiting for users with maintain permission when in ignored roles", async () => { | ||
| process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES = "admin,maintain"; | ||
|
|
||
| mockGithub.rest.repos = { | ||
| getCollaboratorPermissionLevel: vi.fn().mockResolvedValue({ | ||
| data: { | ||
| permission: "maintain", | ||
| }, | ||
| }), | ||
| }; | ||
|
|
||
| await checkRateLimit.main(); | ||
|
|
||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("User 'test-user' has ignored role 'maintain'; skipping rate limit check")); | ||
| expect(mockCore.setOutput).toHaveBeenCalledWith("rate_limit_ok", "true"); | ||
| expect(mockGithub.rest.actions.listWorkflowRuns).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should apply rate limiting for users without ignored roles", async () => { | ||
| process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES = "admin,maintain"; | ||
|
|
||
| mockGithub.rest.repos = { | ||
| getCollaboratorPermissionLevel: vi.fn().mockResolvedValue({ | ||
| data: { | ||
| permission: "write", | ||
| }, | ||
| }), | ||
| }; | ||
|
|
||
| mockGithub.rest.actions = { | ||
| listWorkflowRuns: vi.fn().mockResolvedValue({ | ||
| data: { | ||
| workflow_runs: [], | ||
| }, | ||
| }), | ||
| cancelWorkflowRun: vi.fn(), | ||
| }; | ||
|
|
||
| await checkRateLimit.main(); | ||
|
|
||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("User 'test-user' has permission level: write")); | ||
| expect(mockGithub.rest.actions.listWorkflowRuns).toHaveBeenCalled(); | ||
| expect(mockCore.setOutput).toHaveBeenCalledWith("rate_limit_ok", "true"); | ||
| }); | ||
|
|
||
| it("should continue with rate limiting if permission check fails", async () => { | ||
| process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES = "admin"; | ||
|
|
||
| mockGithub.rest.repos = { | ||
| getCollaboratorPermissionLevel: vi.fn().mockRejectedValue(new Error("API error")), | ||
| }; | ||
|
|
||
| mockGithub.rest.actions = { | ||
| listWorkflowRuns: vi.fn().mockResolvedValue({ | ||
| data: { | ||
| workflow_runs: [], | ||
| }, | ||
| }), | ||
| cancelWorkflowRun: vi.fn(), | ||
| }; | ||
|
|
||
| await checkRateLimit.main(); | ||
|
|
||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not check user permissions")); | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Continuing with rate limit check")); | ||
| expect(mockGithub.rest.actions.listWorkflowRuns).toHaveBeenCalled(); | ||
| expect(mockCore.setOutput).toHaveBeenCalledWith("rate_limit_ok", "true"); | ||
| }); | ||
|
|
||
| it("should handle single ignored role as string", async () => { | ||
| process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES = "admin"; | ||
|
|
||
| mockGithub.rest.repos = { | ||
| getCollaboratorPermissionLevel: vi.fn().mockResolvedValue({ | ||
| data: { | ||
| permission: "admin", | ||
| }, | ||
| }), | ||
| }; | ||
|
|
||
| await checkRateLimit.main(); | ||
|
|
||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Ignored roles: admin")); | ||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("User 'test-user' has ignored role 'admin'; skipping rate limit check")); | ||
| expect(mockCore.setOutput).toHaveBeenCalledWith("rate_limit_ok", "true"); | ||
| }); | ||
| }); | ||
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.
Using
process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES || "admin,maintain,write"makes an explicitly empty env var (used to representignored-roles: []) fall back to the defaults, preventing users from overriding to “no ignored roles”. Use a nullish check (??) instead so""is preserved.This issue also appears on line 45 of the same file.