diff --git a/arcade-services.slnx b/arcade-services.slnx index 3b9292f533..296f673fcf 100644 --- a/arcade-services.slnx +++ b/arcade-services.slnx @@ -59,6 +59,7 @@ + diff --git a/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs b/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs index 1fbaaead92..54cf7f0b88 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs @@ -66,6 +66,13 @@ public interface IRemote /// Pull request information. Task GetPullRequestAsync(string pullRequestUri); + /// + /// Retrieve the commits of a pull request. + /// + /// URI of pull request. + /// The commits contained in the pull request. + Task> GetPullRequestCommitsAsync(string pullRequestUri); + /// /// Create a new pull request. /// diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs index c5dbeb6765..3f3866d187 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs @@ -339,6 +339,11 @@ public Task GetPullRequestAsync(string pullRequestUri) return _remoteGitClient.GetPullRequestAsync(pullRequestUri); } + public Task> GetPullRequestCommitsAsync(string pullRequestUri) + { + return _remoteGitClient.GetPullRequestCommitsAsync(pullRequestUri); + } + public Task GetPullRequestUrlAsync(string repoUri, string headBranch, string targetBranch) { return _remoteGitClient.GetPullRequestUrlAsync(repoUri, headBranch, targetBranch); diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowExtensions.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowExtensions.cs index a897d6531b..71b757b9b8 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowExtensions.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowExtensions.cs @@ -69,6 +69,7 @@ public static IServiceCollection AddCodeflow( services.TryAddTransient(); services.TryAddTransient(); services.TryAddTransient(); + services.TryAddTransient(); services.TryAddTransient(); services.TryAddTransient(); services.TryAddTransient(); diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs new file mode 100644 index 0000000000..97c8d9d771 --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs @@ -0,0 +1,283 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.DotNet.DarcLib.Helpers; +using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo; +using Microsoft.Extensions.Logging; + +#nullable enable +namespace Microsoft.DotNet.DarcLib.VirtualMonoRepo; + +public interface ICodeflowSourceDiffVerifier +{ + /// + /// Verifies that a forward-flow codeflow PR (source repo -> VMR) faithfully contains the source + /// repo's commit diff (oldSha...newSha), accounting for the expected divergences (path remap, + /// excludes, eng/common, version files, no-ops). + /// + Task ForwardFlowMatchesSourceDiffAsync( + string sourceRepoUri, + string vmrUri, + string mappingName, + string oldSha, + string newSha, + string vmrTargetBranch, + string vmrHeadBranch, + CancellationToken cancellationToken = default); +} + +/// +/// Verifies that a forward-flow codeflow PR faithfully reflects the source repo's commit diff, +/// accounting for the expected, legitimate divergences between a source repo and its VMR copy +/// (path remap, cloaked/excluded paths, eng/common, version/metadata files and no-ops). +/// +public class CodeflowSourceDiffVerifier : ICodeflowSourceDiffVerifier +{ + private readonly IVmrCloneManager _vmrCloneManager; + private readonly IRepositoryCloneManager _cloneManager; + private readonly IVmrDependencyTracker _dependencyTracker; + private readonly ISourceManifest _sourceManifest; + private readonly ILogger _logger; + + public CodeflowSourceDiffVerifier( + IVmrCloneManager vmrCloneManager, + IRepositoryCloneManager cloneManager, + IVmrDependencyTracker dependencyTracker, + ISourceManifest sourceManifest, + ILogger logger) + { + _vmrCloneManager = vmrCloneManager; + _cloneManager = cloneManager; + _dependencyTracker = dependencyTracker; + _sourceManifest = sourceManifest; + _logger = logger; + } + + public async Task ForwardFlowMatchesSourceDiffAsync( + string sourceRepoUri, + string vmrUri, + string mappingName, + string oldSha, + string newSha, + string vmrTargetBranch, + string vmrHeadBranch, + CancellationToken cancellationToken = default) + { + _logger.LogInformation( + "Verifying forward flow PR for {mappingName} against source diff {oldSha}...{newSha}", + mappingName, + oldSha, + newSha); + + var srcMappingPath = VmrInfo.GetRelativeRepoSourcesPath(mappingName); + + ILocalGitRepo vmr = await _vmrCloneManager.PrepareVmrAsync( + [vmrUri], + [vmrTargetBranch, vmrHeadBranch], + vmrHeadBranch, + resetToRemote: true, + cancellationToken); + + SourceMapping mapping = _dependencyTracker.GetMapping(mappingName); + + var exclusionPathspecs = GetDiffFilters(mapping, _sourceManifest); + + ILocalGitRepo sourceRepo = await _cloneManager.PrepareCloneAsync( + mapping, + [sourceRepoUri], + [oldSha, newSha], + newSha, + resetToRemote: false, + cancellationToken); + + var srcMappingPrefix = srcMappingPath + "/"; + HashSet sourceRepoChangedFiles = await GetChangedMappingFilesAsync( + sourceRepo, mappingName, oldSha, newSha, exclusionPathspecs: exclusionPathspecs, cancellationToken: cancellationToken); + HashSet vmrPrChangedFiles = await GetChangedMappingFilesAsync( + vmr, mappingName, vmrTargetBranch, vmrHeadBranch, relativePath: srcMappingPrefix, cancellationToken: cancellationToken); + + var filesChangedInBoth = sourceRepoChangedFiles.Where(vmrPrChangedFiles.Contains).ToList(); + var sourceRepoOnlyChanges = sourceRepoChangedFiles.Where(f => !vmrPrChangedFiles.Contains(f)).ToList(); + var filesChangedInPrOnly = vmrPrChangedFiles.Where(f => !sourceRepoChangedFiles.Contains(f)).ToList(); + + if (filesChangedInPrOnly.Count > 0) + { + _logger.LogInformation( + "Source diff verification for {mappingName} failed: {unexpected} file(s) changed in the PR but not in the source diff", + mappingName, + filesChangedInPrOnly.Count); + return false; + } + + // Per-file content compare on the intersection. + foreach (var file in filesChangedInBoth) + { + if (!await ChangedLinesMatchAsync(sourceRepo, vmr, file, srcMappingPath, oldSha, newSha, vmrTargetBranch, vmrHeadBranch, cancellationToken)) + { + _logger.LogInformation( + "Source diff verification for {mappingName} failed: changes to {file} don't match the source diff", + mappingName, + file); + return false; + } + } + + // No-op check on files the source changed but the PR did not. + foreach (var file in sourceRepoOnlyChanges) + { + if (!await IsLegitimateNoOpAsync(sourceRepo, vmr, file, srcMappingPath, newSha, vmrHeadBranch)) + { + _logger.LogInformation( + "Source diff verification for {mappingName} failed: {file} changed in the source diff but is not reflected in the PR", + mappingName, + file); + return false; + } + } + + _logger.LogInformation("Source diff verification for {mappingName} passed", mappingName); + return true; + } + + /// + /// Builds the git pathspec exclusion rules the same way VmrDiffOperation.GetDiffFilters does: + /// the mapping's excludes plus submodule paths under the mapping, turned into git exclusion rules. + /// + private static List GetDiffFilters(SourceMapping mapping, ISourceManifest manifest) + { + var submodules = manifest.Submodules + .Where(s => s.Path.StartsWith(mapping.Name + '/', StringComparison.OrdinalIgnoreCase)) + .Select(s => s.Path.Substring(mapping.Name.Length + 1)); + + return (mapping.Exclude ?? []) + .Concat(submodules) + .Select(VmrPatchHandler.GetExclusionRule) + .ToList(); + } + + /// + /// Runs a three-dot name-only diff (...) scoped to the + /// mapping's location in the repo and returns the mapping-relative paths, after dropping eng/common + /// (non-arcade) and version/metadata files. + /// + private static async Task> GetChangedMappingFilesAsync( + ILocalGitRepo repo, + string mappingName, + string fromRef, + string toRef, + string? relativePath = null, + IReadOnlyCollection? exclusionPathspecs = null, + CancellationToken cancellationToken = default) + { + IReadOnlyCollection pathspecs = string.IsNullOrEmpty(relativePath) + ? [".", .. exclusionPathspecs ?? []] + : [relativePath, .. exclusionPathspecs ?? []]; + + var result = await repo.ExecuteGitCommand( + ["diff", "--name-only", $"{fromRef}...{toRef}", "--", .. pathspecs], + cancellationToken); + result.ThrowIfFailed($"Failed to get the diff between {fromRef} and {toRef}"); + + IEnumerable files = result.GetOutputLines(); + if (!string.IsNullOrEmpty(relativePath)) + { + files = files + .Select(f => f.Substring(relativePath.Length)); + } + + return FilterMappingFiles(files, mappingName); + } + + private static HashSet FilterMappingFiles(IEnumerable files, string mappingName) + { + var engCommonPrefix = Constants.CommonScriptFilesPath + "/"; + var dropEngCommon = mappingName != VmrInfo.ArcadeMappingName; + + return files + .Where(f => !DependencyFileManager.CodeflowDependencyFiles.Contains(f, StringComparer.OrdinalIgnoreCase)) + .Where(f => !dropEngCommon || !f.StartsWith(engCommonPrefix, StringComparison.OrdinalIgnoreCase)) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + } + + /// + /// Compares the zero-context change lines of a single file between the source diff and the PR. + /// Lines that belong to the diff format (and are expected to differ) are ignored before comparing. + /// + private static async Task ChangedLinesMatchAsync( + ILocalGitRepo sourceRepo, + ILocalGitRepo vmr, + string file, + UnixPath srcMappingPath, + string oldSha, + string newSha, + string vmrTargetBranch, + string vmrHeadBranch, + CancellationToken cancellationToken) + { + var sourceResult = await sourceRepo.ExecuteGitCommand(["diff", "-U0", $"{oldSha}...{newSha}", "--", file], cancellationToken); + sourceResult.ThrowIfFailed($"Failed to get the source diff of {file} between {oldSha} and {newSha}"); + + var vmrResult = await vmr.ExecuteGitCommand(["diff", "-U0", $"{vmrTargetBranch}...{vmrHeadBranch}", "--", srcMappingPath / file], cancellationToken); + vmrResult.ThrowIfFailed($"Failed to get the VMR diff of {file} between {vmrTargetBranch} and {vmrHeadBranch}"); + + var sourceChanges = GetChangeLines(sourceResult.GetOutputLines()); + var vmrChanges = GetChangeLines(vmrResult.GetOutputLines()); + + return sourceChanges.SequenceEqual(vmrChanges); + } + + /// + /// Keeps only the +/- change lines from a zero-context diff. Only lines inside a hunk (after an + /// "@@" header) are collected, so the per-file "--- a/file" / "+++ b/file" headers - which share a + /// prefix with genuine content lines such as a removed "-- comment" (rendered as "--- comment") - are + /// excluded structurally rather than by an ambiguous textual prefix match. + /// + private static List GetChangeLines(IReadOnlyCollection lines) + { + var changeLines = new List(); + var insideHunk = false; + + foreach (var line in lines) + { + if (line.StartsWith("@@")) + { + // start of code changes. we want to start adding lines after this, but not the hunk header itself + insideHunk = true; + } + else if (insideHunk && (line.StartsWith('+') || line.StartsWith('-'))) + { + changeLines.Add(line); + } + } + + return changeLines; + } + + /// + /// A file the source changed but the PR did not is only legitimate when the VMR copy is already + /// at the source's new state (equal content, or both absent for an already-reconciled deletion). + /// + private static async Task IsLegitimateNoOpAsync( + ILocalGitRepo sourceRepo, + ILocalGitRepo vmr, + string file, + UnixPath srcMappingPath, + string newSha, + string vmrHeadBranch) + { + var sourceContent = await sourceRepo.GetFileFromGitAsync(file, newSha); + var vmrContent = await vmr.GetFileFromGitAsync(srcMappingPath / file, vmrHeadBranch); + + if (sourceContent == null && vmrContent == null) + { + return true; + } + + return string.Equals(sourceContent, vmrContent, StringComparison.Ordinal); + } +} diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs index ea8895063b..3693117cae 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs @@ -37,5 +37,6 @@ public static void AddDependencyFlowProcessors(this IServiceCollection services) services.AddWorkItemProcessor(); services.AddWorkItemProcessor(); services.AddWorkItemProcessor(); + services.AddWorkItemProcessor(); } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs index 8346a16d23..9087e41055 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs @@ -58,6 +58,12 @@ internal interface IPullRequestStateManager #endregion + #region Codeflow approval check + + Task SetCodeflowApprovalCheck(CodeflowApprovalCheck check); + + #endregion + #region Bulk cleanup /// diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs index 647d0e205c..d7497af100 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs @@ -21,6 +21,7 @@ internal class PullRequestStateManager : IPullRequestStateManager private readonly IRedisCache _mergePolicyEvaluationState; private readonly IReminderManager _pullRequestUpdateReminders; private readonly IReminderManager _pullRequestCheckReminders; + private readonly IReminderManager _codeflowApprovalCheckReminders; public PullRequestStateManager( IPullRequestTarget pullRequestTarget, @@ -33,6 +34,7 @@ public PullRequestStateManager( _mergePolicyEvaluationState = cacheFactory.Create(cacheKey); _pullRequestUpdateReminders = reminderManagerFactory.CreateReminderManager(cacheKey); _pullRequestCheckReminders = reminderManagerFactory.CreateReminderManager(cacheKey); + _codeflowApprovalCheckReminders = reminderManagerFactory.CreateReminderManager(cacheKey); } public Task GetInProgressPullRequestAsync() => @@ -93,6 +95,7 @@ public async Task ClearAllStateAsync(bool isCodeFlow, bool clearPendingUpdates) { await _pullRequestState.TryDeleteAsync(); await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow); + await _codeflowApprovalCheckReminders.UnsetReminderAsync(isCodeFlow); if (clearPendingUpdates) { await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow); @@ -106,4 +109,7 @@ public async Task ScheduleUpdateForLater(InProgressPullRequest pr, SubscriptionU pr.NextBuildsToProcess[update.SubscriptionId] = update.BuildId; await SetInProgressPullRequestAsync(pr); } + + public async Task SetCodeflowApprovalCheck(CodeflowApprovalCheck check) => + await _codeflowApprovalCheckReminders.SetReminderAsync(check, PullRequestUpdater.DefaultReminderDelay, isCodeFlow: true); } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs index 78457a2cfc..0555410552 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs @@ -1,8 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using Maestro.Common; using Maestro.Common.Telemetry; +using Maestro.Data.Models; using Maestro.DataProviders; using Maestro.MergePolicies; using Maestro.WorkItems; @@ -34,6 +34,7 @@ internal class CodeFlowPullRequestUpdater : PullRequestUpdater private readonly ICommentCollector _commentCollector; private readonly IPullRequestStateManager _stateManager; private readonly ISubscriptionEventRecorder _subscriptionEventRecorder; + private readonly ICodeflowSourceDiffVerifier _codeflowSourceDiffVerifier; private readonly ISubscriptionUpdateOutcomeRecorder _outcomeRecorder; private readonly IPullRequestTarget _target; private readonly ILogger _logger; @@ -53,6 +54,7 @@ public CodeFlowPullRequestUpdater( IPullRequestCommenter pullRequestCommenter, IPullRequestStateManager stateManager, ISubscriptionEventRecorder subscriptionEventRecorder, + ICodeflowSourceDiffVerifier codeflowSourceDiffVerifier, ISubscriptionUpdateOutcomeRecorder outcomeRecorder, ILogger logger) : base(target, mergePolicyEvaluator, remoteFactory, sqlClient, pullRequestCommenter, stateManager, subscriptionEventRecorder, outcomeRecorder, logger) @@ -69,6 +71,7 @@ public CodeFlowPullRequestUpdater( _logger = logger; _stateManager = stateManager; _subscriptionEventRecorder = subscriptionEventRecorder; + _codeflowSourceDiffVerifier = codeflowSourceDiffVerifier; _outcomeRecorder = outcomeRecorder; _target = target; } @@ -91,7 +94,7 @@ protected override async Task ProcessSubscriptionUpdat await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); return new SubscriptionUpdateResult( $"The existing PR is already up to date with source repo (commit {update.SourceSha})", - Maestro.Data.Models.SubscriptionOutcomeType.NoUpdate); + SubscriptionOutcomeType.NoUpdate); } if (pr?.BlockedFromFutureUpdates == true && !forceUpdate) @@ -104,7 +107,7 @@ protected override async Task ProcessSubscriptionUpdat await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); return new SubscriptionUpdateResult( "The existing codeflow PR is currently blocked from future updates", - Maestro.Data.Models.SubscriptionOutcomeType.NotUpdatable); + SubscriptionOutcomeType.NotUpdatable); } var subscription = await _sqlClient.GetSubscriptionAsync(update.SubscriptionId); @@ -139,7 +142,7 @@ protected override async Task ProcessSubscriptionUpdat await HandleBlockingCodeflowException(pr); return new SubscriptionUpdateResult( e.Message, - Maestro.Data.Models.SubscriptionOutcomeType.NotUpdatable); + SubscriptionOutcomeType.NotUpdatable); } if (!codeFlowRes.HadUpdates) @@ -147,7 +150,7 @@ protected override async Task ProcessSubscriptionUpdat var msg = pr != null ? "No source code updates detected" : "Codeflow PR not created: no source code updates detected"; - return new SubscriptionUpdateResult(msg, Maestro.Data.Models.SubscriptionOutcomeType.NoUpdate); + return new SubscriptionUpdateResult(msg, SubscriptionOutcomeType.NoUpdate); } if (isForwardFlow) @@ -185,7 +188,7 @@ protected override async Task ProcessSubscriptionUpdat isUnsafeFlow); return new SubscriptionUpdateResult( "Conflict resolution is required by user", - Maestro.Data.Models.SubscriptionOutcomeType.HasConflict); + SubscriptionOutcomeType.HasConflict); } string? oldPrUrl = null; @@ -216,7 +219,7 @@ protected override async Task ProcessSubscriptionUpdat return new SubscriptionUpdateResult( "New codeflow PR created", - Maestro.Data.Models.SubscriptionOutcomeType.Updated); + SubscriptionOutcomeType.Updated); } else { @@ -236,7 +239,7 @@ await UpdateCodeFlowPullRequestAsync( return new SubscriptionUpdateResult( string.Empty, - Maestro.Data.Models.SubscriptionOutcomeType.Updated); + SubscriptionOutcomeType.Updated); } } @@ -265,7 +268,7 @@ await UpdateCodeFlowPullRequestAsync( try { - codeFlowRes = await InvokeFlowAsync(subscription, build, pr, prHeadBranch, forceUpdate, unsafeFlow: false); + codeFlowRes = await InvokeFlowAsync(subscription, build, prHeadBranch, forceUpdate, unsafeFlow: false); } catch (NonLinearCodeflowException e) { @@ -290,7 +293,7 @@ await UpdateCodeFlowPullRequestAsync( subscription.TargetBranch, prHeadBranch); - codeFlowRes = await InvokeFlowAsync(subscription, build, pr, prHeadBranch, forceUpdate, unsafeFlow: true); + codeFlowRes = await InvokeFlowAsync(subscription, build, prHeadBranch, forceUpdate, unsafeFlow: true); } if (codeFlowRes.HadConflicts) @@ -329,7 +332,6 @@ await UpdateCodeFlowPullRequestAsync( private async Task InvokeFlowAsync( SubscriptionDTO subscription, BuildDTO build, - InProgressPullRequest? pr, string branch, bool forceUpdate, bool unsafeFlow) @@ -527,7 +529,8 @@ private async Task RequestManualConflictResolutionAsync( prHeadBranch, codeFlowResult.DependencyUpdates, upstreamRepoDiffs, - unsafeFlown); + unsafeFlown, + skipCodeflowApprovalCheck: true); } else { @@ -557,7 +560,8 @@ await UpdateCodeFlowPullRequestAsync( previousSourceSha, subscription, codeFlowResult.DependencyUpdates, - upstreamRepoDiffs); + upstreamRepoDiffs, + skipCodeflowApprovalCheck: true); // Since we changed the PR state in cache but no commit was pushed, // we need to delete non-transient check results so that they can be re-evaluated @@ -629,7 +633,8 @@ private async Task CreateEmptyPrBranch( string prBranch, List dependencyUpdates, IReadOnlyCollection? upstreamRepoDiffs, - bool unsafeFlow) + bool unsafeFlow, + bool skipCodeflowApprovalCheck = false) { IRemote darcRemote = await _remoteFactory.CreateRemoteAsync(subscription.TargetRepository); var build = await _sqlClient.GetBuildAsync(update.BuildId); @@ -692,6 +697,19 @@ await _subscriptionEventRecorder.AddDependencyFlowEventsAsync( inProgressPr.LastUpdate = DateTime.UtcNow; await _stateManager.SetCheckReminderAsync(inProgressPr, pr, isCodeFlow: true); await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); + if (!skipCodeflowApprovalCheck + && !string.IsNullOrEmpty(previousSourceSha) + && subscription.IsForwardFlow()) + { + await _stateManager.SetCodeflowApprovalCheck(new CodeflowApprovalCheck + { + UpdaterId = _target.UpdaterId, + SubscriptionId = update.SubscriptionId, + PreviousSourceSha = previousSourceSha, + CurrentSourceSha = update.SourceSha, + PullRequestUrl = pr.Url + }); + } _outcomeRecorder.SetPullRequestUrl(inProgressPr.Url); _logger.LogInformation("Code flow pull request created: {prUrl}", pr.Url); @@ -726,7 +744,8 @@ private async Task UpdateCodeFlowPullRequestAsync( string? previousSourceSha, SubscriptionDTO subscription, List newDependencyUpdates, - IReadOnlyCollection? upstreamRepoDiffs) + IReadOnlyCollection? upstreamRepoDiffs, + bool skipCodeflowApprovalCheck = false) { IRemote remote = await _remoteFactory.CreateRemoteAsync(subscription.TargetRepository); var build = await _sqlClient.GetBuildAsync(update.BuildId); @@ -790,6 +809,100 @@ private async Task UpdateCodeFlowPullRequestAsync( pullRequest.BlockedFromFutureUpdates = false; // if a sub is blocked, and someone force triggers it, we can continue flowing afterwards await _stateManager.SetCheckReminderAsync(pullRequest, prInfo!, isCodeFlow: true); await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true); + if (!skipCodeflowApprovalCheck + && !string.IsNullOrEmpty(previousSourceSha) + && subscription.IsForwardFlow()) + { + await _stateManager.SetCodeflowApprovalCheck(new CodeflowApprovalCheck + { + UpdaterId = _target.UpdaterId, + SubscriptionId = update.SubscriptionId, + PreviousSourceSha = previousSourceSha, + CurrentSourceSha = update.SourceSha, + PullRequestUrl = pullRequest.Url + }); + } + } + } + + public async Task RunCodeflowApprovalCheckAsync( + SubscriptionDTO subscription, + CodeflowApprovalCheck codeflowApprovalCheck, + CancellationToken cancellationToken) + { + if (subscription.IsBackflow()) + { + _logger.LogError("Can't run codeflow approval check on backflow subscriptions"); + return; + } + + var pr = await _stateManager.GetInProgressPullRequestAsync(); + + if (pr == null) + { + _logger.LogError("No in-progress PR found for codeflow approval check"); + return; + } + + if (pr.Url != codeflowApprovalCheck.PullRequestUrl) + { + _logger.LogInformation( + "Codeflow approval check PR URL {codeflowPrUrl} does not match in-progress PR URL {inProgressPrUrl}", + codeflowApprovalCheck.PullRequestUrl, + pr.Url); + return; + } + + var remote = await _remoteFactory.CreateRemoteAsync(subscription.TargetRepository); + var prInfo = await remote.GetPullRequestAsync(pr.Url); + + if (prInfo.Status != PrStatus.Open) + { + _logger.LogInformation( + "Skipping codeflow approval check for PR {prUrl} because it is no longer open (status: {status})", + pr.Url, + prInfo.Status); + return; + } + + var commits = await remote.GetPullRequestCommitsAsync(pr.Url); + var nonBotCommits = commits + .Where(commit => !string.Equals(commit.Author, Constants.DarcBotName, StringComparison.Ordinal)) + .ToList(); + if (nonBotCommits.Count > 0) + { + _logger.LogInformation( + "Skipping codeflow approval check for PR {prUrl} because it contains {count} commit(s) not authored by {bot}", + pr.Url, + nonBotCommits.Count, + Constants.DarcBotName); + return; + } + + bool match = await _codeflowSourceDiffVerifier.ForwardFlowMatchesSourceDiffAsync( + subscription.SourceRepository, + subscription.TargetRepository, + subscription.TargetDirectory, + codeflowApprovalCheck.PreviousSourceSha, + codeflowApprovalCheck.CurrentSourceSha, + subscription.TargetBranch, + pr.HeadBranch, + cancellationToken); + if (match) + { + _logger.LogInformation( + "Codeflow approval check for PR {prUrl} passed; posting approval comment", + pr.Url); + await remote.CommentPullRequestAsync( + pr.Url, + $"This pull request only contains source updates from {subscription.SourceRepository} " + + $"between commits {codeflowApprovalCheck.PreviousSourceSha} and {codeflowApprovalCheck.CurrentSourceSha}."); + } + else + { + _logger.LogInformation( + "Forward flow PR {prUrl} contains unexpected changes, not auto approving", + pr.Url); } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs new file mode 100644 index 0000000000..95954611b4 --- /dev/null +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs @@ -0,0 +1,61 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Maestro.Data; +using Maestro.DataProviders; +using Maestro.Services.Common.Cache; +using Maestro.WorkItems; +using Microsoft.EntityFrameworkCore; +using ProductConstructionService.DependencyFlow.Model; +using ProductConstructionService.DependencyFlow.PullRequestUpdaters; +using ProductConstructionService.DependencyFlow.WorkItems; + +namespace ProductConstructionService.DependencyFlow.WorkItemProcessors; + +internal class CodeflowApprovalCheckProcessor : WorkItemProcessor +{ + private readonly BuildAssetRegistryContext _context; + private readonly IPullRequestUpdaterFactory _updaterFactory; + private readonly IDistributedLock _distributedLock; + + public CodeflowApprovalCheckProcessor( + BuildAssetRegistryContext context, + IPullRequestUpdaterFactory updaterFactory, + IDistributedLock distributedLock) + { + _context = context; + _updaterFactory = updaterFactory; + _distributedLock = distributedLock; + } + + public override async Task ProcessWorkItemAsync(CodeflowApprovalCheck workItem, CancellationToken cancellationToken) + { + var subscription = await _context.Subscriptions + .Include(s => s.Channel) + .FirstOrDefaultAsync(s => s.Id == workItem.SubscriptionId, cancellationToken); + + if (subscription == null) + { + throw new InvalidOperationException($"Subscription with ID {workItem.SubscriptionId} not found."); + } + + var pullRequestUpdaterId = PullRequestUpdaterId.CreateUpdaterId(subscription); + var pullRequestUpdater = _updaterFactory.CreatePullRequestUpdater(pullRequestUpdaterId); + + if (pullRequestUpdater is not CodeFlowPullRequestUpdater codeFlowUpdater) + { + throw new InvalidOperationException($"Subscription {subscription.Id} is not a source enabled subscription, cannot run codeflow approval check"); + } + + var mutexKey = pullRequestUpdaterId.Id; + + await _distributedLock.ExecuteWithLockAsync(mutexKey, + async () => await codeFlowUpdater.RunCodeflowApprovalCheckAsync( + SqlBarClient.ToClientModelSubscription(subscription), + workItem, + cancellationToken), + cancellationToken: cancellationToken); + + return true; + } +} diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs new file mode 100644 index 0000000000..923124c9ec --- /dev/null +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs @@ -0,0 +1,12 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace ProductConstructionService.DependencyFlow.WorkItems; + +internal class CodeflowApprovalCheck : DependencyFlowWorkItem +{ + public required Guid SubscriptionId { get; init; } + public required string PullRequestUrl { get; init; } + public required string PreviousSourceSha { get; init; } + public required string CurrentSourceSha { get; init; } +} diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs index ce5c228c2c..da6922824c 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs @@ -287,6 +287,30 @@ protected async Task CallForwardflow( protected IReadOnlyList GetLastFlowCollectedComments() => _lastFlowCollectedComments; + /// + /// Runs the source-diff verification (the same check PCS runs after a forward-flow push) + /// against the current state of the VMR PR branch and returns whether the PR faithfully + /// reflects the source repo's diff (...). + /// The PR branch must be committed before calling this. + /// + protected async Task VerifyForwardFlowSourceDiff(string branchName, string oldSha, string newSha) + { + using var scope = ServiceProvider.CreateScope(); + var cloneManager = scope.ServiceProvider.GetRequiredService(); + await cloneManager.RegisterCloneAsync(VmrPath); + + var verifier = scope.ServiceProvider.GetRequiredService(); + return await verifier.ForwardFlowMatchesSourceDiffAsync( + sourceRepoUri: ProductRepoPath, + vmrUri: VmrPath, + mappingName: Constants.ProductRepoName, + oldSha: oldSha, + newSha: newSha, + vmrTargetBranch: "main", + vmrHeadBranch: branchName, + cancellationToken: _cancellationToken.Token); + } + protected async Task> CallDarcCloakedFileScan(string baselinesFilePath) { using var scope = ServiceProvider.CreateScope(); diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowUpdatingPRsTests.cs b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowUpdatingPRsTests.cs index 556fe91e7f..e6326873d8 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowUpdatingPRsTests.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowUpdatingPRsTests.cs @@ -26,10 +26,14 @@ public async Task ForwardFlowWithChangesInThePrBranchTest() const string backflowBranch = nameof(ForwardFlowWithChangesInThePrBranchTest) + "-bf"; const string forwardFlowBranch = nameof(ForwardFlowWithChangesInThePrBranchTest) + "-ff"; + // The source commit currently reflected in the VMR's main branch + var previousSourceSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); + // Make changes in the product repo await GitOperations.Checkout(ProductRepoPath, "main"); await File.WriteAllTextAsync(ProductRepoPath / "repo.txt", "New file in the repo"); await GitOperations.CommitAll(ProductRepoPath, "New file in the repo"); + var firstFlowSourceSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); // Make changes in the VMR await GitOperations.Checkout(VmrPath, "main"); @@ -49,6 +53,9 @@ public async Task ForwardFlowWithChangesInThePrBranchTest() await GitOperations.CommitAll(VmrPath, "Forward flow commit"); + // The freshly opened FF PR should faithfully reflect the source diff + (await VerifyForwardFlowSourceDiff(forwardFlowBranch, previousSourceSha, firstFlowSourceSha)).Should().BeTrue(); + // 3. Make some changes in the forward flow PR branch await GitOperations.Checkout(VmrPath, forwardFlowBranch); await File.WriteAllTextAsync(_productRepoVmrPath / "repo.txt", "Updated file in the PR"); @@ -58,11 +65,16 @@ public async Task ForwardFlowWithChangesInThePrBranchTest() await GitOperations.MergePrBranch(ProductRepoPath, backflowBranch); // 5. Flow the changes from the repo into the VMR PR + var secondFlowSourceSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); result = await CallForwardflow(Constants.ProductRepoName, ProductRepoPath, forwardFlowBranch); result.ShouldHaveUpdates(); await GitOperations.CommitAll(VmrPath, "Forward flow update", allowEmpty: true); + // The PR now carries an extra manual change (repo.txt) that the source diff doesn't contain, + // so the verification should fail + (await VerifyForwardFlowSourceDiff(forwardFlowBranch, firstFlowSourceSha, secondFlowSourceSha)).Should().BeFalse(); + // Check that the changes in the PR branch are preserved var prFileContent = await File.ReadAllTextAsync(_productRepoVmrPath / "repo.txt"); prFileContent.Should().Be("Updated file in the PR", "The changes in the PR branch should be preserved after the backflow merge."); diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs new file mode 100644 index 0000000000..e81af8dccf --- /dev/null +++ b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs @@ -0,0 +1,289 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using AwesomeAssertions; +using Microsoft.DotNet.DarcLib.Helpers; +using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo; +using Microsoft.DotNet.DarcLib.VirtualMonoRepo; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using NUnit.Framework; + +namespace Microsoft.DotNet.DarcLib.Codeflow.Tests; + +/// +/// Unit tests for . The git/VMR +/// interactions are mocked so each test drives the public verifier with hand-crafted +/// "source repo" and "VMR PR" diffs and asserts the verdict. +/// +[TestFixture] +internal class CodeflowSourceDiffVerifierTests +{ + private const string MappingName = "product-repo1"; + private const string SourceRepoUri = "https://github.com/dotnet/product-repo1"; + private const string VmrUri = "https://github.com/dotnet/dotnet"; + private const string OldSha = "aaaaaaa"; + private const string NewSha = "bbbbbbb"; + private const string VmrTargetBranch = "main"; + private const string VmrHeadBranch = "pr-branch"; + + // VmrInfo.GetRelativeRepoSourcesPath(MappingName) == "src/product-repo1". + private const string SrcMappingPath = "src/" + MappingName; + private const string VmrPrefix = SrcMappingPath + "/"; + + private Mock _vmrCloneManager = null!; + private Mock _cloneManager = null!; + private Mock _dependencyTracker = null!; + private Mock _sourceManifest = null!; + private Mock _sourceRepo = null!; + private Mock _vmr = null!; + private CodeflowSourceDiffVerifier _verifier = null!; + + [SetUp] + public void SetUp() + { + _sourceRepo = new Mock(); + _vmr = new Mock(); + + var mapping = new SourceMapping( + Name: MappingName, + DefaultRemote: SourceRepoUri, + DefaultRef: "main", + Include: [], + Exclude: [], + DisableSynchronization: false); + + _dependencyTracker = new Mock(); + _dependencyTracker.Setup(t => t.GetMapping(MappingName)).Returns(mapping); + + _sourceManifest = new Mock(); + _sourceManifest.Setup(m => m.Submodules).Returns([]); + + _vmrCloneManager = new Mock(); + _vmrCloneManager + .Setup(m => m.PrepareVmrAsync( + It.IsAny>(), + It.IsAny>(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync(() => _vmr.Object); + + _cloneManager = new Mock(); + _cloneManager + .Setup(m => m.PrepareCloneAsync( + It.IsAny(), + It.IsAny>(), + It.IsAny>(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync(() => _sourceRepo.Object); + + _verifier = new CodeflowSourceDiffVerifier( + _vmrCloneManager.Object, + _cloneManager.Object, + _dependencyTracker.Object, + _sourceManifest.Object, + NullLogger.Instance); + } + + private Task VerifyAsync() => _verifier.ForwardFlowMatchesSourceDiffAsync( + SourceRepoUri, + VmrUri, + MappingName, + OldSha, + NewSha, + VmrTargetBranch, + VmrHeadBranch, + CancellationToken.None); + + /// + /// Configures a mocked repo's "git diff" calls. is returned for the + /// "diff --name-only" call (the changed file listing); maps the file argument + /// of a "diff -U0 ... -- <file>" call to that file's zero-context diff text. + /// + private static void SetupDiffs( + Mock repo, + string nameOnlyOutput, + IReadOnlyDictionary fileDiffs) + { + repo.Setup(r => r.ExecuteGitCommand(It.IsAny(), It.IsAny())) + .Returns((string[] args, CancellationToken _) => + { + var output = args.Contains("--name-only") + ? nameOnlyOutput + : fileDiffs.GetValueOrDefault(args[^1], string.Empty); + + return Task.FromResult(new ProcessExecutionResult { ExitCode = 0, StandardOutput = output }); + }); + } + + /// + /// Builds a realistic "git diff -U0" output for a single-line change of . + /// The removed/added content lines are passed verbatim (already including any leading characters of the + /// actual file content, e.g. "-- old" for a SQL comment). + /// + private static string SingleLineDiff(string path, string removedLine, string addedLine) => string.Join('\n', + [ + $"diff --git a/{path} b/{path}", + "index 1111111..2222222 100644", + $"--- a/{path}", + $"+++ b/{path}", + "@@ -1 +1 @@", + "-" + removedLine, + "+" + addedLine, + ]); + + /// + /// Builds a "git diff -U0" output for a file changed in two separate places. With zero context each + /// change region is its own hunk, so this exercises multi-hunk parsing. + /// + private static string TwoHunkDiff( + string path, + (string Removed, string Added) firstHunk, + (string Removed, string Added) secondHunk) => string.Join('\n', + [ + $"diff --git a/{path} b/{path}", + "index 1111111..3333333 100644", + $"--- a/{path}", + $"+++ b/{path}", + "@@ -1 +1 @@", + "-" + firstHunk.Removed, + "+" + firstHunk.Added, + "@@ -5 +5 @@", + "-" + secondHunk.Removed, + "+" + secondHunk.Added, + ]); + + [Test] + public async Task ForwardFlowMatchesSourceDiffAsync_PrMatchesSourceDiff_ReturnsTrue() + { + // Arrange: the source repo and the VMR PR change the same file in the same way. + SetupDiffs(_sourceRepo, "data.txt", new Dictionary + { + ["data.txt"] = SingleLineDiff("data.txt", "old content", "new content"), + }); + SetupDiffs(_vmr, VmrPrefix + "data.txt", new Dictionary + { + [VmrPrefix + "data.txt"] = SingleLineDiff(VmrPrefix + "data.txt", "old content", "new content"), + }); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeTrue(); + } + + [Test] + public async Task ForwardFlowMatchesSourceDiffAsync_PrChangesFileNotInSourceDiff_ReturnsFalse() + { + // Arrange: the PR touches an extra file the source diff never changed. + SetupDiffs(_sourceRepo, "data.txt", new Dictionary + { + ["data.txt"] = SingleLineDiff("data.txt", "old content", "new content"), + }); + SetupDiffs(_vmr, string.Join('\n', VmrPrefix + "data.txt", VmrPrefix + "extra.txt"), new Dictionary()); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeFalse(); + } + + [Test] + public async Task ForwardFlowMatchesSourceDiffAsync_PrChangesFileDifferently_ReturnsFalse() + { + // Arrange: the same file is changed in both, but to different content. + SetupDiffs(_sourceRepo, "data.txt", new Dictionary + { + ["data.txt"] = SingleLineDiff("data.txt", "old content", "new content"), + }); + SetupDiffs(_vmr, VmrPrefix + "data.txt", new Dictionary + { + [VmrPrefix + "data.txt"] = SingleLineDiff(VmrPrefix + "data.txt", "old content", "tampered content"), + }); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeFalse(); + } + + [Test] + public async Task ForwardFlowMatchesSourceDiffAsync_SourceOnlyChangeAlreadyReconciledInVmr_ReturnsTrue() + { + // Arrange: the source changed two files but the PR only changed one. The other (readme.md) is a + // legitimate no-op because the VMR copy already holds the source's new content. + SetupDiffs(_sourceRepo, string.Join('\n', "data.txt", "readme.md"), new Dictionary + { + ["data.txt"] = SingleLineDiff("data.txt", "old content", "new content"), + }); + SetupDiffs(_vmr, VmrPrefix + "data.txt", new Dictionary + { + [VmrPrefix + "data.txt"] = SingleLineDiff(VmrPrefix + "data.txt", "old content", "new content"), + }); + + _sourceRepo.Setup(r => r.GetFileFromGitAsync("readme.md", NewSha, null)).ReturnsAsync("identical readme"); + _vmr.Setup(r => r.GetFileFromGitAsync(VmrPrefix + "readme.md", VmrHeadBranch, null)).ReturnsAsync("identical readme"); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeTrue(); + } + + [Test] + public async Task ForwardFlowMatchesSourceDiffAsync_PrTampersWithDashDashCommentLine_ReturnsFalse() + { + // Arrange: a SQL file whose comment line is changed. + // Source repo: "-- old" -> "-- new" + // VMR PR: "-- DIFFERENT" -> "-- new" + // The PR did NOT faithfully reproduce the source change (it removed a different line), so the + // verifier must reject the PR. In a -U0 diff the removed content lines render as "--- old" / + // "--- DIFFERENT", which share a prefix with the "--- " file header; GetChangeLines must still + // tell them apart (it parses hunks structurally) so the tampering is detected. + SetupDiffs(_sourceRepo, "schema.sql", new Dictionary + { + ["schema.sql"] = SingleLineDiff("schema.sql", "-- old", "-- new"), + }); + SetupDiffs(_vmr, VmrPrefix + "schema.sql", new Dictionary + { + [VmrPrefix + "schema.sql"] = SingleLineDiff(VmrPrefix + "schema.sql", "-- DIFFERENT", "-- new"), + }); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeFalse("the PR removed a different comment line than the source diff and must be rejected"); + } + + [Test] + public async Task ForwardFlowMatchesSourceDiffAsync_MultiHunkFileWithMatchingChanges_ReturnsTrue() + { + // Arrange: a file changed in two separate places (two hunks) identically in source and PR. + SetupDiffs(_sourceRepo, "data.txt", new Dictionary + { + ["data.txt"] = TwoHunkDiff("data.txt", ("first old", "first new"), ("second old", "second new")), + }); + SetupDiffs(_vmr, VmrPrefix + "data.txt", new Dictionary + { + [VmrPrefix + "data.txt"] = TwoHunkDiff(VmrPrefix + "data.txt", ("first old", "first new"), ("second old", "second new")), + }); + + // Act + var result = await VerifyAsync(); + + // Assert + result.Should().BeTrue(); + } +} diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/ForwardFlowTests.cs b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/ForwardFlowTests.cs index f870ac6166..dd2ac1eb52 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/ForwardFlowTests.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/ForwardFlowTests.cs @@ -25,8 +25,12 @@ public async Task OnlyForwardflowsTest() const string branchName = nameof(OnlyForwardflowsTest); + var oldSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); var codeFlowResult = await ChangeRepoFileAndFlowIt("New content in the individual repo", branchName); codeFlowResult.ShouldHaveUpdates(); + var newSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); + await GitOperations.CommitAll(VmrPath, "Commit codeflow"); + (await VerifyForwardFlowSourceDiff(branchName, oldSha, newSha)).Should().BeTrue(); await FinalizeForwardFlow(branchName); // Flow again - should be a no-op @@ -37,9 +41,13 @@ public async Task OnlyForwardflowsTest() CheckFileContents(_productRepoVmrFilePath, "New content in the individual repo"); // Make a change in the repo again + oldSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); codeFlowResult = await ChangeRepoFileAndFlowIt("New content in the individual repo again", branchName); codeFlowResult.ShouldHaveUpdates(); CheckFileContents(_productRepoVmrFilePath, "New content in the individual repo again"); + newSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); + await GitOperations.CommitAll(VmrPath, "Commit codeflow"); + (await VerifyForwardFlowSourceDiff(branchName, oldSha, newSha)).Should().BeTrue(); // Make an additional change in the PR branch before merging await File.WriteAllTextAsync(_productRepoVmrFilePath, "Change that happened in the PR"); @@ -64,8 +72,12 @@ await GitOperations.VerifyMergeConflict(VmrPath, branchName, // The last forward flow will have to recreate all of the flows to be able to apply the changes // Make another flow to VMR to have flows both ways ready + oldSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); codeFlowResult = await ChangeRepoFileAndFlowIt("Again some content in the individual repo", branchName); codeFlowResult.ShouldHaveUpdates(); + newSha = await GitOperations.GetRepoLastCommit(ProductRepoPath); + await GitOperations.CommitAll(VmrPath, "Commit codeflow"); + (await VerifyForwardFlowSourceDiff(branchName, oldSha, newSha)).Should().BeTrue(); await FinalizeForwardFlow(branchName); // The file.txt will keep getting changed and conflicting in each flow diff --git a/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs b/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs index 8f1fa362f2..9827aabe06 100644 --- a/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs +++ b/test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs @@ -7,6 +7,7 @@ using AwesomeAssertions; using Microsoft.DotNet.DarcLib.Helpers; using Microsoft.DotNet.DarcLib.Models; +using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo; using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.DotNet.ProductConstructionService.Client.Models; using Microsoft.Extensions.Logging.Abstractions; diff --git a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index d89483887a..b58a2616f3 100644 --- a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -39,6 +39,7 @@ internal abstract class PullRequestUpdaterTests : SubscriptionOrPullRequestUpdat private Mock _backFlower = null!; private Mock _forwardFlower = null!; private Mock _gitClient = null!; + private Mock _codeflowSourceDiffVerifier = null!; [SetUp] public void PullRequestUpdaterTests_SetUp() @@ -46,6 +47,7 @@ public void PullRequestUpdaterTests_SetUp() _backFlower = new(); _forwardFlower = new(); _gitClient = new(); + _codeflowSourceDiffVerifier = new(); } protected override void RegisterServices(IServiceCollection services) @@ -55,6 +57,7 @@ protected override void RegisterServices(IServiceCollection services) services.AddSingleton(_backFlower.Object); services.AddSingleton(_forwardFlower.Object); services.AddSingleton(_gitClient.Object); + services.AddSingleton(_codeflowSourceDiffVerifier.Object); CodeFlowResult codeFlowRes = new(true, [], new NativePath(VmrPath), []); _forwardFlower.SetReturnsDefault(Task.FromResult(codeFlowRes)); @@ -678,6 +681,21 @@ protected void AndShouldNotHavePullRequestCheckReminder() RemoveExpectedReminder(Subscription); } + protected void AndShouldHaveCodeflowApprovalCheckReminder( + string previousSourceSha, + string currentSourceSha, + string pullRequestUrl) + { + SetExpectedReminder(Subscription, new CodeflowApprovalCheck() + { + UpdaterId = GetPullRequestUpdaterId().ToString(), + SubscriptionId = Subscription.Id, + PreviousSourceSha = previousSourceSha, + CurrentSourceSha = currentSourceSha, + PullRequestUrl = pullRequestUrl + }); + } + protected void AndShouldHaveInProgressPullRequestState( Build forBuild, int nextBuildToProcess = 0, diff --git a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs index 66bc51773b..a06176c445 100644 --- a/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs +++ b/test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs @@ -268,6 +268,7 @@ public async Task UpdateWithManuallyMergedPrAndNewBuild() }; AndShouldHavePullRequestCheckReminder(); + AndShouldHaveCodeflowApprovalCheckReminder(build.Commit, build2.Commit, VmrPullRequestUrl); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } @@ -354,6 +355,7 @@ public async Task UpdateWithManuallyMergedPrAndLatestFlowNotApplied() }; AndShouldHavePullRequestCheckReminder(); + AndShouldHaveCodeflowApprovalCheckReminder(build1.Commit, build2.Commit, VmrPullRequestUrl); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } @@ -522,6 +524,7 @@ public async Task UpdateWithExistingOppositeSubscription() }; AndShouldHavePullRequestCheckReminder(); + AndShouldHaveCodeflowApprovalCheckReminder(build.Commit, build2.Commit, VmrPullRequestUrl); AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); } } diff --git a/test/ProductConstructionService/ProductConstructionService.ScenarioTests.Tests/TestHelpersTest.cs b/test/ProductConstructionService/ProductConstructionService.ScenarioTests.Tests/TestHelpersTest.cs index 5c28293a81..90ab0dacf3 100644 --- a/test/ProductConstructionService/ProductConstructionService.ScenarioTests.Tests/TestHelpersTest.cs +++ b/test/ProductConstructionService/ProductConstructionService.ScenarioTests.Tests/TestHelpersTest.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using AwesomeAssertions; +using ProductConstructionService.ScenarioTests.Helpers; namespace ProductConstructionService.ScenarioTests.Tests;