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
adamzip
force-pushed
the
use-github-etags
branch
from
2440457 to
6964c82
Compare
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
6964c82 to
94341af
Compare
|
@adamzip maybe we could revive this work next sprint, after the configuration repo work is mostly done. The amount of tracked PRs in Maestro doubled now so this might become useful again. |
Definitely! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (1)
src/Maestro/Maestro.DataProviders/RemoteFactory.cs:33
IRedisCacheClientis referenced here butMaestro.Services.Common.Cacheisn’t imported, which will prevent compilation.
private readonly IGitHubTokenProvider _gitHubTokenProvider;
private readonly IAzureDevOpsTokenProvider _azdoTokenProvider;
private readonly IServiceProvider _serviceProvider;
private readonly ICache _redisCacheClient;
public RemoteFactory(
BuildAssetRegistryContext context,
IGitHubTokenProvider gitHubTokenProvider,
IAzureDevOpsTokenProvider azdoTokenProvider,
| string? responseEtag = response.HttpResponse.Headers | ||
| .FirstOrDefault(h => h.Key.Equals("Etag", StringComparison.OrdinalIgnoreCase)) | ||
| .Value; |
There was a problem hiding this comment.
I inspected the code, Headers is a <string, string> dict
| @@ -35,7 +36,8 @@ public GitRepoFactory( | |||
| IProcessManager processManager, | |||
| IFileSystem fileSystem, | |||
| ILoggerFactory loggerFactory, | |||
| string temporaryPath) | |||
| string temporaryPath, | |||
| IRedisCacheClient redisCacheClient) | |||
| public RemoteFactory( | ||
| IAzureDevOpsTokenProvider azdoTokenProvider, | ||
| [FromKeyedServices("github")]IRemoteTokenProvider githubTokenProvider, | ||
| ILoggerFactory loggerFactory, | ||
| ICommandLineOptions options, | ||
| IServiceProvider serviceProvider) | ||
| IServiceProvider serviceProvider, | ||
| IRedisCacheClient redisCacheClient) | ||
| { | ||
| _loggerFactory = loggerFactory; | ||
| _options = options; | ||
| _serviceProvider = serviceProvider; | ||
| _azdoTokenProvider = azdoTokenProvider; | ||
| _githubTokenProvider = githubTokenProvider; | ||
| _redisCacheClient = redisCacheClient; |
#5064
#4842