From d1b9ef6deec7bdf7ed980c196938593af03cf498 Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Mon, 21 Jul 2025 00:46:16 +0200 Subject: [PATCH 01/10] [REFACTORING] Move PullRequest to DarcLib.Models --- .../DarcLib/GitHubClient.cs | 10 +++++----- .../DarcLib/IRemoteGitRepo.cs | 13 ------------- .../DarcLib/Models/PullRequest.cs | 16 ++++++++++++++++ .../RemoteTests.cs | 1 + .../CodeFlowScenarioTestBase.cs | 10 +++++----- 5 files changed, 27 insertions(+), 23 deletions(-) create mode 100644 src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs diff --git a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs index 3a00d8b86e..e29006c41d 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs @@ -300,7 +300,7 @@ public async Task> SearchPullRequestsAsync( /// /// Uri of the pull request /// Information on the pull request. - public async Task GetPullRequestAsync(string pullRequestUrl) + public async Task GetPullRequestAsync(string pullRequestUrl) { (string owner, string repo, int id) = ParsePullRequestUri(pullRequestUrl); Octokit.PullRequest pr = await GetClient(owner, repo).PullRequest.Get(owner, repo, id); @@ -315,7 +315,7 @@ public async Task GetPullRequestAsync(string pullRequestUrl) status = PrStatus.Open; } - return new PullRequest + return new Models.PullRequest { Title = pr.Title, Description = pr.Body, @@ -332,7 +332,7 @@ public async Task GetPullRequestAsync(string pullRequestUrl) /// /// Repo to create the pull request for. /// Pull request data - public async Task CreatePullRequestAsync(string repoUri, PullRequest pullRequest) + public async Task CreatePullRequestAsync(string repoUri, Models.PullRequest pullRequest) { (string owner, string repo) = ParseRepoUri(repoUri); @@ -359,7 +359,7 @@ public async Task CreatePullRequestAsync(string repoUri, PullRequest pul /// /// Uri of pull request to update /// Pull request info to update - public async Task UpdatePullRequestAsync(string pullRequestUri, PullRequest pullRequest) + public async Task UpdatePullRequestAsync(string pullRequestUri, Models.PullRequest pullRequest) { (string owner, string repo, int id) = ParsePullRequestUri(pullRequestUri); @@ -1232,7 +1232,7 @@ public async Task RepoExistsAsync(string repoUri) /// Async task public async Task DeletePullRequestBranchAsync(string pullRequestUri) { - PullRequest pr = await GetPullRequestAsync(pullRequestUri); + Models.PullRequest pr = await GetPullRequestAsync(pullRequestUri); (string owner, string repo, int id) prInfo = ParsePullRequestUri(pullRequestUri); await DeleteBranchAsync(prInfo.owner, prInfo.repo, pr.HeadBranch); } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs b/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs index ed1ffbb05d..c0567d129c 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs @@ -4,7 +4,6 @@ using Maestro.MergePolicyEvaluation; using Microsoft.DotNet.DarcLib.Helpers; using Microsoft.DotNet.DarcLib.Models; -using System; using System.Collections.Generic; using System.Threading.Tasks; @@ -177,15 +176,3 @@ Task> SearchPullRequestsAsync( /// List of comments Task> GetPullRequestCommentsAsync(string pullRequestUrl); } - -#nullable disable -public class PullRequest -{ - public string Title { get; set; } - public string Description { get; set; } - public string BaseBranch { get; set; } - public string HeadBranch { get; set; } - public PrStatus Status { get; set; } - public DateTimeOffset UpdatedAt { get; set; } - public string TargetBranchCommitSha { get; set; } -} diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs new file mode 100644 index 0000000000..96c00d058f --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs @@ -0,0 +1,16 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +namespace Microsoft.DotNet.DarcLib.Models; +public class PullRequest +{ + public string Title { get; set; } + public string Description { get; set; } + public string BaseBranch { get; set; } + public string HeadBranch { get; set; } + public PrStatus Status { get; set; } + public DateTimeOffset UpdatedAt { get; set; } + public string TargetBranchCommitSha { get; set; } +} diff --git a/test/Microsoft.DotNet.DarcLib.Tests/RemoteTests.cs b/test/Microsoft.DotNet.DarcLib.Tests/RemoteTests.cs index 559774f3db..b98d1d97c3 100644 --- a/test/Microsoft.DotNet.DarcLib.Tests/RemoteTests.cs +++ b/test/Microsoft.DotNet.DarcLib.Tests/RemoteTests.cs @@ -5,6 +5,7 @@ using System.Threading.Tasks; using FluentAssertions; using Microsoft.DotNet.DarcLib.Helpers; +using Microsoft.DotNet.DarcLib.Models; using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.DotNet.Internal.Testing.Utility; using Moq; diff --git a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs index d15ae29ce9..e6b06c7ea5 100644 --- a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs @@ -23,7 +23,7 @@ protected async Task CheckForwardFlowGitHubPullRequest( { // When we expect updates from multiple repos (batchable subscriptions), we need to wait until the PR gets updated with the second repository after it is created // Otherwise it might try to validate the contents before all updates are in place - PullRequest pullRequest = repoUpdates.Length > 1 + Octokit.PullRequest pullRequest = repoUpdates.Length > 1 ? await WaitForUpdatedPullRequestAsync(targetRepoName, targetBranch) : await WaitForPullRequestAsync(targetRepoName, targetBranch); @@ -74,7 +74,7 @@ protected async Task CheckBackwardFlowGitHubPullRequest( string commitSha, int buildId) { - PullRequest pullRequest = await WaitForPullRequestAsync(targetRepoName, targetBranch); + Octokit.PullRequest pullRequest = await WaitForPullRequestAsync(targetRepoName, targetBranch); await using (CleanUpPullRequestAfter(TestParameters.GitHubTestOrg, targetRepoName, pullRequest)) { @@ -194,13 +194,13 @@ private static async Task> CreateSourceEnabledSubsc trigger); } - public async Task WaitForPullRequestComment( + public async Task WaitForPullRequestComment( string targetRepo, string targetBranch, string partialComment, int attempts = 40) { - PullRequest pullRequest = await WaitForPullRequestAsync(targetRepo, targetBranch); + Octokit.PullRequest pullRequest = await WaitForPullRequestAsync(targetRepo, targetBranch); while (attempts-- > 0) { @@ -218,7 +218,7 @@ public async Task WaitForPullRequestComment( public static async Task CheckIfPullRequestCommentExists( string targetRepo, - PullRequest pullRequest, + Octokit.PullRequest pullRequest, string[] stringsExpectedInComment) { IReadOnlyList comments = await GitHubApi.Issue.Comment.GetAllForIssue(TestParameters.GitHubTestOrg, targetRepo, pullRequest.Number); From 7b7d1e9fde7ab30a5eebfd0a618be86a9207f3cd Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Mon, 21 Jul 2025 01:03:43 +0200 Subject: [PATCH 02/10] [REFACTORING] Remove dead code in GithubClient --- .../DarcLib/GitHubClient.cs | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs index e29006c41d..cec3a6288b 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs @@ -34,11 +34,7 @@ public class GitHubClient : RemoteRepoBase, IRemoteGitRepo private const string DarcLibVersion = "1.0.0"; private static readonly ProductHeaderValue _product; - private static readonly string CommentMarker = - "\n\n[//]: # (This identifies this comment as a Maestro++ comment)\n"; - private static readonly Regex RepositoryUriPattern = new(@"^/(?[^/]+)/(?[^/]+)/?$"); - private static readonly Regex PullRequestUriPattern = new(@"^/repos/(?[^/]+)/(?[^/]+)/pulls/(?\d+)$"); @@ -438,25 +434,6 @@ public async Task MergeDependencyPullRequestAsync(string pullRequestUrl, MergePu } } - /// - /// Create a new comment, or update the last comment with an updated message, - /// if that comment was created by Darc. - /// - /// Url of pull request - /// Message to post - public async Task CreateOrUpdatePullRequestCommentAsync(string pullRequestUrl, string message) - { - (string owner, string repo, int id) = ParsePullRequestUri(pullRequestUrl); - IssueComment lastComment = (await GetClient(owner, repo).Issue.Comment.GetAllForIssue(owner, repo, id))[^1]; - if (lastComment != null && lastComment.Body.EndsWith(CommentMarker)) - { - await GetClient(owner, repo).Issue.Comment.Update(owner, repo, lastComment.Id, message + CommentMarker); - } - else - { - await GetClient(owner, repo).Issue.Comment.Create(owner, repo, id, message + CommentMarker); - } - } /// /// Returns the ID used to identify the maestro merge policies checks in a PR From 2938e6bc5a6d0953b36493d96f03760d1fddc0f8 Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Mon, 21 Jul 2025 17:55:30 +0200 Subject: [PATCH 03/10] [REFACTORING] Simplify GithubClient.GetLatestPullRequestReviewsAsync --- .../DarcLib/GitHubClient.cs | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs index cec3a6288b..ec8d893224 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs @@ -931,21 +931,14 @@ public async Task> GetLatestPullRequestReviewsAsync(string pullReq var reviews = await GetClient(owner, repo).Repository.PullRequest.Review.GetAll(owner, repo, id); - // Filter out comments because they could come after Approved/ChangedRequested, and they don't change the decision. - reviews = reviews.Where(r => r.State != PullRequestReviewState.Commented).ToImmutableList(); - - // Grab the top review by SubmittedAt from what remains - var newestActionableReviews = reviews.GroupBy(r => r.User.Login) - .ToDictionary(g => g.Key, - g => (from r in reviews - where r.User.Login == g.Key - select r) - .OrderByDescending(r => r.SubmittedAt) - .First()) - .Values; - - return newestActionableReviews.Select(review => - new Review(TranslateReviewState(review.State.Value), pullRequestUrl)).ToList(); + var actionableReviews = reviews + .Where(r => r.State != PullRequestReviewState.Commented) // filter out reviews that don't affect approval/RFC + .GroupBy(r => r.User.Login) + .Select(g => g.OrderByDescending(r => r.SubmittedAt).First()) // pick each user's most recent review + .Select(review => new Review(TranslateReviewState(review.State.Value), pullRequestUrl)) + .ToList(); + + return actionableReviews; } private static ReviewState TranslateReviewState(PullRequestReviewState state) From 560a1c71fb67a6be1ba86d6ba01cd7d180d37ae7 Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Mon, 21 Jul 2025 01:30:59 +0200 Subject: [PATCH 04/10] [BUGFIX] Azdo client fetches wrong branch's SHA 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. --- src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs | 2 +- src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs | 2 +- .../PullRequestUpdater.cs | 4 ++-- .../PullRequestUpdaterTests.cs | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs index 3a8bbac79c..b8068fbfe3 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs @@ -344,7 +344,7 @@ public async Task GetPullRequestAsync(string pullRequestUrl) _ => PrStatus.None, }, UpdatedAt = DateTimeOffset.UtcNow, - TargetBranchCommitSha = pr.LastMergeTargetCommit.CommitId, + HeadBranchCommitSha = pr.LastMergeSourceCommit.CommitId, }; } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs index 96c00d058f..52a6629049 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs @@ -12,5 +12,5 @@ public class PullRequest public string HeadBranch { get; set; } public PrStatus Status { get; set; } public DateTimeOffset UpdatedAt { get; set; } - public string TargetBranchCommitSha { get; set; } + public string HeadBranchCommitSha { get; set; } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 6a8270e7eb..2035814ac3 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -422,11 +422,11 @@ private async Task TryMergingPrAsync( PullRequestUpdateSummary prSummary = CreatePrSummaryFromInProgressPr(pr, targetRepository); MergePolicyEvaluationResults? cachedResults = await _mergePolicyEvaluationState.TryGetStateAsync(); - IEnumerable updatedMergePolicyResults = await _mergePolicyEvaluator.EvaluateAsync(prSummary, remote, policyDefinitions, cachedResults, prInfo.TargetBranchCommitSha); + IEnumerable updatedMergePolicyResults = await _mergePolicyEvaluator.EvaluateAsync(prSummary, remote, policyDefinitions, cachedResults, prInfo.HeadBranchCommitSha); MergePolicyEvaluationResults updatedResult = new MergePolicyEvaluationResults( updatedMergePolicyResults.ToImmutableList(), - prInfo.TargetBranchCommitSha); + prInfo.HeadBranchCommitSha); await _mergePolicyEvaluationState.SetAsync(updatedResult); diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index e0ea4ce320..ef5ec81a31 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -470,7 +470,7 @@ protected IDisposable WithExistingCodeFlowPullRequest( Status = prStatus, HeadBranch = InProgressPrHeadBranch, BaseBranch = TargetBranch, - TargetBranchCommitSha = "sha123456", + HeadBranchCommitSha = "sha123456", }); if (willFlowNewBuild From 6485e94421107a071f346b545897912c1e192280 Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Mon, 21 Jul 2025 01:50:49 +0200 Subject: [PATCH 05/10] Use eTag when fetching GitHub resources --- .../Maestro.DataProviders/RemoteFactory.cs | 6 +- .../Darc/Helpers/RemoteFactory.cs | 11 +- .../DarcLib/GitHubClient.cs | 168 +++++++++++------- .../DarcLib/GitRepoFactory.cs | 10 +- .../Models/GithubResourceConverters.cs | 64 +++++++ .../DarcLib/Models/GithubReview.cs | 26 +++ .../DarcLib/Models/IGithubEtagResource.cs | 13 ++ .../DarcLib/Models/PullRequest.cs | 4 +- .../DarcLib/Models/PullRequestReviews.cs | 12 ++ .../CodeFlowTestsBase.cs | 3 +- .../Helpers/RemoteFactory.cs | 11 +- .../GitHubClientTests.cs | 65 ++++--- 12 files changed, 294 insertions(+), 99 deletions(-) create mode 100644 src/Microsoft.DotNet.Darc/DarcLib/Models/GithubResourceConverters.cs create mode 100644 src/Microsoft.DotNet.Darc/DarcLib/Models/GithubReview.cs create mode 100644 src/Microsoft.DotNet.Darc/DarcLib/Models/IGithubEtagResource.cs create mode 100644 src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequestReviews.cs diff --git a/src/Maestro/Maestro.DataProviders/RemoteFactory.cs b/src/Maestro/Maestro.DataProviders/RemoteFactory.cs index a9f74a9517..3d56de64ef 100644 --- a/src/Maestro/Maestro.DataProviders/RemoteFactory.cs +++ b/src/Maestro/Maestro.DataProviders/RemoteFactory.cs @@ -25,6 +25,7 @@ public class RemoteFactory : IRemoteFactory private readonly IGitHubTokenProvider _gitHubTokenProvider; private readonly IAzureDevOpsTokenProvider _azdoTokenProvider; private readonly IServiceProvider _serviceProvider; + private readonly IRedisCacheClient _redisCacheClient; public RemoteFactory( BuildAssetRegistryContext context, @@ -34,6 +35,7 @@ public RemoteFactory( DarcRemoteMemoryCache memoryCache, OperationManager operations, IProcessManager processManager, + IRedisCacheClient redisCacheClient, ILoggerFactory loggerFactory, IServiceProvider serviceProvider) { @@ -45,6 +47,7 @@ public RemoteFactory( _azdoTokenProvider = azdoTokenProvider; _cache = memoryCache; _serviceProvider = serviceProvider; + _redisCacheClient = redisCacheClient; } public async Task CreateRemoteAsync(string repoUrl) @@ -88,7 +91,8 @@ private async Task GetRemoteGitClient(string repoUrl) new Microsoft.DotNet.DarcLib.GitHubTokenProvider(_gitHubTokenProvider), _processManager, _loggerFactory.CreateLogger(), - _cache.Cache), + _cache.Cache, + _redisCacheClient), GitRepoType.AzureDevOps => new AzureDevOpsClient( _azdoTokenProvider, diff --git a/src/Microsoft.DotNet.Darc/Darc/Helpers/RemoteFactory.cs b/src/Microsoft.DotNet.Darc/Darc/Helpers/RemoteFactory.cs index 589442ad95..568e989301 100644 --- a/src/Microsoft.DotNet.Darc/Darc/Helpers/RemoteFactory.cs +++ b/src/Microsoft.DotNet.Darc/Darc/Helpers/RemoteFactory.cs @@ -18,15 +18,18 @@ internal class RemoteFactory : IRemoteFactory private readonly ILoggerFactory _loggerFactory; private readonly ICommandLineOptions _options; private readonly IServiceProvider _serviceProvider; + private readonly IRedisCacheClient _redisCacheClient; public RemoteFactory( ILoggerFactory loggerFactory, ICommandLineOptions options, - IServiceProvider serviceProvider) + IServiceProvider serviceProvider, + IRedisCacheClient redisCacheClient) { _loggerFactory = loggerFactory; _options = options; _serviceProvider = serviceProvider; + _redisCacheClient = redisCacheClient; } public Task CreateRemoteAsync(string repoUrl) @@ -53,10 +56,10 @@ private IRemoteGitRepo CreateRemoteGitClient(ICommandLineOptions options, string new GitHubClient( options.GetGitHubTokenProvider(), new ProcessManager(_loggerFactory.CreateLogger(), options.GitLocation), - _loggerFactory.CreateLogger(), temporaryRepositoryRoot, - // Caching not in use for Darc local client. - null), + null, // Memory Caching not in use for Darc local client. + _redisCacheClient, + _loggerFactory.CreateLogger()), GitRepoType.AzureDevOps => new AzureDevOpsClient( diff --git a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs index ec8d893224..614141571e 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs @@ -3,12 +3,12 @@ using System; using System.Collections.Generic; -using System.Collections.Immutable; using System.IO; using System.Linq; using System.Net; using System.Net.Http; using System.Reflection; +using System.Security.Policy; using System.Text; using System.Text.RegularExpressions; using System.Threading.Tasks; @@ -44,6 +44,7 @@ public class GitHubClient : RemoteRepoBase, IRemoteGitRepo private readonly string _userAgent = $"DarcLib-{DarcLibVersion}"; private IGitHubClient? _lazyClient = null; private readonly Dictionary<(string, string, string?), string> _gitRefCommitCache; + private readonly IRedisCacheClient _cache; static GitHubClient() { @@ -57,17 +58,19 @@ public GitHubClient( IRemoteTokenProvider remoteTokenProvider, IProcessManager processManager, ILogger logger, - IMemoryCache? cache) - : this(remoteTokenProvider, processManager, logger, null, cache) + IMemoryCache? cache, + IRedisCacheClient redisClient) + : this(remoteTokenProvider, processManager, null, cache, redisClient, logger) { } public GitHubClient( IRemoteTokenProvider remoteTokenProvider, IProcessManager processManager, - ILogger logger, string? temporaryRepositoryPath, - IMemoryCache? cache) + IMemoryCache? cache, + IRedisCacheClient redisClient, + ILogger logger) : base(remoteTokenProvider, processManager, temporaryRepositoryPath, cache, logger) { _tokenProvider = remoteTokenProvider; @@ -78,6 +81,7 @@ public GitHubClient( NullValueHandling = NullValueHandling.Ignore }; _gitRefCommitCache = []; + _cache = redisClient; } public bool AllowRetries { get; set; } = true; @@ -299,28 +303,18 @@ public async Task> SearchPullRequestsAsync( public async Task GetPullRequestAsync(string pullRequestUrl) { (string owner, string repo, int id) = ParsePullRequestUri(pullRequestUrl); - Octokit.PullRequest pr = await GetClient(owner, repo).PullRequest.Get(owner, repo, id); - PrStatus status; - if (pr.State == ItemState.Closed) - { - status = pr.Merged == true ? PrStatus.Merged : PrStatus.Closed; - } - else - { - status = PrStatus.Open; - } + IGitHubClient client = GetClient(owner, repo); - return new Models.PullRequest - { - Title = pr.Title, - Description = pr.Body, - BaseBranch = pr.Base.Ref, - HeadBranch = pr.Head.Ref, - Status = status, - UpdatedAt = pr.UpdatedAt, - TargetBranchCommitSha = pr.Head.Sha, - }; + var resourceUri = ApiUrls.PullRequest(owner, repo, id); + + Models.PullRequest result = await RequestResourceUsingEtagsAsync( + pullRequestUrl, + resourceUri, + client, + GithubResourceConverters.ConvertPullRequest); + + return result; } /// @@ -403,7 +397,7 @@ public async Task MergeDependencyPullRequestAsync(string pullRequestUrl, MergePu IGitHubClient gitHubClient = GetClient(owner, repo); - Octokit.PullRequest pr = await gitHubClient.PullRequest.Get(owner, repo, id); + Models.PullRequest pr = await GetPullRequestAsync(pullRequestUrl); var mergePullRequest = new MergePullRequest { @@ -425,11 +419,11 @@ public async Task MergeDependencyPullRequestAsync(string pullRequestUrl, MergePu { try { - await gitHubClient.Git.Reference.Delete(owner, repo, $"heads/{pr.Head.Ref}"); + await gitHubClient.Git.Reference.Delete(owner, repo, $"heads/{pr.HeadBranch}"); } catch (Exception ex) { - _logger.LogInformation("Couldn't delete branch {sourceBranch} - {message}", pr.Head.Ref, ex.Message); + _logger.LogInformation("Couldn't delete branch {sourceBranch} - {message}", pr.HeadBranch, ex.Message); } } } @@ -449,26 +443,25 @@ public async Task CreateOrUpdatePullRequestMergeStatusInfoAsync(string pullReque { (string owner, string repo, int id) = ParsePullRequestUri(pullRequestUrl); var client = GetClient(owner, repo); - // Get the sha of the latest commit for the current PR - string prSha = (await client.PullRequest.Get(owner, repo, id))?.Head?.Sha - ?? throw new InvalidOperationException("We cannot find the sha of the pull request"); + + Models.PullRequest pr = await GetPullRequestAsync(pullRequestUrl); // Get a list of all the merge policies checks runs for the current PR List existingChecksRuns = - (await client.Check.Run.GetAllForReference(owner, repo, prSha)) + (await client.Check.Run.GetAllForReference(owner, repo, pr.HeadBranchCommitSha)) .CheckRuns.Where(e => e.ExternalId.StartsWith(MergePolicyConstants.MaestroMergePolicyCheckRunPrefix)).ToList(); - var toBeAdded = evaluations.Where(e => existingChecksRuns.All(c => c.ExternalId != CheckRunId(e, prSha))); - var toBeUpdated = existingChecksRuns.Where(c => evaluations.Any(e => c.ExternalId == CheckRunId(e, prSha))); - var toBeDeleted = existingChecksRuns.Where(c => evaluations.All(e => c.ExternalId != CheckRunId(e, prSha))); + var toBeAdded = evaluations.Where(e => existingChecksRuns.All(c => c.ExternalId != CheckRunId(e, pr.HeadBranchCommitSha))); + var toBeUpdated = existingChecksRuns.Where(c => evaluations.Any(e => c.ExternalId == CheckRunId(e, pr.HeadBranchCommitSha))); + var toBeDeleted = existingChecksRuns.Where(c => evaluations.All(e => c.ExternalId != CheckRunId(e, pr.HeadBranchCommitSha))); foreach (var newCheckRunValidation in toBeAdded) { - await client.Check.Run.Create(owner, repo, CheckRunForAdd(newCheckRunValidation, prSha)); + await client.Check.Run.Create(owner, repo, CheckRunForAdd(newCheckRunValidation, pr.HeadBranchCommitSha)); } foreach (var updatedCheckRun in toBeUpdated) { - MergePolicyEvaluationResult eval = evaluations.Last(e => updatedCheckRun.ExternalId == CheckRunId(e, prSha)); + MergePolicyEvaluationResult eval = evaluations.Last(e => updatedCheckRun.ExternalId == CheckRunId(e, pr.HeadBranchCommitSha)); if (eval.IsCachedResult) { _logger.LogInformation("Not updating check run {checkRunId} for PR {pullRequestUrl} because the merge policy was not re-evaluated.", @@ -907,13 +900,12 @@ public async Task> GetPullRequestChecksAsync(string pullRequestUrl) { (string owner, string repo, int id) = ParsePullRequestUri(pullRequestUrl); - var commits = await GetClient(owner, repo).Repository.PullRequest.Commits(owner, repo, id); - var lastCommitSha = commits[commits.Count - 1].Sha; + Models.PullRequest pr = await GetPullRequestAsync(pullRequestUrl); return [ - .. await GetChecksFromStatusApiAsync(owner, repo, lastCommitSha), - .. await GetChecksFromChecksApiAsync(owner, repo, lastCommitSha), + .. await GetChecksFromStatusApiAsync(owner, repo, pr.HeadBranchCommitSha), + .. await GetChecksFromChecksApiAsync(owner, repo, pr.HeadBranchCommitSha), ]; } @@ -929,31 +921,22 @@ public async Task> GetLatestPullRequestReviewsAsync(string pullReq { (string owner, string repo, int id) = ParsePullRequestUri(pullRequestUrl); - var reviews = await GetClient(owner, repo).Repository.PullRequest.Review.GetAll(owner, repo, id); + IGitHubClient client = GetClient(owner, repo); + var pullRequestReviewsUri = ApiUrls.PullRequestReviews(owner, repo, id); - var actionableReviews = reviews - .Where(r => r.State != PullRequestReviewState.Commented) // filter out reviews that don't affect approval/RFC - .GroupBy(r => r.User.Login) - .Select(g => g.OrderByDescending(r => r.SubmittedAt).First()) // pick each user's most recent review - .Select(review => new Review(TranslateReviewState(review.State.Value), pullRequestUrl)) - .ToList(); + var pullRequestReviews = await RequestResourceUsingEtagsAsync>( + pullRequestUrl, + pullRequestReviewsUri, + client, + GithubResourceConverters.ConvertPullRequestReviews); - return actionableReviews; - } + var newestActionableReviews = pullRequestReviews.Reviews + .Where(r => r.Status != ReviewState.Commented) // filter out reviews that don't affect approval/RFC + .GroupBy(r => r.User) + .Select(g => (Review) g.OrderByDescending(r => r.SubmittedAt).First()) // pick each user's most recent review + .ToList(); - private static ReviewState TranslateReviewState(PullRequestReviewState state) - { - return state switch - { - PullRequestReviewState.Approved => ReviewState.Approved, - PullRequestReviewState.ChangesRequested => ReviewState.ChangesRequested, - PullRequestReviewState.Commented => ReviewState.Commented, - // A PR comment could be dismissed by a new push, so this does not count as a rejection. - // Change to a comment - PullRequestReviewState.Dismissed => ReviewState.Commented, - PullRequestReviewState.Pending => ReviewState.Pending, - _ => throw new NotImplementedException($"Unexpected pull request review state {state}"), - }; + return newestActionableReviews; } private async Task> GetChecksFromStatusApiAsync(string owner, string repo, string @ref) @@ -1315,4 +1298,63 @@ public async Task> GetPullRequestCommentsAsync(string pullRequestUr return comments.Select(comment => comment.Body).ToList(); } + + /// + /// This method fills a functionality that's currently missing from Octokit: fetching Github resources using eTags. + /// eTags allow us to cache mutable resources and efficiently check if the resource has changed on Github since the last fetch. + /// + /// The domain class of the resource in our server + /// The class of the resource in Octokit + /// The key used to cache the resource in redis + /// The uri used to request the resource from Github + /// The github client that makes the request + /// Function to convert the resource from Octokit to our domain class + /// The resource of type T + /// + protected virtual async Task RequestResourceUsingEtagsAsync( + string resourceKey, + Uri resourceUri, + IGitHubClient client, + Func resourceConverter) + where T : class, IGithubEtagResource + { + var cachedResource = await _cache.TryGetAsync(resourceKey); + string? entityTag = cachedResource?.Etag; + var headers = new Dictionary + { + { "Accept", "application/vnd.github.v3+json" }, + }; + if (entityTag != null) + { + headers.Add("If-None-Match", entityTag); + } + var response = await client.Connection.Get(resourceUri, headers); + if (response.HttpResponse.StatusCode == HttpStatusCode.NotModified && cachedResource != null) + { + // TODO: Add telemetry for cache hits to measure the impact of this optimization. + return cachedResource; + } + else + { + if (response.HttpResponse.StatusCode == HttpStatusCode.OK) + { + string? etag = response.HttpResponse.Headers + .FirstOrDefault(h => h.Key.Equals("Etag", StringComparison.OrdinalIgnoreCase)) + .Value; + + var resource = resourceConverter(response.Body); + + if (etag != null) + { + resource.Etag = etag; + await _cache.TrySetAsync(resourceKey, resource); + } + return resource; + } + else + { + throw new DarcException($"Failed to get {typeof(T).Name} from GitHub. Status code: {response.HttpResponse.StatusCode}"); + } + } + } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/GitRepoFactory.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitRepoFactory.cs index e827178929..0dd7b7fdce 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/GitRepoFactory.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/GitRepoFactory.cs @@ -24,6 +24,7 @@ public class GitRepoFactory : IGitRepoFactory private readonly IFileSystem _fileSystem; private readonly ILoggerFactory _loggerFactory; private readonly string? _temporaryPath = null; + private readonly IRedisCacheClient _redisCacheClient; public GitRepoFactory( IRemoteTokenProvider remoteTokenProvider, @@ -32,7 +33,8 @@ public GitRepoFactory( IProcessManager processManager, IFileSystem fileSystem, ILoggerFactory loggerFactory, - string temporaryPath) + string temporaryPath, + IRedisCacheClient redisCacheClient) { _remoteTokenProvider = remoteTokenProvider; _azdoTokenProvider = azdoTokenProvider; @@ -41,6 +43,7 @@ public GitRepoFactory( _fileSystem = fileSystem; _loggerFactory = loggerFactory; _temporaryPath = temporaryPath; + _redisCacheClient = redisCacheClient; } public IGitRepo CreateClient(string repoUri) => GitRepoUrlUtils.ParseTypeFromUri(repoUri) switch @@ -54,10 +57,11 @@ public GitRepoFactory( GitRepoType.GitHub => new GitHubClient( _remoteTokenProvider, _processManager, - _loggerFactory.CreateLogger(), _temporaryPath, // Caching not in use for Darc local client. - null), + null, + _redisCacheClient, + _loggerFactory.CreateLogger()), GitRepoType.Local => new LocalLibGit2Client( _remoteTokenProvider, diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/GithubResourceConverters.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/GithubResourceConverters.cs new file mode 100644 index 0000000000..2bbfc328a5 --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/GithubResourceConverters.cs @@ -0,0 +1,64 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Linq; +using Microsoft.DotNet.DarcLib.Models; +using Octokit; + +namespace Microsoft.DotNet.DarcLib; + +internal class GithubResourceConverters +{ + internal static Models.PullRequest ConvertPullRequest(Octokit.PullRequest pr) + { + var status = pr.State == ItemState.Closed ? + (pr.Merged == true ? PrStatus.Merged : PrStatus.Closed) : + PrStatus.Open; + + return new() + { + Title = pr.Title, + Description = pr.Body, + BaseBranch = pr.Base.Ref, + HeadBranch = pr.Head.Ref, + Status = status, + UpdatedAt = pr.UpdatedAt, + HeadBranchCommitSha = pr.Head.Sha, + }; + } + + internal static PullRequestReviews ConvertPullRequestReviews(IEnumerable pullRequestReviews) + { + IEnumerable reviews = pullRequestReviews + .Select(r => new GithubReview( + TranslateReviewState(r.State.Value), + r.PullRequestUrl, + r.User.Login, + r.SubmittedAt)) + .ToList(); + + return new PullRequestReviews + { + Reviews = reviews + }; + } + + + private static ReviewState TranslateReviewState(PullRequestReviewState state) + { + return state switch + { + PullRequestReviewState.Approved => ReviewState.Approved, + PullRequestReviewState.ChangesRequested => ReviewState.ChangesRequested, + PullRequestReviewState.Commented => ReviewState.Commented, + // A PR comment could be dismissed by a new push, so this does not count as a rejection. + // Change to a comment + PullRequestReviewState.Dismissed => ReviewState.Commented, + PullRequestReviewState.Pending => ReviewState.Pending, + _ => throw new NotImplementedException($"Unexpected pull request review state {state}"), + }; + } + +} diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/GithubReview.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/GithubReview.cs new file mode 100644 index 0000000000..153c036428 --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/GithubReview.cs @@ -0,0 +1,26 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +namespace Microsoft.DotNet.DarcLib.Models; + +public class GithubReview : Review +{ + public GithubReview(ReviewState state, string url, string user, DateTimeOffset submittedAt) + : base(state, url) + { + User = user; + SubmittedAt = submittedAt; + } + + /// + /// The user who submitted the review. + /// + public string User { get; set; } + + /// + /// The date and time when the review was submitted. + /// + public DateTimeOffset SubmittedAt { get; private set; } +} diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/IGithubEtagResource.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/IGithubEtagResource.cs new file mode 100644 index 0000000000..e307bf86ad --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/IGithubEtagResource.cs @@ -0,0 +1,13 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.DotNet.DarcLib.Models; + +/// +/// Represents resources for which we use Etag caching via the Github API. +/// Using Etag when requesting resources from Github improves speed and reduces rate-limiting issues. +/// +public interface IGithubEtagResource +{ + string Etag { get; set; } +} diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs index 52a6629049..9348ac76dd 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs @@ -4,7 +4,7 @@ using System; namespace Microsoft.DotNet.DarcLib.Models; -public class PullRequest +public class PullRequest : IGithubEtagResource { public string Title { get; set; } public string Description { get; set; } @@ -13,4 +13,6 @@ public class PullRequest public PrStatus Status { get; set; } public DateTimeOffset UpdatedAt { get; set; } public string HeadBranchCommitSha { get; set; } + public string Etag { get; set; } + } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequestReviews.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequestReviews.cs new file mode 100644 index 0000000000..f92c8af486 --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequestReviews.cs @@ -0,0 +1,12 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; + +namespace Microsoft.DotNet.DarcLib.Models; +public class PullRequestReviews : IGithubEtagResource +{ + public IEnumerable Reviews { get; set; } + public string Etag { get; set; } + +} diff --git a/test/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs b/test/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs index 07598dd13d..d311c3e80b 100644 --- a/test/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs +++ b/test/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs @@ -98,7 +98,8 @@ public void DeleteCurrentTestDirectory() .AddSingleVmrSupport("git", VmrPath, TmpPath, null, null) .AddSingleton(_basicBarClient.Object) .AddTransient() - .AddScoped(); + .AddScoped() + .AddSingleton(); protected static List GetExpectedFilesInVmr( NativePath vmrPath, diff --git a/test/Microsoft.DotNet.DarcLib.Codeflow.Tests/Helpers/RemoteFactory.cs b/test/Microsoft.DotNet.DarcLib.Codeflow.Tests/Helpers/RemoteFactory.cs index 7c2b604494..9abef729e7 100644 --- a/test/Microsoft.DotNet.DarcLib.Codeflow.Tests/Helpers/RemoteFactory.cs +++ b/test/Microsoft.DotNet.DarcLib.Codeflow.Tests/Helpers/RemoteFactory.cs @@ -17,15 +17,18 @@ internal class RemoteFactory : IRemoteFactory private readonly IProcessManager _processManager; private readonly ILoggerFactory _loggerFactory; private readonly IServiceProvider _serviceProvider; + private readonly IRedisCacheClient _redisCacheClient; public RemoteFactory( IProcessManager processManager, ILoggerFactory loggerFactory, - IServiceProvider serviceProvider) + IServiceProvider serviceProvider, + IRedisCacheClient redisCacheClient) { _processManager = processManager; _loggerFactory = loggerFactory; _serviceProvider = serviceProvider; + _redisCacheClient = redisCacheClient; } @@ -53,10 +56,10 @@ private IRemoteGitRepo CreateRemoteGitClient(string repoUrl) new GitHubClient( new ResolvedTokenProvider(null), _processManager, - _loggerFactory.CreateLogger(), temporaryRepositoryRoot, - // Caching not in use for Darc local client. - null), + null, // Caching not in use for Darc local client. + _redisCacheClient, + _loggerFactory.CreateLogger()), GitRepoType.AzureDevOps => new AzureDevOpsClient( diff --git a/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs b/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs index dd33ea0c7c..41c223a5bf 100644 --- a/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs +++ b/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs @@ -1,6 +1,13 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Net; +using System.Text.RegularExpressions; +using System.Threading.Tasks; using FluentAssertions; using Maestro.Common; using Microsoft.DotNet.DarcLib.Helpers; @@ -13,12 +20,6 @@ using Moq; using NUnit.Framework; using Octokit; -using System; -using System.Collections.Concurrent; -using System.Collections.Generic; -using System.Linq; -using System.Net; -using System.Threading.Tasks; namespace Microsoft.DotNet.DarcLib.Tests; @@ -136,6 +137,15 @@ public IReadOnlyDictionary Headers internal class TestGitHubClient : GitHubClient { private IGitHubClient _client; + private Dictionary, List> _reviewData = []; + private static readonly Regex ReviewsUriPattern = + new(@"^/?repos/(?[^/]+)/(?[^/]+)/pulls/(?\d+)/reviews/?$"); + + public void SetReviewData(Dictionary, List> data) + { + _reviewData = data; + } + public void SetGitHubClientObject(IGitHubClient value) { _client = value; @@ -145,9 +155,31 @@ public void SetGitHubClientObject(IGitHubClient value) public override IGitHubClient GetClient(string repoUri) => _client; public TestGitHubClient(string gitExecutable, string accessToken, ILogger logger, string temporaryRepositoryPath, IMemoryCache cache) - : base(new ResolvedTokenProvider(accessToken), new ProcessManager(logger, gitExecutable), logger, temporaryRepositoryPath, cache) + : base(new ResolvedTokenProvider(accessToken), new ProcessManager(logger, gitExecutable), temporaryRepositoryPath, cache, new NoOpRedisClient(), logger) { } + protected override async Task RequestResourceUsingEtagsAsync( + string resourceKey, + Uri resourceUri, + IGitHubClient client, + Func resourceConverter) + { + await Task.FromResult(null); + if (typeof(K) == typeof(List)) + { + var match = ReviewsUriPattern.Match(resourceUri.ToString()); + + (string owner, string repo, int id) = (match.Groups["owner"].Value, match.Groups["repo"].Value, int.Parse(match.Groups["id"].Value)); + + var key = Tuple.Create(owner, repo, id); + if (_reviewData.TryGetValue(key, out var reviews)) + { + return resourceConverter((K)(object)reviews); + } + return resourceConverter((K)(object)new List()); + } + throw new NotImplementedException($"The test client has no implementation for the requested resources of type {typeof(K)}"); + } } #endregion @@ -197,7 +229,7 @@ public void GitHubClientTests_SetUp() public async Task TreeItemCacheTest(bool enableCache) { SimpleCache cache = enableCache ? new SimpleCache() : null; - var client = new Mock(null, null, NullLogger.Instance, null, cache); + var client = new Mock(null, null, null, cache, new NoOpRedisClient(), NullLogger.Instance); List<(string, string, TreeItem)> treeItemsToGet = [ @@ -297,7 +329,7 @@ public async Task TreeItemCacheTest(bool enableCache) [Test] public async Task GetGitTreeItemAbuseExceptionRetryTest() { - var client = new Mock(null, null, NullLogger.Instance, null, new SimpleCache()); + var client = new Mock(null, null, null, new SimpleCache(), new NoOpRedisClient(), NullLogger.Instance); var blob = new Blob("foo", "fakeContent", EncodingType.Utf8, "somesha", 10); var treeItem = new TreeItem("fakePath", "fakeMode", TreeType.Blob, 10, "1", "https://url"); @@ -323,7 +355,7 @@ public async Task GetGitTreeItemAbuseExceptionRetryTest() [Test] public async Task GetGitTreeItemAbuseExceptionRetryWithRateLimitTest() { - var client = new Mock(null, null, NullLogger.Instance, null, new SimpleCache()); + var client = new Mock(null, null, null, new SimpleCache(), new NoOpRedisClient(), NullLogger.Instance); var blob = new Blob("foo", "fakeContent", EncodingType.Utf8, "somesha", 10); var treeItem = new TreeItem("fakePath", "fakeMode", TreeType.Blob, 10, "1", "https://url"); @@ -392,18 +424,7 @@ private async Task> GetLatestReviewsForPullRequestWrapperAsync(Dic { List fakeReviews = []; - // Use Moq to put the return value - OctoKitPullRequestReviewsClient.Setup(x => x.GetAll(It.IsAny(), It.IsAny(), It.IsAny())) - .Callback((string x, string y, int z) => - { - var theKey = new Tuple(x, y, z); - if (data.ContainsKey(theKey)) - { - fakeReviews.AddRange(data[theKey]); - } - - }) - .ReturnsAsync(fakeReviews); + _gitHubClientForTest.SetReviewData(data); return await _gitHubClientForTest.GetLatestPullRequestReviewsAsync(pullRequestUrl); } From 5d52f257e1f20660a643661e6ff08973c9a5e416 Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Tue, 22 Jul 2025 17:23:35 +0200 Subject: [PATCH 06/10] [REFACTORING] Remove unused class --- .../Models/GitHub/GitHubPullRequest.cs | 23 ------------------- 1 file changed, 23 deletions(-) delete mode 100644 src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GitHubPullRequest.cs diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GitHubPullRequest.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GitHubPullRequest.cs deleted file mode 100644 index 15abd697c3..0000000000 --- a/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GitHubPullRequest.cs +++ /dev/null @@ -1,23 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -namespace Microsoft.DotNet.DarcLib.Models.GitHub; - -public class GitHubPullRequest -{ - public GitHubPullRequest(string title, string body, string head, string baseBranch) - { - Title = title; - Body = body; - Head = head; - Base = baseBranch; - } - - public string Title { get; set; } - - public string Body { get; set; } - - public string Head { get; set; } - - public string Base { get; set; } -} From 248eb265bada42330a36fe3ae71cf1bbc609e4e2 Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Tue, 22 Jul 2025 17:26:38 +0200 Subject: [PATCH 07/10] Move GH resources to GH namespace + rename class --- src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs | 2 +- .../{Models => Helpers}/GithubResourceConverters.cs | 9 +++++---- .../GithubPullRequestReviews.cs} | 3 ++- .../DarcLib/Models/{ => GitHub}/GithubReview.cs | 2 +- .../DarcLib/Models/{ => GitHub}/IGithubEtagResource.cs | 2 +- src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs | 1 + 6 files changed, 11 insertions(+), 8 deletions(-) rename src/Microsoft.DotNet.Darc/DarcLib/{Models => Helpers}/GithubResourceConverters.cs (84%) rename src/Microsoft.DotNet.Darc/DarcLib/Models/{PullRequestReviews.cs => GitHub/GithubPullRequestReviews.cs} (75%) rename src/Microsoft.DotNet.Darc/DarcLib/Models/{ => GitHub}/GithubReview.cs (92%) rename src/Microsoft.DotNet.Darc/DarcLib/Models/{ => GitHub}/IGithubEtagResource.cs (89%) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs index 614141571e..ce0e7ec2f6 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs @@ -924,7 +924,7 @@ public async Task> GetLatestPullRequestReviewsAsync(string pullReq IGitHubClient client = GetClient(owner, repo); var pullRequestReviewsUri = ApiUrls.PullRequestReviews(owner, repo, id); - var pullRequestReviews = await RequestResourceUsingEtagsAsync>( + var pullRequestReviews = await RequestResourceUsingEtagsAsync>( pullRequestUrl, pullRequestReviewsUri, client, diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/GithubResourceConverters.cs b/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs similarity index 84% rename from src/Microsoft.DotNet.Darc/DarcLib/Models/GithubResourceConverters.cs rename to src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs index 2bbfc328a5..dd2ab31655 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Models/GithubResourceConverters.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs @@ -5,16 +5,17 @@ using System.Collections.Generic; using System.Linq; using Microsoft.DotNet.DarcLib.Models; +using Microsoft.DotNet.DarcLib.Models.GitHub; using Octokit; -namespace Microsoft.DotNet.DarcLib; +namespace Microsoft.DotNet.DarcLib.Helpers; internal class GithubResourceConverters { internal static Models.PullRequest ConvertPullRequest(Octokit.PullRequest pr) { var status = pr.State == ItemState.Closed ? - (pr.Merged == true ? PrStatus.Merged : PrStatus.Closed) : + pr.Merged == true ? PrStatus.Merged : PrStatus.Closed : PrStatus.Open; return new() @@ -29,7 +30,7 @@ internal static Models.PullRequest ConvertPullRequest(Octokit.PullRequest pr) }; } - internal static PullRequestReviews ConvertPullRequestReviews(IEnumerable pullRequestReviews) + internal static GithubPullRequestReviews ConvertPullRequestReviews(IEnumerable pullRequestReviews) { IEnumerable reviews = pullRequestReviews .Select(r => new GithubReview( @@ -39,7 +40,7 @@ internal static PullRequestReviews ConvertPullRequestReviews(IEnumerable Reviews { get; set; } public string Etag { get; set; } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/GithubReview.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubReview.cs similarity index 92% rename from src/Microsoft.DotNet.Darc/DarcLib/Models/GithubReview.cs rename to src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubReview.cs index 153c036428..95c257ab3e 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Models/GithubReview.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubReview.cs @@ -3,7 +3,7 @@ using System; -namespace Microsoft.DotNet.DarcLib.Models; +namespace Microsoft.DotNet.DarcLib.Models.GitHub; public class GithubReview : Review { diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/IGithubEtagResource.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/IGithubEtagResource.cs similarity index 89% rename from src/Microsoft.DotNet.Darc/DarcLib/Models/IGithubEtagResource.cs rename to src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/IGithubEtagResource.cs index e307bf86ad..22bd4a1129 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Models/IGithubEtagResource.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/IGithubEtagResource.cs @@ -1,7 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -namespace Microsoft.DotNet.DarcLib.Models; +namespace Microsoft.DotNet.DarcLib.Models.GitHub; /// /// Represents resources for which we use Etag caching via the Github API. diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs index 9348ac76dd..a78f8a5aee 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using Microsoft.DotNet.DarcLib.Models.GitHub; namespace Microsoft.DotNet.DarcLib.Models; public class PullRequest : IGithubEtagResource From ea70c12fb8d3d4a19e0635e49f298d9717b87ac4 Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Tue, 22 Jul 2025 17:29:31 +0200 Subject: [PATCH 08/10] enable nullable + remove newlines --- .../DarcLib/Helpers/GithubResourceConverters.cs | 4 +--- test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs b/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs index dd2ab31655..b786ebf098 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs @@ -8,6 +8,7 @@ using Microsoft.DotNet.DarcLib.Models.GitHub; using Octokit; +#nullable enable namespace Microsoft.DotNet.DarcLib.Helpers; internal class GithubResourceConverters @@ -17,7 +18,6 @@ internal static Models.PullRequest ConvertPullRequest(Octokit.PullRequest pr) var status = pr.State == ItemState.Closed ? pr.Merged == true ? PrStatus.Merged : PrStatus.Closed : PrStatus.Open; - return new() { Title = pr.Title, @@ -46,7 +46,6 @@ internal static GithubPullRequestReviews ConvertPullRequestReviews(IEnumerable

throw new NotImplementedException($"Unexpected pull request review state {state}"), }; } - } diff --git a/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs b/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs index 41c223a5bf..121f9f0dec 100644 --- a/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs +++ b/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs @@ -139,7 +139,7 @@ internal class TestGitHubClient : GitHubClient private IGitHubClient _client; private Dictionary, List> _reviewData = []; private static readonly Regex ReviewsUriPattern = - new(@"^/?repos/(?[^/]+)/(?[^/]+)/pulls/(?\d+)/reviews/?$"); + new(@"^/?repos/(?[^/]+)/(?[^/]+)/pulls/(?\d+)/reviews/?$"); public void SetReviewData(Dictionary, List> data) { From 34767df3b3d920506851d83e77e8d141fe674720 Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Thu, 24 Jul 2025 17:15:47 +0200 Subject: [PATCH 09/10] CR changes --- .../DarcLib/Helpers/GithubResourceConverters.cs | 4 ++-- .../DarcLib/Models/GitHub/GithubPullRequestReviews.cs | 2 +- test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs | 5 ++--- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs b/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs index b786ebf098..c895fd8f59 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs @@ -32,13 +32,13 @@ internal static Models.PullRequest ConvertPullRequest(Octokit.PullRequest pr) internal static GithubPullRequestReviews ConvertPullRequestReviews(IEnumerable pullRequestReviews) { - IEnumerable reviews = pullRequestReviews + var reviews = pullRequestReviews .Select(r => new GithubReview( TranslateReviewState(r.State.Value), r.PullRequestUrl, r.User.Login, r.SubmittedAt)) - .ToList(); + .ToArray(); return new GithubPullRequestReviews { diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubPullRequestReviews.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubPullRequestReviews.cs index 13917c516a..a8775978ff 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubPullRequestReviews.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubPullRequestReviews.cs @@ -7,7 +7,7 @@ namespace Microsoft.DotNet.DarcLib.Models; public class GithubPullRequestReviews : IGithubEtagResource { - public IEnumerable Reviews { get; set; } + public IReadOnlyCollection Reviews { get; set; } public string Etag { get; set; } } diff --git a/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs b/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs index 121f9f0dec..0deffa13ca 100644 --- a/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs +++ b/test/Microsoft.DotNet.DarcLib.Tests/GitHubClientTests.cs @@ -164,7 +164,6 @@ protected override async Task RequestResourceUsingEtagsAsync( IGitHubClient client, Func resourceConverter) { - await Task.FromResult(null); if (typeof(K) == typeof(List)) { var match = ReviewsUriPattern.Match(resourceUri.ToString()); @@ -174,9 +173,9 @@ protected override async Task RequestResourceUsingEtagsAsync( var key = Tuple.Create(owner, repo, id); if (_reviewData.TryGetValue(key, out var reviews)) { - return resourceConverter((K)(object)reviews); + return await Task.FromResult(resourceConverter((K)(object)reviews)); } - return resourceConverter((K)(object)new List()); + return await Task.FromResult(resourceConverter((K)(object)new List())); } throw new NotImplementedException($"The test client has no implementation for the requested resources of type {typeof(K)}"); } From 6964c82c83f64a581f30db5fb4c881198a7cbfb9 Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Fri, 25 Jul 2025 03:47:41 +0200 Subject: [PATCH 10/10] modify cache key --- src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs index ce0e7ec2f6..3d52fe9fca 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs @@ -307,9 +307,10 @@ public async Task> SearchPullRequestsAsync( IGitHubClient client = GetClient(owner, repo); var resourceUri = ApiUrls.PullRequest(owner, repo, id); + string cacheKey = $"{owner}_{repo}_{id}"; Models.PullRequest result = await RequestResourceUsingEtagsAsync( - pullRequestUrl, + cacheKey, resourceUri, client, GithubResourceConverters.ConvertPullRequest); @@ -923,9 +924,10 @@ public async Task> GetLatestPullRequestReviewsAsync(string pullReq IGitHubClient client = GetClient(owner, repo); var pullRequestReviewsUri = ApiUrls.PullRequestReviews(owner, repo, id); + string cacheKey = $"{owner}_{repo}_id"; var pullRequestReviews = await RequestResourceUsingEtagsAsync>( - pullRequestUrl, + cacheKey, pullRequestReviewsUri, client, GithubResourceConverters.ConvertPullRequestReviews);