Use GitHub etags #5066
Use GitHub etags #5066
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces GitHub ETag caching functionality to improve API performance and reduce rate limiting issues when fetching GitHub resources. The implementation adds Redis-based caching support and refactors pull request models to use internal types instead of Octokit types.
- Implements ETag-based caching mechanism for GitHub API resources to reduce redundant requests
- Refactors pull request models to use internal DarcLib types instead of directly exposing Octokit types
- Adds Redis cache client integration across multiple components and test projects
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.DotNet.Darc/DarcLib/Models/*.cs | New model classes and interfaces supporting ETag caching |
| src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs | Implements ETag-based resource fetching and removes unused comment functionality |
| src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs | Updates to use HeadBranchCommitSha instead of TargetBranchCommitSha |
| test/* files | Updates constructors and type references to support new Redis cache dependency |
| src/*/RemoteFactory.cs | Adds Redis cache client parameter to factory constructors |
Comments suppressed due to low confidence (1)
src/Microsoft.DotNet.Darc/DarcLib/Models/GithubResourceConverters.cs:12
- [nitpick] The class name 'GitResourceConverters' should be 'GithubResourceConverters' to match the filename and maintain consistency with the GitHub naming convention used elsewhere in the codebase.
internal class GitResourceConverters
adamzip
force-pushed
the
use-github-etags
branch
2 times, most recently
from
3d0485e to
76a28f5
Compare
| IGitHubClient client, | ||
| Func<K, T> resourceConverter) | ||
| { | ||
| await Task.FromResult<T>(null); |
There was a problem hiding this comment.
Not sure I understand the point of this implementation in general
There was a problem hiding this comment.
We are testing GithubClient's GetLatestPullRequestReviewsAsync as a whole, but we need to mock the part of GithubClient (the overriden method implementation you are commenting on) that actually fetches the reviews.
I hate this implementation too so let me know if you have another idea.
Even though RequestResourceUsingEtagsAsync is just one method, maybe it should be on a different layer than GithubClient - just how the Octokit client that actually fetches the resource is on a different layer from GithubClient, which has logic for what to do with the base resources
TargetBranchCommitSha was erroneously called "target branch" because it represents the branch in the codeflow that belongs to the `target` repo. Howevever, in practice it is actually the PR Head branch. Therefore it is now renamed to HeadBranchCommitSha. Furthermore, setting HeadBRanchCommitSha is fixed in the Azdo client.
adamzip
force-pushed
the
use-github-etags
branch
from
2440457 to
6964c82
Compare
3 participants
#5064