From 001dc4ccb56658c5083ebb51e81480bd36e3d58e Mon Sep 17 00:00:00 2001 From: dkurepa Date: Wed, 22 Jan 2025 16:00:53 +0100 Subject: [PATCH 01/24] writing the new conflict test --- .../ScenarioTests/ScenarioTests_CodeFlow.cs | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs index 8c6f3509b3..ae82f88d60 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs @@ -1,6 +1,9 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using LibGit2Sharp; +using Microsoft.DotNet.DarcLib; +using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.DotNet.ProductConstructionService.Client.Models; using NUnit.Framework; @@ -153,4 +156,103 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, testRepoFolder, async ( } }); } + + [Test] + public async Task Conflict() + { + var channelName = GetTestChannelName(); + var branchName = GetTestBranchName(); + var productRepo = GetGitHubRepoUrl(TestRepository.TestRepo1Name); + var targetBranchName = GetTestBranchName(); + + await using AsyncDisposableValue testChannel = await CreateTestChannelAsync(channelName); + + await using AsyncDisposableValue subscriptionId = await CreateForwardFlowSubscriptionAsync( + channelName, + TestRepository.TestRepo1Name, + TestRepository.VmrTestRepoName, + targetBranchName, + UpdateFrequency.None.ToString(), + TestParameters.GitHubTestOrg, + targetDirectory: TestRepository.TestRepo1Name); + + TemporaryDirectory vmrDirectory = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); + TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); + var newFilePath = Path.Combine(reposFolder.Directory, TestFileName); + var newFileInVmrPath = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFileName); + + await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => + { + using (ChangeDirectory(reposFolder.Directory)) + { + await using (await CheckoutBranchAsync(branchName)) + { + // Make a change in a product repo + TestContext.WriteLine("Making code changes to the repo"); + await File.WriteAllTextAsync(newFilePath, TestFilesContent[TestFileName]); + + await GitAddAllAsync(); + await GitCommitAsync("Add new file"); + + // Push it to github + await using (await PushGitBranchAsync("origin", branchName)) + { + var repoSha = (await GitGetCurrentSha()).TrimEnd(); + + // Create a new build from the commit and add it to a channel + Build build = await CreateBuildAsync( + GetGitHubRepoUrl(TestRepository.TestRepo1Name), + branchName, + repoSha, + "1", + []); + + TestContext.WriteLine("Adding build to channel"); + await AddBuildToChannelAsync(build.Id, channelName); + + TestContext.WriteLine("Triggering the subscription"); + await TriggerSubscriptionAsync(subscriptionId.Value); + + TestContext.WriteLine("Waiting for the PR to show up"); + Octokit.PullRequest pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); + + // Now make a change directly in the PR + using (ChangeDirectory(vmrDirectory.Directory)) + { + await CheckoutRemoteRefAsync(pr.Head.Ref); + File.WriteAllText(newFileInVmrPath, "file edited in PR"); + + await GitAddAllAsync(); + await GitCommitAsync("Edit file in PR"); + await RunGitAsync("push", "-u", "origin", pr.Head.Ref); + } + + // Make a change in a product repo again, it should cause a conflict + TestContext.WriteLine("Making code changes to the repo that should cause a conflict in the service"); + await File.WriteAllTextAsync(newFilePath, "conflicting changes"); + + await GitAddAllAsync(); + await GitCommitAsync("Add conflicting changes"); + await RunGitAsync("push"); + + TestContext.WriteLine("Creating a build from the new commit"); + build = await CreateBuildAsync( + GetGitHubRepoUrl(TestRepository.TestRepo1Name), + branchName, + (await GitGetCurrentSha()).TrimEnd(), + "2", + []); + + TestContext.WriteLine("Adding build to channel"); + await AddBuildToChannelAsync(build.Id, channelName); + + TestContext.WriteLine("Triggering the subscription"); + await TriggerSubscriptionAsync(subscriptionId.Value); + + TestContext.WriteLine("Waiting for conflict comment to show up on the PR"); + } + } + } + }); + } } From 5ba53fea9519648d3875832fb0dd62b17e3f17f6 Mon Sep 17 00:00:00 2001 From: dkurepa Date: Mon, 27 Jan 2025 13:49:14 +0100 Subject: [PATCH 02/24] Conflict stuff --- .../DarcLib/AzureDevOpsClient.cs | 20 +++++ .../DarcLib/GitHubClient.cs | 6 ++ src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs | 8 ++ .../DarcLib/IRemoteGitRepo.cs | 10 ++- src/Microsoft.DotNet.Darc/DarcLib/Remote.cs | 6 ++ .../DarcLib/VirtualMonoRepo/Exceptions.cs | 40 +++++++++- .../DarcLib/VirtualMonoRepo/VmrBackflower.cs | 2 +- .../VirtualMonoRepo/VmrForwardFlower.cs | 2 +- .../RedisCache.cs | 6 +- .../InProgressPullRequest.cs | 9 +++ .../PullRequestUpdater.cs | 77 +++++++++++++++++-- .../MockRedisCache.cs | 4 +- .../CodeFlowScenarioTestBase.cs | 32 ++++++++ .../ScenarioTestBase.cs | 33 ++++---- .../ScenarioTests/ScenarioTests_CodeFlow.cs | 32 +++++++- .../FakeRedisCache.cs | 2 +- 16 files changed, 255 insertions(+), 34 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs index dcf516d6a6..8b5bf96cfd 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs @@ -1554,4 +1554,24 @@ private static string TruncateDescriptionIfNeeded(string str) } return str; } + + public async Task CommentPullRequestAsync(string pullRequestUri, string comment) + { + (string accountName, string projectName, string repoName, int id) = ParsePullRequestUri(pullRequestUri); + + using VssConnection connection = CreateVssConnection(accountName); + using GitHttpClient client = await connection.GetClientAsync(); + + var prComment = new Comment() + { + CommentType = CommentType.Text, + Content = $"{comment}{CommentMarker}" + }; + + var newCommentThread = new GitPullRequestCommentThread() + { + Comments = [prComment] + }; + await client.CreateThreadAsync(newCommentThread, repoName, id); + } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs index 1e73333b04..0c70d28e26 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs @@ -1309,4 +1309,10 @@ public async Task DeletePullRequestBranchAsync(string pullRequestUri) (string owner, string repo, int id) prInfo = ParsePullRequestUri(pullRequestUri); await DeleteBranchAsync(prInfo.owner, prInfo.repo, pr.HeadBranch); } + + public async Task CommentPullRequestAsync(string pullRequestUri, string comment) + { + (string owner, string repo, int id) = ParsePullRequestUri(pullRequestUri); + await GetClient(owner, repo).Issue.Comment.Create(owner, repo, id, comment); + } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs b/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs index e4253d314b..0e29b3563f 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs @@ -188,5 +188,13 @@ Task> CommitUpdatesAsync( /// Location for the .git directory, or null for default Task CloneAsync(string repoUri, string commit, string targetDirectory, bool checkoutSubmodules, string gitDirectory); + /// + /// Comment on an existing pull request + /// + /// Uri of the pull request + /// Comment message + /// + Task CommentPullRequestAsync(string pullRequestUri, string comment); + #endregion } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs b/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs index d61fb75113..0d5eac3625 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs @@ -108,7 +108,7 @@ Task> SearchPullRequestsAsync( /// or remove each merge policy check that isn't in evaluations /// /// Url of pull request - /// List of merge policies + /// List of merge policies Task CreateOrUpdatePullRequestMergeStatusInfoAsync(string pullRequestUrl, IReadOnlyList evaluations); /// @@ -168,6 +168,14 @@ Task> SearchPullRequestsAsync( /// Repository to find the branch in /// Branch to find Task DoesBranchExistAsync(string repoUri, string branch); + + /// + /// Comment on an existing pull request + /// + /// Uri of the pull request + /// Comment message + /// + Task CommentPullRequestAsync(string pullRequestUri, string comment); } #nullable disable diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs index 8501a546d6..0d968a4687 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs @@ -405,4 +405,10 @@ public async Task> GetCommonScriptFilesAsync(string repoUri, strin return files; } + + public async Task CommentPullRequestAsync(string pullRequestUri, string comment) + { + CheckForValidGitClient(); + await _remoteGitClient.CommentPullRequestAsync(pullRequestUri, comment); + } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs index 06bfb74d45..7efcff78e4 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs @@ -2,7 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.IO; +using System.Linq; +using System.Text.RegularExpressions; using Microsoft.DotNet.DarcLib.Helpers; #nullable enable @@ -24,8 +27,41 @@ private static string GetExceptionMessage(VmrIngestionPatch patch, ProcessExecut + result; } -public class ConflictInPrBranchException(VmrIngestionPatch patch, string targetBranch) +public class ConflictInPrBranchException(ProcessExecutionResult conflictResult, string targetBranch) : Exception($"Failed to flow changes due to conflicts in the target branch ({targetBranch})") { - public VmrIngestionPatch Patch { get; } = patch; + public List FilesInConflict { get; } = ParseResult(conflictResult); + + private const string AlreadyExistsRegex = "patch failed: (.+): already exist in index"; + private const string PatchFailedRegex = "error: patch failed: (.*):"; + private const string PatchDoesNotApplyRegex = "error: (.+): patch does not apply"; + private const string FileDoesNotExistRegex = "error: (.+): does not exist in index"; + + private static string[] ConflictRegex = + [ + AlreadyExistsRegex, + PatchFailedRegex, + PatchDoesNotApplyRegex, + FileDoesNotExistRegex + ]; + + private static List ParseResult(ProcessExecutionResult result) + { + List filesInConflict = new(); + var errors = result.StandardError.Split(Environment.NewLine); + foreach (var error in errors) + { + foreach (var regex in ConflictRegex) + { + var match = Regex.Match(error, regex); + if (match.Success) + { + filesInConflict.Add(match.Groups[1].Value); + break; + } + } + } + // Convert VMR paths to normal repo paths, for example src/repo/file.cs -> file.cs + return filesInConflict.Select(file => file.Split('/', 3)[2]).Distinct().ToList(); + } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs index 223d4d794e..80eb3c644d 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs @@ -361,7 +361,7 @@ protected override async Task SameDirectionFlowAsync( if (!rebaseConflicts) { _logger.LogInformation("Failed to update a PR branch because of a conflict. Stopping the flow.."); - throw new ConflictInPrBranchException(e.Patch, targetBranch); + throw new ConflictInPrBranchException(e.Result, targetBranch); } // Otherwise, we have a conflicting change in the last backflow PR (before merging) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs index 605d3fea7e..e5cd4075e5 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs @@ -293,7 +293,7 @@ protected override async Task SameDirectionFlowAsync( if (!rebaseConflicts) { _logger.LogInformation("Failed to update a PR branch because of a conflict. Stopping the flow.."); - throw new ConflictInPrBranchException(e.Patch, targetBranch); + throw new ConflictInPrBranchException(e.Result, targetBranch); } // This happens when a conflicting change was made in the last backflow PR (before merging) diff --git a/src/ProductConstructionService/ProductConstructionService.Common/RedisCache.cs b/src/ProductConstructionService/ProductConstructionService.Common/RedisCache.cs index fe7c17e005..3ad02e7129 100644 --- a/src/ProductConstructionService/ProductConstructionService.Common/RedisCache.cs +++ b/src/ProductConstructionService/ProductConstructionService.Common/RedisCache.cs @@ -13,7 +13,7 @@ public interface IRedisCache Task GetAsync(string key); Task GetAsync(); Task SetAsync(string value, TimeSpan? expiration = null); - Task TryDeleteAsync(); + Task TryDeleteAsync(); Task TryGetAsync(); IAsyncEnumerable GetKeysAsync(string pattern); } @@ -44,9 +44,9 @@ public async Task SetAsync(string value, TimeSpan? expiration = null) return value.HasValue ? value.ToString() : null; } - public async Task TryDeleteAsync() + public async Task TryDeleteAsync() { - await Cache.KeyDeleteAsync(_stateKey); + return await Cache.KeyDeleteAsync(_stateKey); } public async Task GetAsync() diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs index 67950cdb91..c0a4116df9 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs @@ -50,4 +50,13 @@ public class InProgressPullRequest : DependencyFlowWorkItem, IPullRequest [DataMember] public DateTime? NextCheck { get; set; } + + [DataMember] + public InProgressPullRequestState State { get; set; } +} + +public enum InProgressPullRequestState +{ + Open, + Conflict } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 3f124e5c4c..47dd76d54b 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Text; using Maestro.Data.Models; using Maestro.MergePolicies; using Maestro.MergePolicyEvaluation; @@ -26,7 +27,7 @@ namespace ProductConstructionService.DependencyFlow; internal abstract class PullRequestUpdater : IPullRequestUpdater { #if DEBUG - private static readonly TimeSpan DefaultReminderDelay = TimeSpan.FromMinutes(3); + private static readonly TimeSpan DefaultReminderDelay = TimeSpan.FromMinutes(1); #else private static readonly TimeSpan DefaultReminderDelay = TimeSpan.FromMinutes(5); #endif @@ -138,10 +139,10 @@ public async Task UpdateAssetsAsync( public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem update) { _logger.LogInformation("Processing pending updates for subscription {subscriptionId}", update.SubscriptionId); + bool isCodeFlow = update.SubscriptionType == SubscriptionType.DependenciesAndSources; // Check if we track an on-going PR already InProgressPullRequest? pr = await _pullRequestState.TryGetStateAsync(); - bool isCodeFlow = update.SubscriptionType == SubscriptionType.DependenciesAndSources; if (pr == null) { @@ -149,12 +150,21 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up } else { + // Check if the PR has a conflict + if (pr.State == InProgressPullRequestState.Conflict) + { + // Set a reminder to check if the PR has been merged + // TODO we should add a commit of the PR branch to the cache, and then check if it got updated + await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow); + return false; + } + var prStatus = await GetPullRequestStatusAsync(pr, isCodeFlow); switch (prStatus) { case PullRequestStatus.Completed: case PullRequestStatus.Invalid: - // If the PR is completed, we will open a new one + // If the PR is completed, we will open a new one, and we should clean the redis from preventing it pr = null; break; case PullRequestStatus.InProgressCanUpdate: @@ -170,7 +180,9 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up } } - // Code flow updates are handled separetely + // check f the PR is blocked. We can only do this after checking if it's completed, otherwise, we'd never clear redis + + // Code flow updates are handled separately if (isCodeFlow) { return await ProcessCodeFlowUpdateAsync(update, pr); @@ -798,9 +810,14 @@ private async Task SetPullRequestCheckReminder(InProgressPullRequest prState, bo private async Task ClearAllStateAsync(bool isCodeFlow) { - await _pullRequestState.TryDeleteAsync(); + var pr = await _pullRequestState.TryDeleteAsync(); await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow); - await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow); + // If the cache has been cleared, that means there was an update waiting for the PR to be merged + // In that case, we don't want to remove it, since we want to process it + if (pr?.State != InProgressPullRequestState.Conflict) + { + await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow); + } } /// @@ -901,6 +918,40 @@ await CreateCodeFlowPullRequestAsync( { await UpdateAssetsAndSources(update, pr); } + catch (ConflictInPrBranchException conflictException) + { + // The PR we're trying to update has a conflict with the source repo. We will it as blocked, not allowing any updates from this + // subscription till it's merged. We'll set a reminder to check if PR has been merged. When it is, we'll unblock updates from the repo + StringBuilder sb = new(); + sb.AppendLine($"There was a conflict in the PR branch when flowing source from {update.SourceRepo}/tree/{update.SourceSha}"); + sb.AppendLine("Conflicting files:"); + foreach (var file in conflictException.FilesInConflict) + { + sb.AppendLine($" - {file}"); + } + sb.AppendLine(); + sb.AppendLine("Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged"); + + (var targetRepo, var _) = await GetTargetAsync(); + var remote = await _remoteFactory.CreateRemoteAsync(targetRepo); + + await remote.CommentPullRequestAsync(pr.Url, sb.ToString()); + + await _pullRequestCheckReminders.SetReminderAsync( + new PullRequestCheck() + { + UpdaterId = Id.ToString(), + Url = pr.Url, + IsCodeFlow = isCodeFlow + }, + DefaultReminderDelay, + isCodeFlow: true); + + await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow: true); + await UpdateInProgressPullRequestState(InProgressPullRequestState.Conflict); + + return false; + } catch (Exception e) { // TODO https://github.com/dotnet/arcade-services/issues/4198: Notify us about these kind of failures @@ -1167,5 +1218,19 @@ await AddDependencyFlowEventsAsync( } } + private async Task UpdateInProgressPullRequestState(InProgressPullRequestState newState) + { + var inProgressPr = await _pullRequestState.TryGetStateAsync(); + + if (inProgressPr == null) + { + return; + } + + inProgressPr.State = newState; + + await _pullRequestState.SetAsync(inProgressPr); + } + #endregion } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCache.cs b/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCache.cs index d91fe3fe34..2997b6af53 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCache.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCache.cs @@ -36,11 +36,11 @@ public Task SetAsync(string value, TimeSpan? expiration = null) return Task.CompletedTask; } - public Task TryDeleteAsync() + public Task TryDeleteAsync() { _data.Remove(_key); - return Task.CompletedTask; + return Task.FromResult(true); } public Task TryGetAsync() => throw new NotImplementedException(); diff --git a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs index a8ef31add7..7bbcaf2485 100644 --- a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs @@ -144,4 +144,36 @@ private static async Task> CreateSourceEnabledSubsc targetIsAzDo, trigger); } + + public async Task WaitForPullRequestComment( + string targetRepo, + string targetBranch, + string partialComment, + int attempts = 40) + { + PullRequest pullRequest = await WaitForPullRequestAsync(targetRepo, targetBranch); + + while (attempts-- > 0) + { + pullRequest = await GitHubApi.PullRequest.Get(TestParameters.GitHubTestOrg, targetRepo, pullRequest.Number); + IReadOnlyList comments = await GitHubApi.Issue.Comment.GetAllForIssue(TestParameters.GitHubTestOrg, targetRepo, pullRequest.Number); + if (comments.Any(comment => comment.Body.Contains(partialComment))) + { + return pullRequest; + } + await Task.Delay(5000); + } + + throw new ScenarioTestException($"Comment containing '{partialComment}' was not found in the pull request."); + } + + public async Task CheckConflictPullRequestComment( + string targetRepo, + string targetBranch, + PullRequest pullRequest, + string expectedComment) + { + IReadOnlyList comments = await GitHubApi.Issue.Comment.GetAllForIssue(TestParameters.GitHubTestOrg, targetRepo, pullRequest.Number); + comments.Should().ContainSingle(comment => comment.Body.Contains(expectedComment)); + } } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs index 34cf5d75a3..2cd72e3a94 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs @@ -82,7 +82,7 @@ public static void ConfigureDarcArgs() throw new ScenarioTestException($"No pull request was created in {targetRepo} targeting {targetBranch}"); } - private async Task WaitForUpdatedPullRequestAsync(string targetRepo, string targetBranch, int attempts = 40) + protected async Task WaitForUpdatedPullRequestAsync(string targetRepo, string targetBranch, int attempts = 40) { Octokit.Repository repo = await GitHubApi.Repository.Get(TestParameters.GitHubTestOrg, targetRepo); Octokit.PullRequest pr = await WaitForPullRequestAsync(targetRepo, targetBranch); @@ -1036,21 +1036,26 @@ protected string GetUniqueAssetName(string packageName) protected static IAsyncDisposable CleanUpPullRequestAfter(string owner, string repo, Octokit.PullRequest pullRequest) => AsyncDisposable.Create(async () => { - try - { - var pullRequestUpdate = new Octokit.PullRequestUpdate - { - State = Octokit.ItemState.Closed - }; + await ClosePullRequest(owner, repo, pullRequest); + }); - await GitHubApi.Repository.PullRequest.Update(owner, repo, pullRequest.Number, pullRequestUpdate); - await GitHubApi.Git.Reference.Delete(owner, repo, $"heads/{pullRequest.Head.Ref}"); - } - catch + protected static async Task ClosePullRequest(string owner, string repo, Octokit.PullRequest pullRequest) + { + try + { + var pullRequestUpdate = new Octokit.PullRequestUpdate { - // Closed already - } - }); + State = Octokit.ItemState.Closed + }; + + await GitHubApi.Repository.PullRequest.Update(owner, repo, pullRequest.Number, pullRequestUpdate); + await GitHubApi.Git.Reference.Delete(owner, repo, $"heads/{pullRequest.Head.Ref}"); + } + catch + { + // Closed already + } + } protected async Task CreateTargetBranchAndExecuteTest(string targetBranchName, TemporaryDirectory targetDirectory, Func test) { diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs index ae82f88d60..41c2ef0e2b 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using LibGit2Sharp; -using Microsoft.DotNet.DarcLib; using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.DotNet.ProductConstructionService.Client.Models; using NUnit.Framework; @@ -28,6 +26,17 @@ internal class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase { TestFileName, "@@ -0,0 +1 @@\n+test\n\\ No newline at end of file" } }; + private const string CommitPlaceholder = ""; + private const string ConflictMessage = $""" + There was a conflict in the PR branch when flowing source from https://github.com/maestro-auth-test/maestro-test1/tree/{CommitPlaceholder} + Conflicting files: + - 1newFile.txt + - newFile.txt + + Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged + + """; + [Test] public async Task Vmr_ForwardFlowTest() { @@ -180,6 +189,8 @@ public async Task Conflict() TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); var newFilePath = Path.Combine(reposFolder.Directory, TestFileName); var newFileInVmrPath = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFileName); + var newFilePath2 = Path.Combine(reposFolder.Directory, "1" + TestFileName); + var newFileInVmrPath2 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, "1" + TestFileName); await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => { @@ -190,6 +201,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () // Make a change in a product repo TestContext.WriteLine("Making code changes to the repo"); await File.WriteAllTextAsync(newFilePath, TestFilesContent[TestFileName]); + await File.WriteAllTextAsync(newFilePath2, "just a second fileasd"); await GitAddAllAsync(); await GitCommitAsync("Add new file"); @@ -221,6 +233,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () { await CheckoutRemoteRefAsync(pr.Head.Ref); File.WriteAllText(newFileInVmrPath, "file edited in PR"); + File.Delete(newFileInVmrPath2); await GitAddAllAsync(); await GitCommitAsync("Edit file in PR"); @@ -230,16 +243,18 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () // Make a change in a product repo again, it should cause a conflict TestContext.WriteLine("Making code changes to the repo that should cause a conflict in the service"); await File.WriteAllTextAsync(newFilePath, "conflicting changes"); + await File.WriteAllTextAsync(newFilePath2, "just a second file"); await GitAddAllAsync(); await GitCommitAsync("Add conflicting changes"); await RunGitAsync("push"); + repoSha = (await GitGetCurrentSha()).TrimEnd(); TestContext.WriteLine("Creating a build from the new commit"); build = await CreateBuildAsync( GetGitHubRepoUrl(TestRepository.TestRepo1Name), branchName, - (await GitGetCurrentSha()).TrimEnd(), + repoSha, "2", []); @@ -250,6 +265,17 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () await TriggerSubscriptionAsync(subscriptionId.Value); TestContext.WriteLine("Waiting for conflict comment to show up on the PR"); + + pr = await WaitForUpdatedPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); + await CheckConflictPullRequestComment( + TestRepository.VmrTestRepoName, + targetBranchName, + pr, + ConflictMessage.Replace(CommitPlaceholder, repoSha)); + + await ClosePullRequest(TestParameters.GitHubTestOrg, TestRepository.VmrTestRepoName, pr); + + pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); } } } diff --git a/test/ProductConstructionService.WorkItem.Tests/FakeRedisCache.cs b/test/ProductConstructionService.WorkItem.Tests/FakeRedisCache.cs index e43da30349..edf44fb995 100644 --- a/test/ProductConstructionService.WorkItem.Tests/FakeRedisCache.cs +++ b/test/ProductConstructionService.WorkItem.Tests/FakeRedisCache.cs @@ -14,7 +14,7 @@ public Task SetAsync(string value, TimeSpan? expiration = null) return Task.CompletedTask; } - public Task TryDeleteAsync() => throw new NotImplementedException(); + public Task TryDeleteAsync() => throw new NotImplementedException(); public Task TryGetAsync() => Task.FromResult(_value); public Task GetAsync() => Task.FromResult(_value); public Task GetAsync(string key) => throw new NotImplementedException(); From 89beccb9ce469e465cf0d7ff56259fe2ce6175e4 Mon Sep 17 00:00:00 2001 From: dkurepa Date: Mon, 27 Jan 2025 15:19:57 +0100 Subject: [PATCH 03/24] More things --- .../DarcLib/VirtualMonoRepo/Exceptions.cs | 2 +- .../ScenarioTestBase.cs | 4 - .../ScenarioTests/ScenarioTests_CodeFlow.cs | 155 +++--------------- .../ScenarioTests_CodeFlowConflicts.cs | 137 ++++++++++++++++ 4 files changed, 158 insertions(+), 140 deletions(-) create mode 100644 test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs index 7efcff78e4..bbfdf10938 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs @@ -37,7 +37,7 @@ public class ConflictInPrBranchException(ProcessExecutionResult conflictResult, private const string PatchDoesNotApplyRegex = "error: (.+): patch does not apply"; private const string FileDoesNotExistRegex = "error: (.+): does not exist in index"; - private static string[] ConflictRegex = + private static readonly string[] ConflictRegex = [ AlreadyExistsRegex, PatchFailedRegex, diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs index 2c64702662..d5d857f521 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs @@ -82,11 +82,7 @@ public static void ConfigureDarcArgs() throw new ScenarioTestException($"No pull request was created in {targetRepo} targeting {targetBranch}"); } -<<<<<<< HEAD - protected async Task WaitForUpdatedPullRequestAsync(string targetRepo, string targetBranch, int attempts = 40) -======= protected async Task WaitForUpdatedPullRequestAsync(string targetRepo, string targetBranch, int attempts = 30) ->>>>>>> source/main { Octokit.Repository repo = await GitHubApi.Repository.Get(TestParameters.GitHubTestOrg, targetRepo); Octokit.PullRequest pr = await WaitForPullRequestAsync(targetRepo, targetBranch); diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs index 0cfbaeb30b..b5d5cfb690 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.DotNet.ProductConstructionService.Client.Models; using NUnit.Framework; @@ -11,21 +10,23 @@ namespace ProductConstructionService.ScenarioTests.ScenarioTests; [TestFixture] [Category("PostDeployment")] [Category("CodeFlow")] -internal class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase +internal partial class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase { - private const string TestFileName = "newFile.txt"; + private const string TestFile1Name = "newFile1.txt"; + private const string TestFile2Name = "newFile2.txt"; private const string DefaultPatch = "@@ -0,0 +1 @@\n+test\n\\ No newline at end of file"; private static readonly Dictionary TestFilesContent = new() { - { TestFileName, "test" } + { TestFile1Name, "test" }, + { TestFile2Name, "test" }, }; private static readonly Dictionary TestFilePatches = new() { - { $"{TestFileName}", DefaultPatch }, - { $"src/{TestRepository.TestRepo1Name}/{TestFileName}", DefaultPatch }, - { $"src/{TestRepository.TestRepo2Name}/{TestFileName}", DefaultPatch }, + { $"{TestFile1Name}", DefaultPatch }, + { $"src/{TestRepository.TestRepo1Name}/{TestFile1Name}", DefaultPatch }, + { $"src/{TestRepository.TestRepo2Name}/{TestFile1Name}", DefaultPatch }, }; private const string CommitPlaceholder = ""; @@ -60,7 +61,7 @@ public async Task Vmr_ForwardFlowTest() TemporaryDirectory vmrDirectory = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); - var newFilePath = Path.Combine(reposFolder.Directory, TestFileName); + var newFilePath = Path.Combine(reposFolder.Directory, TestFile1Name); await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => { @@ -70,7 +71,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () { // Make a change in a product repo TestContext.WriteLine("Making code changes to the repo"); - await File.WriteAllTextAsync(newFilePath, TestFilesContent[TestFileName]); + await File.WriteAllTextAsync(newFilePath, TestFilesContent[TestFile1Name]); await GitAddAllAsync(); await GitCommitAsync("Add new file"); @@ -100,7 +101,7 @@ await CheckForwardFlowGitHubPullRequest( [(TestRepository.TestRepo1Name, repoSha)], TestRepository.VmrTestRepoName, targetBranchName, - [$"src/{TestRepository.TestRepo1Name}/{TestFileName}"], + [$"src/{TestRepository.TestRepo1Name}/{TestFile1Name}"], TestFilePatches); } } @@ -129,7 +130,7 @@ public async Task Vmr_BackwardFlowTest() TemporaryDirectory testRepoFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); - var newFilePath = Path.Combine(reposFolder.Directory, "src", TestRepository.TestRepo1Name, TestFileName); + var newFilePath = Path.Combine(reposFolder.Directory, "src", TestRepository.TestRepo1Name, TestFile1Name); await CreateTargetBranchAndExecuteTest(targetBranchName, testRepoFolder, async () => { @@ -139,7 +140,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, testRepoFolder, async ( { // Make a change in the VMR TestContext.WriteLine("Making code changes in the VMR"); - File.WriteAllText(newFilePath, TestFilesContent[TestFileName]); + File.WriteAllText(newFilePath, TestFilesContent[TestFile1Name]); await GitAddAllAsync(); await GitCommitAsync("Add new file"); @@ -170,7 +171,7 @@ await CheckBackwardFlowGitHubPullRequest( TestRepository.VmrTestRepoName, TestRepository.TestRepo1Name, targetBranchName, - [TestFileName], + [TestFile1Name], TestFilePatches, repoSha, build.Id); @@ -214,8 +215,8 @@ public async Task Vmr_BatchedForwardFlowTest() TemporaryDirectory vmrDirectory = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); TemporaryDirectory repo1 = await CloneRepositoryAsync(TestRepository.TestRepo1Name); TemporaryDirectory repo2 = await CloneRepositoryAsync(TestRepository.TestRepo2Name); - var newFile1Path = Path.Combine(repo1.Directory, TestFileName); - var newFile2Path = Path.Combine(repo2.Directory, TestFileName); + var newFile1Path = Path.Combine(repo1.Directory, TestFile1Name); + var newFile2Path = Path.Combine(repo2.Directory, TestFile1Name); await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => { @@ -224,7 +225,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () { // Make a change in a product repo TestContext.WriteLine("Making code changes to the repo"); - await File.WriteAllTextAsync(newFile1Path, TestFilesContent[TestFileName]); + await File.WriteAllTextAsync(newFile1Path, TestFilesContent[TestFile1Name]); await GitAddAllAsync(); await GitCommitAsync("Add new file"); @@ -238,7 +239,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () { // Make a change in a product repo TestContext.WriteLine("Making code changes to the repo"); - await File.WriteAllTextAsync(newFile2Path, TestFilesContent[TestFileName]); + await File.WriteAllTextAsync(newFile2Path, TestFilesContent[TestFile1Name]); await GitAddAllAsync(); await GitCommitAsync("Add new file"); @@ -281,8 +282,8 @@ await CheckForwardFlowGitHubPullRequest( TestRepository.VmrTestRepoName, targetBranchName, [ - $"src/{TestRepository.TestRepo1Name}/{TestFileName}", - $"src/{TestRepository.TestRepo2Name}/{TestFileName}" + $"src/{TestRepository.TestRepo1Name}/{TestFile1Name}", + $"src/{TestRepository.TestRepo2Name}/{TestFile1Name}" ], TestFilePatches); } @@ -291,120 +292,4 @@ await CheckForwardFlowGitHubPullRequest( } }); } - - [Test] - public async Task Conflict() - { - var channelName = GetTestChannelName(); - var branchName = GetTestBranchName(); - var productRepo = GetGitHubRepoUrl(TestRepository.TestRepo1Name); - var targetBranchName = GetTestBranchName(); - - await using AsyncDisposableValue testChannel = await CreateTestChannelAsync(channelName); - - await using AsyncDisposableValue subscriptionId = await CreateForwardFlowSubscriptionAsync( - channelName, - TestRepository.TestRepo1Name, - TestRepository.VmrTestRepoName, - targetBranchName, - UpdateFrequency.None.ToString(), - TestParameters.GitHubTestOrg, - targetDirectory: TestRepository.TestRepo1Name); - - TemporaryDirectory vmrDirectory = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); - TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); - var newFilePath = Path.Combine(reposFolder.Directory, TestFileName); - var newFileInVmrPath = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFileName); - var newFilePath2 = Path.Combine(reposFolder.Directory, "1" + TestFileName); - var newFileInVmrPath2 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, "1" + TestFileName); - - await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => - { - using (ChangeDirectory(reposFolder.Directory)) - { - await using (await CheckoutBranchAsync(branchName)) - { - // Make a change in a product repo - TestContext.WriteLine("Making code changes to the repo"); - await File.WriteAllTextAsync(newFilePath, TestFilesContent[TestFileName]); - await File.WriteAllTextAsync(newFilePath2, "just a second fileasd"); - - await GitAddAllAsync(); - await GitCommitAsync("Add new file"); - - // Push it to github - await using (await PushGitBranchAsync("origin", branchName)) - { - var repoSha = (await GitGetCurrentSha()).TrimEnd(); - - // Create a new build from the commit and add it to a channel - Build build = await CreateBuildAsync( - GetGitHubRepoUrl(TestRepository.TestRepo1Name), - branchName, - repoSha, - "1", - []); - - TestContext.WriteLine("Adding build to channel"); - await AddBuildToChannelAsync(build.Id, channelName); - - TestContext.WriteLine("Triggering the subscription"); - await TriggerSubscriptionAsync(subscriptionId.Value); - - TestContext.WriteLine("Waiting for the PR to show up"); - Octokit.PullRequest pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); - - // Now make a change directly in the PR - using (ChangeDirectory(vmrDirectory.Directory)) - { - await CheckoutRemoteRefAsync(pr.Head.Ref); - File.WriteAllText(newFileInVmrPath, "file edited in PR"); - File.Delete(newFileInVmrPath2); - - await GitAddAllAsync(); - await GitCommitAsync("Edit file in PR"); - await RunGitAsync("push", "-u", "origin", pr.Head.Ref); - } - - // Make a change in a product repo again, it should cause a conflict - TestContext.WriteLine("Making code changes to the repo that should cause a conflict in the service"); - await File.WriteAllTextAsync(newFilePath, "conflicting changes"); - await File.WriteAllTextAsync(newFilePath2, "just a second file"); - - await GitAddAllAsync(); - await GitCommitAsync("Add conflicting changes"); - await RunGitAsync("push"); - - repoSha = (await GitGetCurrentSha()).TrimEnd(); - TestContext.WriteLine("Creating a build from the new commit"); - build = await CreateBuildAsync( - GetGitHubRepoUrl(TestRepository.TestRepo1Name), - branchName, - repoSha, - "2", - []); - - TestContext.WriteLine("Adding build to channel"); - await AddBuildToChannelAsync(build.Id, channelName); - - TestContext.WriteLine("Triggering the subscription"); - await TriggerSubscriptionAsync(subscriptionId.Value); - - TestContext.WriteLine("Waiting for conflict comment to show up on the PR"); - - pr = await WaitForUpdatedPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); - await CheckConflictPullRequestComment( - TestRepository.VmrTestRepoName, - targetBranchName, - pr, - ConflictMessage.Replace(CommitPlaceholder, repoSha)); - - await ClosePullRequest(TestParameters.GitHubTestOrg, TestRepository.VmrTestRepoName, pr); - - pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); - } - } - } - }); - } } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs new file mode 100644 index 0000000000..77afb34544 --- /dev/null +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs @@ -0,0 +1,137 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.DotNet.DarcLib.VirtualMonoRepo; +using Microsoft.DotNet.ProductConstructionService.Client.Models; +using NUnit.Framework; + +namespace ProductConstructionService.ScenarioTests.ScenarioTests; + +[TestFixture] +[Category("PostDeployment")] +[Category("CodeFlow")] +internal partial class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase +{ + [Test] + public async Task Conflict() + { + var channelName = GetTestChannelName(); + var branchName = GetTestBranchName(); + var productRepo = GetGitHubRepoUrl(TestRepository.TestRepo1Name); + var targetBranchName = GetTestBranchName(); + + await using AsyncDisposableValue testChannel = await CreateTestChannelAsync(channelName); + + await using AsyncDisposableValue subscriptionId = await CreateForwardFlowSubscriptionAsync( + channelName, + TestRepository.TestRepo1Name, + TestRepository.VmrTestRepoName, + targetBranchName, + UpdateFrequency.None.ToString(), + TestParameters.GitHubTestOrg, + targetDirectory: TestRepository.TestRepo1Name); + + TemporaryDirectory vmrDirectory = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); + TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); + var newFilePath1 = Path.Combine(reposFolder.Directory, TestFile1Name); + var newFileInVmrPath1 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFile1Name); + var newFilePath2 = Path.Combine(reposFolder.Directory, TestFile2Name); + var newFileInVmrPath2 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFile2Name); + + await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => + { + using (ChangeDirectory(reposFolder.Directory)) + { + await using (await CheckoutBranchAsync(branchName)) + { + // Make a change in a product repo + TestContext.WriteLine("Making code changes to the repo"); + await File.WriteAllTextAsync(newFilePath1, "not important"); + await File.WriteAllTextAsync(newFilePath2, "not important"); + + await GitAddAllAsync(); + await GitCommitAsync("Add new files"); + + // Push it to github + await using (await PushGitBranchAsync("origin", branchName)) + { + var repoSha = (await GitGetCurrentSha()).TrimEnd(); + + // Create a new build from the commit and add it to a channel + Build build = await CreateBuildAsync( + GetGitHubRepoUrl(TestRepository.TestRepo1Name), + branchName, + repoSha, + "1", + []); + + TestContext.WriteLine("Adding build to channel"); + await AddBuildToChannelAsync(build.Id, channelName); + + TestContext.WriteLine("Triggering the subscription"); + await TriggerSubscriptionAsync(subscriptionId.Value); + + TestContext.WriteLine("Waiting for the PR to show up"); + Octokit.PullRequest pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); + + // Now make a change directly in the PR + using (ChangeDirectory(vmrDirectory.Directory)) + { + await CheckoutRemoteRefAsync(pr.Head.Ref); + File.WriteAllText(newFileInVmrPath1, "file edited in PR"); + File.Delete(newFileInVmrPath2); + + await GitAddAllAsync(); + await GitCommitAsync("Edit files in PR"); + await RunGitAsync("push", "-u", "origin", pr.Head.Ref); + } + + // Make a change in a product repo again, it should cause a conflict + TestContext.WriteLine("Making code changes to the repo that should cause a conflict in the service"); + await File.WriteAllTextAsync(newFilePath1, TestFilesContent[TestFile1Name]); + await File.WriteAllTextAsync(newFilePath2, TestFilesContent[TestFile2Name]); + + await GitAddAllAsync(); + await GitCommitAsync("Add conflicting changes"); + await RunGitAsync("push"); + + repoSha = (await GitGetCurrentSha()).TrimEnd(); + TestContext.WriteLine("Creating a build from the new commit"); + build = await CreateBuildAsync( + GetGitHubRepoUrl(TestRepository.TestRepo1Name), + branchName, + repoSha, + "2", + []); + + TestContext.WriteLine("Adding build to channel"); + await AddBuildToChannelAsync(build.Id, channelName); + + TestContext.WriteLine("Triggering the subscription"); + await TriggerSubscriptionAsync(subscriptionId.Value); + + TestContext.WriteLine("Waiting for conflict comment to show up on the PR"); + pr = await WaitForUpdatedPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); + await CheckConflictPullRequestComment( + TestRepository.VmrTestRepoName, + targetBranchName, + pr, + ConflictMessage.Replace(CommitPlaceholder, repoSha)); + + await ClosePullRequest(TestParameters.GitHubTestOrg, TestRepository.VmrTestRepoName, pr); + + await CheckForwardFlowGitHubPullRequest( + [(TestRepository.TestRepo1Name, repoSha)], + TestRepository.VmrTestRepoName, + targetBranchName, + [ + TestFile1Name, + TestFile2Name + ], + TestFilePatches); + } + } + } + }); + } +} From 8673a3bfc07d9efe3b6aaddb57ab0db6895f6019 Mon Sep 17 00:00:00 2001 From: Premek Vysoky Date: Mon, 27 Jan 2025 15:06:53 +0100 Subject: [PATCH 04/24] Fix VMR cloning in PCS Broken by https://github.com/dotnet/arcade-services/issues/4376 --- .../VirtualMonoRepo/ForwardFlowOperation.cs | 2 +- .../DarcLib/VirtualMonoRepo/PcsVmrForwardFlower.cs | 3 ++- .../DarcLib/VirtualMonoRepo/VmrForwardFlower.cs | 11 +++++++++-- .../VmrTestsBase.cs | 1 + 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ForwardFlowOperation.cs b/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ForwardFlowOperation.cs index 7730fa335f..6cde4f63b7 100644 --- a/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ForwardFlowOperation.cs +++ b/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ForwardFlowOperation.cs @@ -43,6 +43,6 @@ await GetBaseBranch(new NativePath(_options.VmrPath)), await GetTargetBranch(repoPath), _vmrInfo.VmrPath, _options.DiscardPatches, - cancellationToken); + cancellationToken: cancellationToken); } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/PcsVmrForwardFlower.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/PcsVmrForwardFlower.cs index fcb420b546..a366b6a2bb 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/PcsVmrForwardFlower.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/PcsVmrForwardFlower.cs @@ -65,7 +65,7 @@ public async Task FlowForwardAsync( string headBranch, CancellationToken cancellationToken = default) { - // Prepare repo + bool prBranchExisted = await PrepareVmr(subscription.TargetRepository, subscription.TargetBranch, headBranch, cancellationToken); SourceMapping mapping = _dependencyTracker.GetMapping(subscription.TargetDirectory); ISourceComponent repoVersion = _sourceManifest.GetRepoVersion(mapping.Name); List remotes = new[] { mapping.DefaultRemote, repoVersion.RemoteUri } @@ -89,6 +89,7 @@ public async Task FlowForwardAsync( headBranch, subscription.TargetRepository, discardPatches: true, + prBranchExisted, cancellationToken); } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs index 866660246c..dc97f34fbb 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs @@ -33,6 +33,7 @@ public interface IVmrForwardFlower /// New/existing branch to make the changes on /// URI of the VMR to update /// Keep patch files? + /// Whether the PR branch already exists in the VMR. Null when we don't as the VMR needs to be prepared /// True when there were changes to be flown Task FlowForwardAsync( string mapping, @@ -43,6 +44,7 @@ Task FlowForwardAsync( string headBranch, string targetVmrUri, bool discardPatches = false, + bool? prBranchExisted = null, CancellationToken cancellationToken = default); } @@ -92,9 +94,14 @@ public async Task FlowForwardAsync( string headBranch, string targetVmrUri, bool discardPatches = false, + bool? prBranchExisted = null, CancellationToken cancellationToken = default) { - bool prBranchExisted = await PrepareVmr(targetVmrUri, targetBranch, headBranch, cancellationToken); + // Null means, we don't know and we need to clone the VMR + if (!prBranchExisted.HasValue) + { + prBranchExisted = await PrepareVmr(targetVmrUri, targetBranch, headBranch, cancellationToken); + } ILocalGitRepo sourceRepo = _localGitRepoFactory.Create(repoPath); SourceMapping mapping = _dependencyTracker.GetMapping(mappingName); @@ -116,7 +123,7 @@ public async Task FlowForwardAsync( targetBranch, headBranch, discardPatches, - rebaseConflicts: !prBranchExisted, + rebaseConflicts: !prBranchExisted.Value, cancellationToken); if (hasChanges) diff --git a/test/Microsoft.DotNet.Darc.VirtualMonoRepo.E2E.Tests/VmrTestsBase.cs b/test/Microsoft.DotNet.Darc.VirtualMonoRepo.E2E.Tests/VmrTestsBase.cs index 22d13185c2..b40e4f6b9a 100644 --- a/test/Microsoft.DotNet.Darc.VirtualMonoRepo.E2E.Tests/VmrTestsBase.cs +++ b/test/Microsoft.DotNet.Darc.VirtualMonoRepo.E2E.Tests/VmrTestsBase.cs @@ -249,6 +249,7 @@ protected async Task CallDarcForwardflow( "main", branch, VmrPath, + discardPatches: false, cancellationToken: _cancellationToken.Token); } From 6ad1da0ff90b1482e2809ebc55c909de1099de33 Mon Sep 17 00:00:00 2001 From: dkurepa Date: Mon, 27 Jan 2025 17:52:40 +0100 Subject: [PATCH 05/24] Add scenario test for other scenario --- .../PullRequestUpdater.cs | 34 ++++- .../ScenarioTestBase.cs | 19 ++- .../ScenarioTests/ScenarioTests_CodeFlow.cs | 11 +- .../ScenarioTests_CodeFlowConflicts.cs | 143 +++++++++++++++--- .../ScenarioTests/ScenarioTests_SdkUpdate.cs | 2 +- 5 files changed, 172 insertions(+), 37 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index bdf327af3b..bd19b9d2a8 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Text; +using Maestro.Common; using Maestro.Data.Models; using Maestro.MergePolicies; using Maestro.MergePolicyEvaluation; @@ -150,8 +151,13 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up } else { - // Check if the PR has a conflict - if (pr.State == InProgressPullRequestState.Conflict) + (var targetRepository, var targetBranch) = await GetTargetAsync(); + var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); + var latestCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); + // GetLatestCommitAsync use this + // Check if we think the PR has a conflict. If we think so, check if the PR head branch still has the same commit as the one we remembered. + // If it does, we should try to update the PR again, the conflicts might be resolved + if (pr.State == InProgressPullRequestState.Conflict && latestCommit == pr.SourceSha) { // Set a reminder to check if the PR has been merged // TODO we should add a commit of the PR branch to the cache, and then check if it got updated @@ -243,8 +249,7 @@ protected virtual Task TagSourceRepositoryGitHubContactsIfPossibleAsync(InProgre { _logger.LogInformation("Querying status for pull request {prUrl}", pr.Url); - (var targetRepository, _) = await GetTargetAsync(); - IRemote remote = await _remoteFactory.CreateRemoteAsync(targetRepository); + IRemote remote = await GetRemoteAsync(); PrStatus status; try @@ -936,10 +941,13 @@ await CreateCodeFlowPullRequestAsync( sb.AppendLine(); sb.AppendLine("Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged"); - (var targetRepo, var _) = await GetTargetAsync(); - var remote = await _remoteFactory.CreateRemoteAsync(targetRepo); + (var targetRepository, _) = await GetTargetAsync(); + var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); await remote.CommentPullRequestAsync(pr.Url, sb.ToString()); + // If the headBranch gets updated, we will retry to update it with previously conflicting changes. If these changes still cause a conflict, we should update the + // InProgressPullRequest with the latest commit from the remote branch + var remoteCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); await _pullRequestCheckReminders.SetReminderAsync( new PullRequestCheck() @@ -952,7 +960,7 @@ await _pullRequestCheckReminders.SetReminderAsync( isCodeFlow: true); await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow: true); - await UpdateInProgressPullRequestState(InProgressPullRequestState.Conflict); + await UpdateInProgressPullRequestState(InProgressPullRequestState.Conflict, remoteCommit); return false; } @@ -1222,7 +1230,7 @@ await AddDependencyFlowEventsAsync( } } - private async Task UpdateInProgressPullRequestState(InProgressPullRequestState newState) + private async Task UpdateInProgressPullRequestState(InProgressPullRequestState newState, string? newCommit = null) { var inProgressPr = await _pullRequestState.TryGetStateAsync(); @@ -1232,9 +1240,19 @@ private async Task UpdateInProgressPullRequestState(InProgressPullRequestState n } inProgressPr.State = newState; + if (!string.IsNullOrEmpty(newCommit)) + { + inProgressPr.SourceSha = newCommit; + } await _pullRequestState.SetAsync(inProgressPr); } + private async Task GetRemoteAsync() + { + (var targetRepository, _) = await GetTargetAsync(); + return await _remoteFactory.CreateRemoteAsync(targetRepository); + } + #endregion } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs index d5d857f521..e42afc7bd8 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs @@ -1057,10 +1057,10 @@ protected static async Task ClosePullRequest(string owner, string repo, Octokit. } } - protected static async Task CreateTargetBranchAndExecuteTest(string targetBranchName, TemporaryDirectory targetDirectory, Func test) + protected static async Task CreateTargetBranchAndExecuteTest(string targetBranchName, string targetDirectory, Func test) { // first create a new target branch - using (ChangeDirectory(targetDirectory.Directory)) + using (ChangeDirectory(targetDirectory)) { await using (await CheckoutBranchAsync(targetBranchName)) { @@ -1072,4 +1072,19 @@ protected static async Task CreateTargetBranchAndExecuteTest(string targetBranch } } } + + protected static async Task WaitForNewCommitInPullRequest(string repo, Octokit.PullRequest pr) + { + var attempts = 30; + while (attempts-- > 0) + { + pr = await GitHubApi.PullRequest.Get(TestParameters.GitHubTestOrg, repo, pr.Number); + if (pr.Commits > 1) + { + return; + } + await Task.Delay(TimeSpan.FromSeconds(20)); + } + throw new ScenarioTestException($"The created pull request for repo targeting {pr.Base.Ref} did not have a new commit within {attempts} minutes"); + } } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs index b5d5cfb690..72b1a2f582 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs @@ -27,14 +27,15 @@ internal partial class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase { $"{TestFile1Name}", DefaultPatch }, { $"src/{TestRepository.TestRepo1Name}/{TestFile1Name}", DefaultPatch }, { $"src/{TestRepository.TestRepo2Name}/{TestFile1Name}", DefaultPatch }, + { $"src/{TestRepository.TestRepo2Name}/{TestFile1Name}", DefaultPatch } }; private const string CommitPlaceholder = ""; private const string ConflictMessage = $""" There was a conflict in the PR branch when flowing source from https://github.com/maestro-auth-test/maestro-test1/tree/{CommitPlaceholder} Conflicting files: - - 1newFile.txt - - newFile.txt + - {TestFile1Name} + - {TestFile2Name} Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged @@ -63,7 +64,7 @@ public async Task Vmr_ForwardFlowTest() TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); var newFilePath = Path.Combine(reposFolder.Directory, TestFile1Name); - await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => + await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory.Directory, async () => { using (ChangeDirectory(reposFolder.Directory)) { @@ -132,7 +133,7 @@ public async Task Vmr_BackwardFlowTest() TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); var newFilePath = Path.Combine(reposFolder.Directory, "src", TestRepository.TestRepo1Name, TestFile1Name); - await CreateTargetBranchAndExecuteTest(targetBranchName, testRepoFolder, async () => + await CreateTargetBranchAndExecuteTest(targetBranchName, testRepoFolder.Directory, async () => { using (ChangeDirectory(reposFolder.Directory)) { @@ -218,7 +219,7 @@ public async Task Vmr_BatchedForwardFlowTest() var newFile1Path = Path.Combine(repo1.Directory, TestFile1Name); var newFile2Path = Path.Combine(repo2.Directory, TestFile1Name); - await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => + await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory.Directory, async () => { using (ChangeDirectory(repo1.Directory)) await using (await CheckoutBranchAsync(branch1Name)) diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs index 77afb34544..44c18a5ea8 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.ServiceModel.Channels; using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.DotNet.ProductConstructionService.Client.Models; using NUnit.Framework; @@ -13,7 +14,7 @@ namespace ProductConstructionService.ScenarioTests.ScenarioTests; internal partial class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase { [Test] - public async Task Conflict() + public async Task ConflictPrClosedTest() { var channelName = GetTestChannelName(); var branchName = GetTestBranchName(); @@ -38,11 +39,121 @@ public async Task Conflict() var newFilePath2 = Path.Combine(reposFolder.Directory, TestFile2Name); var newFileInVmrPath2 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFile2Name); + await PrepareConflictPR( + vmrDirectory.Directory, + reposFolder.Directory, + branchName, + targetBranchName, + newFilePath1, + newFilePath2, + newFileInVmrPath1, + newFileInVmrPath2, + channelName, + subscriptionId.Value, + async () => + { + var pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); + + await ClosePullRequest(TestParameters.GitHubTestOrg, TestRepository.VmrTestRepoName, pr); + + using (ChangeDirectory(reposFolder.Directory)) + { + await CheckForwardFlowGitHubPullRequest( + [(TestRepository.TestRepo1Name, (await GitGetCurrentSha()).TrimEnd())], + TestRepository.VmrTestRepoName, + targetBranchName, + [ + $"src/{TestRepository.TestRepo1Name}/{TestFile1Name}", + $"src/{TestRepository.TestRepo1Name}/{TestFile2Name}" + ], + TestFilePatches); + } + }); + } + + [Test] + public async Task ConflictResolvedTest() + { + var channelName = GetTestChannelName(); + var branchName = GetTestBranchName(); + var productRepo = GetGitHubRepoUrl(TestRepository.TestRepo1Name); + var targetBranchName = GetTestBranchName(); + + await using AsyncDisposableValue testChannel = await CreateTestChannelAsync(channelName); + + await using AsyncDisposableValue subscriptionId = await CreateForwardFlowSubscriptionAsync( + channelName, + TestRepository.TestRepo1Name, + TestRepository.VmrTestRepoName, + targetBranchName, + UpdateFrequency.None.ToString(), + TestParameters.GitHubTestOrg, + targetDirectory: TestRepository.TestRepo1Name); + + TemporaryDirectory vmrDirectory = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); + TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); + var newFilePath1 = Path.Combine(reposFolder.Directory, TestFile1Name); + var newFileInVmrPath1 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFile1Name); + var newFilePath2 = Path.Combine(reposFolder.Directory, TestFile2Name); + var newFileInVmrPath2 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFile2Name); + + await PrepareConflictPR( + vmrDirectory.Directory, + reposFolder.Directory, + branchName, + targetBranchName, + newFilePath1, + newFilePath2, + newFileInVmrPath1, + newFileInVmrPath2, + channelName, + subscriptionId.Value, + async () => + { + var pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); + + using (ChangeDirectory(vmrDirectory.Directory)) + { + TestContext.WriteLine("Reverting the commit that caused the conflict"); + await CheckoutRemoteRefAsync(pr.Head.Ref); + await RunGitAsync("revert", await GitGetCurrentSha()); + await RunGitAsync("push", "origin", pr.Head.Ref); + } + + using (ChangeDirectory(reposFolder.Directory)) + { + await WaitForNewCommitInPullRequest(TestRepository.VmrTestRepoName, pr); + await CheckForwardFlowGitHubPullRequest( + [(TestRepository.TestRepo1Name, (await GitGetCurrentSha()).TrimEnd())], + TestRepository.VmrTestRepoName, + targetBranchName, + [ + $"src/{TestRepository.TestRepo1Name}/{TestFile1Name}", + $"src/{TestRepository.TestRepo1Name}/{TestFile2Name}" + ], + TestFilePatches); + } + }); + } + + private async Task PrepareConflictPR( + string vmrDirectory, + string productRepoDirectory, + string sourceBranchName, + string targetBranchName, + string newFilePath1, + string newFilePath2, + string newFileInVmrPath1, + string newFileInVmrPath2, + string channelName, + string subscriptionId, + Func test) + { await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => { - using (ChangeDirectory(reposFolder.Directory)) + using (ChangeDirectory(productRepoDirectory)) { - await using (await CheckoutBranchAsync(branchName)) + await using (await CheckoutBranchAsync(sourceBranchName)) { // Make a change in a product repo TestContext.WriteLine("Making code changes to the repo"); @@ -53,14 +164,14 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () await GitCommitAsync("Add new files"); // Push it to github - await using (await PushGitBranchAsync("origin", branchName)) + await using (await PushGitBranchAsync("origin", sourceBranchName)) { var repoSha = (await GitGetCurrentSha()).TrimEnd(); // Create a new build from the commit and add it to a channel Build build = await CreateBuildAsync( GetGitHubRepoUrl(TestRepository.TestRepo1Name), - branchName, + sourceBranchName, repoSha, "1", []); @@ -69,13 +180,13 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () await AddBuildToChannelAsync(build.Id, channelName); TestContext.WriteLine("Triggering the subscription"); - await TriggerSubscriptionAsync(subscriptionId.Value); + await TriggerSubscriptionAsync(subscriptionId); TestContext.WriteLine("Waiting for the PR to show up"); Octokit.PullRequest pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); // Now make a change directly in the PR - using (ChangeDirectory(vmrDirectory.Directory)) + using (ChangeDirectory(vmrDirectory)) { await CheckoutRemoteRefAsync(pr.Head.Ref); File.WriteAllText(newFileInVmrPath1, "file edited in PR"); @@ -99,7 +210,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () TestContext.WriteLine("Creating a build from the new commit"); build = await CreateBuildAsync( GetGitHubRepoUrl(TestRepository.TestRepo1Name), - branchName, + sourceBranchName, repoSha, "2", []); @@ -108,27 +219,17 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () await AddBuildToChannelAsync(build.Id, channelName); TestContext.WriteLine("Triggering the subscription"); - await TriggerSubscriptionAsync(subscriptionId.Value); + await TriggerSubscriptionAsync(subscriptionId); TestContext.WriteLine("Waiting for conflict comment to show up on the PR"); - pr = await WaitForUpdatedPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); + pr = await WaitForPullRequestComment(TestRepository.VmrTestRepoName, targetBranchName, "conflict"); await CheckConflictPullRequestComment( TestRepository.VmrTestRepoName, targetBranchName, pr, ConflictMessage.Replace(CommitPlaceholder, repoSha)); - await ClosePullRequest(TestParameters.GitHubTestOrg, TestRepository.VmrTestRepoName, pr); - - await CheckForwardFlowGitHubPullRequest( - [(TestRepository.TestRepo1Name, repoSha)], - TestRepository.VmrTestRepoName, - targetBranchName, - [ - TestFile1Name, - TestFile2Name - ], - TestFilePatches); + await test(); } } } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_SdkUpdate.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_SdkUpdate.cs index aa7d729047..5b3a48b1fa 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_SdkUpdate.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_SdkUpdate.cs @@ -185,7 +185,7 @@ public async Task ArcadeSdkVmrUpdate_E2E() TemporaryDirectory testRepoFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); TemporaryDirectory vmrFolder = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); - await CreateTargetBranchAndExecuteTest(targetBranch, testRepoFolder, async () => + await CreateTargetBranchAndExecuteTest(targetBranch, testRepoFolder.Directory, async () => { using (ChangeDirectory(vmrFolder.Directory)) { From b4ba888f85000b0c894d3026657ca2a3d04e7055 Mon Sep 17 00:00:00 2001 From: dkurepa Date: Mon, 27 Jan 2025 18:45:38 +0100 Subject: [PATCH 06/24] what is wrong here.. --- .../ScenarioTestBase.cs | 4 ++-- .../ScenarioTests/ScenarioTests_CodeFlow.cs | 2 +- .../ScenarioTests/ScenarioTests_CodeFlowConflicts.cs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs index e42afc7bd8..09b9a094e6 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs @@ -1073,13 +1073,13 @@ protected static async Task CreateTargetBranchAndExecuteTest(string targetBranch } } - protected static async Task WaitForNewCommitInPullRequest(string repo, Octokit.PullRequest pr) + protected static async Task WaitForNewCommitInPullRequest(string repo, Octokit.PullRequest pr, int numberOfCommits = 2) { var attempts = 30; while (attempts-- > 0) { pr = await GitHubApi.PullRequest.Get(TestParameters.GitHubTestOrg, repo, pr.Number); - if (pr.Commits > 1) + if (pr.Commits >= numberOfCommits) { return; } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs index 72b1a2f582..3d9f6ef067 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs @@ -26,7 +26,7 @@ internal partial class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase { { $"{TestFile1Name}", DefaultPatch }, { $"src/{TestRepository.TestRepo1Name}/{TestFile1Name}", DefaultPatch }, - { $"src/{TestRepository.TestRepo2Name}/{TestFile1Name}", DefaultPatch }, + { $"src/{TestRepository.TestRepo1Name}/{TestFile2Name}", DefaultPatch }, { $"src/{TestRepository.TestRepo2Name}/{TestFile1Name}", DefaultPatch } }; diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs index 44c18a5ea8..baf1d0a696 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs @@ -116,13 +116,13 @@ await PrepareConflictPR( { TestContext.WriteLine("Reverting the commit that caused the conflict"); await CheckoutRemoteRefAsync(pr.Head.Ref); - await RunGitAsync("revert", await GitGetCurrentSha()); + await RunGitAsync("revert", (await GitGetCurrentSha()).TrimEnd()); await RunGitAsync("push", "origin", pr.Head.Ref); } using (ChangeDirectory(reposFolder.Directory)) { - await WaitForNewCommitInPullRequest(TestRepository.VmrTestRepoName, pr); + await WaitForNewCommitInPullRequest(TestRepository.VmrTestRepoName, pr, 4); await CheckForwardFlowGitHubPullRequest( [(TestRepository.TestRepo1Name, (await GitGetCurrentSha()).TrimEnd())], TestRepository.VmrTestRepoName, From 8eb1f894875252d64d593eaa3019aa411fe75b61 Mon Sep 17 00:00:00 2001 From: dkurepa Date: Tue, 28 Jan 2025 11:31:38 +0100 Subject: [PATCH 07/24] fix unit tests --- .../DarcLib/VirtualMonoRepo/Exceptions.cs | 15 ++++++++++----- .../DarcLib/VirtualMonoRepo/VmrBackflower.cs | 2 +- .../DarcLib/VirtualMonoRepo/VmrForwardFlower.cs | 2 +- .../PullRequestUpdaterTests.cs | 4 ++++ 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs index bbfdf10938..71871b8a54 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs @@ -27,10 +27,10 @@ private static string GetExceptionMessage(VmrIngestionPatch patch, ProcessExecut + result; } -public class ConflictInPrBranchException(ProcessExecutionResult conflictResult, string targetBranch) +public class ConflictInPrBranchException(ProcessExecutionResult conflictResult, string targetBranch, bool isForwardFlow) : Exception($"Failed to flow changes due to conflicts in the target branch ({targetBranch})") { - public List FilesInConflict { get; } = ParseResult(conflictResult); + public List FilesInConflict { get; } = ParseResult(conflictResult, isForwardFlow); private const string AlreadyExistsRegex = "patch failed: (.+): already exist in index"; private const string PatchFailedRegex = "error: patch failed: (.*):"; @@ -45,7 +45,7 @@ public class ConflictInPrBranchException(ProcessExecutionResult conflictResult, FileDoesNotExistRegex ]; - private static List ParseResult(ProcessExecutionResult result) + private static List ParseResult(ProcessExecutionResult result, bool isForwardFlow) { List filesInConflict = new(); var errors = result.StandardError.Split(Environment.NewLine); @@ -61,7 +61,12 @@ private static List ParseResult(ProcessExecutionResult result) } } } - // Convert VMR paths to normal repo paths, for example src/repo/file.cs -> file.cs - return filesInConflict.Select(file => file.Split('/', 3)[2]).Distinct().ToList(); + if (isForwardFlow) + { + // Convert VMR paths to normal repo paths, for example src/repo/file.cs -> file.cs + return filesInConflict.Select(file => file.Split('/', 3)[2]).Distinct().ToList(); + } + // If we're backflowing, the file paths are already normal + return filesInConflict; } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs index 701ef9f826..233967c569 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs @@ -251,7 +251,7 @@ protected override async Task SameDirectionFlowAsync( if (!rebaseConflicts) { _logger.LogInformation("Failed to update a PR branch because of a conflict. Stopping the flow.."); - throw new ConflictInPrBranchException(e.Result, targetBranch); + throw new ConflictInPrBranchException(e.Result, targetBranch, isForwardFlow: false); } // Otherwise, we have a conflicting change in the last backflow PR (before merging) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs index dc97f34fbb..e62a8756f2 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs @@ -217,7 +217,7 @@ protected override async Task SameDirectionFlowAsync( if (!rebaseConflicts) { _logger.LogInformation("Failed to update a PR branch because of a conflict. Stopping the flow.."); - throw new ConflictInPrBranchException(e.Result, targetBranch); + throw new ConflictInPrBranchException(e.Result, targetBranch, isForwardFlow: true); } // This happens when a conflicting change was made in the last backflow PR (before merging) diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index da66914eb7..df3dd6f89d 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -442,6 +442,10 @@ protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, PrStatus p .Returns(Task.CompletedTask); } + remote. + Setup(x => x.GetLatestCommitAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(string.Empty); + return Disposable.Create(remote.VerifyAll); } From ab7f82a97087a1a3b7301627f051ab614a8e063e Mon Sep 17 00:00:00 2001 From: dkurepa Date: Tue, 28 Jan 2025 11:38:24 +0100 Subject: [PATCH 08/24] some improvements --- .../DarcLib/AzureDevOpsClient.cs | 2 +- .../DarcLib/IRemoteGitRepo.cs | 1 - .../PullRequestUpdater.cs | 25 +++++++++++-------- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs index 15f6faf64f..25e32a8a36 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs @@ -1563,7 +1563,7 @@ private static string TruncateDescriptionIfNeeded(string str) public async Task CommentPullRequestAsync(string pullRequestUri, string comment) { - (string accountName, string projectName, string repoName, int id) = ParsePullRequestUri(pullRequestUri); + (string accountName, string _, string repoName, int id) = ParsePullRequestUri(pullRequestUri); using VssConnection connection = CreateVssConnection(accountName); using GitHttpClient client = await connection.GetClientAsync(); diff --git a/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs b/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs index 0d5eac3625..b9eff6d282 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs @@ -174,7 +174,6 @@ Task> SearchPullRequestsAsync( /// /// Uri of the pull request /// Comment message - /// Task CommentPullRequestAsync(string pullRequestUri, string comment); } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index bd19b9d2a8..7bb7a1ed45 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Security.Policy; using System.Text; using Maestro.Common; using Maestro.Data.Models; @@ -151,18 +152,20 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up } else { - (var targetRepository, var targetBranch) = await GetTargetAsync(); - var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); - var latestCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); - // GetLatestCommitAsync use this - // Check if we think the PR has a conflict. If we think so, check if the PR head branch still has the same commit as the one we remembered. - // If it does, we should try to update the PR again, the conflicts might be resolved - if (pr.State == InProgressPullRequestState.Conflict && latestCommit == pr.SourceSha) + // Check if we think the PR has a conflict + if (pr.State == InProgressPullRequestState.Conflict) { - // Set a reminder to check if the PR has been merged - // TODO we should add a commit of the PR branch to the cache, and then check if it got updated - await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow); - return false; + // If we think so, check if the PR head branch still has the same commit as the one we remembered. + // If it does, we should try to update the PR again, the conflicts might be resolved + (var targetRepository, var targetBranch) = await GetTargetAsync(); + var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); + var latestCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); + if (latestCommit == pr.SourceSha) + { + // Set a reminder to check on the PR again + await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow); + return false; + } } var prStatus = await GetPullRequestStatusAsync(pr, isCodeFlow); From 80c14e188913092c3cc1c8420d00b66bb3075704 Mon Sep 17 00:00:00 2001 From: dkurepa Date: Tue, 28 Jan 2025 12:01:54 +0100 Subject: [PATCH 09/24] more things --- .../PullRequestUpdater.cs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 7bb7a1ed45..2af94519ec 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -156,7 +156,7 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up if (pr.State == InProgressPullRequestState.Conflict) { // If we think so, check if the PR head branch still has the same commit as the one we remembered. - // If it does, we should try to update the PR again, the conflicts might be resolved + // If it doesn't, we should try to update the PR again, the conflicts might be resolved (var targetRepository, var targetBranch) = await GetTargetAsync(); var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); var latestCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); @@ -173,7 +173,7 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up { case PullRequestStatus.Completed: case PullRequestStatus.Invalid: - // If the PR is completed, we will open a new one, and we should clean the redis from preventing it + // If the PR is completed, we will open a new one pr = null; break; case PullRequestStatus.InProgressCanUpdate: @@ -189,8 +189,6 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up } } - // check f the PR is blocked. We can only do this after checking if it's completed, otherwise, we'd never clear redis - // Code flow updates are handled separately if (isCodeFlow) { @@ -824,8 +822,8 @@ private async Task ClearAllStateAsync(bool isCodeFlow) { var pr = await _pullRequestState.TryDeleteAsync(); await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow); - // If the cache has been cleared, that means there was an update waiting for the PR to be merged - // In that case, we don't want to remove it, since we want to process it + // If the pull request we deleted from the cache had a conflict, we shouldn't unset the update reminder + // as there was an update that was previously blocked if (pr?.State != InProgressPullRequestState.Conflict) { await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow); @@ -932,10 +930,13 @@ await CreateCodeFlowPullRequestAsync( } catch (ConflictInPrBranchException conflictException) { - // The PR we're trying to update has a conflict with the source repo. We will it as blocked, not allowing any updates from this - // subscription till it's merged. We'll set a reminder to check if PR has been merged. When it is, we'll unblock updates from the repo + // The PR we're trying to update has a conflict with the source repo. We will mark it as blocked, not allowing any updates from this + // subscription till it's merged, or the conflict resolved. We'll set a reminder to check on it. StringBuilder sb = new(); - sb.AppendLine($"There was a conflict in the PR branch when flowing source from {update.SourceRepo}/tree/{update.SourceSha}"); + var commitUri = update.SourceRepo.Contains("github.com") + ? $"{update.SourceRepo}/tree/{update.SourceSha}" + : $"{update.SourceRepo}?version=GC{update.SourceSha}"; + sb.AppendLine($"There was a conflict in the PR branch when flowing source from {commitUri}"); sb.AppendLine("Conflicting files:"); foreach (var file in conflictException.FilesInConflict) { From bf32b13fae070f7534b5097034eb2f684ecd46de Mon Sep 17 00:00:00 2001 From: dkurepa Date: Tue, 28 Jan 2025 12:56:43 +0100 Subject: [PATCH 10/24] tests are passing locally?? --- .../PullRequestUpdaterTests.cs | 4 ---- .../CodeFlowScenarioTestBase.cs | 2 +- .../ScenarioTests/ScenarioTests_CodeFlow.cs | 10 ---------- .../ScenarioTests_CodeFlowConflicts.cs | 13 ++++++++++++- 4 files changed, 13 insertions(+), 16 deletions(-) diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index df3dd6f89d..da66914eb7 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -442,10 +442,6 @@ protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, PrStatus p .Returns(Task.CompletedTask); } - remote. - Setup(x => x.GetLatestCommitAsync(It.IsAny(), It.IsAny())) - .ReturnsAsync(string.Empty); - return Disposable.Create(remote.VerifyAll); } diff --git a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs index 09ecb5e948..336a0b9cc5 100644 --- a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs @@ -205,7 +205,7 @@ public async Task WaitForPullRequestComment( throw new ScenarioTestException($"Comment containing '{partialComment}' was not found in the pull request."); } - public async Task CheckConflictPullRequestComment( + public async Task CheckIfPullRequestCommentExists( string targetRepo, string targetBranch, PullRequest pullRequest, diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs index 3d9f6ef067..5a65cac6b9 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs @@ -30,16 +30,6 @@ internal partial class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase { $"src/{TestRepository.TestRepo2Name}/{TestFile1Name}", DefaultPatch } }; - private const string CommitPlaceholder = ""; - private const string ConflictMessage = $""" - There was a conflict in the PR branch when flowing source from https://github.com/maestro-auth-test/maestro-test1/tree/{CommitPlaceholder} - Conflicting files: - - {TestFile1Name} - - {TestFile2Name} - - Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged - - """; [Test] public async Task Vmr_ForwardFlowTest() diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs index baf1d0a696..e49ac5dbbc 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs @@ -13,6 +13,17 @@ namespace ProductConstructionService.ScenarioTests.ScenarioTests; [Category("CodeFlow")] internal partial class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase { + private const string CommitPlaceholder = ""; + private const string ConflictMessage = $""" + There was a conflict in the PR branch when flowing source from https://github.com/maestro-auth-test/maestro-test1/tree/{CommitPlaceholder} + Conflicting files: + - {TestFile1Name} + - {TestFile2Name} + + Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged + + """; + [Test] public async Task ConflictPrClosedTest() { @@ -223,7 +234,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () TestContext.WriteLine("Waiting for conflict comment to show up on the PR"); pr = await WaitForPullRequestComment(TestRepository.VmrTestRepoName, targetBranchName, "conflict"); - await CheckConflictPullRequestComment( + await CheckIfPullRequestCommentExists( TestRepository.VmrTestRepoName, targetBranchName, pr, From 07b24e907fdf5fbbd1256c612edf5fbf3bfe9db1 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> Date: Tue, 28 Jan 2025 13:49:11 +0100 Subject: [PATCH 11/24] Update src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Přemek Vysoký --- .../PullRequestUpdater.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 2af94519ec..257791c2bf 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -141,10 +141,9 @@ public async Task UpdateAssetsAsync( public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem update) { _logger.LogInformation("Processing pending updates for subscription {subscriptionId}", update.SubscriptionId); - bool isCodeFlow = update.SubscriptionType == SubscriptionType.DependenciesAndSources; - // Check if we track an on-going PR already InProgressPullRequest? pr = await _pullRequestState.TryGetStateAsync(); + bool isCodeFlow = update.SubscriptionType == SubscriptionType.DependenciesAndSources; if (pr == null) { From cd68e659709b115d53a0ffb6086995ef56eae22a Mon Sep 17 00:00:00 2001 From: dkurepa Date: Tue, 28 Jan 2025 15:17:13 +0100 Subject: [PATCH 12/24] Address part of the feedback --- .../DarcLib/VirtualMonoRepo/Exceptions.cs | 20 ++++++------ .../InProgressPullRequest.cs | 4 +-- .../PullRequestUpdater.cs | 32 +++++++++---------- .../WorkItems/SubscriptionUpdateWorkItem.cs | 13 ++++++++ 4 files changed, 40 insertions(+), 29 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs index 71871b8a54..9139910b20 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs @@ -27,17 +27,17 @@ private static string GetExceptionMessage(VmrIngestionPatch patch, ProcessExecut + result; } -public class ConflictInPrBranchException(ProcessExecutionResult conflictResult, string targetBranch, bool isForwardFlow) +public class ConflictInPrBranchException(ProcessExecutionResult gitMergeResult, string targetBranch, bool isForwardFlow) : Exception($"Failed to flow changes due to conflicts in the target branch ({targetBranch})") { - public List FilesInConflict { get; } = ParseResult(conflictResult, isForwardFlow); + public List FilesInConflict { get; } = ParseResult(gitMergeResult, isForwardFlow); - private const string AlreadyExistsRegex = "patch failed: (.+): already exist in index"; - private const string PatchFailedRegex = "error: patch failed: (.*):"; - private const string PatchDoesNotApplyRegex = "error: (.+): patch does not apply"; - private const string FileDoesNotExistRegex = "error: (.+): does not exist in index"; + private static readonly Regex AlreadyExistsRegex = new("patch failed: (.+): already exist in index"); + private static readonly Regex PatchFailedRegex = new("error: patch failed: (.*):"); + private static readonly Regex PatchDoesNotApplyRegex = new("error: (.+): patch does not apply"); + private static readonly Regex FileDoesNotExistRegex = new("error: (.+): does not exist in index"); - private static readonly string[] ConflictRegex = + private static readonly Regex[] ConflictRegex = [ AlreadyExistsRegex, PatchFailedRegex, @@ -45,15 +45,15 @@ public class ConflictInPrBranchException(ProcessExecutionResult conflictResult, FileDoesNotExistRegex ]; - private static List ParseResult(ProcessExecutionResult result, bool isForwardFlow) + private static List ParseResult(ProcessExecutionResult gitMergeResult, bool isForwardFlow) { List filesInConflict = new(); - var errors = result.StandardError.Split(Environment.NewLine); + var errors = gitMergeResult.StandardError.Split(Environment.NewLine); foreach (var error in errors) { foreach (var regex in ConflictRegex) { - var match = Regex.Match(error, regex); + var match = regex.Match(error); if (match.Success) { filesInConflict.Add(match.Groups[1].Value); diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs index c0a4116df9..b837db7443 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs @@ -52,11 +52,11 @@ public class InProgressPullRequest : DependencyFlowWorkItem, IPullRequest public DateTime? NextCheck { get; set; } [DataMember] - public InProgressPullRequestState State { get; set; } + public InProgressPullRequestState MergeState { get; set; } } public enum InProgressPullRequestState { - Open, + Mergeable, Conflict } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 2af94519ec..01c0cec87b 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -153,7 +153,7 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up else { // Check if we think the PR has a conflict - if (pr.State == InProgressPullRequestState.Conflict) + if (pr.MergeState == InProgressPullRequestState.Conflict) { // If we think so, check if the PR head branch still has the same commit as the one we remembered. // If it doesn't, we should try to update the PR again, the conflicts might be resolved @@ -225,8 +225,8 @@ public async Task CheckPullRequestAsync(PullRequestCheck pullRequestCheck) if (inProgressPr == null) { _logger.LogInformation("No in-progress pull request found for a PR check"); - await ClearAllStateAsync(isCodeFlow: true); - await ClearAllStateAsync(isCodeFlow: false); + await ClearAllStateAsync(isCodeFlow: true, clearPendingUpdates: true); + await ClearAllStateAsync(isCodeFlow: false, clearPendingUpdates: true); return false; } @@ -285,7 +285,7 @@ await AddDependencyFlowEventsAsync( mergePolicyResult, pr.Url); - await ClearAllStateAsync(isCodeFlow); + await ClearAllStateAsync(isCodeFlow, clearPendingUpdates: pr.MergeState == InProgressPullRequestState.Mergeable); return PullRequestStatus.Completed; case MergePolicyCheckResult.FailedPolicies: @@ -329,7 +329,7 @@ await AddDependencyFlowEventsAsync( _logger.LogInformation("PR {url} has been manually {action}. Stopping tracking it", pr.Url, status.ToString().ToLowerInvariant()); - await ClearAllStateAsync(isCodeFlow); + await ClearAllStateAsync(isCodeFlow, clearPendingUpdates: pr.MergeState == InProgressPullRequestState.Mergeable); // Also try to clean up the PR branch. try @@ -818,13 +818,13 @@ private async Task SetPullRequestCheckReminder(InProgressPullRequest prState, bo await _pullRequestState.SetAsync(prState); } - private async Task ClearAllStateAsync(bool isCodeFlow) + private async Task ClearAllStateAsync(bool isCodeFlow, bool clearPendingUpdates) { - var pr = await _pullRequestState.TryDeleteAsync(); + await _pullRequestState.TryDeleteAsync(); await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow); // If the pull request we deleted from the cache had a conflict, we shouldn't unset the update reminder // as there was an update that was previously blocked - if (pr?.State != InProgressPullRequestState.Conflict) + if (!clearPendingUpdates) { await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow); } @@ -933,14 +933,11 @@ await CreateCodeFlowPullRequestAsync( // The PR we're trying to update has a conflict with the source repo. We will mark it as blocked, not allowing any updates from this // subscription till it's merged, or the conflict resolved. We'll set a reminder to check on it. StringBuilder sb = new(); - var commitUri = update.SourceRepo.Contains("github.com") - ? $"{update.SourceRepo}/tree/{update.SourceSha}" - : $"{update.SourceRepo}?version=GC{update.SourceSha}"; - sb.AppendLine($"There was a conflict in the PR branch when flowing source from {commitUri}"); + sb.AppendLine($"There was a conflict in the PR branch when flowing source from {update.GetRepoAtCommitUri()}"); sb.AppendLine("Conflicting files:"); foreach (var file in conflictException.FilesInConflict) { - sb.AppendLine($" - {file}"); + sb.AppendLine($" - {update.GetFileAtCommitUri(file)}"); } sb.AppendLine(); sb.AppendLine("Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged"); @@ -962,9 +959,10 @@ await _pullRequestCheckReminders.SetReminderAsync( }, DefaultReminderDelay, isCodeFlow: true); - - await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow: true); - await UpdateInProgressPullRequestState(InProgressPullRequestState.Conflict, remoteCommit); + + pr.MergeState = InProgressPullRequestState.Conflict; + pr.SourceSha = remoteCommit; + await SetPullRequestCheckReminder(pr, isCodeFlow: true); return false; } @@ -1243,7 +1241,7 @@ private async Task UpdateInProgressPullRequestState(InProgressPullRequestState n return; } - inProgressPr.State = newState; + inProgressPr.MergeState = newState; if (!string.IsNullOrEmpty(newCommit)) { inProgressPr.SourceSha = newCommit; diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs index 720518e9e4..fa383a197c 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs @@ -35,3 +35,16 @@ public class SubscriptionUpdateWorkItem : DependencyFlowWorkItem [DataMember] public bool IsCoherencyUpdate { get; init; } } + +public static class SubscriptionUpdateWorkItemExtensions +{ + public static string GetRepoAtCommitUri(this SubscriptionUpdateWorkItem update) => + update.SourceRepo.Contains("github.com") + ? $"{update.SourceRepo}/tree/{update.SourceSha}" + : $"{update.SourceRepo}?version=GC{update.SourceSha}"; + + public static string GetFileAtCommitUri(this SubscriptionUpdateWorkItem update, string filePath) => + update.SourceRepo.Contains("github.com") + ? $"{update.SourceRepo}/blob/{update.SourceSha}/{filePath}" + : $"{update.SourceRepo}?version=GC{update.SourceSha}&path={filePath}"; +} From a40dda8476dbb06a3ca7ab1d9c1261cedd224c41 Mon Sep 17 00:00:00 2001 From: dkurepa Date: Tue, 28 Jan 2025 18:10:07 +0100 Subject: [PATCH 13/24] More stuff resolved --- .../PullRequestUpdater.cs | 48 +++++++------------ .../ScenarioTestBase.cs | 9 +++- .../ScenarioTests_CodeFlowConflicts.cs | 4 +- 3 files changed, 28 insertions(+), 33 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 6e6280a64a..634737c993 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -151,22 +151,6 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up } else { - // Check if we think the PR has a conflict - if (pr.MergeState == InProgressPullRequestState.Conflict) - { - // If we think so, check if the PR head branch still has the same commit as the one we remembered. - // If it doesn't, we should try to update the PR again, the conflicts might be resolved - (var targetRepository, var targetBranch) = await GetTargetAsync(); - var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); - var latestCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); - if (latestCommit == pr.SourceSha) - { - // Set a reminder to check on the PR again - await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow); - return false; - } - } - var prStatus = await GetPullRequestStatusAsync(pr, isCodeFlow); switch (prStatus) { @@ -249,7 +233,8 @@ protected virtual Task TagSourceRepositoryGitHubContactsIfPossibleAsync(InProgre { _logger.LogInformation("Querying status for pull request {prUrl}", pr.Url); - IRemote remote = await GetRemoteAsync(); + (var targetRepository, var _) = await GetTargetAsync(); + var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); PrStatus status; try @@ -294,6 +279,17 @@ await AddDependencyFlowEventsAsync( case MergePolicyCheckResult.NoPolicies: case MergePolicyCheckResult.FailedToMerge: _logger.LogInformation("Pull request {url} still active (updatable) - keeping tracking it", pr.Url); + // Check if we think the PR has a conflict + if (pr.MergeState == InProgressPullRequestState.Conflict) + { + // If we think so, check if the PR head branch still has the same commit as the one we remembered. + // If it doesn't, we should try to update the PR again, the conflicts might be resolved + var latestCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); + if (latestCommit == pr.SourceSha) + { + return PullRequestStatus.InProgressCannotUpdate; + } + } await SetPullRequestCheckReminder(pr, isCodeFlow); return PullRequestStatus.InProgressCanUpdate; @@ -933,10 +929,10 @@ await CreateCodeFlowPullRequestAsync( // subscription till it's merged, or the conflict resolved. We'll set a reminder to check on it. StringBuilder sb = new(); sb.AppendLine($"There was a conflict in the PR branch when flowing source from {update.GetRepoAtCommitUri()}"); - sb.AppendLine("Conflicting files:"); + sb.AppendLine("Files conflicting with the head branch:"); foreach (var file in conflictException.FilesInConflict) { - sb.AppendLine($" - {update.GetFileAtCommitUri(file)}"); + sb.AppendLine($" - [file]({update.GetFileAtCommitUri(file)})"); } sb.AppendLine(); sb.AppendLine("Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged"); @@ -949,19 +945,11 @@ await CreateCodeFlowPullRequestAsync( // InProgressPullRequest with the latest commit from the remote branch var remoteCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); - await _pullRequestCheckReminders.SetReminderAsync( - new PullRequestCheck() - { - UpdaterId = Id.ToString(), - Url = pr.Url, - IsCodeFlow = isCodeFlow - }, - DefaultReminderDelay, - isCodeFlow: true); - pr.MergeState = InProgressPullRequestState.Conflict; pr.SourceSha = remoteCommit; - await SetPullRequestCheckReminder(pr, isCodeFlow: true); + await _pullRequestState.SetAsync(pr); + await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow: true); + await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow); return false; } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs index 09b9a094e6..000f462a1b 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs @@ -1049,12 +1049,19 @@ protected static async Task ClosePullRequest(string owner, string repo, Octokit. }; await GitHubApi.Repository.PullRequest.Update(owner, repo, pullRequest.Number, pullRequestUpdate); - await GitHubApi.Git.Reference.Delete(owner, repo, $"heads/{pullRequest.Head.Ref}"); } catch { // Closed already } + try + { + await GitHubApi.Git.Reference.Delete(owner, repo, $"heads/{pullRequest.Head.Ref}"); + } + catch + { + // branch already deleted + } } protected static async Task CreateTargetBranchAndExecuteTest(string targetBranchName, string targetDirectory, Func test) diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs index e49ac5dbbc..efc60c67cc 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs @@ -234,11 +234,11 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () TestContext.WriteLine("Waiting for conflict comment to show up on the PR"); pr = await WaitForPullRequestComment(TestRepository.VmrTestRepoName, targetBranchName, "conflict"); - await CheckIfPullRequestCommentExists( + /*await CheckIfPullRequestCommentExists( TestRepository.VmrTestRepoName, targetBranchName, pr, - ConflictMessage.Replace(CommitPlaceholder, repoSha)); + ConflictMessage.Replace(CommitPlaceholder, repoSha));*/ await test(); } From 68b976a25870f29b40c31f900ae9008231991b12 Mon Sep 17 00:00:00 2001 From: dkurepa Date: Wed, 29 Jan 2025 15:26:10 +0100 Subject: [PATCH 14/24] Add DependencyFlow tests for conflict handling --- .../PullRequestUpdater.cs | 25 +---- .../PendingCodeFlowUpdatesTests.cs | 77 +++++++++++++++ .../PullRequestUpdaterTests.cs | 97 +++++++++++++++++-- .../UpdateAssetsForCodeFlowTests.cs | 2 +- 4 files changed, 166 insertions(+), 35 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 634737c993..47c6490eca 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -1064,6 +1064,7 @@ private async Task UpdateAssetsAndSources(SubscriptionUpdateWorkItem updat }); pullRequest.LastUpdate = DateTime.UtcNow; + pullRequest.MergeState = InProgressPullRequestState.Mergeable; await SetPullRequestCheckReminder(pullRequest, true); await _pullRequestUpdateReminders.UnsetReminderAsync(true); @@ -1219,29 +1220,5 @@ await AddDependencyFlowEventsAsync( } } - private async Task UpdateInProgressPullRequestState(InProgressPullRequestState newState, string? newCommit = null) - { - var inProgressPr = await _pullRequestState.TryGetStateAsync(); - - if (inProgressPr == null) - { - return; - } - - inProgressPr.MergeState = newState; - if (!string.IsNullOrEmpty(newCommit)) - { - inProgressPr.SourceSha = newCommit; - } - - await _pullRequestState.SetAsync(inProgressPr); - } - - private async Task GetRemoteAsync() - { - (var targetRepository, _) = await GetTargetAsync(); - return await _remoteFactory.CreateRemoteAsync(targetRepository); - } - #endregion } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs index 37870d5c51..d375cb2341 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs @@ -77,4 +77,81 @@ public async Task PendingUpdatesUpdatablePr() AndShouldHaveInProgressPullRequestState(newBuild); } } + + [Test] + public async Task PendingUpdatesInConflictWithCurrent() + { + GivenATestChannel(); + GivenACodeFlowSubscription( + new SubscriptionPolicy + { + Batchable = false, + UpdateFrequency = UpdateFrequency.EveryBuild + }); + Build oldBuild = GivenANewBuild(true); + Build newBuild = GivenANewBuild(true); + newBuild.Commit = "sha456"; + + using (WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true, flowerWillHaveConflict: true)) + { + await WhenProcessPendingUpdatesAsyncIsCalled(newBuild, isCodeFlow: true); + + ThenShouldHavePendingUpdateState(newBuild, isCodeFlow: true); + AndShouldNotHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState( + oldBuild, + overwriteBuildCommit: ConflictPRRemoteSha, + prState: InProgressPullRequestState.Conflict); + } + } + + [Test] + public async Task PendingUpdateNotUpdatablePrAlreadyInConflict() + { + GivenATestChannel(); + GivenACodeFlowSubscription( + new SubscriptionPolicy + { + Batchable = false, + UpdateFrequency = UpdateFrequency.EveryBuild + }); + Build build = GivenANewBuild(true); + + using (WithExistingCodeFlowPullRequest(build, canUpdate: true, prAlreadyHasConflict: true)) + { + await WhenProcessPendingUpdatesAsyncIsCalled(build, isCodeFlow: true); + + ThenShouldHavePendingUpdateState(build, isCodeFlow: true); + AndShouldNotHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState( + build, + overwriteBuildCommit: ConflictPRRemoteSha, + prState: InProgressPullRequestState.Conflict); + } + } + + + [Test] + public async Task PendingUpdateUpdatableConflictInPrResolved() + { + GivenATestChannel(); + GivenACodeFlowSubscription( + new SubscriptionPolicy + { + Batchable = false, + UpdateFrequency = UpdateFrequency.EveryBuild + }); + Build oldBuild = GivenANewBuild(true); + Build newBuild = GivenANewBuild(true); + newBuild.Commit = "sha456"; + using (WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true, prAlreadyHasConflict: true, latestCommitToReturn: "sha4")) + { + ExpectPrMetadataToBeUpdated(); + await WhenProcessPendingUpdatesAsyncIsCalled(newBuild, isCodeFlow: true); + ThenCodeShouldHaveBeenFlownForward(newBuild); + AndShouldHaveNoPendingUpdateState(); + AndShouldHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState(newBuild); + } + } } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index da66914eb7..2a94e25969 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Security.Cryptography; using FluentAssertions; using Maestro.Data; using Maestro.Data.Models; @@ -28,6 +29,7 @@ internal abstract class PullRequestUpdaterTests : SubscriptionOrPullRequestUpdat private const long InstallationId = 1174; protected const string InProgressPrUrl = "https://github.com/owner/repo/pull/10"; protected string? InProgressPrHeadBranch { get; private set; } = "pr.head.branch"; + protected const string ConflictPRRemoteSha = "sha3"; private Mock _backFlower = null!; private Mock _forwardFlower = null!; @@ -390,12 +392,35 @@ protected IDisposable WithExistingPullRequest(Build forBuild, PrStatus prStatus, return Disposable.Create(remote.VerifyAll); } - protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, bool canUpdate) + protected IDisposable WithExistingCodeFlowPullRequest( + Build forBuild, + bool canUpdate, + bool flowerWillHaveConflict = false, + bool prAlreadyHasConflict = false, + string latestCommitToReturn = ConflictPRRemoteSha) => canUpdate - ? WithExistingCodeFlowPullRequest(forBuild, PrStatus.Open, null) - : WithExistingCodeFlowPullRequest(forBuild, PrStatus.Open, MergePolicyEvaluationStatus.Pending); - - protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, PrStatus prStatus, MergePolicyEvaluationStatus? policyEvaluationStatus) + ? WithExistingCodeFlowPullRequest( + forBuild, + PrStatus.Open, + null, + flowerWillHaveConflict, + prAlreadyHasConflict, + latestCommitToReturn) + : WithExistingCodeFlowPullRequest( + forBuild, + PrStatus.Open, + MergePolicyEvaluationStatus.Pending, + flowerWillHaveConflict, + prAlreadyHasConflict, + latestCommitToReturn); + + protected IDisposable WithExistingCodeFlowPullRequest( + Build forBuild, + PrStatus prStatus, + MergePolicyEvaluationStatus? policyEvaluationStatus, + bool flowerWillHaveConflict = false, + bool prAlreadyHasConflict = false, + string latestCommitToReturn = ConflictPRRemoteSha) { var prUrl = Subscription.TargetDirectory != null ? VmrPullRequestUrl @@ -407,7 +432,17 @@ protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, PrStatus p AfterDbUpdateActions.Add(() => { - var pr = CreatePullRequestState(forBuild, prUrl); + var pr = CreatePullRequestState( + forBuild, + prUrl, + overwriteBuildCommit: + prAlreadyHasConflict + ? ConflictPRRemoteSha + : forBuild.Commit, + prState: + prAlreadyHasConflict + ? InProgressPullRequestState.Conflict + : InProgressPullRequestState.Mergeable); SetState(Subscription, pr); SetExpectedState(Subscription, pr); }); @@ -442,6 +477,29 @@ protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, PrStatus p .Returns(Task.CompletedTask); } + if (flowerWillHaveConflict) + { + remote + .Setup(x => x.CommentPullRequestAsync(It.IsAny(), It.IsAny())) + .Returns(Task.CompletedTask); + + ProcessExecutionResult gitMergeResult = new(); + _forwardFlower.Setup(x => x.FlowForwardAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Throws(() => new ConflictInPrBranchException(gitMergeResult, "branch", true)); + + } + + if (flowerWillHaveConflict || prAlreadyHasConflict) + { + remote + .Setup(x => x.GetLatestCommitAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(latestCommitToReturn); + } + return Disposable.Create(remote.VerifyAll); } @@ -459,17 +517,33 @@ protected void AndShouldHavePullRequestCheckReminder() }); } + protected void AndShouldNotHavePullRequestCheckReminder() + { + RemoveExpectedReminder(Subscription); + } + protected void AndShouldHaveInProgressPullRequestState( Build forBuild, bool? coherencyCheckSuccessful = true, List? coherencyErrors = null, - InProgressPullRequest? expectedState = null) + InProgressPullRequest? expectedState = null, + string? overwriteBuildCommit = null, + InProgressPullRequestState prState = InProgressPullRequestState.Mergeable) { var prUrl = Subscription.SourceEnabled ? VmrPullRequestUrl : InProgressPrUrl; - SetExpectedState(Subscription, expectedState ?? CreatePullRequestState(forBuild, prUrl, coherencyCheckSuccessful, coherencyErrors)); + SetExpectedState( + Subscription, + expectedState + ?? CreatePullRequestState( + forBuild, + prUrl, + coherencyCheckSuccessful, + coherencyErrors, + overwriteBuildCommit, + prState)); } protected void ThenShouldHaveInProgressPullRequestState(Build forBuild, InProgressPullRequest? expectedState = null) @@ -524,12 +598,14 @@ protected InProgressPullRequest CreatePullRequestState( Build forBuild, string prUrl, bool? coherencyCheckSuccessful = true, - List? coherencyErrors = null) + List? coherencyErrors = null, + string? overwriteBuildCommit = null, + InProgressPullRequestState prState = InProgressPullRequestState.Mergeable) => new() { UpdaterId = GetPullRequestUpdaterId().ToString(), HeadBranch = InProgressPrHeadBranch, - SourceSha = forBuild.Commit, + SourceSha = overwriteBuildCommit ?? forBuild.Commit, ContainedSubscriptions = [ new SubscriptionPullRequestUpdate @@ -549,5 +625,6 @@ protected InProgressPullRequest CreatePullRequestState( CoherencyCheckSuccessful = coherencyCheckSuccessful, CoherencyErrors = coherencyErrors, Url = prUrl, + MergeState = prState }; } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs index 4058b4b378..279996518f 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs @@ -9,7 +9,7 @@ namespace ProductConstructionService.DependencyFlow.Tests; /// /// Tests the code flow PR update logic. -/// The tests are writter in the order in which the different phases of the PR are written. +/// The tests are writer in the order in which the different phases of the PR are written. /// Each test should have the inner state that is left behind by the previous state. /// [TestFixture, NonParallelizable] From 30d1b322995a7cb4762e61dc808f4ae48f1ad1c7 Mon Sep 17 00:00:00 2001 From: dkurepa Date: Wed, 29 Jan 2025 16:00:47 +0100 Subject: [PATCH 15/24] Don't check whole conflict comment in scenario tests, just check if the correct files are there --- .../PullRequestUpdater.cs | 6 +++++- .../CodeFlowScenarioTestBase.cs | 9 +++++++-- .../ScenarioTests_CodeFlowConflicts.cs | 19 +++++-------------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 47c6490eca..e2eda93d5d 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -932,7 +932,7 @@ await CreateCodeFlowPullRequestAsync( sb.AppendLine("Files conflicting with the head branch:"); foreach (var file in conflictException.FilesInConflict) { - sb.AppendLine($" - [file]({update.GetFileAtCommitUri(file)})"); + sb.AppendLine($" - [{file}]({update.GetFileAtCommitUri(file)})"); } sb.AppendLine(); sb.AppendLine("Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged"); @@ -1010,6 +1010,10 @@ private async Task UpdateAssetsAndSources(SubscriptionUpdateWorkItem updat cancellationToken: default); } } + catch (Exception e) when (e is ConflictInPrBranchException) + { + throw; + } catch (Exception e) { _logger.LogError(e, "Failed to flow changes for build {buildId} in subscription {subscriptionId}", diff --git a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs index 336a0b9cc5..56b5c2c863 100644 --- a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs @@ -209,9 +209,14 @@ public async Task CheckIfPullRequestCommentExists( string targetRepo, string targetBranch, PullRequest pullRequest, - string expectedComment) + string[] filesInConflict) { IReadOnlyList comments = await GitHubApi.Issue.Comment.GetAllForIssue(TestParameters.GitHubTestOrg, targetRepo, pullRequest.Number); - comments.Should().ContainSingle(comment => comment.Body.Contains(expectedComment)); + var conflictComment = comments.FirstOrDefault(comment => comment.Body.Contains("conflict")); + + foreach (var file in filesInConflict) + { + conflictComment!.Body.Should().Contain(file); + } } } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs index efc60c67cc..650a8fcfbe 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.ServiceModel.Channels; using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.DotNet.ProductConstructionService.Client.Models; using NUnit.Framework; @@ -13,17 +12,6 @@ namespace ProductConstructionService.ScenarioTests.ScenarioTests; [Category("CodeFlow")] internal partial class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase { - private const string CommitPlaceholder = ""; - private const string ConflictMessage = $""" - There was a conflict in the PR branch when flowing source from https://github.com/maestro-auth-test/maestro-test1/tree/{CommitPlaceholder} - Conflicting files: - - {TestFile1Name} - - {TestFile2Name} - - Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged - - """; - [Test] public async Task ConflictPrClosedTest() { @@ -234,11 +222,14 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () TestContext.WriteLine("Waiting for conflict comment to show up on the PR"); pr = await WaitForPullRequestComment(TestRepository.VmrTestRepoName, targetBranchName, "conflict"); - /*await CheckIfPullRequestCommentExists( + await CheckIfPullRequestCommentExists( TestRepository.VmrTestRepoName, targetBranchName, pr, - ConflictMessage.Replace(CommitPlaceholder, repoSha));*/ + [ + $"[{TestFile1Name}](https://github.com/{TestRepository.TestOrg}/{TestRepository.TestRepo1Name}/blob/{repoSha}/{TestFile1Name})", + $"[{TestFile2Name}](https://github.com/{TestRepository.TestOrg}/{TestRepository.TestRepo1Name}/blob/{repoSha}/{TestFile2Name})" + ]); await test(); } From b6df5c2fef082b9e4e43c33b58445112b1ca6569 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> Date: Wed, 29 Jan 2025 17:12:18 +0100 Subject: [PATCH 16/24] Update src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs --- .../PullRequestUpdater.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index e2eda93d5d..ed7055893d 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -233,7 +233,7 @@ protected virtual Task TagSourceRepositoryGitHubContactsIfPossibleAsync(InProgre { _logger.LogInformation("Querying status for pull request {prUrl}", pr.Url); - (var targetRepository, var _) = await GetTargetAsync(); + (var targetRepository, _) = await GetTargetAsync(); var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); PrStatus status; From 1cf035258eea25ba2e0c6fcdd421506176ce1a2b Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> Date: Wed, 29 Jan 2025 17:37:58 +0100 Subject: [PATCH 17/24] Update test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Přemek Vysoký --- .../PullRequestUpdaterTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index 2a94e25969..2a1bf2fd43 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -395,7 +395,7 @@ protected IDisposable WithExistingPullRequest(Build forBuild, PrStatus prStatus, protected IDisposable WithExistingCodeFlowPullRequest( Build forBuild, bool canUpdate, - bool flowerWillHaveConflict = false, + bool newChangeWillConflict = false, bool prAlreadyHasConflict = false, string latestCommitToReturn = ConflictPRRemoteSha) => canUpdate From 63b310ae2d00fcbb9747c01c4e15bbcb0c4f0f1c Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> Date: Wed, 29 Jan 2025 17:38:14 +0100 Subject: [PATCH 18/24] Update test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Přemek Vysoký --- .../CodeFlowScenarioTestBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs index 56b5c2c863..2bb6a4ebdb 100644 --- a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs @@ -199,7 +199,7 @@ public async Task WaitForPullRequestComment( { return pullRequest; } - await Task.Delay(5000); + await Task.Delay(TimeSpan.FromSeconds(5)); } throw new ScenarioTestException($"Comment containing '{partialComment}' was not found in the pull request."); From 6831822ec6180b7179baa05179f8bf5530376682 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> Date: Wed, 29 Jan 2025 17:38:28 +0100 Subject: [PATCH 19/24] Update test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Přemek Vysoký --- .../ScenarioTestBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs index 000f462a1b..e6cf349fe5 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs @@ -1092,6 +1092,6 @@ protected static async Task WaitForNewCommitInPullRequest(string repo, Octokit.P } await Task.Delay(TimeSpan.FromSeconds(20)); } - throw new ScenarioTestException($"The created pull request for repo targeting {pr.Base.Ref} did not have a new commit within {attempts} minutes"); + throw new ScenarioTestException($"The created pull request for repo targeting {pr.Base.Ref} did not have a new commit within {attempts * 20 / 60} minutes"); } } From ac92f0843965c3a886eab0006afdb3350201b733 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> Date: Wed, 29 Jan 2025 17:38:57 +0100 Subject: [PATCH 20/24] Update src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Přemek Vysoký --- .../PullRequestUpdater.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index ed7055893d..ba194f604a 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -951,7 +951,7 @@ await CreateCodeFlowPullRequestAsync( await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow: true); await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow); - return false; + return true; } catch (Exception e) { From a31cd3189e9882708583eeb27afc3840f6c42fec Mon Sep 17 00:00:00 2001 From: dkurepa Date: Wed, 29 Jan 2025 17:59:32 +0100 Subject: [PATCH 21/24] Address feedback --- .../DarcLib/VirtualMonoRepo/Exceptions.cs | 6 ++ .../PullRequestUpdater.cs | 83 ++++++++++++------- .../CodeFlowScenarioTestBase.cs | 4 +- 3 files changed, 62 insertions(+), 31 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs index 9139910b20..a363724a9d 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs @@ -27,6 +27,12 @@ private static string GetExceptionMessage(VmrIngestionPatch patch, ProcessExecut + result; } +/// +/// Exception thrown when the service can't apply an update to the PR branch due to a conflict +/// +/// +/// +/// public class ConflictInPrBranchException(ProcessExecutionResult gitMergeResult, string targetBranch, bool isForwardFlow) : Exception($"Failed to flow changes due to conflicts in the target branch ({targetBranch})") { diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index ba194f604a..535d19b557 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -1,9 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Security.Policy; using System.Text; -using Maestro.Common; using Maestro.Data.Models; using Maestro.MergePolicies; using Maestro.MergePolicyEvaluation; @@ -269,6 +267,7 @@ await AddDependencyFlowEventsAsync( mergePolicyResult, pr.Url); + // If the PR we just merged was in conflict with an update we previously tried to apply, we shouldn't delete the reminder for the update await ClearAllStateAsync(isCodeFlow, clearPendingUpdates: pr.MergeState == InProgressPullRequestState.Mergeable); return PullRequestStatus.Completed; @@ -925,33 +924,10 @@ await CreateCodeFlowPullRequestAsync( } catch (ConflictInPrBranchException conflictException) { - // The PR we're trying to update has a conflict with the source repo. We will mark it as blocked, not allowing any updates from this - // subscription till it's merged, or the conflict resolved. We'll set a reminder to check on it. - StringBuilder sb = new(); - sb.AppendLine($"There was a conflict in the PR branch when flowing source from {update.GetRepoAtCommitUri()}"); - sb.AppendLine("Files conflicting with the head branch:"); - foreach (var file in conflictException.FilesInConflict) - { - sb.AppendLine($" - [{file}]({update.GetFileAtCommitUri(file)})"); - } - sb.AppendLine(); - sb.AppendLine("Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged"); - - (var targetRepository, _) = await GetTargetAsync(); - var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); - - await remote.CommentPullRequestAsync(pr.Url, sb.ToString()); - // If the headBranch gets updated, we will retry to update it with previously conflicting changes. If these changes still cause a conflict, we should update the - // InProgressPullRequest with the latest commit from the remote branch - var remoteCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); - - pr.MergeState = InProgressPullRequestState.Conflict; - pr.SourceSha = remoteCommit; - await _pullRequestState.SetAsync(pr); - await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow: true); - await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow); - - return true; + return await HandlePrUpdateConflictAsync( + conflictException, + update, + pr); } catch (Exception e) { @@ -1224,5 +1200,54 @@ await AddDependencyFlowEventsAsync( } } + private async Task HandlePrUpdateConflictAsync( + ConflictInPrBranchException conflictException, + SubscriptionUpdateWorkItem update, + InProgressPullRequest pr) + { + // The PR we're trying to update has a conflict with the source repo. We will mark it as blocked, not allowing any updates from this + // subscription till it's merged, or the conflict resolved. We'll set a reminder to check on it. + StringBuilder sb = new(); + sb.AppendLine($"There was a conflict in the PR branch when flowing source from {update.GetRepoAtCommitUri()}"); + sb.AppendLine("Files conflicting with the head branch:"); + foreach (var file in conflictException.FilesInConflict) + { + sb.AppendLine($" - [{file}]({update.GetFileAtCommitUri(file)})"); + } + sb.AppendLine(); + sb.AppendLine("Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged"); + + (var targetRepository, _) = await GetTargetAsync(); + var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); + + try + { + await remote.CommentPullRequestAsync(pr.Url, sb.ToString()); + } + catch (Exception e) + { + _logger.LogWarning("Posting comment to {prUrl} failed with exception {message}", pr.Url, e.Message); + } + // If the headBranch gets updated, we will retry to update it with previously conflicting changes. If these changes still cause a conflict, we should update the + // InProgressPullRequest with the latest commit from the remote branch + var remoteCommit = pr.SourceSha; + try + { + remoteCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); + } + catch (Exception e) + { + _logger.LogWarning("Couldn't get latest commit of {repo}/{commit}. Failed with exception {message}", targetRepository, pr.HeadBranch, e.Message); + } + + pr.MergeState = InProgressPullRequestState.Conflict; + pr.SourceSha = remoteCommit; + await _pullRequestState.SetAsync(pr); + await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow: true); + await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow: true); + + return true; + } + #endregion } diff --git a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs index 2bb6a4ebdb..2095bd098b 100644 --- a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs @@ -212,11 +212,11 @@ public async Task CheckIfPullRequestCommentExists( string[] filesInConflict) { IReadOnlyList comments = await GitHubApi.Issue.Comment.GetAllForIssue(TestParameters.GitHubTestOrg, targetRepo, pullRequest.Number); - var conflictComment = comments.FirstOrDefault(comment => comment.Body.Contains("conflict")); + var conflictComment = comments.First(comment => comment.Body.Contains("conflict")); foreach (var file in filesInConflict) { - conflictComment!.Body.Should().Contain(file); + conflictComment.Body.Should().Contain(file); } } } From 6544f92503609386c8d6359dad9878dbc769b8f2 Mon Sep 17 00:00:00 2001 From: dkurepa Date: Thu, 30 Jan 2025 10:30:14 +0100 Subject: [PATCH 22/24] Fix build --- .../PendingCodeFlowUpdatesTests.cs | 2 +- .../PullRequestUpdaterTests.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs index d375cb2341..9c34540710 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs @@ -92,7 +92,7 @@ public async Task PendingUpdatesInConflictWithCurrent() Build newBuild = GivenANewBuild(true); newBuild.Commit = "sha456"; - using (WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true, flowerWillHaveConflict: true)) + using (WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true, newChangeWillConflict: true)) { await WhenProcessPendingUpdatesAsyncIsCalled(newBuild, isCodeFlow: true); diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index 2a1bf2fd43..b7f6785e5e 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -403,14 +403,14 @@ protected IDisposable WithExistingCodeFlowPullRequest( forBuild, PrStatus.Open, null, - flowerWillHaveConflict, + newChangeWillConflict, prAlreadyHasConflict, latestCommitToReturn) : WithExistingCodeFlowPullRequest( forBuild, PrStatus.Open, MergePolicyEvaluationStatus.Pending, - flowerWillHaveConflict, + newChangeWillConflict, prAlreadyHasConflict, latestCommitToReturn); From d3fe00b33bf7c5d09dd3dafec8704bdf03fb9fa8 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> Date: Thu, 30 Jan 2025 10:32:53 +0100 Subject: [PATCH 23/24] Update src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Přemek Vysoký --- src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs index a363724a9d..52ed515ad1 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs @@ -29,6 +29,7 @@ private static string GetExceptionMessage(VmrIngestionPatch patch, ProcessExecut /// /// Exception thrown when the service can't apply an update to the PR branch due to a conflict +/// between the source repo and a change that was made in the PR after it was opened. /// /// /// From 1182062373a1fec62e72e6e0cee698699a6b3f12 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> Date: Thu, 30 Jan 2025 10:33:01 +0100 Subject: [PATCH 24/24] Update src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Přemek Vysoký --- .../DarcLib/VirtualMonoRepo/Exceptions.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs index 52ed515ad1..10a4247867 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs @@ -31,9 +31,6 @@ private static string GetExceptionMessage(VmrIngestionPatch patch, ProcessExecut /// Exception thrown when the service can't apply an update to the PR branch due to a conflict /// between the source repo and a change that was made in the PR after it was opened. /// -/// -/// -/// public class ConflictInPrBranchException(ProcessExecutionResult gitMergeResult, string targetBranch, bool isForwardFlow) : Exception($"Failed to flow changes due to conflicts in the target branch ({targetBranch})") {