Use etags in Github requests #4846
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements etag-based caching for GitHub API resource requests while updating various usages of pull request properties across the codebase. Key changes include updating pull request types to use Octokit types, adding caching via StackExchange.Redis in GitHubClient, and adjusting properties from TargetBranchCommitSha to HeadCommitSha where appropriate.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs | Uses Octokit.PullRequest instead of the ambiguous PullRequest type in tests. |
| test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs | Updates constructor parameters to accommodate additional Redis caching support. |
| src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs | Implements etag-based caching and updates pull request and review fetching logic. |
| src/Microsoft.DotNet.Darc/DarcLib/Models/IRemoteGitResource.cs | Introduces a new resource model with an etag property. |
| src/Maestro/Maestro.DataProviders/RemoteFactory.cs | Adds a direct connection to Redis, with a note to replace it with DI. |
Comments suppressed due to low confidence (1)
src/Microsoft.DotNet.Darc/DarcLib/Models/IRemoteGitResource.cs:5
- The name 'IRemoteGitResource' suggests an interface, but it is declared as a class. Consider renaming it to 'RemoteGitResource' or converting it to an interface for clarity.
public class IRemoteGitResource
| GitPullRequestReviews reviews = await RequestResourceUsingEtagsAsync<GitPullRequestReviews, ICollection<PullRequestReview>>( | ||
| pullRequestUrl, | ||
| "endpoint", |
There was a problem hiding this comment.
The endpoint argument is hardcoded as 'endpoint' in the RequestResourceUsingEtagsAsync call for pull request reviews. Consider constructing the correct endpoint URL to ensure the right API resource is fetched.
| GitPullRequestReviews reviews = await RequestResourceUsingEtagsAsync<GitPullRequestReviews, ICollection<PullRequestReview>>( | |
| pullRequestUrl, | |
| "endpoint", | |
| string reviewsEndpoint = $"/repos/{owner}/{repo}/pulls/{id}/reviews"; | |
| GitPullRequestReviews reviews = await RequestResourceUsingEtagsAsync<GitPullRequestReviews, ICollection<PullRequestReview>>( | |
| pullRequestUrl, | |
| reviewsEndpoint, |
Copilot uses AI. Check for mistakes.
| @@ -44,6 +47,7 @@ public RemoteFactory( | |||
| _azdoTokenProvider = azdoTokenProvider; | |||
| _cache = memoryCache; | |||
| _serviceProvider = serviceProvider; | |||
| _connection = ConnectionMultiplexer.Connect(options); //todo replace this with DI injection of IConnectionMultiplexer. We shouldn't do this in non-singleton classes, or even in general | |||
There was a problem hiding this comment.
Instantiating ConnectionMultiplexer directly in the constructor can lead to connection lifecycle issues. Consider using dependency injection to manage the connection instance.
Copilot uses AI. Check for mistakes.
| IGitHubClient client = GetClient(owner, repo); | ||
| GitPullRequestReviews reviews = await RequestResourceUsingEtagsAsync<GitPullRequestReviews, ICollection<PullRequestReview>>( | ||
| pullRequestUrl, | ||
| "endpoint", |
There was a problem hiding this comment.
TODO: replace this placeholder
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| namespace Microsoft.DotNet.DarcLib.Models; | ||
| public class IRemoteGitResource |
There was a problem hiding this comment.
rename class or turn into an interface
adamzip
changed the title
adamzip
force-pushed
the
use-github-etags
branch
3 times, most recently
from
63b8d98 to
d75009f
Compare
adamzip
force-pushed
the
use-github-etags
branch
from
d75009f to
dbe7b72
Compare
#5064