implement change provider - github#98
Conversation
rashmi-prithyani
requested review from
a team,
behinddwalls and
sbalabanov
as code owners
| // GraphQLURL is the GitHub GraphQL API endpoint | ||
| // (e.g., "https://api.github.com/graphql" or "https://ghe.example.com/api/graphql"). | ||
| GraphQLURL string |
There was a problem hiding this comment.
i think we don't need it, HTTPClient should encapsulate it
rashmi-prithyani
force-pushed
the
rprithyani/implementGithubChangeProvider
branch
from
9507bc0 to
a3b5ed8
Compare
rashmi-prithyani
changed the title
| if httpClient == nil { | ||
| timeout := config.Timeout | ||
| if timeout <= 0 { | ||
| timeout = DefaultTimeout | ||
| } | ||
| httpClient = createDefaultClient(config.Token, timeout) | ||
| } |
There was a problem hiding this comment.
why not just always required correct client from the caller?
| ) | ||
|
|
||
| // Config holds configuration for connecting to a GitHub backend. | ||
| type Config struct { |
There was a problem hiding this comment.
why do we need it? can we not make it part of caller responsibility? The problem i have with this is somewhat very dependent on how token is generated, so having it outside allows one to be flexible about it.
I could provide tokenProvider into oauth2 client which can create a httpclient that i can pass around without having to make provider be aware about how token is generated
rashmi-prithyani
force-pushed
the
rprithyani/implementGithubChangeProvider
branch
3 times, most recently
from
2c5f9ff to
b5cff4e
Compare
| }).Counter("get_errors").Inc(1) | ||
| fetchErrors = append(fetchErrors, fmt.Errorf("PR #%d: %w", cid.PRNumber, err)) | ||
| failedPRs = append(failedPRs, cid.PRNumber) | ||
| continue // Continue to next PR |
There was a problem hiding this comment.
do we need to continue?
There was a problem hiding this comment.
Thought here is that if we fail to get change info for one of the PRs in a stack, we still continue to fetch changes for the other PRs in the stack.
Then this would be marked as a retryable error and we could attempt to fetch in next attempt. That's why added the continue. What do you think ?
| type Client struct { | ||
| httpClient *http.Client | ||
| baseURL string | ||
| } |
There was a problem hiding this comment.
any reason we need it? httpClient should be enough, it can have baseURL in it, i think we can get rid of config file completely.
There was a problem hiding this comment.
I created this config file in response to this previous comment - #98 (comment)
If we are ok to pass in the base url as a param to provider struct, I can revert to the previous version. By itself I dont think the httpCleint has facility to configure a baseURL.
- Rename ID to URI with full scheme://org/repo/pr/sha format - Remove defensive checks and excessive logging - Simplify validation utilities (pure functions) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace Params with Config to encapsulate BaseURL + Token + Timeout together, addressing review feedback about HTTPClient encapsulation. Changes: - Add Config type with BaseURL, Token, Timeout, and optional HTTPClient - Derive GraphQL URL from BaseURL (no separate parameter needed) - Add configurable timeout (defaults to 30s) - Simplify main.go to use DefaultConfig() - Update all tests to use Config instead of Params Benefits: - Impossible to misconfigure (URL and auth bundled) - Multi-instance support (github.com + GHE) - Flexible timeout per instance - Cleaner API surface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace Config pattern with Client wrapper that encapsulates HTTP client and GraphQL URL. Simplifies provider by giving caller full control over authentication and endpoint configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rashmi-prithyani
force-pushed
the
rprithyani/implementGithubChangeProvider
branch
from
b5cff4e to
5c812f8
Compare
rashmi-prithyani
changed the title
|
Closing this one in favor of #141 which has cherry picked these changes and also addressed code review comments. |
2 participants
Summary
implements change provider - github
information
Test Plan
CI and tests
Issues