[BB-1538] Wrap GitHub API Errors with Grpc Codes #108
Conversation
mateoHernandez123
requested review from
btipling and
laurenleach
as code owners
WalkthroughThis PR centralizes GitHub API error handling by adding Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Connector as Connector code
participant GitHub as GitHub API (client)
participant Wrapper as wrapGitHubError
participant gRPC as gRPC status
Connector->>GitHub: API call (e.g., List/Get/Add)
GitHub-->>Connector: response (resp) + error (err)
alt err == nil
Connector->>Connector: continue normal flow
else err != nil
Connector->>Wrapper: wrapGitHubError(err, resp, "context message")
Wrapper-->>gRPC: map to status (Unavailable / Unauthenticated / PermissionDenied / Unknown)
Wrapper-->>Connector: return gRPC error
end
Note over Wrapper,gRPC: Wrapper inspects resp and helpers (isRatelimited, isAuthError, isPermissionError)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (3)pkg/connector/helpers.go (1)
pkg/connector/enterprise_role.go (1)
pkg/connector/connector.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (14)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sumand included by none
📒 Files selected for processing (10)
pkg/connector/api_token.go(1 hunks)pkg/connector/connector.go(4 hunks)pkg/connector/enterprise_role.go(2 hunks)pkg/connector/helpers.go(2 hunks)pkg/connector/invitation.go(2 hunks)pkg/connector/org.go(9 hunks)pkg/connector/org_role.go(3 hunks)pkg/connector/repository.go(7 hunks)pkg/connector/team.go(7 hunks)pkg/connector/user.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/connector/enterprise_role.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/helpers.go (2)
vendor/github.com/conductorone/baton-sdk/pkg/lambda/grpc/transport.go (1)
Response(146-148)vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
pkg/connector/connector.go (1)
vendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.go (1)
WrapErrors(316-325)
🪛 GitHub Check: go-lint
pkg/connector/repository.go
[failure] 386-386:
variable error has same name as predeclared identifier (predeclared)
pkg/connector/team.go
[failure] 304-304:
variable error has same name as predeclared identifier (predeclared)
[failure] 357-357:
variable error has same name as predeclared identifier (predeclared)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (2)
pkg/connector/org.go (1)
103-130: Consistent GitHub error wrapping looks goodGreat to see every GitHub call now funneled through
wrapGitHubError, including the membership guard that still skips on 403s. This keeps the control flow unchanged while guaranteeing uniform gRPC status conversion. Nicely done.pkg/connector/connector.go (1)
222-279: Installation error path now surfaces precise gRPC codesWrapping both validation and constructor installation lookups with
wrapGitHubErroris exactly what we needed—callers now get consistent status codes plus context without any behavior change on success. Looks solid.
mateoHernandez123
force-pushed
the
mateoHernandez123/wrap-github-errors-grpc-codes
branch
from
336fa3d to
da94690
Compare
| } | ||
|
|
||
| if isRatelimited(resp) { | ||
| return uhttp.WrapErrors(codes.Unavailable, "too many requests", err) |
There was a problem hiding this comment.
like if its rate limited.... shouldn't we be returning that
There was a problem hiding this comment.
@pquerna this is wrapping the error with the status code so baton-sdk will wait and retry
There was a problem hiding this comment.
Don't we want the rate limit metadata?
There was a problem hiding this comment.
We do and it seems like the GitHub go SDK does expose it. I have filed a separate Jira issue for it https://conductorone.atlassian.net/browse/BB-1660
| return nil | ||
| } | ||
|
|
||
| if isRatelimited(resp) { |
There was a problem hiding this comment.
We could add the rate limit here from the error as per https://conductorone.atlassian.net/browse/BB-1660 or we could do it in a separate PR
There was a problem hiding this comment.
Yes, it is best to do a specific PR to resolve that
mateoHernandez123
deleted the
mateoHernandez123/wrap-github-errors-grpc-codes
branch
Summary by CodeRabbit