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/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/GitHubClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs index 3a00d8b86e..3d52fe9fca 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; @@ -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+)$"); @@ -48,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() { @@ -61,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; @@ -82,6 +81,7 @@ public GitHubClient( NullValueHandling = NullValueHandling.Ignore }; _gitRefCommitCache = []; + _cache = redisClient; } public bool AllowRetries { get; set; } = true; @@ -300,31 +300,22 @@ 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); - 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 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); + string cacheKey = $"{owner}_{repo}_{id}"; + + Models.PullRequest result = await RequestResourceUsingEtagsAsync( + cacheKey, + resourceUri, + client, + GithubResourceConverters.ConvertPullRequest); + + return result; } /// @@ -332,7 +323,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 +350,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); @@ -407,7 +398,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 { @@ -429,34 +420,15 @@ 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); } } } - /// - /// 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 @@ -472,26 +444,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.", @@ -930,13 +901,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), ]; } @@ -952,38 +922,23 @@ 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); + string cacheKey = $"{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(); + var pullRequestReviews = await RequestResourceUsingEtagsAsync>( + cacheKey, + pullRequestReviewsUri, + client, + GithubResourceConverters.ConvertPullRequestReviews); - // 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; + 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(); - return newestActionableReviews.Select(review => - new Review(TranslateReviewState(review.State.Value), pullRequestUrl)).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) @@ -1232,7 +1187,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); } @@ -1345,4 +1300,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/Helpers/GithubResourceConverters.cs b/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs new file mode 100644 index 0000000000..c895fd8f59 --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/Helpers/GithubResourceConverters.cs @@ -0,0 +1,63 @@ +// 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 Microsoft.DotNet.DarcLib.Models.GitHub; +using Octokit; + +#nullable enable +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 : + 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 GithubPullRequestReviews ConvertPullRequestReviews(IEnumerable pullRequestReviews) + { + var reviews = pullRequestReviews + .Select(r => new GithubReview( + TranslateReviewState(r.State.Value), + r.PullRequestUrl, + r.User.Login, + r.SubmittedAt)) + .ToArray(); + + return new GithubPullRequestReviews + { + 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/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/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; } -} diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubPullRequestReviews.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubPullRequestReviews.cs new file mode 100644 index 0000000000..a8775978ff --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubPullRequestReviews.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. + +using System.Collections.Generic; +using Microsoft.DotNet.DarcLib.Models.GitHub; + +namespace Microsoft.DotNet.DarcLib.Models; +public class GithubPullRequestReviews : IGithubEtagResource +{ + public IReadOnlyCollection Reviews { get; set; } + public string Etag { get; set; } + +} diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubReview.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/GithubReview.cs new file mode 100644 index 0000000000..95c257ab3e --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/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.GitHub; + +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/GitHub/IGithubEtagResource.cs b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/IGithubEtagResource.cs new file mode 100644 index 0000000000..22bd4a1129 --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/GitHub/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.GitHub; + +/// +/// 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 new file mode 100644 index 0000000000..a78f8a5aee --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/Models/PullRequest.cs @@ -0,0 +1,19 @@ +// 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 Microsoft.DotNet.DarcLib.Models.GitHub; + +namespace Microsoft.DotNet.DarcLib.Models; +public class PullRequest : IGithubEtagResource +{ + 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 HeadBranchCommitSha { get; set; } + public string Etag { 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/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..0deffa13ca 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,30 @@ 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) + { + 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 await Task.FromResult(resourceConverter((K)(object)reviews)); + } + 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)}"); + } } #endregion @@ -197,7 +228,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 +328,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 +354,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 +423,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); } 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.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 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);