From 004a58d04a8f83ea558fc4f2146ab65d44e7b7a6 Mon Sep 17 00:00:00 2001 From: "dotnet-maestro[bot]" <42748379+dotnet-maestro[bot]@users.noreply.github.com> Date: Fri, 11 Oct 2024 10:25:47 +0000 Subject: [PATCH 1/7] [main] Update dependencies from dotnet/arcade (#4037) Co-authored-by: dotnet-maestro[bot] --- NuGet.config | 3 ++- eng/Version.Details.xml | 24 ++++++++++++------------ eng/Versions.props | 10 +++++----- eng/common/tools.ps1 | 2 +- global.json | 2 +- 5 files changed, 21 insertions(+), 20 deletions(-) diff --git a/NuGet.config b/NuGet.config index ab4a3b537a..6c74936cac 100644 --- a/NuGet.config +++ b/NuGet.config @@ -1,4 +1,4 @@ - + @@ -213,4 +213,5 @@ + diff --git a/eng/Version.Details.xml b/eng/Version.Details.xml index beda674943..dbc38558d8 100644 --- a/eng/Version.Details.xml +++ b/eng/Version.Details.xml @@ -91,29 +91,29 @@ - + https://github.com/dotnet/arcade - 69abe6b2063083c0b35fc3a5b16cb2bdbaf5e8b0 + e5b13e054339e41d422212a0ecaf24fec20cb5a1 - + https://github.com/dotnet/arcade - 69abe6b2063083c0b35fc3a5b16cb2bdbaf5e8b0 + e5b13e054339e41d422212a0ecaf24fec20cb5a1 - + https://github.com/dotnet/arcade - 69abe6b2063083c0b35fc3a5b16cb2bdbaf5e8b0 + e5b13e054339e41d422212a0ecaf24fec20cb5a1 - + https://github.com/dotnet/arcade - 69abe6b2063083c0b35fc3a5b16cb2bdbaf5e8b0 + e5b13e054339e41d422212a0ecaf24fec20cb5a1 - + https://github.com/dotnet/arcade - 69abe6b2063083c0b35fc3a5b16cb2bdbaf5e8b0 + e5b13e054339e41d422212a0ecaf24fec20cb5a1 - + https://github.com/dotnet/arcade - 69abe6b2063083c0b35fc3a5b16cb2bdbaf5e8b0 + e5b13e054339e41d422212a0ecaf24fec20cb5a1 https://github.com/dotnet/dnceng diff --git a/eng/Versions.props b/eng/Versions.props index db0d0bfc39..1392924cd6 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -9,11 +9,11 @@ true 1.0.0-preview.1 - 8.0.0-beta.24475.3 - 8.0.0-beta.24475.3 - 8.0.0-beta.24475.3 - 8.0.0-beta.24475.3 - 8.0.0-beta.24475.3 + 8.0.0-beta.24508.1 + 8.0.0-beta.24508.1 + 8.0.0-beta.24508.1 + 8.0.0-beta.24508.1 + 8.0.0-beta.24508.1 17.4.1 1.1.0-beta.24376.1 1.1.0-beta.24376.1 diff --git a/eng/common/tools.ps1 b/eng/common/tools.ps1 index eb188cfda4..a2dedaa529 100644 --- a/eng/common/tools.ps1 +++ b/eng/common/tools.ps1 @@ -892,7 +892,7 @@ function IsWindowsPlatform() { } function Get-Darc($version) { - $darcPath = "$TempDir\darc\$(New-Guid)" + $darcPath = "$TempDir\darc\$([guid]::NewGuid())" if ($version -ne $null) { & $PSScriptRoot\darc-init.ps1 -toolpath $darcPath -darcVersion $version | Out-Host } else { diff --git a/global.json b/global.json index a608430cc0..132f6a8675 100644 --- a/global.json +++ b/global.json @@ -15,6 +15,6 @@ } }, "msbuild-sdks": { - "Microsoft.DotNet.Arcade.Sdk": "8.0.0-beta.24475.3" + "Microsoft.DotNet.Arcade.Sdk": "8.0.0-beta.24508.1" } } From ecad690639854fe0c5cb0f8bd07129acc7e0e842 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C5=99emek=20Vysok=C3=BD?= Date: Fri, 11 Oct 2024 14:14:04 +0200 Subject: [PATCH 2/7] Execute codeflow logic directly + cache improvements (#4030) --- Directory.Packages.props | 2 +- src/Maestro/README.md | 72 +-- .../PullRequestActor.cs | 200 +------ .../DarcLib/GitRepoCloner.cs | 22 +- .../DarcLib/LocalLibGit2Client.cs | 4 +- .../Controllers/CodeFlowController.cs | 57 -- .../InitializationBackgroundService.cs | 1 + .../PcsStartup.cs | 25 +- .../VirtualMonoRepo/VmrConfiguration.cs | 16 +- .../Generated/CodeFlow.cs | 108 ---- .../Generated/Models/CodeFlowRequest.cs | 29 - .../ProductConstructionServiceApi.cs | 4 - .../IRedisCacheFactory.cs | 9 +- .../RedisMutex.cs | 8 +- .../BatchedPullRequestUpdater.cs | 19 +- .../DependencyFlowConfiguration.cs | 3 +- .../IPullRequestUpdater.cs | 4 +- .../{WorkItems => }/InProgressPullRequest.cs | 15 +- .../NonBatchedPullRequestUpdater.cs | 24 +- .../PullRequestBuilder.cs | 6 +- .../PullRequestPolicyFailureNotifier.cs | 1 - .../PullRequestUpdater.cs | 519 +++++++++++------- .../PullRequestUpdaterId.cs | 11 - .../SubscriptionTriggerer.cs | 10 +- .../CodeFlowWorkItemProcessor.cs | 133 ----- .../PullRequestCheckProcessor.cs | 21 +- .../SubscriptionUpdateProcessor.cs | 4 +- .../WorkItems/CodeFlowWorkItem.cs | 32 -- ...rWorkItem.cs => DependencyFlowWorkItem.cs} | 4 +- .../PullRequestCheck.cs} | 9 +- .../WorkItems/SubscriptionUpdateWorkItem.cs | 2 +- .../ReminderManager.cs | 4 +- .../MockRedisCacheFactory.cs | 2 +- .../PendingCodeFlowUpdatesTests.cs | 48 +- .../PendingUpdatesTests.cs | 27 +- .../PullRequestBuilderTests.cs | 2 +- .../PullRequestPolicyFailureNotifierTests.cs | 9 +- .../PullRequestUpdaterTests.cs | 270 +++++---- .../SubscriptionOrPullRequestUpdaterTests.cs | 6 +- .../TestsWithMocks.cs | 2 +- .../UpdateAssetsForCodeFlowTests.cs | 157 +++--- .../UpdateAssetsTests.cs | 40 +- .../UpdaterTests.cs | 16 +- .../PendingCodeFlowUpdatesTests.cs | 112 ---- .../PullRequestActorTests.cs | 53 +- .../UpdateAssetsForCodeFlowTests.cs | 237 -------- 46 files changed, 777 insertions(+), 1582 deletions(-) delete mode 100644 src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Controllers/CodeFlowController.cs delete mode 100644 src/ProductConstructionService/ProductConstructionService.Client/Generated/CodeFlow.cs delete mode 100644 src/ProductConstructionService/ProductConstructionService.Client/Generated/Models/CodeFlowRequest.cs rename src/ProductConstructionService/ProductConstructionService.DependencyFlow/{WorkItems => }/InProgressPullRequest.cs (68%) delete mode 100644 src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeFlowWorkItemProcessor.cs delete mode 100644 src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeFlowWorkItem.cs rename src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/{ActorWorkItem.cs => DependencyFlowWorkItem.cs} (75%) rename src/ProductConstructionService/ProductConstructionService.DependencyFlow/{CodeFlowStatus.cs => WorkItems/PullRequestCheck.cs} (52%) delete mode 100644 test/SubscriptionActorService.Tests/PendingCodeFlowUpdatesTests.cs delete mode 100644 test/SubscriptionActorService.Tests/UpdateAssetsForCodeFlowTests.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index 393fdaefab..75bf6e3ce9 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -37,7 +37,7 @@ - + diff --git a/src/Maestro/README.md b/src/Maestro/README.md index ae2b0e1a95..72e7000fcd 100644 --- a/src/Maestro/README.md +++ b/src/Maestro/README.md @@ -7,15 +7,15 @@ The following diagram shows how a dependency update PR progresses from opening t ```mermaid flowchart SubscriptionTrigger(Subscription is triggered) - Exist{Does a PR\nalready exist?} - State{What state\nis the PR in?} - PolicyState{What state\nare the check\npolicies in?} + Exist{Does a PR
already exist?} + State{What state
is the PR in?} + PolicyState{What state
are the check
policies in?} Create(Create a new PR) - CleanUp((Clean up\nbranch)) + CleanUp((Clean up
branch)) TagPeople(Notify/tag people) - UpdatePR(Update PR,\nif possible) + UpdatePR(Update PR,
if possible) MergePR(Merge PR) - UpdateLastBuild((Update\nLastAppliedBuild\nin BAR)) + UpdateLastBuild((Update
LastAppliedBuild
in BAR)) Timer(Periodic timer) Exist--Yes-->State @@ -53,66 +53,6 @@ class SubscriptionTrigger,Timer,ExternalImpulse External linkStyle 2,12 stroke-width:2px,fill:none,stroke:#FFEE00,color:#FF9900 ``` -The above flow only applies to the usual dependency flow updates (version files, etc.). The flow works a little bit different for the code-enabled subscriptions that flow code between product repos and the VMR. -For those, the PR branch is created by the [Product Construction Service](../ProductConstructionService). - -```mermaid -flowchart - SubscriptionTrigger(Code-enabled\nsubscription triggered) - PRExists{Does a PR\nalready exist?} - BranchExists{Does a branch\n exist?} - CreatePR(Create a new PR) - PRState{What state\nis the PR in?} - CleanUp((Clean up\nbranch)) - TagPeople(Notify/tag people) - UpdatePR(Call to PCS\nUpdate branch) - MergePR(Merge PR) - Timer(Periodic timer) - CreateBranch(Call to PCS:\nRequest PR branch) - PolicyState{What state\nare the check\npolicies in?} - UpdateLastBuild((Update\nLastAppliedBuild\nin BAR,\nclean up branch)) - - SubscriptionTrigger-->PRExists - PRExists--No -->BranchExists - PRExists--Yes-->PRState - BranchExists--Yes-->CreatePR - BranchExists--No -->CreateBranch - CreateBranch--PCS pushes a branch,\ntriggers the subscription-->SubscriptionTrigger - - PRState--Open-->PolicyState - PRState--Merged-->UpdateLastBuild - PRState--Closed-->CleanUp - - PolicyState--Checks OK-->MergePR - MergePR-->UpdateLastBuild - PolicyState--Failed checks-->TagPeople - TagPeople-->Timer - %% Cannot update - PolicyState--Not updatable PR\npending policies, conflicts.. -->Timer - %% Can update - Timer--Check PR-->PRState - CreatePR--Set timer-->Timer - UpdatePR--Set timer-->Timer - -subgraph Legend - MaestroAction(Action) - ExternalAction(External call) - ExternalImpulse(Trigger) - EndOfFlow((End of flow)) -end - -classDef Action fill:#00DD00,stroke:#006600,stroke-width:1px,color:#006600 -classDef End fill:#9999EE,stroke:#0000AA,stroke-width:1px,color:#0000AA -classDef External fill:#FFEE00,stroke:#FF9900,stroke-width:1px,color:#666600 -classDef PCSCall fill:#DD0000,stroke:#660000,stroke-width:1px,color:#ffffff - -class CreatePR,TagPeople,NoAction,MergePR,MaestroAction Action -class UpdateLastBuild,CleanUp,EndOfFlow End -class SubscriptionTrigger,Timer,ExternalImpulse,PCSFinished External -class CreateBranch,UpdatePR,ExternalAction PCSCall -``` - - ## Validation Process in dev and int environments For any non-deployment code changes, the expectation is to have run the tests corresponding to the service locally to confirm that the change works before putting up the PR. The tests for each of the major areas in arcade-services are as below: diff --git a/src/Maestro/SubscriptionActorService/PullRequestActor.cs b/src/Maestro/SubscriptionActorService/PullRequestActor.cs index 179bd1b06e..9d87a786bf 100644 --- a/src/Maestro/SubscriptionActorService/PullRequestActor.cs +++ b/src/Maestro/SubscriptionActorService/PullRequestActor.cs @@ -6,6 +6,7 @@ using System.Linq; using System.Net; using System.Net.Http; +using System.Security.Policy; using System.Threading.Tasks; using Maestro.Contracts; using Maestro.Data; @@ -1079,205 +1080,12 @@ private async Task GetRepositoryBranchUpdate() /// /// Alternative to ProcessPendingUpdatesAsync that is used in the code flow (VMR) scenario. /// - private async Task> ProcessCodeFlowUpdatesAsync( + private Task> ProcessCodeFlowUpdatesAsync( List updates, InProgressPullRequest? pr) { - // TODO https://github.com/dotnet/arcade-services/issues/3378: Support batched PRs for code flow updates - if (updates.Count > 1) - { - updates = [..updates.DistinctBy(u => u.BuildId)]; - if (updates.Count > 1) - { - _logger.LogWarning("Code flow updates cannot be batched with other updates. Will process the last update only."); - } - } - - var update = updates.Last(); - - CodeFlowStatus? codeFlowStatus = await _codeFlowState.TryGetStateAsync(); - - // The E2E order of things for is: - // 1. We send a request to PCS and wait for a branch to be created. We note down this in the codeflow status. We set a reminder. - // 2. When reminder kicks in, we check if the branch is created. If not, we repeat the reminder. - // 3. When branch is created, we create the PR and set the usual reminder of watching a PR (common with the regular subscriptions). - // 4. For new updates, we only delegate those to PCS which will push in the branch. - if (pr == null) - { - (string targetRepository, string targetBranch) = await GetTargetAsync(); - - // Step 1. Let PCS create a branch for us - if (codeFlowStatus == null) - { - return await RequestCodeFlowBranchAsync(update, targetBranch); - } - - // Step 2. Wait for the branch to be created - IRemote remote = await _remoteFactory.GetRemoteAsync(targetRepository, _logger); - if (!await remote.BranchExistsAsync(targetRepository, codeFlowStatus.PrBranch)) - { - _logger.LogInformation("Branch {branch} for subscription {subscriptionId} not created yet. Will check again later", - codeFlowStatus.PrBranch, - update.SubscriptionId); - - await _pullRequestUpdateState.StoreStateAsync([update]); - await _pullRequestUpdateState.SetReminderAsync(dueTimeInMinutes: 3); - - return ActionResult.Create(true, $"Pending updates applied. Branch {codeFlowStatus.PrBranch} not created yet"); - } - - // Step 3. Create a PR - string prUrl = await CreateCodeFlowPullRequestAsync( - update, - targetRepository, - codeFlowStatus.PrBranch, - targetBranch); - - return ActionResult.Create(true, $"Pending updates applied. PR {prUrl} created"); - } - - // Technically, this should never happen as we create the code flow data before we even create the PR - if (codeFlowStatus == null) - { - _logger.LogError("Missing code flow data for subscription {subscription}", update.SubscriptionId); - await _pullRequestUpdateState.RemoveStateAsync(); - await _pullRequestUpdateState.UnsetReminderAsync(); - await _pullRequestCheckState.UnsetReminderAsync(); - return ActionResult.Create(false, "Missing code flow data."); - } - - // Step 4. Update the PR (if needed) - - // Compare last SHA with the build SHA to see if we need to delegate this update to PCS - if (update.SourceSha == codeFlowStatus.SourceSha) - { - _logger.LogInformation("PR {url} for {subscription} is up to date ({sha})", - pr.Url, - update.SubscriptionId, - update.SourceSha); - return ActionResult.Create(false, "PR cannot be updated"); - } - - try - { - await _pcsClient.CodeFlow.FlowBuildAsync(new CodeFlowRequest(update.SubscriptionId, update.BuildId) - { - PrBranch = codeFlowStatus.PrBranch, - PrUrl = pr.Url, - }); - - codeFlowStatus.SourceSha = update.SourceSha; - - await _codeFlowState.StoreStateAsync(codeFlowStatus); - await _pullRequestState.StoreStateAsync(pr); - await _pullRequestCheckState.SetReminderAsync(); - - await _pullRequestUpdateState.RemoveStateAsync(); - await _pullRequestUpdateState.UnsetReminderAsync(); - } - catch (Exception e) - { - // TODO https://github.com/dotnet/arcade-services/issues/3318: Handle this - _logger.LogError(e, "Failed to request branch update for PR {url} for subscription {subscriptionId}", - pr.Url, - update.SubscriptionId); - } - - return ActionResult.Create(true, "New changes requested from PCS"); - } - - private async Task> RequestCodeFlowBranchAsync(UpdateAssetsParameters update, string targetBranch) - { - CodeFlowStatus codeFlowUpdate = new() - { - PrBranch = GetNewBranchName(targetBranch), - SourceSha = update.SourceSha, - }; - - _logger.LogInformation( - "New code flow request for subscription {subscriptionId}. Requesting branch {branch} from PCS", - update.SubscriptionId, - codeFlowUpdate.PrBranch); - - try - { - await _pcsClient.CodeFlow.FlowBuildAsync(new CodeFlowRequest(update.SubscriptionId, update.BuildId) - { - PrBranch = codeFlowUpdate.PrBranch, - }); - } - catch (Exception e) - { - // TODO https://github.com/dotnet/arcade-services/issues/3318: Handle this - _logger.LogError(e, "Failed to request new branch {branch} for subscription {subscriptionId}", - codeFlowUpdate.PrBranch, - update.SubscriptionId); - return ActionResult.Create(false, $"Failed to call PCS when requesting branch {codeFlowUpdate.PrBranch}"); - } - - await _codeFlowState.StoreStateAsync(codeFlowUpdate); - await _pullRequestUpdateState.StoreItemStateAsync(update); - await _pullRequestUpdateState.SetReminderAsync(dueTimeInMinutes: 3); - - return ActionResult.Create(true, $"Pending updates applied. Branch {codeFlowUpdate.PrBranch} requested from PCS"); - } - - private async Task CreateCodeFlowPullRequestAsync( - UpdateAssetsParameters update, - string targetRepository, - string prBranch, - string targetBranch) - { - IRemote darcRemote = await _remoteFactory.GetRemoteAsync(targetRepository, _logger); - - try - { - string title = await _pullRequestBuilder.GenerateCodeFlowPRTitleAsync(update, targetBranch); - string description = await _pullRequestBuilder.GenerateCodeFlowPRDescriptionAsync(update); - - string prUrl = await darcRemote.CreatePullRequestAsync( - targetRepository, - new PullRequest - { - Title = title, - Description = description, - BaseBranch = targetBranch, - HeadBranch = prBranch, - }); - - InProgressPullRequest inProgressPr = new() - { - Url = prUrl, - ContainedSubscriptions = - [ - new() - { - SubscriptionId = update.SubscriptionId, - BuildId = update.BuildId - } - ] - }; - - await AddDependencyFlowEventsAsync( - inProgressPr.ContainedSubscriptions, - DependencyFlowEventType.Created, - DependencyFlowEventReason.New, - MergePolicyCheckResult.PendingPolicies, - prUrl); - - await _pullRequestState.StoreStateAsync(inProgressPr); - await _pullRequestCheckState.SetReminderAsync(); - - await _pullRequestUpdateState.RemoveStateAsync(); - await _pullRequestUpdateState.UnsetReminderAsync(); - - return prUrl; - } - catch - { - await darcRemote.DeleteBranchAsync(targetRepository, prBranch); - throw; - } + _logger.LogWarning("Code flow updates cannot be batched with other updates. Will process the last update only."); + return Task.FromResult(ActionResult.Create(false, $"Code flow subscriptions are not supported in Maestro")); } #endregion diff --git a/src/Microsoft.DotNet.Darc/DarcLib/GitRepoCloner.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitRepoCloner.cs index 9cd9f468ba..05b1afb7c6 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/GitRepoCloner.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/GitRepoCloner.cs @@ -55,21 +55,21 @@ private Task CloneAsync(string repoUri, string? commit, string targetDirectory, CloneOptions cloneOptions = new() { Checkout = false, - CredentialsProvider = (url, user, cred) => - new UsernamePasswordCredentials - { - // The PAT is actually the only thing that matters here, the username - // will be ignored. - Username = RemoteTokenProvider.GitRemoteUser, - Password = _remoteTokenProvider.GetTokenForRepository(repoUri), - }, }; + cloneOptions.FetchOptions.CredentialsProvider = (_, __, ___) => + new UsernamePasswordCredentials + { + // The PAT is actually the only thing that matters here, the username + // will be ignored. + Username = RemoteTokenProvider.GitRemoteUser, + Password = _remoteTokenProvider.GetTokenForRepository(repoUri), + }; + _logger.LogInformation("Cloning {repoUri} to {targetDirectory}", repoUri, targetDirectory); try { - _logger.LogDebug($"Cloning {repoUri} to {targetDirectory}"); string repoPath = Repository.Clone( repoUri, targetDirectory, @@ -126,7 +126,9 @@ private static void CheckoutSubmodules(Repository repo, CloneOptions submoduleCl foreach (Submodule sub in repo.Submodules) { log.LogDebug($"Updating submodule {sub.Name} at {sub.Path} for {repo.Info.WorkingDirectory}. GitDirParent: {gitDirParentPath}"); - repo.Submodules.Update(sub.Name, new SubmoduleUpdateOptions { CredentialsProvider = submoduleCloneOptions.CredentialsProvider, Init = true }); + var options = new SubmoduleUpdateOptions { Init = true }; + options.FetchOptions.CredentialsProvider = submoduleCloneOptions.FetchOptions.CredentialsProvider; + repo.Submodules.Update(sub.Name, options); string normalizedSubPath = sub.Path.Replace('\\', Path.DirectorySeparatorChar).Replace('/', Path.DirectorySeparatorChar); string subRepoPath = Path.Combine(repo.Info.WorkingDirectory, normalizedSubPath); diff --git a/src/Microsoft.DotNet.Darc/DarcLib/LocalLibGit2Client.cs b/src/Microsoft.DotNet.Darc/DarcLib/LocalLibGit2Client.cs index 14d14b6546..aeb1e8271d 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/LocalLibGit2Client.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/LocalLibGit2Client.cs @@ -356,8 +356,8 @@ public async Task Push( CredentialsProvider = (url, user, cred) => new UsernamePasswordCredentials { - Username = _remoteTokenProvider.GetTokenForRepository(remoteUrl), - Password = string.Empty + Username = Constants.GitHubBotUserName, + Password = _remoteTokenProvider.GetTokenForRepository(remoteUrl), } }; diff --git a/src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Controllers/CodeFlowController.cs b/src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Controllers/CodeFlowController.cs deleted file mode 100644 index 418fd78c06..0000000000 --- a/src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Controllers/CodeFlowController.cs +++ /dev/null @@ -1,57 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.ComponentModel.DataAnnotations; -using Microsoft.AspNetCore.ApiVersioning; -using Microsoft.AspNetCore.Mvc; -using Microsoft.DotNet.DarcLib; -using Microsoft.DotNet.Maestro.Client.Models; -using ProductConstructionService.Api.VirtualMonoRepo; -using ProductConstructionService.DependencyFlow.WorkItems; -using ProductConstructionService.WorkItems; - -namespace ProductConstructionService.Api.Api.v2020_02_20.Controllers; - -[Route("codeflow")] -[ApiVersion("2020-02-20")] -public class CodeFlowController( - IBasicBarClient barClient, - IWorkItemProducerFactory workItemProducerFactory) - : ControllerBase -{ - private readonly IBasicBarClient _barClient = barClient; - private readonly IWorkItemProducerFactory _workItemProducerFactory = workItemProducerFactory; - - [HttpPost(Name = "Flow")] - public async Task FlowBuild([Required, FromBody] Maestro.Api.Model.v2020_02_20.CodeFlowRequest request) - { - if (!ModelState.IsValid) - { - return BadRequest(ModelState); - } - - Subscription subscription = await _barClient.GetSubscriptionAsync(request.SubscriptionId); - if (subscription == null) - { - return NotFound($"Subscription {request.SubscriptionId} not found"); - } - - Build build = await _barClient.GetBuildAsync(request.BuildId); - if (build == null) - { - return NotFound($"Build {request.BuildId} not found"); - } - - await _workItemProducerFactory - .CreateProducer() - .ProduceWorkItemAsync(new() - { - BuildId = request.BuildId, - SubscriptionId = request.SubscriptionId, - PrBranch = request.PrBranch, - PrUrl = request.PrUrl, - }); - - return Ok(); - } -} diff --git a/src/ProductConstructionService/ProductConstructionService.Api/InitializationBackgroundService.cs b/src/ProductConstructionService/ProductConstructionService.Api/InitializationBackgroundService.cs index 62b43fb421..c59ce3924d 100644 --- a/src/ProductConstructionService/ProductConstructionService.Api/InitializationBackgroundService.cs +++ b/src/ProductConstructionService/ProductConstructionService.Api/InitializationBackgroundService.cs @@ -27,6 +27,7 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken) // If Vmr cloning is taking more than 20 min, something is wrong var linkedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(stoppingToken, new CancellationTokenSource(TimeSpan.FromMinutes(20)).Token); + // TODO: VmrUri should be loaded from configuration and passed to the singleton via Configure() IVmrInfo vmrInfo = scope.ServiceProvider.GetRequiredService(); vmrInfo.VmrUri = options.VmrUri; IVmrCloneManager vmrCloneManager = scope.ServiceProvider.GetRequiredService(); diff --git a/src/ProductConstructionService/ProductConstructionService.Api/PcsStartup.cs b/src/ProductConstructionService/ProductConstructionService.Api/PcsStartup.cs index eebc33c048..49fdfa9a02 100644 --- a/src/ProductConstructionService/ProductConstructionService.Api/PcsStartup.cs +++ b/src/ProductConstructionService/ProductConstructionService.Api/PcsStartup.cs @@ -7,8 +7,8 @@ using System.Text; using Azure.Identity; using EntityFrameworkCore.Triggers; -using FluentValidation.AspNetCore; using Maestro.Authentication; +using Maestro.Common; using Maestro.Common.AzureDevOpsTokens; using Maestro.Data; using Maestro.Data.Models; @@ -30,7 +30,6 @@ using Newtonsoft.Json.Serialization; using ProductConstructionService.Api.Api; using ProductConstructionService.Api.Configuration; -using ProductConstructionService.Api.Controllers; using ProductConstructionService.Api.Pages.DependencyFlow; using ProductConstructionService.Api.Telemetry; using ProductConstructionService.Api.VirtualMonoRepo; @@ -39,8 +38,7 @@ using ProductConstructionService.WorkItems; using ProductConstructionService.DependencyFlow; using ProductConstructionService.ServiceDefaults; -using Microsoft.Net.Http.Headers; -using System.Net.Http.Headers; +using Microsoft.Extensions.DependencyInjection.Extensions; namespace ProductConstructionService.Api; @@ -172,6 +170,15 @@ internal static async Task ConfigurePcs( // TODO (https://github.com/dotnet/arcade-services/issues/3880) - Remove subscriptionIdGenerator builder.Services.AddSingleton(sp => new(RunningService.PCS)); + // This needs to precede the AddVmrRegistrations call as we want a GitHub provider using the app installations + // Otherwise, AddVmrRegistrations would add one based on PATs (like we give it in darc) + builder.Services.TryAddSingleton(sp => + { + var azdoTokenProvider = sp.GetRequiredService(); + var gitHubTokenProvider = sp.GetRequiredService(); + return new RemoteTokenProvider(azdoTokenProvider, new Microsoft.DotNet.DarcLib.GitHubTokenProvider(gitHubTokenProvider)); + }); + await builder.AddRedisCache(authRedis); builder.AddBuildAssetRegistry(); builder.AddMetricRecorder(); @@ -200,17 +207,11 @@ internal static async Task ConfigurePcs( if (initializeService) { - builder.AddVmrInitialization(); + builder.InitializeVmrFromRemote(); } else { - // This is expected in local flows and it's useful to learn about this early - var vmrPath = builder.Configuration.GetVmrPath(); - if (!Directory.Exists(vmrPath)) - { - throw new InvalidOperationException($"VMR not found at {vmrPath}. " + - $"Either run the service in initialization mode or clone a VMR into {vmrPath}."); - } + builder.InitializeVmrFromDisk(); } builder.AddServiceDefaults(); diff --git a/src/ProductConstructionService/ProductConstructionService.Api/VirtualMonoRepo/VmrConfiguration.cs b/src/ProductConstructionService/ProductConstructionService.Api/VirtualMonoRepo/VmrConfiguration.cs index 33e0cdf3e0..bf4dc6beb5 100644 --- a/src/ProductConstructionService/ProductConstructionService.Api/VirtualMonoRepo/VmrConfiguration.cs +++ b/src/ProductConstructionService/ProductConstructionService.Api/VirtualMonoRepo/VmrConfiguration.cs @@ -22,7 +22,7 @@ public static void AddVmrRegistrations(this WebApplicationBuilder builder) builder.Services.AddVmrManagers("git", vmrPath, tmpPath, gitHubToken: null, azureDevOpsToken: null); } - public static void AddVmrInitialization(this WebApplicationBuilder builder) + public static void InitializeVmrFromRemote(this WebApplicationBuilder builder) { string vmrUri = builder.Configuration.GetRequiredValue(VmrUriKey); builder.Services.AddSingleton(new InitializationBackgroundServiceOptions(vmrUri)); @@ -30,8 +30,18 @@ public static void AddVmrInitialization(this WebApplicationBuilder builder) builder.Services.AddHealthChecks().AddCheck(VmrReadyHealthCheckName, tags: [VmrReadyHealthCheckTag]); } - public static string GetVmrPath(this IConfiguration configuration) + public static void InitializeVmrFromDisk(this WebApplicationBuilder builder) { - return configuration.GetRequiredValue(VmrPathKey); + // This is expected in local flows and it's useful to learn about this early + var vmrPath = builder.Configuration.GetRequiredValue(VmrPathKey); + if (!Directory.Exists(vmrPath)) + { + throw new InvalidOperationException($"VMR not found at {vmrPath}. " + + $"Either run the service in initialization mode or clone a VMR into {vmrPath}."); + } + + // TODO: Change IVmrInfo to be loaded from configurations and call Configure() here + var vmrInfo = builder.Services.BuildServiceProvider().GetRequiredService(); + vmrInfo.VmrUri = builder.Configuration.GetRequiredValue(VmrUriKey); } } diff --git a/src/ProductConstructionService/ProductConstructionService.Client/Generated/CodeFlow.cs b/src/ProductConstructionService/ProductConstructionService.Client/Generated/CodeFlow.cs deleted file mode 100644 index 18c5041309..0000000000 --- a/src/ProductConstructionService/ProductConstructionService.Client/Generated/CodeFlow.cs +++ /dev/null @@ -1,108 +0,0 @@ -// 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.IO; -using System.Text; -using System.Threading; -using System.Threading.Tasks; -using Azure; -using Azure.Core; - - - -namespace ProductConstructionService.Client -{ - public partial interface ICodeFlow - { - Task FlowBuildAsync( - Models.CodeFlowRequest body, - CancellationToken cancellationToken = default - ); - - } - - internal partial class CodeFlow : IServiceOperations, ICodeFlow - { - public CodeFlow(ProductConstructionServiceApi client) - { - Client = client ?? throw new ArgumentNullException(nameof(client)); - } - - public ProductConstructionServiceApi Client { get; } - - partial void HandleFailedRequest(RestApiException ex); - - partial void HandleFailedFlowBuildRequest(RestApiException ex); - - public async Task FlowBuildAsync( - Models.CodeFlowRequest body, - CancellationToken cancellationToken = default - ) - { - - if (body == default(Models.CodeFlowRequest)) - { - throw new ArgumentNullException(nameof(body)); - } - - const string apiVersion = "2020-02-20"; - - var _baseUri = Client.Options.BaseUri; - var _url = new RequestUriBuilder(); - _url.Reset(_baseUri); - _url.AppendPath( - "/api/codeflow", - false); - - _url.AppendQuery("api-version", Client.Serialize(apiVersion)); - - - using (var _req = Client.Pipeline.CreateRequest()) - { - _req.Uri = _url; - _req.Method = RequestMethod.Post; - - if (body != default(Models.CodeFlowRequest)) - { - _req.Content = RequestContent.Create(Encoding.UTF8.GetBytes(Client.Serialize(body))); - _req.Headers.Add("Content-Type", "application/json; charset=utf-8"); - } - - using (var _res = await Client.SendAsync(_req, cancellationToken).ConfigureAwait(false)) - { - if (_res.Status < 200 || _res.Status >= 300) - { - await OnFlowBuildFailed(_req, _res).ConfigureAwait(false); - } - - - return; - } - } - } - - internal async Task OnFlowBuildFailed(Request req, Response res) - { - string content = null; - if (res.ContentStream != null) - { - using (var reader = new StreamReader(res.ContentStream)) - { - content = await reader.ReadToEndAsync().ConfigureAwait(false); - } - } - - var ex = new RestApiException( - req, - res, - content, - Client.Deserialize(content) - ); - HandleFailedFlowBuildRequest(ex); - HandleFailedRequest(ex); - Client.OnFailedRequest(ex); - throw ex; - } - } -} diff --git a/src/ProductConstructionService/ProductConstructionService.Client/Generated/Models/CodeFlowRequest.cs b/src/ProductConstructionService/ProductConstructionService.Client/Generated/Models/CodeFlowRequest.cs deleted file mode 100644 index a9e9c47afb..0000000000 --- a/src/ProductConstructionService/ProductConstructionService.Client/Generated/Models/CodeFlowRequest.cs +++ /dev/null @@ -1,29 +0,0 @@ -// 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 Newtonsoft.Json; - -namespace ProductConstructionService.Client.Models -{ - public partial class CodeFlowRequest - { - public CodeFlowRequest(Guid subscriptionId, int buildId) - { - SubscriptionId = subscriptionId; - BuildId = buildId; - } - - [JsonProperty("subscriptionId")] - public Guid SubscriptionId { get; set; } - - [JsonProperty("buildId")] - public int BuildId { get; set; } - - [JsonProperty("prBranch")] - public string PrBranch { get; set; } - - [JsonProperty("prUrl")] - public string PrUrl { get; set; } - } -} diff --git a/src/ProductConstructionService/ProductConstructionService.Client/Generated/ProductConstructionServiceApi.cs b/src/ProductConstructionService/ProductConstructionService.Client/Generated/ProductConstructionServiceApi.cs index 0ce126777d..d907620152 100644 --- a/src/ProductConstructionService/ProductConstructionService.Client/Generated/ProductConstructionServiceApi.cs +++ b/src/ProductConstructionService/ProductConstructionService.Client/Generated/ProductConstructionServiceApi.cs @@ -31,7 +31,6 @@ public partial interface IProductConstructionServiceApi IBuilds Builds { get; } IBuildTime BuildTime { get; } IChannels Channels { get; } - ICodeFlow CodeFlow { get; } IDefaultChannels DefaultChannels { get; } IGoal Goal { get; } IPipelines Pipelines { get; } @@ -122,8 +121,6 @@ public HttpPipeline Pipeline public IChannels Channels { get; } - public ICodeFlow CodeFlow { get; } - public IDefaultChannels DefaultChannels { get; } public IGoal Goal { get; } @@ -148,7 +145,6 @@ public ProductConstructionServiceApi(ProductConstructionServiceApiOptions option Builds = new Builds(this); BuildTime = new BuildTime(this); Channels = new Channels(this); - CodeFlow = new CodeFlow(this); DefaultChannels = new DefaultChannels(this); Goal = new Goal(this); Pipelines = new Pipelines(this); diff --git a/src/ProductConstructionService/ProductConstructionService.Common/IRedisCacheFactory.cs b/src/ProductConstructionService/ProductConstructionService.Common/IRedisCacheFactory.cs index 8489124642..af93ddfd04 100644 --- a/src/ProductConstructionService/ProductConstructionService.Common/IRedisCacheFactory.cs +++ b/src/ProductConstructionService/ProductConstructionService.Common/IRedisCacheFactory.cs @@ -8,7 +8,7 @@ namespace ProductConstructionService.Common; public interface IRedisCacheFactory { - IRedisCache Create(string stateKey) where T : class; + IRedisCache Create(string stateKey, bool includeTypeInKey = true) where T : class; IRedisCache Create(string stateKey); } @@ -23,8 +23,13 @@ public RedisCacheFactory(ConfigurationOptions options, ILogger logge _logger = logger; } - public IRedisCache Create(string stateKey) where T : class + public IRedisCache Create(string stateKey, bool includeTypeInKey = true) where T : class { + if (includeTypeInKey) + { + stateKey = $"{typeof(T).Name}_{stateKey}"; + } + return new RedisCache(Create(stateKey), _logger); } diff --git a/src/ProductConstructionService/ProductConstructionService.Common/RedisMutex.cs b/src/ProductConstructionService/ProductConstructionService.Common/RedisMutex.cs index adfbb32fde..aedf8d4f3b 100644 --- a/src/ProductConstructionService/ProductConstructionService.Common/RedisMutex.cs +++ b/src/ProductConstructionService/ProductConstructionService.Common/RedisMutex.cs @@ -35,7 +35,8 @@ public async Task EnterWhenAvailable( { lockTime = TimeSpan.FromHours(1); } - IRedisCache mutexCache = _cacheFactory.Create($"{mutexName}_mutex"); + + IRedisCache mutexCache = _cacheFactory.Create($"Mutex_{mutexName}"); try { @@ -49,16 +50,17 @@ public async Task EnterWhenAvailable( MutexWakeUpTimeSeconds, () => _logger.LogInformation("Waiting for mutex {mutexName} mutexName to become available", mutexName))); _logger.LogInformation("Taking mutex {mutexName}", mutexName); + // If for whatever reason we get stuck in action, we don't want the mutex to lock forever - // It will release the lock after lockTimeHours + // It will release the lock after lockTime await mutexCache.SetAsync("busy", lockTime); return await action(); } finally { - _logger.LogInformation("Releasing mutex {mutexName}", mutexName); // When we're done, or get an exception, release the mutex and let others try + _logger.LogInformation("Releasing mutex {mutexName}", mutexName); await mutexCache.TryDeleteAsync(); } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/BatchedPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/BatchedPullRequestUpdater.cs index 68133be9d8..d54db450be 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/BatchedPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/BatchedPullRequestUpdater.cs @@ -4,16 +4,13 @@ using Maestro.Data; using Maestro.Data.Models; using Microsoft.DotNet.DarcLib; +using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.Extensions.Logging; using ProductConstructionService.Common; using ProductConstructionService.WorkItems; namespace ProductConstructionService.DependencyFlow; -/// -/// A for batched subscriptions that reads its Target and Merge Policies -/// from the configuration for a repository -/// internal class BatchedPullRequestUpdater : PullRequestUpdater { private readonly BatchedPullRequestUpdaterId _id; @@ -29,7 +26,12 @@ public BatchedPullRequestUpdater( IPullRequestBuilder pullRequestBuilder, IRedisCacheFactory cacheFactory, IReminderManagerFactory reminderManagerFactory, - IWorkItemProducerFactory workItemProducerFactory, + IBasicBarClient barClient, + ILocalLibGit2Client gitClient, + IVmrInfo vmrInfo, + IPcsVmrForwardFlower vmrForwardFlower, + IPcsVmrBackFlower vmrBackFlower, + ITelemetryRecorder telemetryRecorder, ILogger logger) : base( id, @@ -40,7 +42,12 @@ public BatchedPullRequestUpdater( pullRequestBuilder, cacheFactory, reminderManagerFactory, - workItemProducerFactory, + barClient, + gitClient, + vmrInfo, + vmrForwardFlower, + vmrBackFlower, + telemetryRecorder, logger) { _id = id; diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs index a2fe73e927..2e1f8f57bc 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs @@ -28,8 +28,7 @@ public static void AddDependencyFlowProcessors(this IServiceCollection services) services.TryAddSingleton(); services.AddWorkItemProcessor(); - services.AddWorkItemProcessor(); - services.AddWorkItemProcessor(); + services.AddWorkItemProcessor(); services.AddWorkItemProcessor(); services.AddWorkItemProcessor(); } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestUpdater.cs index a749d36669..8509465836 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestUpdater.cs @@ -7,8 +7,8 @@ namespace ProductConstructionService.DependencyFlow; public interface IPullRequestUpdater { - Task SynchronizeInProgressPullRequestAsync( - InProgressPullRequest pullRequestCheck); + Task CheckPullRequestAsync( + PullRequestCheck pullRequestCheck); Task ProcessPendingUpdatesAsync( SubscriptionUpdateWorkItem update); diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/InProgressPullRequest.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs similarity index 68% rename from src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/InProgressPullRequest.cs rename to src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs index 2efb2f1d0f..97fb1aa9ed 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/InProgressPullRequest.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs @@ -3,15 +3,22 @@ using System.Runtime.Serialization; using Maestro.Contracts; +using ProductConstructionService.DependencyFlow.WorkItems; #nullable disable -namespace ProductConstructionService.DependencyFlow.WorkItems; +namespace ProductConstructionService.DependencyFlow; [DataContract] -public class InProgressPullRequest : ActorWorkItem, IPullRequest +public class InProgressPullRequest : DependencyFlowWorkItem, IPullRequest { [DataMember] - public string Url { get; set; } + public required string Url { get; set; } + + [DataMember] + public required string HeadBranch { get; set; } + + [DataMember] + public required string SourceSha { get; set; } [DataMember] public bool? CoherencyCheckSuccessful { get; set; } @@ -23,7 +30,7 @@ public class InProgressPullRequest : ActorWorkItem, IPullRequest public MergePolicyCheckResult MergePolicyResult { get; init; } [DataMember] - public List ContainedSubscriptions { get; init; } + public List ContainedSubscriptions { get; set; } [DataMember] public List Contained { get; init; } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/NonBatchedPullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/NonBatchedPullRequestUpdater.cs index 25d0feea2e..e7896ddff3 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/NonBatchedPullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/NonBatchedPullRequestUpdater.cs @@ -4,17 +4,13 @@ using Maestro.Data; using Maestro.Data.Models; using Microsoft.DotNet.DarcLib; +using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.Extensions.Logging; using ProductConstructionService.Common; -using ProductConstructionService.DependencyFlow.WorkItems; using ProductConstructionService.WorkItems; namespace ProductConstructionService.DependencyFlow; -/// -/// A that reads its Merge Policies and Target information from a -/// non-batched subscription object -/// internal class NonBatchedPullRequestUpdater : PullRequestUpdater { private readonly Lazy> _lazySubscription; @@ -33,7 +29,12 @@ public NonBatchedPullRequestUpdater( IPullRequestPolicyFailureNotifier pullRequestPolicyFailureNotifier, IRedisCacheFactory cacheFactory, IReminderManagerFactory reminderManagerFactory, - IWorkItemProducerFactory workItemProducerFactory, + IBasicBarClient barClient, + ILocalLibGit2Client gitClient, + IVmrInfo vmrInfo, + IPcsVmrForwardFlower vmrForwardFlower, + IPcsVmrBackFlower vmrBackFlower, + ITelemetryRecorder telemetryRecorder, ILogger logger) : base( id, @@ -44,7 +45,12 @@ public NonBatchedPullRequestUpdater( pullRequestBuilder, cacheFactory, reminderManagerFactory, - workItemProducerFactory, + barClient, + gitClient, + vmrInfo, + vmrForwardFlower, + vmrBackFlower, + telemetryRecorder, logger) { _lazySubscription = new Lazy>(RetrieveSubscription); @@ -91,7 +97,7 @@ protected override async Task> GetMergePoli return subscription?.PolicyObject?.MergePolicies ?? []; } - public override async Task SynchronizeInProgressPullRequestAsync( + protected override async Task CheckInProgressPullRequestAsync( InProgressPullRequest pullRequestCheck) { Subscription? subscription = await GetSubscription(); @@ -100,6 +106,6 @@ public override async Task SynchronizeInProgressPullRequestAsync( return false; } - return await base.SynchronizeInProgressPullRequestAsync(pullRequestCheck); + return await base.CheckInProgressPullRequestAsync(pullRequestCheck); } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestBuilder.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestBuilder.cs index 4737c9c3c6..4e33e91e0c 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestBuilder.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestBuilder.cs @@ -36,7 +36,7 @@ Task CalculatePRDescriptionAndCommitUpdatesAsync( /// Current in progress pull request information /// Pull request title Task GeneratePRTitleAsync( - InProgressPullRequest inProgressPr, + List subscriptions, string targetBranch); /// @@ -78,12 +78,12 @@ public PullRequestBuilder( _logger = logger; } - public async Task GeneratePRTitleAsync(InProgressPullRequest inProgressPr, string targetBranch) + public async Task GeneratePRTitleAsync(List subscriptions, string targetBranch) { // Get the unique subscription IDs. It may be possible for a coherency update // to not have any contained subscription. In this case // we return a different title. - var uniqueSubscriptionIds = inProgressPr.ContainedSubscriptions + var uniqueSubscriptionIds = subscriptions .Select(subscription => subscription.SubscriptionId) .Distinct() .ToArray(); diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestPolicyFailureNotifier.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestPolicyFailureNotifier.cs index 66bff62874..6eef47b593 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestPolicyFailureNotifier.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestPolicyFailureNotifier.cs @@ -4,7 +4,6 @@ using Microsoft.DotNet.DarcLib; using Microsoft.DotNet.GitHub.Authentication; using Microsoft.Extensions.Logging; -using ProductConstructionService.DependencyFlow.WorkItems; namespace ProductConstructionService.DependencyFlow; diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 2d053e1329..84e69cd523 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -6,6 +6,8 @@ using Maestro.Data.Models; using Maestro.MergePolicyEvaluation; using Microsoft.DotNet.DarcLib; +using Microsoft.DotNet.DarcLib.Helpers; +using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.Extensions.Logging; using ProductConstructionService.Common; using ProductConstructionService.DependencyFlow.WorkItems; @@ -21,28 +23,27 @@ namespace ProductConstructionService.DependencyFlow; /// internal abstract class PullRequestUpdater : IPullRequestUpdater { - private static readonly TimeSpan DefaultReminderDuration = TimeSpan.FromMinutes(5); + private static readonly TimeSpan DefaultReminderDelay = TimeSpan.FromMinutes(5); - private readonly PullRequestUpdaterId _id; private readonly IMergePolicyEvaluator _mergePolicyEvaluator; private readonly IRemoteFactory _remoteFactory; private readonly IPullRequestUpdaterFactory _updaterFactory; private readonly ICoherencyUpdateResolver _coherencyUpdateResolver; private readonly IPullRequestBuilder _pullRequestBuilder; - // TODO (https://github.com/dotnet/arcade-services/issues/3866): When removed, remove the mocks from tests - private readonly IWorkItemProducerFactory _workItemProducerFactory; + private readonly IBasicBarClient _barClient; + private readonly ILocalLibGit2Client _gitClient; + private readonly IVmrInfo _vmrInfo; + private readonly IPcsVmrForwardFlower _vmrForwardFlower; + private readonly IPcsVmrBackFlower _vmrBackFlower; + private readonly ITelemetryRecorder _telemetryRecorder; private readonly ILogger _logger; protected readonly IReminderManager _pullRequestUpdateReminders; - protected readonly IReminderManager _pullRequestCheckReminders; + protected readonly IReminderManager _pullRequestCheckReminders; protected readonly IRedisCache _pullRequestState; - protected readonly IRedisCache _codeFlowState; - public PullRequestUpdaterId Id { get { return _id; } } + public PullRequestUpdaterId Id { get; } - /// - /// Creates a new PullRequestActor - /// public PullRequestUpdater( PullRequestUpdaterId id, IMergePolicyEvaluator mergePolicyEvaluator, @@ -52,22 +53,32 @@ public PullRequestUpdater( IPullRequestBuilder pullRequestBuilder, IRedisCacheFactory cacheFactory, IReminderManagerFactory reminderManagerFactory, - IWorkItemProducerFactory workItemProducerFactory, + IBasicBarClient barClient, + ILocalLibGit2Client gitClient, + IVmrInfo vmrInfo, + IPcsVmrForwardFlower vmrForwardFlower, + IPcsVmrBackFlower vmrBackFlower, + ITelemetryRecorder telemetryRecorder, ILogger logger) { - _id = id; + Id = id; _mergePolicyEvaluator = mergePolicyEvaluator; _remoteFactory = remoteFactory; _updaterFactory = updaterFactory; _coherencyUpdateResolver = coherencyUpdateResolver; _pullRequestBuilder = pullRequestBuilder; - _workItemProducerFactory = workItemProducerFactory; + _barClient = barClient; + _gitClient = gitClient; + _vmrInfo = vmrInfo; + _vmrForwardFlower = vmrForwardFlower; + _vmrBackFlower = vmrBackFlower; + _telemetryRecorder = telemetryRecorder; _logger = logger; - _pullRequestUpdateReminders = reminderManagerFactory.CreateReminderManager(id.Id); - _pullRequestCheckReminders = reminderManagerFactory.CreateReminderManager(id.Id); - _pullRequestState = cacheFactory.Create(id.Id); - _codeFlowState = cacheFactory.Create(id.Id); + var cacheKey = id.ToString(); + _pullRequestUpdateReminders = reminderManagerFactory.CreateReminderManager(cacheKey); + _pullRequestCheckReminders = reminderManagerFactory.CreateReminderManager(cacheKey); + _pullRequestState = cacheFactory.Create(cacheKey); } protected abstract Task<(string repository, string branch)> GetTargetAsync(); @@ -93,13 +104,21 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up } else { - var canUpdate = await SynchronizeInProgressPullRequestAsync(pr); - if (!canUpdate) + switch (await GetPullRequestStatusAsync(pr)) { - _logger.LogInformation("PR {url} for subscription {subscriptionId} cannot be updated at this time", pr.Url, update.SubscriptionId); - await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDuration); - await _pullRequestCheckReminders.UnsetReminderAsync(); - return false; + case PullRequestStatus.Completed: + case PullRequestStatus.Invalid: + // If the PR is completed, we will open a new one + pr = null; + break; + case PullRequestStatus.InProgressCanUpdate: + // If we can update it, we will do it below + break; + default: + _logger.LogInformation("PR {url} for subscription {subscriptionId} cannot be updated at this time", pr.Url, update.SubscriptionId); + await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay); + await _pullRequestCheckReminders.UnsetReminderAsync(); + return false; } } @@ -133,70 +152,55 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up return true; } - protected virtual Task TagSourceRepositoryGitHubContactsIfPossibleAsync(InProgressPullRequest pr) + public async Task CheckPullRequestAsync(PullRequestCheck pullRequestCheck) { - // Only do actual stuff in the non-batched implementation - return Task.CompletedTask; - } + var inProgressPr = await _pullRequestState.TryGetStateAsync(); - /// - /// Synchronizes an in progress pull request. - /// This will update current state if the pull request has been manually closed or merged. - /// This will evaluate merge policies on an in progress pull request and merge the pull request if policies allow. - /// - /// - /// True, if the open pull request can be updated. - /// - public virtual async Task SynchronizeInProgressPullRequestAsync(InProgressPullRequest pullRequestCheck) - { - if (string.IsNullOrEmpty(pullRequestCheck.Url)) + if (inProgressPr == null) { - await _pullRequestState.TryDeleteAsync(); - await _codeFlowState.TryDeleteAsync(); - _logger.LogWarning("Removing invalid PR {url} from state memory", pullRequestCheck.Url); + _logger.LogInformation("No in-progress pull request found for a PR check"); return false; } - PullRequestStatus result = await GetPullRequestStateAsync(pullRequestCheck); + return await CheckInProgressPullRequestAsync(inProgressPr); + } - _logger.LogInformation("Pull request {url} is {result}", pullRequestCheck.Url, result); + protected virtual async Task CheckInProgressPullRequestAsync(InProgressPullRequest pullRequestCheck) + { + _logger.LogInformation("Checking in-progress pull request {url}", pullRequestCheck.Url); - switch (result) + var status = await GetPullRequestStatusAsync(pullRequestCheck); + + // Schedule another check if PR is not merged yet + if (status == PullRequestStatus.InProgressCanUpdate || status == PullRequestStatus.InProgressCannotUpdate) { - // If the PR was merged or closed, we are done with it and we don't - // need to periodically run the synchronization any longer. - case PullRequestStatus.Completed: - case PullRequestStatus.UnknownPR: - return false; - case PullRequestStatus.InProgressCanUpdate: - await SetPullRequestCheckReminder(pullRequestCheck); - return true; - case PullRequestStatus.InProgressCannotUpdate: - await SetPullRequestCheckReminder(pullRequestCheck); - return false; - case PullRequestStatus.Invalid: - // We could have gotten here if there was an exception during - // the synchronization process. This was typical in the past - // when we would regularly get credential exceptions on github tokens - // that were just obtained. We don't want to unregister the reminder in these cases. - return false; - default: - _logger.LogError("Unknown pull request synchronization result {result}", result); - return false; + _logger.LogInformation("Pull request {url} still active - keeping tracking it", pullRequestCheck.Url); + await SetPullRequestCheckReminder(pullRequestCheck); } + else + { + _logger.LogInformation("Pull request {url} was completed/closed - stopped tracking it", pullRequestCheck.Url); + } + + return status != PullRequestStatus.Invalid; } - private async Task GetPullRequestStateAsync(InProgressPullRequest pr) + protected virtual Task TagSourceRepositoryGitHubContactsIfPossibleAsync(InProgressPullRequest pr) + { + // Only do actual stuff in the non-batched implementation + return Task.CompletedTask; + } + + protected async Task GetPullRequestStatusAsync(InProgressPullRequest pr) { - var prUrl = pr.Url; - _logger.LogInformation("Synchronizing pull request {prUrl}", prUrl); + _logger.LogInformation("Querying status for pull request {prUrl}", pr.Url); (var targetRepository, _) = await GetTargetAsync(); IRemote remote = await _remoteFactory.GetRemoteAsync(targetRepository, _logger); - _logger.LogInformation("Getting status for pull request: {url}", prUrl); - PrStatus status = await remote.GetPullRequestStatusAsync(prUrl); - _logger.LogInformation("Pull request {url} is {status}", prUrl, status); + PrStatus status = await remote.GetPullRequestStatusAsync(pr.Url); + _logger.LogInformation("Pull request {url} is {status}", pr.Url, status); + switch (status) { // If the PR is currently open, then evaluate the merge policies, which will potentially @@ -204,7 +208,7 @@ private async Task GetPullRequestStateAsync(InProgressPullReq case PrStatus.Open: var mergePolicyResult = await CheckMergePolicyAsync(pr, remote); - _logger.LogInformation("Policy check status for pull request {url} is {result}", prUrl, mergePolicyResult); + _logger.LogInformation("Policy check status for pull request {url} is {result}", pr.Url, mergePolicyResult); switch (mergePolicyResult) { @@ -215,7 +219,7 @@ await AddDependencyFlowEventsAsync( DependencyFlowEventType.Completed, DependencyFlowEventReason.AutomaticallyMerged, mergePolicyResult, - prUrl); + pr.Url); await ClearAllStateAsync(); return PullRequestStatus.Completed; @@ -252,19 +256,19 @@ await AddDependencyFlowEventsAsync( DependencyFlowEventType.Completed, reason, pr.MergePolicyResult, - prUrl); + pr.Url); await ClearAllStateAsync(); // Also try to clean up the PR branch. try { - _logger.LogInformation("Trying to clean up the branch for pull request {url}", prUrl); - await remote.DeletePullRequestBranchAsync(prUrl); + _logger.LogInformation("Trying to clean up the branch for pull request {url}", pr.Url); + await remote.DeletePullRequestBranchAsync(pr.Url); } catch (DarcException) { - _logger.LogInformation("Failed to delete branch associated with pull request {url}", prUrl); + _logger.LogInformation("Failed to delete branch associated with pull request {url}", pr.Url); } _logger.LogInformation("PR has been manually {action}", status); @@ -405,12 +409,19 @@ public async Task UpdateAssetsAsync( } else { - canUpdate = await SynchronizeInProgressPullRequestAsync(pr); + var status = await GetPullRequestStatusAsync(pr); + canUpdate = status == PullRequestStatus.InProgressCanUpdate; + + if (status == PullRequestStatus.Completed || status == PullRequestStatus.Invalid) + { + // If the PR is completed, we will open a new one + pr = null; + } } var update = new SubscriptionUpdateWorkItem { - ActorId = _id.ToString(), + UpdaterId = Id.ToString(), SubscriptionId = subscriptionId, SubscriptionType = type, BuildId = buildId, @@ -423,7 +434,7 @@ public async Task UpdateAssetsAsync( // Regardless of code flow or regular PR, if the PR are not complete, postpone the update if (pr != null && !canUpdate) { - await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDuration); + await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay); _logger.LogInformation("Pull request '{prUrl}' cannot be updated, update queued", pr!.Url); return true; } @@ -493,21 +504,35 @@ public async Task UpdateAssetsAsync( targetRepository, newBranchName); + var containedSubscriptions = repoDependencyUpdate.RequiredUpdates + // Coherency updates do not have subscription info. + .Where(u => !u.update.IsCoherencyUpdate) + .Select( + u => new SubscriptionPullRequestUpdate + { + SubscriptionId = u.update.SubscriptionId, + BuildId = u.update.BuildId + }) + .ToList(); + + var prUrl = await darcRemote.CreatePullRequestAsync( + targetRepository, + new PullRequest + { + Title = await _pullRequestBuilder.GeneratePRTitleAsync(containedSubscriptions, targetBranch), + Description = description, + BaseBranch = targetBranch, + HeadBranch = newBranchName, + }); + var inProgressPr = new InProgressPullRequest { - ActorId = _id.ToString(), - - // Calculate the subscriptions contained within the - // update. Coherency updates do not have subscription info. - ContainedSubscriptions = repoDependencyUpdate.RequiredUpdates - .Where(u => !u.update.IsCoherencyUpdate) - .Select( - u => new SubscriptionPullRequestUpdate - { - SubscriptionId = u.update.SubscriptionId, - BuildId = u.update.BuildId - }) - .ToList(), + UpdaterId = Id.ToString(), + Url = prUrl, + HeadBranch = newBranchName, + SourceSha = update.SourceSha, + + ContainedSubscriptions = containedSubscriptions, RequiredUpdates = repoDependencyUpdate.RequiredUpdates .SelectMany(update => update.deps) @@ -523,16 +548,6 @@ public async Task UpdateAssetsAsync( CoherencyErrors = repoDependencyUpdate.CoherencyErrors }; - var prUrl = await darcRemote.CreatePullRequestAsync( - targetRepository, - new PullRequest - { - Title = await _pullRequestBuilder.GeneratePRTitleAsync(inProgressPr, targetBranch), - Description = description, - BaseBranch = targetBranch, - HeadBranch = newBranchName, - }); - if (!string.IsNullOrEmpty(prUrl)) { inProgressPr.Url = prUrl; @@ -550,11 +565,11 @@ await AddDependencyFlowEventsAsync( // If we did not create a PR, then mark the dependency flow as completed as nothing to do. await AddDependencyFlowEventsAsync( - inProgressPr.ContainedSubscriptions, - DependencyFlowEventType.Completed, - DependencyFlowEventReason.NothingToDo, - MergePolicyCheckResult.PendingPolicies, - null); + inProgressPr.ContainedSubscriptions, + DependencyFlowEventType.Completed, + DependencyFlowEventReason.NothingToDo, + MergePolicyCheckResult.PendingPolicies, + null); // Something wrong happened when trying to create the PR but didn't throw an exception (probably there was no diff). // We need to delete the branch also in this case. @@ -642,7 +657,7 @@ await AddDependencyFlowEventsAsync( targetRepository, pullRequest.HeadBranch); - pullRequest.Title = await _pullRequestBuilder.GeneratePRTitleAsync(pr, targetBranch); + pullRequest.Title = await _pullRequestBuilder.GeneratePRTitleAsync(pr.ContainedSubscriptions, targetBranch); await darcRemote.UpdatePullRequestAsync(pr.Url, pullRequest); await SetPullRequestCheckReminder(pr); @@ -700,7 +715,7 @@ private class TargetRepoDependencyUpdate /// Given a set of input updates from builds, determine what updates /// are required in the target repository. /// - /// Updates + /// Update /// Target repository to calculate updates for /// PR head branch /// Target branch @@ -793,7 +808,7 @@ await UpdateSubscriptionsForMergedPRAsync( // since coherency can be run even without any updates. var coherencyUpdateParameters = new SubscriptionUpdateWorkItem { - ActorId = _id.Id, + UpdaterId = Id.Id, IsCoherencyUpdate = true }; repoDependencyUpdate.RequiredUpdates.Add((coherencyUpdateParameters, coherencyUpdates.ToList())); @@ -807,14 +822,13 @@ await UpdateSubscriptionsForMergedPRAsync( private async Task SetPullRequestCheckReminder(InProgressPullRequest prState) { - await _pullRequestCheckReminders.SetReminderAsync(prState, DefaultReminderDuration); + await _pullRequestCheckReminders.SetReminderAsync(new() { UpdaterId = Id.ToString() }, DefaultReminderDelay); await _pullRequestState.SetAsync(prState); } private async Task ClearAllStateAsync() { await _pullRequestState.TryDeleteAsync(); - await _codeFlowState.TryDeleteAsync(); await _pullRequestCheckReminders.UnsetReminderAsync(); await _pullRequestUpdateReminders.UnsetReminderAsync(); } @@ -828,7 +842,19 @@ private async Task ProcessCodeFlowUpdateAsync( SubscriptionUpdateWorkItem update, InProgressPullRequest? pr) { - CodeFlowStatus? codeFlowStatus = await _codeFlowState.TryGetStateAsync(); + // Compare last SHA with the build SHA to see if we already have this SHA in the PR + if (update.SourceSha == pr?.SourceSha) + { + _logger.LogInformation("PR {url} for {subscription} is already up to date ({sha})", + pr.Url, + update.SubscriptionId, + update.SourceSha); + + await SetPullRequestCheckReminder(pr); + await _pullRequestUpdateReminders.UnsetReminderAsync(); + + return true; + } // The E2E order of things for is: // 1. We send a request to PCS and wait for a branch to be created. We note down this in the codeflow status. We set a reminder. @@ -839,137 +865,225 @@ private async Task ProcessCodeFlowUpdateAsync( { (var targetRepository, var targetBranch) = await GetTargetAsync(); - // Step 1. Let PCS create a branch for us - if (codeFlowStatus == null) + // Step 1. Create a branch for us + var prBranch = await CreateCodeFlowBranchAsync(update, targetBranch); + if (prBranch == null) { - return await RequestCodeFlowBranchAsync(update, targetBranch); - } - - // Step 2. Wait for the branch to be created - IRemote remote = await _remoteFactory.GetRemoteAsync(targetRepository, _logger); - if (!await remote.BranchExistsAsync(targetRepository, codeFlowStatus.PrBranch)) - { - _logger.LogInformation("Branch {branch} for subscription {subscriptionId} not created yet. Will check again later", - codeFlowStatus.PrBranch, - update.SubscriptionId); - - await _pullRequestUpdateReminders.SetReminderAsync(update, TimeSpan.FromMinutes(3)); + _logger.LogInformation("No changes required for subscription {subscriptionId}, no pull request created", update.SubscriptionId); return true; } - // Step 3. Create a PR - var prUrl = await CreateCodeFlowPullRequestAsync( + // Step 2. Create a PR + await CreateCodeFlowPullRequestAsync( update, targetRepository, - codeFlowStatus.PrBranch, + prBranch, targetBranch); - _logger.LogInformation("Pending updates applied. PR {prUrl} created", prUrl); return true; } - // Technically, this should never happen as we create the code flow data before we even create the PR - if (codeFlowStatus == null) + // Step 3. Update the PR + try { - _logger.LogError("Missing code flow data for subscription {subscription}", update.SubscriptionId); - await _pullRequestUpdateReminders.UnsetReminderAsync(); - await _pullRequestCheckReminders.UnsetReminderAsync(); - return false; + await UpdateAssetsAndSources(update, pr); } - - // Step 4. Update the PR (if needed) - - // Compare last SHA with the build SHA to see if we need to delegate this update to PCS - if (update.SourceSha == codeFlowStatus.SourceSha) + catch (Exception e) { - _logger.LogInformation("PR {url} for {subscription} is up to date ({sha})", + // TODO https://github.com/dotnet/arcade-services/issues/3318: Handle this - Maybe we need to set a reminder and try again? + _logger.LogError(e, "Failed to update sources and packages for PR {url} of subscription {subscriptionId}", pr.Url, - update.SubscriptionId, - update.SourceSha); + update.SubscriptionId); return false; } + _logger.LogInformation("Code flow update processed for pull request {prUrl}", pr.Url); + return true; + } + + /// + /// Updates an existing code-flow branch with new changes. Returns true if there were updates to push. + /// + private async Task UpdateAssetsAndSources(SubscriptionUpdateWorkItem update, InProgressPullRequest pullRequest) + { + var subscription = await _barClient.GetSubscriptionAsync(update.SubscriptionId) + ?? throw new Exception($"Subscription {update.SubscriptionId} not found"); + var build = await _barClient.GetBuildAsync(update.BuildId) + ?? throw new Exception($"Build {update.BuildId} not found"); + + var isForwardFlow = subscription.TargetDirectory != null; + + _logger.LogInformation( + "{direction}-flowing build {buildId} for subscription {subscriptionId} targeting {repo} / {targetBranch} to new branch {newBranch}", + isForwardFlow ? "Forward" : "Back", + update.BuildId, + subscription.Id, + subscription.TargetRepository, + subscription.TargetBranch, + pullRequest.HeadBranch); + + bool hadUpdates; + NativePath targetRepo; + try { - // TODO (https://github.com/dotnet/arcade-services/issues/3866): Execute code flow logic immediately - var producer = _workItemProducerFactory.CreateProducer(); - await producer.ProduceWorkItemAsync(new CodeFlowWorkItem + if (isForwardFlow) { - BuildId = update.BuildId, - SubscriptionId = update.SubscriptionId, - PrBranch = codeFlowStatus.PrBranch, - PrUrl = pr.Url, - }); + targetRepo = _vmrInfo.VmrPath; + hadUpdates = await _vmrForwardFlower.FlowForwardAsync( + subscription.TargetDirectory!, + build, + subscription.TargetBranch, + pullRequest.HeadBranch, + cancellationToken: default); + } + else + { + (hadUpdates, targetRepo) = await _vmrBackFlower.FlowBackAsync( + subscription.SourceDirectory!, + build, + subscription.TargetBranch, + pullRequest.HeadBranch, + cancellationToken: default); + } + } + catch (Exception e) + { + _logger.LogError(e, "Failed to flow changes for build {buildId} in subscription {subscriptionId}", + update.BuildId, + subscription.Id); + throw; + } - codeFlowStatus.SourceSha = update.SourceSha; + if (hadUpdates) + { + _logger.LogInformation("Code changes for {subscriptionId} ready in local branch {branch}", + subscription.Id, + subscription.TargetBranch); - // TODO (https://github.com/dotnet/arcade-services/issues/3866): We need to update the InProgressPullRequest fully, assets and other info just like we do in UpdatePullRequestAsync - // Right now, we are not flowing packages in codeflow subscriptions yet, so this functionality is no there - // For now, we manually update the info the unit tests expect - pr.ContainedSubscriptions.Clear(); - pr.ContainedSubscriptions.Add(new SubscriptionPullRequestUpdate + // TODO https://github.com/dotnet/arcade-services/issues/3318: Handle failures (conflict, non-ff etc) + using (var scope = _telemetryRecorder.RecordGitOperation(TrackedGitOperation.Push, subscription.TargetRepository)) { - SubscriptionId = update.SubscriptionId, - BuildId = update.BuildId - }); - - await _codeFlowState.SetAsync(codeFlowStatus); - await SetPullRequestCheckReminder(pr); - await _pullRequestUpdateReminders.UnsetReminderAsync(); + await _gitClient.Push(targetRepo, pullRequest.HeadBranch, subscription.TargetRepository); + scope.SetSuccess(); + } } - catch (Exception e) + else { - // TODO https://github.com/dotnet/arcade-services/issues/3318: Handle this - _logger.LogError(e, "Failed to request branch update for PR {url} for subscription {subscriptionId}", - pr.Url, - update.SubscriptionId); + _logger.LogInformation("There were no code-flow updates for subscription {subscriptionId}", + subscription.Id); } - _logger.LogInformation("New code flow changes requested"); + pullRequest.SourceSha = update.SourceSha; + + // TODO (https://github.com/dotnet/arcade-services/issues/3866): We need to update the InProgressPullRequest fully, assets and other info just like we do in UpdatePullRequestAsync + // Right now, we are not flowing packages in codeflow subscriptions yet, so this functionality is no there + // For now, we manually update the info the unit tests expect + pullRequest.ContainedSubscriptions.Clear(); + pullRequest.ContainedSubscriptions = + [ + new SubscriptionPullRequestUpdate + { + SubscriptionId = update.SubscriptionId, + BuildId = update.BuildId + } + ]; + + await SetPullRequestCheckReminder(pullRequest); + await _pullRequestUpdateReminders.UnsetReminderAsync(); + return true; } - private async Task RequestCodeFlowBranchAsync(SubscriptionUpdateWorkItem update, string targetBranch) + /// + /// Creates the code flow branch for the given subscription update. + /// + private async Task CreateCodeFlowBranchAsync(SubscriptionUpdateWorkItem update, string targetBranch) { - CodeFlowStatus codeFlowUpdate = new() - { - PrBranch = GetNewBranchName(targetBranch), - SourceSha = update.SourceSha, - }; + string newBranchName = GetNewBranchName(targetBranch); _logger.LogInformation( "New code flow request for subscription {subscriptionId}. Requesting branch {branch} from PCS", update.SubscriptionId, - codeFlowUpdate.PrBranch); + newBranchName); + + var subscription = await _barClient.GetSubscriptionAsync(update.SubscriptionId) + ?? throw new Exception($"Subscription {update.SubscriptionId} not found"); + + if (!subscription.SourceEnabled || (subscription.SourceDirectory ?? subscription.TargetDirectory) == null) + { + throw new Exception($"Subscription {update.SubscriptionId} is not source enabled or source directory is not set"); + } + + var build = await _barClient.GetBuildAsync(update.BuildId) + ?? throw new Exception($"Build {update.BuildId} not found"); + var isForwardFlow = subscription.TargetDirectory != null; + + _logger.LogInformation( + "{direction}-flowing build {buildId} for subscription {subscriptionId} targeting {repo} / {targetBranch} to new branch {newBranch}", + isForwardFlow ? "Forward" : "Back", + build.Id, + subscription.Id, + subscription.TargetRepository, + subscription.TargetBranch, + newBranchName); + + bool hadUpdates; + NativePath targetRepo; try { - // TODO (https://github.com/dotnet/arcade-services/issues/3866): Execute code flow logic immediately - var producer = _workItemProducerFactory.CreateProducer(); - await producer.ProduceWorkItemAsync(new CodeFlowWorkItem + if (isForwardFlow) { - BuildId = update.BuildId, - SubscriptionId = update.SubscriptionId, - PrBranch = codeFlowUpdate.PrBranch, - }); + targetRepo = _vmrInfo.VmrPath; + hadUpdates = await _vmrForwardFlower.FlowForwardAsync( + subscription.TargetDirectory!, + build, + subscription.TargetBranch, + newBranchName, + cancellationToken: default); + } + else + { + (hadUpdates, targetRepo) = await _vmrBackFlower.FlowBackAsync( + subscription.SourceDirectory!, + build, + subscription.TargetBranch, + newBranchName, + cancellationToken: default); + } } catch (Exception e) { - // TODO https://github.com/dotnet/arcade-services/issues/3318: Handle this - _logger.LogError(e, "Failed to request new branch {branch} for subscription {subscriptionId}", - codeFlowUpdate.PrBranch, - update.SubscriptionId); - return false; + _logger.LogError(e, "Failed to flow changes for build {buildId} in subscription {subscriptionId}", + build.Id, + subscription.Id); + throw; } - await _codeFlowState.SetAsync(codeFlowUpdate); - await _pullRequestUpdateReminders.SetReminderAsync(update, TimeSpan.FromMinutes(3)); + if (!hadUpdates) + { + _logger.LogInformation("There were no code-flow updates for subscription {subscriptionId}", + subscription.Id); + return null; + } - _logger.LogInformation("Pending updates applied. Branch {prBranch} requested from PCS", codeFlowUpdate.PrBranch); - return true; + _logger.LogInformation("Code changes for {subscriptionId} ready in local branch {branch}", + subscription.Id, + subscription.TargetBranch); + + // TODO https://github.com/dotnet/arcade-services/issues/3318: Handle failures (conflict, non-ff etc) + using (var scope = _telemetryRecorder.RecordGitOperation(TrackedGitOperation.Push, subscription.TargetRepository)) + { + await _gitClient.Push(targetRepo, newBranchName, subscription.TargetRepository); + scope.SetSuccess(); + } + + _logger.LogInformation("Code-flow branch {prBranch} pushed", newBranchName); + return newBranchName; } - private async Task CreateCodeFlowPullRequestAsync( + private async Task CreateCodeFlowPullRequestAsync( SubscriptionUpdateWorkItem update, string targetRepository, string prBranch, @@ -992,10 +1106,13 @@ private async Task CreateCodeFlowPullRequestAsync( HeadBranch = prBranch, }); + // TODO (https://github.com/dotnet/arcade-services/issues/3866): Populate fully (assets, coherency checks..) InProgressPullRequest inProgressPr = new() { - ActorId = _id.ToString(), + UpdaterId = Id.ToString(), Url = prUrl, + HeadBranch = prBranch, + SourceSha = update.SourceSha, ContainedSubscriptions = [ new() @@ -1003,7 +1120,7 @@ private async Task CreateCodeFlowPullRequestAsync( SubscriptionId = update.SubscriptionId, BuildId = update.BuildId } - ], + ] }; await AddDependencyFlowEventsAsync( @@ -1016,10 +1133,12 @@ await AddDependencyFlowEventsAsync( await SetPullRequestCheckReminder(inProgressPr); await _pullRequestUpdateReminders.UnsetReminderAsync(); - return prUrl; + _logger.LogInformation("Code flow pull request created: {prUrl}", prUrl); } - catch + catch (Exception e) { + _logger.LogError(e, "Failed to create code flow pull request for subscription {subscriptionId}", + update.SubscriptionId); await darcRemote.DeleteBranchAsync(targetRepository, prBranch); throw; } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaterId.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaterId.cs index 6cac0f2b61..2ec90312ef 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaterId.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaterId.cs @@ -37,11 +37,6 @@ public class BatchedPullRequestUpdaterId : PullRequestUpdaterId public string Repository { get; } public string Branch { get; } - /// - /// Creates an identifying the PullRequestActor responsible for pull requests for all batched - /// subscriptions - /// targeting the (, ) pair. - /// public BatchedPullRequestUpdaterId(string repository, string branch) : base(Encode(repository) + ":" + Encode(branch)) { @@ -89,9 +84,3 @@ protected static string Decode(string value) return Encoding.UTF8.GetString(Convert.FromBase64String(value)); } } - -public enum ActorIdKind -{ - Guid, - String -} diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/SubscriptionTriggerer.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/SubscriptionTriggerer.cs index eb0f221c07..3caae0e2af 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/SubscriptionTriggerer.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/SubscriptionTriggerer.cs @@ -121,7 +121,7 @@ await AddDependencyFlowEventAsync( .ThenInclude(a => a.Locations) .FirstAsync(b => b.Id == buildId); - IPullRequestUpdater pullRequestActor; + IPullRequestUpdater pullRequestUpdater; if (subscription.PolicyObject.Batchable) { @@ -129,7 +129,7 @@ await AddDependencyFlowEventAsync( subscription.TargetBranch, subscription.TargetRepository); - pullRequestActor = _updaterFactory.CreatePullRequestUpdater( + pullRequestUpdater = _updaterFactory.CreatePullRequestUpdater( new BatchedPullRequestUpdaterId(subscription.TargetRepository, subscription.TargetBranch)); } else @@ -137,7 +137,7 @@ await AddDependencyFlowEventAsync( _logger.LogInformation("Creating pull request updater for subscription {subscriptionId}", _subscriptionId); - pullRequestActor = _updaterFactory.CreatePullRequestUpdater( + pullRequestUpdater = _updaterFactory.CreatePullRequestUpdater( new NonBatchedPullRequestUpdaterId(_subscriptionId)); } @@ -150,12 +150,12 @@ await AddDependencyFlowEventAsync( .ToList(); await _redisMutex.EnterWhenAvailable( - pullRequestActor.Id.ToString(), + pullRequestUpdater.Id.ToString(), async () => { _logger.LogInformation("Running asset update for {subscriptionId}", _subscriptionId); - await pullRequestActor.UpdateAssetsAsync( + await pullRequestUpdater.UpdateAssetsAsync( _subscriptionId, subscription.SourceEnabled ? SubscriptionType.DependenciesAndSources diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeFlowWorkItemProcessor.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeFlowWorkItemProcessor.cs deleted file mode 100644 index 3b47fb1d16..0000000000 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeFlowWorkItemProcessor.cs +++ /dev/null @@ -1,133 +0,0 @@ -// 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; -using Microsoft.DotNet.DarcLib.Helpers; -using Microsoft.DotNet.DarcLib.VirtualMonoRepo; -using Microsoft.DotNet.Maestro.Client; -using Microsoft.DotNet.Maestro.Client.Models; -using Microsoft.Extensions.Logging; -using ProductConstructionService.DependencyFlow.WorkItems; -using ProductConstructionService.WorkItems; - -namespace ProductConstructionService.DependencyFlow.WorkItemProcessors; - -public class CodeFlowWorkItemProcessor( - IVmrInfo vmrInfo, - IBasicBarClient barClient, - IMaestroApi maestroApi, - IPcsVmrBackFlower vmrBackFlower, - IPcsVmrForwardFlower vmrForwardFlower, - ILocalLibGit2Client gitClient, - ITelemetryRecorder telemetryRecorder, - ILogger logger) - : WorkItemProcessor -{ - private readonly IVmrInfo _vmrInfo = vmrInfo; - private readonly IBasicBarClient _barClient = barClient; - private readonly IMaestroApi _maestroApi = maestroApi; - private readonly IPcsVmrBackFlower _vmrBackFlower = vmrBackFlower; - private readonly IPcsVmrForwardFlower _vmrForwardFlower = vmrForwardFlower; - private readonly ILocalLibGit2Client _gitClient = gitClient; - private readonly ITelemetryRecorder _telemetryRecorder = telemetryRecorder; - private readonly ILogger _logger = logger; - - public override async Task ProcessWorkItemAsync(CodeFlowWorkItem codeflowWorkItem, CancellationToken cancellationToken) - { - Subscription? subscription = await _barClient.GetSubscriptionAsync(codeflowWorkItem.SubscriptionId); - - if (subscription == null) - { - _logger.LogError("Subscription {subscriptionId} not found", codeflowWorkItem.SubscriptionId); - return false; - } - - if (!subscription.SourceEnabled || (subscription.SourceDirectory ?? subscription.TargetDirectory) == null) - { - _logger.LogError("Subscription {subscriptionId} is not source enabled or source directory is not set", codeflowWorkItem.SubscriptionId); - return false; - } - - Build? build = await _barClient.GetBuildAsync(codeflowWorkItem.BuildId); - if (build == null) - { - _logger.LogError("Build {buildId} not found", codeflowWorkItem.BuildId); - return false; - } - - var isForwardFlow = subscription.TargetDirectory != null; - - _logger.LogInformation( - "{direction}-flowing build {buildId} for subscription {subscriptionId} targeting {repo} / {targetBranch} to new branch {newBranch}", - isForwardFlow ? "Forward" : "Back", - build.Id, - subscription.Id, - subscription.TargetRepository, - subscription.TargetBranch, - codeflowWorkItem.PrBranch); - - bool hadUpdates; - NativePath targetRepo; - - try - { - if (isForwardFlow) - { - targetRepo = _vmrInfo.VmrPath; - hadUpdates = await _vmrForwardFlower.FlowForwardAsync( - subscription.TargetDirectory!, - build, - subscription.TargetBranch, - codeflowWorkItem.PrBranch, - cancellationToken); - } - else - { - (hadUpdates, targetRepo) = await _vmrBackFlower.FlowBackAsync( - subscription.SourceDirectory!, - build, - subscription.TargetBranch, - codeflowWorkItem.PrBranch, - cancellationToken); - } - } - catch (Exception e) - { - _logger.LogError(e, "Failed to flow changes for build {buildId} in subscription {subscriptionId}", - build.Id, - subscription.Id); - throw; - } - - if (!hadUpdates) - { - _logger.LogInformation("There were no code-flow updates for subscription {subscriptionId}", - subscription.Id); - return true; - } - - _logger.LogInformation("Code changes for {subscriptionId} ready in local branch {branch}", - subscription.Id, - subscription.TargetBranch); - - // TODO https://github.com/dotnet/arcade-services/issues/3318: Handle failures (conflict, non-ff etc) - using (var scope = _telemetryRecorder.RecordGitOperation(TrackedGitOperation.Push, subscription.TargetRepository)) - { - await _gitClient.Push(targetRepo, codeflowWorkItem.PrBranch, subscription.TargetRepository); - scope.SetSuccess(); - } - - // When no PR is created yet, we notify Maestro that the branch is ready - if (codeflowWorkItem.PrUrl == null) - { - _logger.LogInformation( - "Notifying Maestro that subscription code changes for {subscriptionId} are ready in local branch {branch}", - subscription.Id, - subscription.TargetBranch); - - await _maestroApi.Subscriptions.TriggerSubscriptionAsync(codeflowWorkItem.BuildId, subscription.Id, default); - } - - return true; - } -} diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/PullRequestCheckProcessor.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/PullRequestCheckProcessor.cs index 728d3cbdf0..81e51798b5 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/PullRequestCheckProcessor.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/PullRequestCheckProcessor.cs @@ -7,28 +7,35 @@ namespace ProductConstructionService.DependencyFlow.WorkItemProcessors; -public class PullRequestCheckProcessor : WorkItemProcessor +public class PullRequestCheckProcessor : WorkItemProcessor { private readonly IPullRequestUpdaterFactory _updaterFactory; private readonly IRedisMutex _redisMutex; + private readonly IReminderManagerFactory _reminderFactory; - public PullRequestCheckProcessor(IPullRequestUpdaterFactory updaterFactory, IRedisMutex redisMutex) + public PullRequestCheckProcessor( + IPullRequestUpdaterFactory updaterFactory, + IRedisMutex redisMutex, + IReminderManagerFactory reminderFactory) { _updaterFactory = updaterFactory; _redisMutex = redisMutex; + _reminderFactory = reminderFactory; } public override async Task ProcessWorkItemAsync( - InProgressPullRequest workItem, + PullRequestCheck workItem, CancellationToken cancellationToken) { return await _redisMutex.EnterWhenAvailable( - workItem.ActorId, + workItem.UpdaterId, async () => { - var updater = _updaterFactory.CreatePullRequestUpdater(PullRequestUpdaterId.Parse(workItem.ActorId)); - await updater.SynchronizeInProgressPullRequestAsync(workItem); - return true; + var reminders = _reminderFactory.CreateReminderManager(workItem.UpdaterId); + await reminders.UnsetReminderAsync(); + + var updater = _updaterFactory.CreatePullRequestUpdater(PullRequestUpdaterId.Parse(workItem.UpdaterId)); + return await updater.CheckPullRequestAsync(workItem); }); } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/SubscriptionUpdateProcessor.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/SubscriptionUpdateProcessor.cs index 64d0c6b808..ca01e45029 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/SubscriptionUpdateProcessor.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/SubscriptionUpdateProcessor.cs @@ -23,10 +23,10 @@ public override async Task ProcessWorkItemAsync( CancellationToken cancellationToken) { return await _redisMutex.EnterWhenAvailable( - workItem.ActorId, + workItem.UpdaterId, async () => { - var updater = _updaterFactory.CreatePullRequestUpdater(PullRequestUpdaterId.Parse(workItem.ActorId)); + var updater = _updaterFactory.CreatePullRequestUpdater(PullRequestUpdaterId.Parse(workItem.UpdaterId)); await updater.ProcessPendingUpdatesAsync(workItem); return true; }); diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeFlowWorkItem.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeFlowWorkItem.cs deleted file mode 100644 index f42bb7ff12..0000000000 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeFlowWorkItem.cs +++ /dev/null @@ -1,32 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using ProductConstructionService.WorkItems; - -namespace ProductConstructionService.DependencyFlow.WorkItems; - -/// -/// Main code flow work item which causes new code changes to be flown to a new branch in the target repo. -/// -public class CodeFlowWorkItem : WorkItem -{ - /// - /// Subscription that is being flown/triggered. - /// - public required Guid SubscriptionId { get; init; } - - /// - /// Build that is being flown. - /// - public required int BuildId { get; init; } - - /// - /// Name of the PR branch that will be created in the target repo. - /// - public required string PrBranch { get; init; } - - /// - /// URL to the code flow PR. - /// - public string? PrUrl { get; init; } -} diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/ActorWorkItem.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/DependencyFlowWorkItem.cs similarity index 75% rename from src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/ActorWorkItem.cs rename to src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/DependencyFlowWorkItem.cs index f4cbac0e15..c283b1d5b9 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/ActorWorkItem.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/DependencyFlowWorkItem.cs @@ -8,8 +8,8 @@ namespace ProductConstructionService.DependencyFlow.WorkItems; [DataContract] -public abstract class ActorWorkItem : WorkItem +public abstract class DependencyFlowWorkItem : WorkItem { [DataMember] - public required string ActorId { get; init; } + public required string UpdaterId { get; init; } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/CodeFlowStatus.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/PullRequestCheck.cs similarity index 52% rename from src/ProductConstructionService/ProductConstructionService.DependencyFlow/CodeFlowStatus.cs rename to src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/PullRequestCheck.cs index 02331bab35..5b1ef25a79 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/CodeFlowStatus.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/PullRequestCheck.cs @@ -4,14 +4,9 @@ using System.Runtime.Serialization; #nullable disable -namespace ProductConstructionService.DependencyFlow; +namespace ProductConstructionService.DependencyFlow.WorkItems; [DataContract] -public class CodeFlowStatus +public class PullRequestCheck : DependencyFlowWorkItem { - [DataMember] - public string PrBranch { get; set; } - - [DataMember] - public string SourceSha { get; set; } } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs index 5e51ac2807..eba7bbc202 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs @@ -8,7 +8,7 @@ namespace ProductConstructionService.DependencyFlow.WorkItems; [DataContract] -public class SubscriptionUpdateWorkItem : ActorWorkItem +public class SubscriptionUpdateWorkItem : DependencyFlowWorkItem { [DataMember] public Guid SubscriptionId { get; init; } diff --git a/src/ProductConstructionService/ProductConstructionService.WorkItems/ReminderManager.cs b/src/ProductConstructionService/ProductConstructionService.WorkItems/ReminderManager.cs index d315cdd63c..84989ba4b2 100644 --- a/src/ProductConstructionService/ProductConstructionService.WorkItems/ReminderManager.cs +++ b/src/ProductConstructionService/ProductConstructionService.WorkItems/ReminderManager.cs @@ -23,14 +23,14 @@ public ReminderManager( string key) { _workItemProducerFactory = workItemProducerFactory; - _receiptCache = cacheFactory.Create($"ReminderReceipt_{key}"); + _receiptCache = cacheFactory.Create($"Reminder_{key}", includeTypeInKey: false); } public async Task SetReminderAsync(T payload, TimeSpan visibilityTimeout) { var client = _workItemProducerFactory.CreateProducer(); var sendReceipt = await client.ProduceWorkItemAsync(payload, visibilityTimeout); - await _receiptCache.SetAsync(new ReminderArguments(sendReceipt.PopReceipt, sendReceipt.MessageId), visibilityTimeout); + await _receiptCache.SetAsync(new ReminderArguments(sendReceipt.PopReceipt, sendReceipt.MessageId), visibilityTimeout + TimeSpan.FromHours(4)); } public async Task UnsetReminderAsync() diff --git a/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCacheFactory.cs b/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCacheFactory.cs index 9572402571..e793bfd0f2 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCacheFactory.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCacheFactory.cs @@ -14,7 +14,7 @@ public IRedisCache Create(string key) return new MockRedisCache(key, Data); } - public IRedisCache Create(string key) where T : class + public IRedisCache Create(string key, bool includeTypeInKey = true) where T : class { return new MockRedisCache(key, Data); } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs index abb60ab15e..296e99efc2 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.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 FluentAssertions; using Maestro.Data.Models; using NUnit.Framework; @@ -22,16 +21,13 @@ public async Task PendingCodeFlowUpdatesNotUpdatablePr() }); Build build = GivenANewBuild(true); - WithExistingCodeFlowStatus(build); - WithExistingPrBranch(); AndPendingUpdates(build, isCodeFlow: true); - WithExistingCodeFlowPullRequest(build, canUpdate: false); - await WhenProcessPendingUpdatesAsyncIsCalled(build, isCodeFlow: true); - - ThenPcsShouldNotHaveBeenCalled(build, InProgressPrUrl); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHaveInProgressPullRequestState(build); + using (WithExistingCodeFlowPullRequest(build, canUpdate: false)) + { + await WhenProcessPendingUpdatesAsyncIsCalled(build, isCodeFlow: true); + AndShouldHaveInProgressPullRequestState(build); + } } [Test] @@ -46,16 +42,13 @@ public async Task PendingUpdatesUpdatablePrButNoNewBuild() }); Build build = GivenANewBuild(true); - WithExistingCodeFlowStatus(build); - WithExistingPrBranch(); - WithExistingCodeFlowPullRequest(build, canUpdate: true); - - await WhenProcessPendingUpdatesAsyncIsCalled(build, isCodeFlow: true); + using (WithExistingCodeFlowPullRequest(build, canUpdate: true)) + { + await WhenProcessPendingUpdatesAsyncIsCalled(build, isCodeFlow: true); - ThenPcsShouldNotHaveBeenCalled(build, InProgressPrUrl); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHaveInProgressPullRequestState(build); - AndShouldHavePullRequestCheckReminder(build); + AndShouldHaveInProgressPullRequestState(build); + AndShouldHavePullRequestCheckReminder(); + } } [Test] @@ -72,17 +65,14 @@ public async Task PendingUpdatesUpdatablePr() Build newBuild = GivenANewBuild(true); newBuild.Commit = "sha456"; - WithExistingCodeFlowStatus(oldBuild); - WithExistingPrBranch(); - - WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true); - await WhenProcessPendingUpdatesAsyncIsCalled(newBuild, isCodeFlow: true); + using (WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true)) + { + await WhenProcessPendingUpdatesAsyncIsCalled(newBuild, isCodeFlow: true); - ThenPcsShouldHaveBeenCalled(newBuild, InProgressPrUrl, out var prBranch); - AndShouldHaveNoPendingUpdateState(); - AndShouldHavePullRequestCheckReminder(newBuild); - prBranch.Should().Be(InProgressPrHeadBranch); - AndShouldHaveCodeFlowState(newBuild, InProgressPrHeadBranch); - AndShouldHaveInProgressPullRequestState(newBuild); + ThenCodeShouldHaveBeenFlownForward(newBuild); + AndShouldHaveNoPendingUpdateState(); + AndShouldHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState(newBuild); + } } } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PendingUpdatesTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PendingUpdatesTests.cs index f751b3f137..854415e86f 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PendingUpdatesTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PendingUpdatesTests.cs @@ -23,8 +23,10 @@ public async Task PendingUpdatesNotUpdatablePr() GivenAPendingUpdateReminder(b); AndPendingUpdates(b); - WithExistingPullRequest(b, canUpdate: false); - await WhenProcessPendingUpdatesAsyncIsCalled(b); + using (WithExistingPullRequest(b, canUpdate: false)) + { + await WhenProcessPendingUpdatesAsyncIsCalled(b); + } } [Test] @@ -43,15 +45,16 @@ public async Task PendingUpdatesUpdatablePr() AndPendingUpdates(b); WithRequireNonCoherencyUpdates(); WithNoRequiredCoherencyUpdates(); - WithExistingPullRequest(b, canUpdate: true); - - await WhenProcessPendingUpdatesAsyncIsCalled(b); - - ThenGetRequiredUpdatesShouldHaveBeenCalled(b, true); - ThenUpdateReminderIsRemoved(); - AndPendingUpdateIsRemoved(); - AndCommitUpdatesShouldHaveBeenCalled(b); - AndUpdatePullRequestShouldHaveBeenCalled(); - AndShouldHavePullRequestCheckReminder(b); + using (WithExistingPullRequest(b, canUpdate: true)) + { + await WhenProcessPendingUpdatesAsyncIsCalled(b); + + ThenGetRequiredUpdatesShouldHaveBeenCalled(b, true); + ThenUpdateReminderIsRemoved(); + AndPendingUpdateIsRemoved(); + AndCommitUpdatesShouldHaveBeenCalled(b); + AndUpdatePullRequestShouldHaveBeenCalled(); + AndShouldHavePullRequestCheckReminder(); + } } } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestBuilderTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestBuilderTests.cs index d6233199ec..64fa5c2c69 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestBuilderTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestBuilderTests.cs @@ -244,7 +244,7 @@ private static SubscriptionUpdateWorkItem GivenSubscriptionUpdate( string guid, SubscriptionType type = SubscriptionType.Dependencies) => new() { - ActorId = guid, + UpdaterId = guid, IsCoherencyUpdate = isCoherencyUpdate, SourceRepo = "The best repo", SubscriptionId = new Guid(guid), diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestPolicyFailureNotifierTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestPolicyFailureNotifierTests.cs index 0273cef910..d9f0b4b789 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestPolicyFailureNotifierTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestPolicyFailureNotifierTests.cs @@ -11,7 +11,6 @@ using Microsoft.Extensions.Logging.Abstractions; using Moq; using NUnit.Framework; -using ProductConstructionService.DependencyFlow.WorkItems; using ClientModels = Microsoft.DotNet.Maestro.Client.Models; namespace ProductConstructionService.DependencyFlow.Tests; @@ -198,8 +197,10 @@ private InProgressPullRequest GetInProgressPullRequest(string url, int contained // the "Url" and ContainedSubscriptions fields in InProgressPullRequestObjects return new InProgressPullRequest() { - ActorId = new BatchedPullRequestUpdaterId(FakeRepoName, "main").Id, + UpdaterId = new BatchedPullRequestUpdaterId(FakeRepoName, "main").Id, Url = url, + HeadBranch = "pr.head.branch", + SourceSha = "pr.head.sha", ContainedSubscriptions = containedSubscriptions, SourceRepoNotified = false }; @@ -218,8 +219,10 @@ private InProgressPullRequest GetInProgressPullRequestWithoutTags(string url) return new InProgressPullRequest() { - ActorId = new BatchedPullRequestUpdaterId(FakeRepoName, "main").Id, + UpdaterId = new BatchedPullRequestUpdaterId(FakeRepoName, "main").Id, Url = url, + HeadBranch = "pr.head.branch", + SourceSha = "pr.head.sha", ContainedSubscriptions = containedSubscriptions, SourceRepoNotified = false }; diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index a83702c383..baf92756b2 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -8,10 +8,13 @@ using Maestro.DataProviders; using Maestro.MergePolicyEvaluation; using Microsoft.DotNet.DarcLib; +using Microsoft.DotNet.DarcLib.Helpers; +using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.DotNet.GitHub.Authentication; using Microsoft.Extensions.DependencyInjection; using Microsoft.VisualStudio.Services.Common; using Moq; +using NUnit.Framework; using ProductConstructionService.DependencyFlow.WorkItems; using Asset = Maestro.Contracts.Asset; using AssetData = Microsoft.DotNet.Maestro.Client.Models.AssetData; @@ -22,20 +25,37 @@ internal abstract class PullRequestUpdaterTests : SubscriptionOrPullRequestUpdat { private const long InstallationId = 1174; protected const string InProgressPrUrl = "https://github.com/owner/repo/pull/10"; - protected const string InProgressPrHeadBranch = "pr.head.branch"; + protected string? InProgressPrHeadBranch { get; private set; } = "pr.head.branch"; - private string _newBranch = null!; + private Mock _backFlower = null!; + private Mock _forwardFlower = null!; + private Mock _gitClient = null!; + + [SetUp] + public void PullRequestUpdaterTests_SetUp() + { + _backFlower = new(); + _forwardFlower = new(); + _gitClient = new(); + } protected override void RegisterServices(IServiceCollection services) { base.RegisterServices(services); + services.AddSingleton(_backFlower.Object); + services.AddSingleton(_forwardFlower.Object); + services.AddSingleton(_gitClient.Object); + + _forwardFlower.SetReturnsDefault(Task.FromResult(true)); + _backFlower.SetReturnsDefault(Task.FromResult((true, new NativePath(TargetRepo)))); + _gitClient.SetReturnsDefault(Task.CompletedTask); + services.AddGitHubTokenProvider(); services.AddSingleton(); services.AddScoped(); services.AddTransient(); - services.AddSingleton(MergePolicyEvaluator.Object); - services.AddSingleton(UpdateResolver.Object); + services.AddVmrManagers("git", VmrPath, TmpPath, null, null); } protected override Task BeforeExecute(IServiceProvider context) @@ -47,6 +67,9 @@ protected override Task BeforeExecute(IServiceProvider context) RepositoryName = TargetRepo, InstallationId = InstallationId }); + + context.GetRequiredService().VmrUri = VmrUri; + return base.BeforeExecute(context); } @@ -80,7 +103,7 @@ protected void ThenGetRequiredUpdatesShouldHaveBeenCalled(Build withBuild, bool protected void AndCreateNewBranchShouldHaveBeenCalled() { - var captureNewBranch = new CaptureMatch(newBranch => _newBranch = newBranch); + var captureNewBranch = new CaptureMatch(newBranch => InProgressPrHeadBranch = newBranch); DarcRemotes[TargetRepo] .Verify(r => r.CreateNewBranchAsync(TargetRepo, TargetBranch, Capture.With(captureNewBranch))); } @@ -92,7 +115,7 @@ protected void AndCommitUpdatesShouldHaveBeenCalled(Build withUpdatesFromBuild) .Verify( r => r.CommitUpdatesAsync( TargetRepo, - _newBranch ?? InProgressPrHeadBranch, + InProgressPrHeadBranch, RemoteFactory.Object, It.IsAny(), Capture.In(updatedDependencies), @@ -127,7 +150,7 @@ protected void AndCreatePullRequestShouldHaveBeenCalled() new() { BaseBranch = TargetBranch, - HeadBranch = _newBranch + HeadBranch = InProgressPrHeadBranch } }, options => options.Excluding(pr => pr.Title).Excluding(pr => pr.Description)); @@ -138,8 +161,8 @@ protected void AndCreatePullRequestShouldHaveBeenCalled() protected void AndCodeFlowPullRequestShouldHaveBeenCreated() { var pullRequests = new List(); - DarcRemotes[TargetRepo] - .Verify(r => r.CreatePullRequestAsync(TargetRepo, Capture.In(pullRequests))); + DarcRemotes[VmrUri] + .Verify(r => r.CreatePullRequestAsync(VmrUri, Capture.In(pullRequests))); pullRequests.Should() .BeEquivalentTo( @@ -148,7 +171,7 @@ protected void AndCodeFlowPullRequestShouldHaveBeenCreated() new() { BaseBranch = TargetBranch, - HeadBranch = InProgressPrHeadBranch, + HeadBranch = pullRequests.First().HeadBranch, } }, options => options @@ -156,25 +179,41 @@ protected void AndCodeFlowPullRequestShouldHaveBeenCreated() .Excluding(pr => pr.Description)); } - protected void ThenPcsShouldNotHaveBeenCalled(Build build, string? prUrl = null) + protected void ThenCodeShouldHaveBeenBackflown(Build build) { - CodeFlowWorkItemsProduced - .Should() - .NotContain(request => request.BuildId == build.Id && (prUrl == null || request.PrUrl == prUrl)); + _backFlower + .Verify(b => b.FlowBackAsync( + Subscription.SourceDirectory, + It.Is(b => b.Id == build.Id && b.Commit == build.Commit), + TargetBranch, + It.IsAny(), + It.IsAny()), + Times.Once); + + _gitClient.Verify( + g => g.Push(TargetRepo, It.IsAny(), TargetRepo, It.IsAny()), + Times.Once); } - protected void ThenPcsShouldHaveBeenCalled(Build build, string? prUrl, out string prBranch) - => AndPcsShouldHaveBeenCalled(build, prUrl, out prBranch); - - protected void AndPcsShouldHaveBeenCalled(Build build, string? prUrl, out string prBranch) + protected void ThenCodeShouldHaveBeenFlownForward(Build build) { - var workItem = CodeFlowWorkItemsProduced - .FirstOrDefault(request => request.SubscriptionId == Subscription.Id && request.BuildId == build.Id && (prUrl == null || request.PrUrl == prUrl)); - - workItem.Should().NotBeNull(); - prBranch = workItem!.PrBranch; + _forwardFlower + .Verify(b => b.FlowForwardAsync( + Subscription.TargetDirectory, + It.Is(b => b.Id == build.Id && b.Commit == build.Commit), + TargetBranch, + It.IsAny(), + It.IsAny()), + Times.Once); + + _gitClient.Verify( + g => g.Push(VmrPath, It.IsAny(), VmrUri, It.IsAny()), + Times.Once); } + protected void AndCodeShouldHaveBeenFlownForward(Build build) + => ThenCodeShouldHaveBeenFlownForward(build); + protected static void ValidatePRDescriptionContainsLinks(PullRequest pr) { pr.Description.Should().Contain("][1]"); @@ -183,23 +222,19 @@ protected static void ValidatePRDescriptionContainsLinks(PullRequest pr) protected void CreatePullRequestShouldReturnAValidValue() { - DarcRemotes[TargetRepo] - .Setup(s => s.CreatePullRequestAsync(It.IsAny(), It.IsAny())) - .ReturnsAsync(InProgressPrUrl); - } + var targetRepo = Subscription.TargetDirectory != null ? VmrUri : TargetRepo; + var prUrl = Subscription.TargetDirectory != null ? VmrPullRequestUrl : InProgressPrUrl; - protected void WithExistingPrBranch() - { - DarcRemotes[TargetRepo] - .Setup(s => s.BranchExistsAsync(TargetRepo, It.IsAny())) - .ReturnsAsync(true); - } - - protected void WithoutExistingPrBranch() - { - DarcRemotes[TargetRepo] - .Setup(s => s.BranchExistsAsync(TargetRepo, It.IsAny())) - .ReturnsAsync(false); + DarcRemotes.GetOrAddValue(targetRepo, () => new Mock()) + .Setup(s => s.CreatePullRequestAsync(It.IsAny(), It.IsAny())) + .Callback((repo, pr) => + { + if (targetRepo == VmrUri) + { + InProgressPrHeadBranch = pr.HeadBranch; + } + }) + .ReturnsAsync(prUrl); } protected void AndUpdatePullRequestShouldHaveBeenCalled() @@ -214,7 +249,7 @@ protected void AndUpdatePullRequestShouldHaveBeenCalled() new() { BaseBranch = TargetBranch, - HeadBranch = _newBranch ?? InProgressPrHeadBranch + HeadBranch = InProgressPrHeadBranch, } }, options => options.Excluding(pr => pr.Title).Excluding(pr => pr.Description)); @@ -285,95 +320,130 @@ protected void WithFailsStrictCheckForCoherencyUpdates() }); } - protected void WithExistingPullRequest(Build forBuild, bool canUpdate) + protected IDisposable WithExistingPullRequest(Build forBuild, bool canUpdate) + => canUpdate + ? WithExistingPullRequest(forBuild, PrStatus.Open, null) + : WithExistingPullRequest(forBuild, PrStatus.Open, MergePolicyEvaluationStatus.Pending); + + protected IDisposable WithExistingPullRequest(Build forBuild, PrStatus prStatus, MergePolicyEvaluationStatus? policyEvaluationStatus) { + var prUrl = Subscription.TargetDirectory != null + ? VmrPullRequestUrl + : InProgressPrUrl; + AfterDbUpdateActions.Add(() => { - var pr = CreatePullRequestCheckReminder(forBuild); + var pr = CreatePullRequestState(forBuild, prUrl); SetState(Subscription, pr); SetExpectedState(Subscription, pr); }); - var remote = DarcRemotes.GetOrAddValue(TargetRepo, () => CreateMock()); - DarcRemotes[TargetRepo] - .Setup(x => x.GetPullRequestStatusAsync(InProgressPrUrl)) - .ReturnsAsync(PrStatus.Open); + var targetRepo = Subscription.TargetDirectory != null ? VmrUri : TargetRepo; - DarcRemotes[TargetRepo] - .Setup(r => r.GetPullRequestAsync(InProgressPrUrl)) - .ReturnsAsync( - new PullRequest - { - HeadBranch = InProgressPrHeadBranch, - BaseBranch = TargetBranch - }); + var remote = DarcRemotes.GetOrAddValue(targetRepo, () => CreateMock()); + remote + .Setup(x => x.GetPullRequestStatusAsync(prUrl)) + .ReturnsAsync(PrStatus.Open); - var results = canUpdate - ? new MergePolicyEvaluationResults([]) - : new MergePolicyEvaluationResults( + var results = policyEvaluationStatus.HasValue + ? new MergePolicyEvaluationResults( [ new MergePolicyEvaluationResult( - MergePolicyEvaluationStatus.Pending, + policyEvaluationStatus.Value, "Check", "Fake one", Mock.Of(x => x.Name == "Policy" && x.DisplayName == "Some policy")) - ]); + ]) + : new MergePolicyEvaluationResults([]); + + if (prStatus == PrStatus.Open && !policyEvaluationStatus.HasValue) + { + remote + .Setup(r => r.GetPullRequestAsync(prUrl)) + .ReturnsAsync( + new PullRequest + { + HeadBranch = InProgressPrHeadBranch, + BaseBranch = TargetBranch + }); + } + + remote + .Setup(r => r.CreateOrUpdatePullRequestMergeStatusInfoAsync(prUrl, results.Results)) + .Returns(Task.CompletedTask); MergePolicyEvaluator .Setup(x => x.EvaluateAsync( - It.Is(pr => pr.Url == InProgressPrUrl), + It.Is(pr => pr.Url == prUrl), It.IsAny(), It.IsAny>())) .ReturnsAsync(results); + + return Disposable.Create(remote.VerifyAll); } - protected void WithExistingCodeFlowPullRequest(Build forBuild, bool canUpdate) + protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, bool canUpdate) + => canUpdate + ? WithExistingCodeFlowPullRequest(forBuild, PrStatus.Open, null) + : WithExistingCodeFlowPullRequest(forBuild, PrStatus.Open, MergePolicyEvaluationStatus.Pending); + + protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, PrStatus prStatus, MergePolicyEvaluationStatus? policyEvaluationStatus) { + var prUrl = Subscription.TargetDirectory != null + ? VmrPullRequestUrl + : InProgressPrUrl; + + var targetRepo = Subscription.TargetDirectory != null + ? VmrUri + : TargetRepo; + AfterDbUpdateActions.Add(() => { - var pr = CreatePullRequestCheckReminder(forBuild); + var pr = CreatePullRequestState(forBuild, prUrl); SetState(Subscription, pr); SetExpectedState(Subscription, pr); }); - DarcRemotes[TargetRepo] - .Setup(x => x.GetPullRequestStatusAsync(InProgressPrUrl)) - .ReturnsAsync(PrStatus.Open); - - var results = canUpdate - ? new MergePolicyEvaluationResults([]) - : new MergePolicyEvaluationResults( + var results = policyEvaluationStatus.HasValue + ? new MergePolicyEvaluationResults( [ new MergePolicyEvaluationResult( - MergePolicyEvaluationStatus.Pending, + policyEvaluationStatus.Value, "Check", "Fake one", Mock.Of(x => x.Name == "Policy" && x.DisplayName == "Some policy")) - ]); + ]) + : new MergePolicyEvaluationResults([]); MergePolicyEvaluator .Setup(x => x.EvaluateAsync( - It.Is(pr => pr.Url == InProgressPrUrl), + It.Is(pr => pr.Url == prUrl), It.IsAny(), It.IsAny>())) .ReturnsAsync(results); - } - protected void WithExistingCodeFlowStatus(Build build) - { - AfterDbUpdateActions.Add(() => + var remote = DarcRemotes.GetOrAddValue(targetRepo, () => CreateMock()); + remote + .Setup(x => x.GetPullRequestStatusAsync(prUrl)) + .ReturnsAsync(prStatus); + + if (prStatus == PrStatus.Open) { - SetState(Subscription, new CodeFlowStatus - { - PrBranch = InProgressPrHeadBranch, - SourceSha = build.Commit, - }); - }); + remote + .Setup(x => x.CreateOrUpdatePullRequestMergeStatusInfoAsync(prUrl, results.Results)) + .Returns(Task.CompletedTask); + } + + return Disposable.Create(remote.VerifyAll); } - protected void AndShouldHavePullRequestCheckReminder(Build forBuild, InProgressPullRequest? expectedState = null) + protected void AndShouldHavePullRequestCheckReminder() { - SetExpectedReminder(Subscription, expectedState ?? CreatePullRequestCheckReminder(forBuild)); + var prUrl = Subscription.SourceEnabled + ? VmrPullRequestUrl + : InProgressPrUrl; + + SetExpectedReminder(Subscription, CreatePullRequestCheck()); } protected void AndShouldHaveInProgressPullRequestState( @@ -382,21 +452,16 @@ protected void AndShouldHaveInProgressPullRequestState( List? coherencyErrors = null, InProgressPullRequest? expectedState = null) { - SetExpectedState(Subscription, expectedState ?? CreatePullRequestCheckReminder(forBuild, coherencyCheckSuccessful, coherencyErrors)); + var prUrl = Subscription.SourceEnabled + ? VmrPullRequestUrl + : InProgressPrUrl; + + SetExpectedState(Subscription, expectedState ?? CreatePullRequestState(forBuild, prUrl, coherencyCheckSuccessful, coherencyErrors)); } protected void ThenShouldHaveInProgressPullRequestState(Build forBuild, InProgressPullRequest? expectedState = null) => AndShouldHaveInProgressPullRequestState(forBuild, expectedState: expectedState); - protected void AndShouldHaveCodeFlowState(Build forBuild, string? prBranch = null) - { - SetExpectedState(Subscription, new CodeFlowStatus - { - SourceSha = forBuild.Commit, - PrBranch = prBranch, - }); - } - protected void AndShouldHaveNoPendingUpdateState() { RemoveExpectedReminder(Subscription); @@ -426,7 +491,7 @@ protected IPullRequestUpdater CreatePullRequestActor(IServiceProvider context) protected SubscriptionUpdateWorkItem CreateSubscriptionUpdate(Build forBuild, bool isCodeFlow = false) => new() { - ActorId = GetPullRequestUpdaterId().ToString(), + UpdaterId = GetPullRequestUpdaterId().ToString(), SubscriptionId = Subscription.Id, SubscriptionType = isCodeFlow ? SubscriptionType.DependenciesAndSources : SubscriptionType.Dependencies, BuildId = forBuild.Id, @@ -442,13 +507,16 @@ protected SubscriptionUpdateWorkItem CreateSubscriptionUpdate(Build forBuild, bo IsCoherencyUpdate = false, }; - protected InProgressPullRequest CreatePullRequestCheckReminder( + protected InProgressPullRequest CreatePullRequestState( Build forBuild, + string prUrl, bool? coherencyCheckSuccessful = true, List? coherencyErrors = null) => new() { - ActorId = GetPullRequestUpdaterId().ToString(), + UpdaterId = GetPullRequestUpdaterId().ToString(), + HeadBranch = InProgressPrHeadBranch, + SourceSha = forBuild.Commit, ContainedSubscriptions = [ new SubscriptionPullRequestUpdate @@ -467,6 +535,12 @@ protected InProgressPullRequest CreatePullRequestCheckReminder( .ToList(), CoherencyCheckSuccessful = coherencyCheckSuccessful, CoherencyErrors = coherencyErrors, - Url = InProgressPrUrl, + Url = prUrl, + }; + + protected PullRequestCheck CreatePullRequestCheck() + => new() + { + UpdaterId = GetPullRequestUpdaterId().ToString(), }; } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/SubscriptionOrPullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/SubscriptionOrPullRequestUpdaterTests.cs index bf2cf4581f..c94b24f3c3 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/SubscriptionOrPullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/SubscriptionOrPullRequestUpdaterTests.cs @@ -33,6 +33,8 @@ protected override void RegisterServices(IServiceCollection services) { base.RegisterServices(services); + services.AddSingleton(MergePolicyEvaluator.Object); + services.AddSingleton(UpdateResolver.Object); services.AddSingleton(HostingEnvironment.Object); services.AddBuildAssetRegistry(options => { @@ -99,12 +101,12 @@ internal void GivenACodeFlowSubscription(SubscriptionPolicy policy) { Channel = Channel, SourceRepository = SourceRepo, - TargetRepository = TargetRepo, + TargetRepository = VmrUri, TargetBranch = TargetBranch, PolicyObject = policy, SourceEnabled = true, - SourceDirectory = "repo", + TargetDirectory = "repo", ExcludedAssets = [new AssetFilter() { Filter = "Excluded.Package" }], }; ContextUpdates.Add(context => context.Subscriptions.Add(Subscription)); diff --git a/test/ProductConstructionService.DependencyFlow.Tests/TestsWithMocks.cs b/test/ProductConstructionService.DependencyFlow.Tests/TestsWithMocks.cs index d8ce83b8b7..bc0aac883f 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/TestsWithMocks.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/TestsWithMocks.cs @@ -25,6 +25,6 @@ public void TestsWithMocks_TearDown() protected Mock CreateMock(MockBehavior behavior = MockBehavior.Default) where T : class { - return _mocks.Create(); + return _mocks.Create(behavior); } } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs index 75d5c35ba1..fc25ff1845 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using Maestro.Data.Models; +using Microsoft.DotNet.DarcLib; using NUnit.Framework; namespace ProductConstructionService.DependencyFlow.Tests; @@ -15,7 +16,7 @@ namespace ProductConstructionService.DependencyFlow.Tests; internal class UpdateAssetsForCodeFlowTests : UpdateAssetsPullRequestUpdaterTests { [Test] - public async Task UpdateWithNoExistingStateOrPrBranch() + public async Task UpdateWithNoExistingState() { GivenATestChannel(); GivenACodeFlowSubscription( @@ -24,52 +25,10 @@ public async Task UpdateWithNoExistingStateOrPrBranch() Batchable = false, UpdateFrequency = UpdateFrequency.EveryBuild, }); - Build build = GivenANewBuild(true); - - await WhenUpdateAssetsAsyncIsCalled(build); - - ThenShouldHavePendingUpdateState(build); - AndPcsShouldHaveBeenCalled(build, prUrl: null, out var requestedBranch); - AndShouldHaveCodeFlowState(build, requestedBranch); - } - - [Test] - public async Task WaitForPrBranch() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = false, - UpdateFrequency = UpdateFrequency.EveryBuild, - }); - Build build = GivenANewBuild(true); - - GivenPendingUpdates(build); - WithExistingCodeFlowStatus(build); - WithoutExistingPrBranch(); - - await WhenUpdateAssetsAsyncIsCalled(build); - ThenShouldHavePendingUpdateState(build); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - } - - [Test] - public async Task UpdateWithPrBranchReady() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = false, - UpdateFrequency = UpdateFrequency.EveryBuild, - }); Build build = GivenANewBuild(true); GivenPendingUpdates(build); - WithExistingCodeFlowStatus(build); - WithExistingPrBranch(); CreatePullRequestShouldReturnAValidValue(); await WhenUpdateAssetsAsyncIsCalled(build); @@ -78,10 +37,12 @@ public async Task UpdateWithPrBranchReady() // with assets and other info just like we do in UpdatePullRequestAsync. // Right now, we are not flowing packages in codeflow subscriptions yet, so this functionality is no there // For now, we manually update the info the unit tests expect. - var expectedState = new WorkItems.InProgressPullRequest() + var expectedState = new InProgressPullRequest() { - ActorId = GetPullRequestUpdaterId(Subscription).Id, - Url = InProgressPrUrl, + UpdaterId = GetPullRequestUpdaterId(Subscription).Id, + Url = VmrPullRequestUrl, + HeadBranch = InProgressPrHeadBranch, + SourceSha = build.Commit, ContainedSubscriptions = [ new() @@ -93,11 +54,10 @@ public async Task UpdateWithPrBranchReady() }; ThenUpdateReminderIsRemoved(); - ThenPcsShouldNotHaveBeenCalled(build); AndCodeFlowPullRequestShouldHaveBeenCreated(); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHavePullRequestCheckReminder(build, expectedState); - AndShouldHaveInProgressPullRequestState(build, coherencyCheckSuccessful: null, expectedState: expectedState); + AndCodeShouldHaveBeenFlownForward(build); + AndShouldHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState(build, expectedState: expectedState); AndPendingUpdateIsRemoved(); } @@ -113,17 +73,13 @@ public async Task UpdateWithPrNotUpdatable() }); Build build = GivenANewBuild(true); - WithExistingCodeFlowStatus(build); - WithExistingPrBranch(); - WithExistingPullRequest(build, canUpdate: false); - - await WhenUpdateAssetsAsyncIsCalled(build); + using (WithExistingCodeFlowPullRequest(build, canUpdate: false)) + { + await WhenUpdateAssetsAsyncIsCalled(build); - ThenShouldHavePendingUpdateState(build); - ThenPcsShouldNotHaveBeenCalled(build, InProgressPrUrl); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHaveInProgressPullRequestState(build, coherencyCheckSuccessful: true); - AndShouldHavePullRequestCheckReminder(build); + ThenShouldHavePendingUpdateState(build); + AndShouldHaveInProgressPullRequestState(build, coherencyCheckSuccessful: true); + } } [Test] @@ -138,16 +94,13 @@ public async Task UpdateWithPrUpdatableButNoUpdates() }); Build build = GivenANewBuild(true); - WithExistingCodeFlowStatus(build); - WithExistingPrBranch(); - WithExistingPullRequest(build, canUpdate: true); - - await WhenUpdateAssetsAsyncIsCalled(build); + using (WithExistingCodeFlowPullRequest(build, canUpdate: true)) + { + await WhenUpdateAssetsAsyncIsCalled(build); - ThenPcsShouldNotHaveBeenCalled(build, InProgressPrUrl); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHavePullRequestCheckReminder(build); - AndShouldHaveInProgressPullRequestState(build); + AndShouldHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState(build); + } } [Test] @@ -165,16 +118,66 @@ public async Task UpdateCodeFlowPrWithNewBuild() Build newBuild = GivenANewBuild(true); newBuild.Commit = "sha456"; - WithExistingCodeFlowStatus(oldBuild); - WithExistingPrBranch(); - WithExistingPullRequest(oldBuild, canUpdate: true); + using (WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true)) + { + await WhenUpdateAssetsAsyncIsCalled(newBuild); + + ThenShouldHaveInProgressPullRequestState(newBuild); + AndCodeShouldHaveBeenFlownForward(newBuild); + AndShouldHavePullRequestCheckReminder(); + } + } - await WhenUpdateAssetsAsyncIsCalled(newBuild); + [Test] + public async Task UpdateWithManuallyMergedPrAndNewBuild() + { + GivenATestChannel(); + GivenACodeFlowSubscription( + new SubscriptionPolicy + { + Batchable = false, + UpdateFrequency = UpdateFrequency.EveryBuild, + }); + Build build = GivenANewBuild(true); + Build build2 = GivenANewBuild(true); - ThenPcsShouldHaveBeenCalled(newBuild, InProgressPrUrl, out _); - AndShouldHaveCodeFlowState(newBuild, InProgressPrHeadBranch); - AndShouldHavePullRequestCheckReminder(newBuild); - AndShouldHaveInProgressPullRequestState(newBuild); + using (WithExistingCodeFlowPullRequest(build, PrStatus.Merged, null)) + { + // Original PR is merged, we should try to delete the branch + DarcRemotes[VmrUri] + .Setup(x => x.DeletePullRequestBranchAsync(VmrPullRequestUrl)) + .Returns(Task.CompletedTask) + .Verifiable(); + + // URI of the new PR that should get created + VmrPullRequestUrl = $"{VmrUri}/pulls/2"; + CreatePullRequestShouldReturnAValidValue(); + + await WhenUpdateAssetsAsyncIsCalled(build2); + + // TODO (https://github.com/dotnet/arcade-services/issues/3866): We need to populate InProgressPullRequest fully + // with assets and other info just like we do in UpdatePullRequestAsync. + // Right now, we are not flowing packages in codeflow subscriptions yet, so this functionality is no there + // For now, we manually update the info the unit tests expect. + var expectedState = new InProgressPullRequest() + { + UpdaterId = GetPullRequestUpdaterId(Subscription).Id, + Url = VmrPullRequestUrl, + HeadBranch = InProgressPrHeadBranch, + SourceSha = build2.Commit, + ContainedSubscriptions = + [ + new() + { + SubscriptionId = Subscription.Id, + BuildId = build2.Id, + } + ] + }; + + AndShouldHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState(build2, expectedState: expectedState); + } } protected override void ThenShouldHavePendingUpdateState(Build forBuild, bool _ = false) diff --git a/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsTests.cs index cb675bd067..87fd89754c 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsTests.cs @@ -34,7 +34,7 @@ public async Task UpdateWithAssetsNoExistingPR(bool batchable) AndCreateNewBranchShouldHaveBeenCalled(); AndCommitUpdatesShouldHaveBeenCalled(b); AndCreatePullRequestShouldHaveBeenCalled(); - AndShouldHavePullRequestCheckReminder(b); + AndShouldHavePullRequestCheckReminder(); AndShouldHaveInProgressPullRequestState(b); } @@ -53,15 +53,17 @@ public async Task UpdateWithAssetsExistingPR(bool batchable) WithRequireNonCoherencyUpdates(); WithNoRequiredCoherencyUpdates(); - WithExistingPullRequest(b, canUpdate: true); - await WhenUpdateAssetsAsyncIsCalled(b); + using (WithExistingPullRequest(b, canUpdate: true)) + { + await WhenUpdateAssetsAsyncIsCalled(b); - ThenGetRequiredUpdatesShouldHaveBeenCalled(b, true); - AndCommitUpdatesShouldHaveBeenCalled(b); - AndUpdatePullRequestShouldHaveBeenCalled(); - AndShouldHavePullRequestCheckReminder(b); - AndShouldHaveInProgressPullRequestState(b); + ThenGetRequiredUpdatesShouldHaveBeenCalled(b, true); + AndCommitUpdatesShouldHaveBeenCalled(b); + AndUpdatePullRequestShouldHaveBeenCalled(); + AndShouldHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState(b); + } } [TestCase(false)] @@ -79,13 +81,13 @@ public async Task UpdateWithAssetsExistingPRNotUpdatable(bool batchable) WithRequireNonCoherencyUpdates(); WithNoRequiredCoherencyUpdates(); - WithExistingPullRequest(b, canUpdate: false); + using (WithExistingPullRequest(b, canUpdate: false)) + { + await WhenUpdateAssetsAsyncIsCalled(b); - await WhenUpdateAssetsAsyncIsCalled(b); - - ThenShouldHavePendingUpdateState(b); - AndShouldHaveInProgressPullRequestState(b); - AndShouldHavePullRequestCheckReminder(b); + ThenShouldHavePendingUpdateState(b); + AndShouldHaveInProgressPullRequestState(b); + } } [TestCase(false)] @@ -134,15 +136,7 @@ public async Task UpdateWithAssetsWhenStrictAlgorithmFails(bool batchable) AndCreateNewBranchShouldHaveBeenCalled(); AndCommitUpdatesShouldHaveBeenCalled(b); AndCreatePullRequestShouldHaveBeenCalled(); - AndShouldHavePullRequestCheckReminder(b, CreatePullRequestCheckReminder(b, - coherencyCheckSuccessful: false, - coherencyErrors: [ - new CoherencyErrorDetails() - { - Error = "Repo @ commit does not contain dependency fakeDependency", - PotentialSolutions = new List() - } - ])); + AndShouldHavePullRequestCheckReminder(); AndShouldHaveInProgressPullRequestState(b, coherencyCheckSuccessful: false, coherencyErrors: [ diff --git a/test/ProductConstructionService.DependencyFlow.Tests/UpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/UpdaterTests.cs index ec5404f5cd..06b1816217 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/UpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/UpdaterTests.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 Azure.Storage.Queues.Models; using FluentAssertions; using Maestro.Data.Models; using Microsoft.DotNet.DarcLib; @@ -14,7 +13,6 @@ using Moq; using NUnit.Framework; using ProductConstructionService.Common; -using ProductConstructionService.DependencyFlow.WorkItems; using ProductConstructionService.WorkItems; namespace ProductConstructionService.DependencyFlow.Tests; @@ -29,6 +27,10 @@ internal abstract class UpdaterTests : TestsWithServices protected const string TargetBranch = "target.branch"; protected const string NewBuildNumber = "build.number"; protected const string NewCommit = "sha2"; + protected const string VmrPath = "D:\\vmr"; + protected const string TmpPath = "D:\\tmp"; + protected const string VmrUri = "https://github.com/maestro-auth-test/dnceng-vmr"; + protected string VmrPullRequestUrl = $"{VmrUri}/pulls/1"; protected Dictionary ExpectedCacheState { get; private set; } = null!; protected Dictionary ExpectedReminders { get; private set; } = null!; @@ -41,9 +43,6 @@ internal abstract class UpdaterTests : TestsWithServices protected Mock UpdateResolver { get; private set; } = null!; protected Mock MergePolicyEvaluator { get; private set; } = null!; - protected List CodeFlowWorkItemsProduced { get; private set; } = null!; - - protected override void RegisterServices(IServiceCollection services) { base.RegisterServices(services); @@ -62,12 +61,6 @@ protected override void RegisterServices(IServiceCollection services) // TODO (https://github.com/dotnet/arcade-services/issues/3866): Can be removed once we execute code flow directly // (when we remove producer factory from the constructor) Mock workItemProducerFactoryMock = new(); - Mock> workItemProducerMock = new(); - workItemProducerMock.Setup(w => w.ProduceWorkItemAsync(It.IsAny(), TimeSpan.Zero)) - .ReturnsAsync(QueuesModelFactory.SendReceipt("message", DateTimeOffset.Now, DateTimeOffset.Now, "popReceipt", DateTimeOffset.Now)) - .Callback((item, _) => CodeFlowWorkItemsProduced.Add(item)); - workItemProducerFactoryMock.Setup(w => w.CreateProducer()) - .Returns(workItemProducerMock.Object); services.AddSingleton(workItemProducerFactoryMock.Object); RemoteFactory @@ -90,7 +83,6 @@ public void UpdaterTests_SetUp() }; MergePolicyEvaluator = new(); UpdateResolver = new(); - CodeFlowWorkItemsProduced = []; } [TearDown] diff --git a/test/SubscriptionActorService.Tests/PendingCodeFlowUpdatesTests.cs b/test/SubscriptionActorService.Tests/PendingCodeFlowUpdatesTests.cs deleted file mode 100644 index d9fdfe7ccd..0000000000 --- a/test/SubscriptionActorService.Tests/PendingCodeFlowUpdatesTests.cs +++ /dev/null @@ -1,112 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Threading.Tasks; -using FluentAssertions; -using Maestro.Data.Models; -using NUnit.Framework; -using SubscriptionActorService.StateModel; - -namespace SubscriptionActorService.Tests; - -[TestFixture, NonParallelizable] -internal class PendingCodeFlowUpdatesTests : PendingUpdatePullRequestActorTests -{ - [Test] - public async Task PendingCodeFlowUpdatesNotUpdatablePr() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = true, - UpdateFrequency = UpdateFrequency.EveryBuild - }); - Build build = GivenANewBuild(true); - - GivenAPendingUpdateReminder(); - WithExistingCodeFlowStatus(build); - WithExistingPrBranch(); - AndPendingUpdates(build, isCodeFlow: true); - - using (WithExistingCodeFlowPullRequest(SynchronizePullRequestResult.InProgressCannotUpdate)) - { - await WhenProcessPendingUpdatesAsyncIsCalled(); - - ThenPcsShouldNotHaveBeenCalled(build, InProgressPrUrl); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHaveFollowingState( - pullRequestUpdateReminder: true, - pullRequestUpdateState: true, - pullRequestState: true, - codeFlowState: true); - } - } - - [Test] - public async Task PendingUpdatesUpdatablePrButNoNewBuild() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = true, - UpdateFrequency = UpdateFrequency.EveryBuild - }); - Build build = GivenANewBuild(true); - - GivenAPendingUpdateReminder(); - WithExistingCodeFlowStatus(build); - WithExistingPrBranch(); - AndPendingUpdates(build, isCodeFlow: true); - - using (WithExistingCodeFlowPullRequest(SynchronizePullRequestResult.InProgressCanUpdate)) - { - await WhenProcessPendingUpdatesAsyncIsCalled(); - - ThenPcsShouldNotHaveBeenCalled(build, InProgressPrUrl); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHaveFollowingState( - pullRequestUpdateReminder: true, - pullRequestUpdateState: true, - pullRequestState: true, - codeFlowState: true); - } - } - - [Test] - public async Task PendingUpdatesUpdatablePr() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = true, - UpdateFrequency = UpdateFrequency.EveryBuild - }); - Build oldBuild = GivenANewBuild(true); - Build newBuild = GivenANewBuild(true); - newBuild.Commit = "sha456"; - - GivenAPendingUpdateReminder(); - WithExistingCodeFlowStatus(oldBuild); - WithExistingPrBranch(); - AndPendingUpdates(newBuild, isCodeFlow: true); - ExpectPcsToGetCalled(newBuild); - - using (WithExistingCodeFlowPullRequest(SynchronizePullRequestResult.InProgressCanUpdate)) - { - await WhenProcessPendingUpdatesAsyncIsCalled(); - - ThenPcsShouldHaveBeenCalled(newBuild, InProgressPrUrl, out var prBranch); - AndShouldHaveNoPendingUpdateState(); - AndShouldHavePullRequestCheckReminder(); - prBranch.Should().Be(InProgressPrHeadBranch); - AndShouldHaveCodeFlowState(newBuild, InProgressPrHeadBranch); - AndShouldHaveFollowingState( - pullRequestCheckReminder: true, - pullRequestState: true, - codeFlowState: true); - } - } -} diff --git a/test/SubscriptionActorService.Tests/PullRequestActorTests.cs b/test/SubscriptionActorService.Tests/PullRequestActorTests.cs index 3ecf6d93a1..0925eb0c27 100644 --- a/test/SubscriptionActorService.Tests/PullRequestActorTests.cs +++ b/test/SubscriptionActorService.Tests/PullRequestActorTests.cs @@ -27,7 +27,6 @@ using Asset = Maestro.Contracts.Asset; using AssetData = Microsoft.DotNet.Maestro.Client.Models.AssetData; -using CodeFlowRequest = ProductConstructionService.Client.Models.CodeFlowRequest; using SynchronizePullRequestAction = System.Linq.Expressions.Expression>>>; namespace SubscriptionActorService.Tests; @@ -45,7 +44,6 @@ internal abstract class PullRequestActorTests : SubscriptionOrPullRequestActorTe private Mock _remoteFactory = null!; private Mock _updateResolver = null!; private Mock _mergePolicyEvaluator = null!; - private Mock _pcsClientCodeFlow = null!; private string _newBranch = null!; @@ -60,7 +58,6 @@ public void PullRequestActorTests_SetUp() _mergePolicyEvaluator = CreateMock(); _remoteFactory = new(MockBehavior.Strict); _updateResolver = new(MockBehavior.Strict); - _pcsClientCodeFlow = CreateMock(MockBehavior.Strict); } protected override void RegisterServices(IServiceCollection services) @@ -85,6 +82,7 @@ protected override void RegisterServices(IServiceCollection services) services.AddScoped(); services.AddTransient(); services.AddSingleton(_updateResolver.Object); + services.AddSingleton(Mock.Of()); _remoteFactory.Setup(f => f.GetRemoteAsync(It.IsAny(), It.IsAny())) .ReturnsAsync( @@ -92,11 +90,6 @@ protected override void RegisterServices(IServiceCollection services) _darcRemotes.GetOrAddValue(repo, () => CreateMock()).Object); services.AddSingleton(_remoteFactory.Object); - // PCS client - var pcsApi = Mock.Of(x => x.CodeFlow == _pcsClientCodeFlow.Object); - _pcsClientCodeFlow.SetReturnsDefault(Task.CompletedTask); - services.AddSingleton(pcsApi); - base.RegisterServices(services); } @@ -222,50 +215,6 @@ protected void AndCodeFlowPullRequestShouldHaveBeenCreated() .Excluding(pr => pr.Description)); } - protected void ThenPcsShouldNotHaveBeenCalled(Build build, string? prUrl = null) - { - var pcsRequests = new List(); - _pcsClientCodeFlow - .Verify( - r => r.FlowBuildAsync( - It.Is(request => request.BuildId == build.Id && (prUrl == null || request.PrUrl == prUrl)), - It.IsAny()), - Times.Never); - } - - protected void ThenPcsShouldHaveBeenCalled(Build build, string? prUrl, out string prBranch) - => AndPcsShouldHaveBeenCalled(build, prUrl, out prBranch); - - protected void AndPcsShouldHaveBeenCalled(Build build, string? prUrl, out string prBranch) - { - var pcsRequests = new List(); - _pcsClientCodeFlow - .Verify(r => r.FlowBuildAsync(Capture.In(pcsRequests), It.IsAny())); - - pcsRequests.Should() - .BeEquivalentTo( - new List - { - new(Subscription.Id, build.Id) - { - PrUrl = prUrl, - } - }, - options => options.Excluding(r => r.PrBranch)); - - prBranch = pcsRequests[0].PrBranch; - } - - protected void ExpectPcsToGetCalled(Build build, string? prUrl = null) - { - _pcsClientCodeFlow - .Setup(r => r.FlowBuildAsync( - It.Is(r => r.BuildId == build.Id && r.SubscriptionId == Subscription.Id && r.PrUrl == prUrl), - It.IsAny())) - .Returns(Task.CompletedTask) - .Verifiable(); - } - protected static void ValidatePRDescriptionContainsLinks(PullRequest pr) { pr.Description.Should().Contain("][1]"); diff --git a/test/SubscriptionActorService.Tests/UpdateAssetsForCodeFlowTests.cs b/test/SubscriptionActorService.Tests/UpdateAssetsForCodeFlowTests.cs deleted file mode 100644 index 7a91a9e814..0000000000 --- a/test/SubscriptionActorService.Tests/UpdateAssetsForCodeFlowTests.cs +++ /dev/null @@ -1,237 +0,0 @@ -// 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.Tasks; -using Maestro.Data.Models; -using NUnit.Framework; -using SubscriptionActorService.StateModel; -using Asset = Maestro.Contracts.Asset; - -namespace SubscriptionActorService.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. -/// Each test should have the inner state that is left behind by the previous state. -/// -[TestFixture, NonParallelizable] -internal class UpdateAssetsForCodeFlowTests : UpdateAssetsPullRequestActorTests -{ - [Test] - public async Task UpdateWithNoExistingStateOrPrBranch() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = false, - UpdateFrequency = UpdateFrequency.EveryBuild, - }); - Build build = GivenANewBuild(true); - - ExpectPcsToGetCalled(build); - - await WhenUpdateAssetsAsyncIsCalled(build); - - ThenShouldHavePendingUpdateState(build); - AndPcsShouldHaveBeenCalled(build, prUrl: null, out var requestedBranch); - AndShouldHaveCodeFlowState(build, requestedBranch); - AndShouldHaveFollowingState( - pullRequestUpdateState: true, - pullRequestUpdateReminder: true, - codeFlowState: true); - } - - [Test] - public async Task WaitForPrBranch() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = false, - UpdateFrequency = UpdateFrequency.EveryBuild, - }); - Build build = GivenANewBuild(true); - - GivenPendingUpdates(build); - WithExistingCodeFlowStatus(build); - WithoutExistingPrBranch(); - - await WhenUpdateAssetsAsyncIsCalled(build); - - ThenShouldHavePendingUpdateState(build); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHaveFollowingState( - codeFlowState: true, - pullRequestUpdateState: true, - pullRequestUpdateReminder: true); - } - - [Test] - public async Task UpdateWithPrBranchReady() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = false, - UpdateFrequency = UpdateFrequency.EveryBuild, - }); - Build build = GivenANewBuild(true); - - GivenPendingUpdates(build); - WithExistingCodeFlowStatus(build); - WithExistingPrBranch(); - CreatePullRequestShouldReturnAValidValue(); - - await WhenUpdateAssetsAsyncIsCalled(build); - - ThenUpdateReminderIsRemoved(); - ThenPcsShouldNotHaveBeenCalled(build); - AndCodeFlowPullRequestShouldHaveBeenCreated(); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHavePullRequestCheckReminder(); - AndShouldHaveInProgressCodeFlowPullRequestState(build); - AndDependencyFlowEventsShouldBeAdded(); - AndPendingUpdateIsRemoved(); - AndShouldHaveFollowingState( - codeFlowState: true, - pullRequestState: true, - pullRequestCheckReminder: true); - } - - [Test] - public async Task UpdateWithPrNotUpdatable() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = false, - UpdateFrequency = UpdateFrequency.EveryBuild, - }); - Build build = GivenANewBuild(true); - - WithExistingCodeFlowStatus(build); - WithExistingPrBranch(); - - using (WithExistingCodeFlowPullRequest(SynchronizePullRequestResult.InProgressCannotUpdate)) - { - await WhenUpdateAssetsAsyncIsCalled(build); - - ThenShouldHavePendingUpdateState(build); - ThenPcsShouldNotHaveBeenCalled(build, InProgressPrUrl); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHaveFollowingState( - codeFlowState: true, - pullRequestState: true, - pullRequestUpdateState: true, - pullRequestUpdateReminder: true); - } - } - - [Test] - public async Task UpdateWithPrUpdatableButNoUpdates() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = false, - UpdateFrequency = UpdateFrequency.EveryBuild, - }); - Build build = GivenANewBuild(true); - - GivenAPullRequestCheckReminder(); - WithExistingCodeFlowStatus(build); - WithExistingPrBranch(); - - using (WithExistingCodeFlowPullRequest(SynchronizePullRequestResult.InProgressCanUpdate)) - { - await WhenUpdateAssetsAsyncIsCalled(build); - - ThenPcsShouldNotHaveBeenCalled(build, InProgressPrUrl); - AndShouldHaveCodeFlowState(build, InProgressPrHeadBranch); - AndShouldHavePullRequestCheckReminder(); - AndShouldHaveFollowingState( - codeFlowState: true, - pullRequestState: true, - pullRequestCheckReminder: true); - } - } - - [Test] - public async Task UpdateCodeFlowPrWithNewBuild() - { - GivenATestChannel(); - GivenACodeFlowSubscription( - new SubscriptionPolicy - { - Batchable = false, - UpdateFrequency = UpdateFrequency.EveryBuild, - }); - - Build oldBuild = GivenANewBuild(true); - Build newBuild = GivenANewBuild(true); - newBuild.Commit = "sha456"; - - GivenAPullRequestCheckReminder(); - WithExistingCodeFlowStatus(oldBuild); - WithExistingPrBranch(); - ExpectPcsToGetCalled(newBuild); - - using (WithExistingCodeFlowPullRequest(SynchronizePullRequestResult.InProgressCanUpdate)) - { - await WhenUpdateAssetsAsyncIsCalled(newBuild); - - ThenPcsShouldHaveBeenCalled(newBuild, InProgressPrUrl, out _); - AndShouldHaveCodeFlowState(newBuild, InProgressPrHeadBranch); - AndShouldHavePullRequestCheckReminder(); - AndShouldHaveFollowingState( - codeFlowState: true, - pullRequestState: true, - pullRequestCheckReminder: true); - } - } - - protected override void ThenShouldHavePendingUpdateState(Build forBuild, bool _ = false) - { - base.ThenShouldHavePendingUpdateState(forBuild, true); - } - - protected void GivenPendingUpdates(Build forBuild) - { - AfterDbUpdateActions.Add( - () => - { - var updates = new List - { - new() - { - SubscriptionId = Subscription.Id, - Type = Maestro.Contracts.SubscriptionType.DependenciesAndSources, - BuildId = forBuild.Id, - SourceRepo = forBuild.GitHubRepository ?? forBuild.AzureDevOpsRepository, - SourceSha = forBuild.Commit, - Assets = forBuild.Assets - .Select(a => new Asset {Name = a.Name, Version = a.Version}) - .ToList(), - IsCoherencyUpdate = false, - } - }; - - var reminder = new MockReminderManager.Reminder( - PullRequestActorImplementation.PullRequestUpdateKey, - null, - TimeSpan.FromMinutes(3), - TimeSpan.FromMinutes(3)); - - StateManager.Data[PullRequestActorImplementation.PullRequestUpdateKey] = updates; - Reminders.Data[PullRequestActorImplementation.PullRequestUpdateKey] = reminder; - }); - } -} From 2abe68d9673e4d584cc16f7ac98b2c84a965fb58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C5=99emek=20Vysok=C3=BD?= Date: Mon, 14 Oct 2024 10:41:32 +0200 Subject: [PATCH 3/7] Update codeflow PR title/descriptions with every update (#4039) --- .../PullRequestUpdater.cs | 19 +++++++++++++++---- .../PendingCodeFlowUpdatesTests.cs | 2 ++ .../PendingUpdatePullRequestUpdaterTests.cs | 2 ++ .../PullRequestUpdaterTests.cs | 8 ++++++++ .../UpdateAssetsForCodeFlowTests.cs | 2 ++ 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 84e69cd523..4174534a16 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -983,12 +983,23 @@ private async Task UpdateAssetsAndSources(SubscriptionUpdateWorkItem updat pullRequest.ContainedSubscriptions = [ new SubscriptionPullRequestUpdate - { - SubscriptionId = update.SubscriptionId, - BuildId = update.BuildId - } + { + SubscriptionId = update.SubscriptionId, + BuildId = update.BuildId + } ]; + // Update PR's metadata + var title = await _pullRequestBuilder.GenerateCodeFlowPRTitleAsync(update, subscription.TargetBranch); + var description = await _pullRequestBuilder.GenerateCodeFlowPRDescriptionAsync(update); + + var remote = await _remoteFactory.GetRemoteAsync(subscription.TargetRepository, _logger); + await remote.UpdatePullRequestAsync(pullRequest.Url, new PullRequest + { + Title = title, + Description = description + }); + await SetPullRequestCheckReminder(pullRequest); await _pullRequestUpdateReminders.UnsetReminderAsync(); diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs index 296e99efc2..37870d5c51 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs @@ -67,6 +67,8 @@ public async Task PendingUpdatesUpdatablePr() using (WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true)) { + ExpectPrMetadataToBeUpdated(); + await WhenProcessPendingUpdatesAsyncIsCalled(newBuild, isCodeFlow: true); ThenCodeShouldHaveBeenFlownForward(newBuild); diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PendingUpdatePullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PendingUpdatePullRequestUpdaterTests.cs index 3cb9b4db3f..dac6ae125f 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PendingUpdatePullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PendingUpdatePullRequestUpdaterTests.cs @@ -2,6 +2,8 @@ // The .NET Foundation licenses this file to you under the MIT license. using Maestro.Data.Models; +using Microsoft.DotNet.DarcLib; +using Moq; using ProductConstructionService.DependencyFlow.WorkItems; namespace ProductConstructionService.DependencyFlow.Tests; diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index baf92756b2..4db816e6fc 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -73,6 +73,14 @@ protected override Task BeforeExecute(IServiceProvider context) return base.BeforeExecute(context); } + protected void ExpectPrMetadataToBeUpdated() + { + DarcRemotes[Subscription.TargetRepository] + .Setup(x => x.UpdatePullRequestAsync( + Subscription.SourceEnabled ? VmrPullRequestUrl : InProgressPrUrl, + It.IsAny())); + } + protected void ThenGetRequiredUpdatesShouldHaveBeenCalled(Build withBuild, bool prExists) { var assets = new List>(); diff --git a/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs index fc25ff1845..451d6ac1c5 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs @@ -120,6 +120,8 @@ public async Task UpdateCodeFlowPrWithNewBuild() using (WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true)) { + ExpectPrMetadataToBeUpdated(); + await WhenUpdateAssetsAsyncIsCalled(newBuild); ThenShouldHaveInProgressPullRequestState(newBuild); From 0cf9026b4cd1297732037a07c4951078bb5d00cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C5=99emek=20Vysok=C3=BD?= Date: Mon, 14 Oct 2024 12:55:49 +0200 Subject: [PATCH 4/7] Clear reminders from cache upon receiving (#4042) --- .../PullRequestCheckProcessor.cs | 2 +- .../ReminderManager.cs | 18 +++++++++++++++++- .../MockReminderManager.cs | 6 ++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/PullRequestCheckProcessor.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/PullRequestCheckProcessor.cs index 81e51798b5..47945c01d8 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/PullRequestCheckProcessor.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/PullRequestCheckProcessor.cs @@ -32,7 +32,7 @@ public override async Task ProcessWorkItemAsync( async () => { var reminders = _reminderFactory.CreateReminderManager(workItem.UpdaterId); - await reminders.UnsetReminderAsync(); + await reminders.ReminderReceivedAsync(); var updater = _updaterFactory.CreatePullRequestUpdater(PullRequestUpdaterId.Parse(workItem.UpdaterId)); return await updater.CheckPullRequestAsync(workItem); diff --git a/src/ProductConstructionService/ProductConstructionService.WorkItems/ReminderManager.cs b/src/ProductConstructionService/ProductConstructionService.WorkItems/ReminderManager.cs index 84989ba4b2..9e0e1caa55 100644 --- a/src/ProductConstructionService/ProductConstructionService.WorkItems/ReminderManager.cs +++ b/src/ProductConstructionService/ProductConstructionService.WorkItems/ReminderManager.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 Azure; using ProductConstructionService.Common; namespace ProductConstructionService.WorkItems; @@ -10,6 +11,8 @@ public interface IReminderManager where T : WorkItem Task SetReminderAsync(T reminder, TimeSpan dueTime); Task UnsetReminderAsync(); + + Task ReminderReceivedAsync(); } public class ReminderManager : IReminderManager where T : WorkItem @@ -42,7 +45,20 @@ public async Task UnsetReminderAsync() } var client = _workItemProducerFactory.CreateProducer(); - await client.DeleteWorkItemAsync(receipt.MessageId, receipt.PopReceipt); + + try + { + await client.DeleteWorkItemAsync(receipt.MessageId, receipt.PopReceipt); + } + catch (RequestFailedException e) when (e.Message.Contains("The specified message does not exist")) + { + // The message was already deleted, so we can ignore this exception. + } + } + + public async Task ReminderReceivedAsync() + { + await _receiptCache.TryDeleteAsync(); } private class ReminderArguments diff --git a/test/ProductConstructionService.DependencyFlow.Tests/MockReminderManager.cs b/test/ProductConstructionService.DependencyFlow.Tests/MockReminderManager.cs index e0a1bbd147..b168a77a7d 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/MockReminderManager.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/MockReminderManager.cs @@ -32,4 +32,10 @@ public Task UnsetReminderAsync() Data.Remove(_key); return Task.CompletedTask; } + + public Task ReminderReceivedAsync() + { + Data.Remove(_key); + return Task.CompletedTask; + } } From 435787b7f943a4888f2138d48c8298995deb62d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C5=99emek=20Vysok=C3=BD?= Date: Tue, 15 Oct 2024 10:51:33 +0200 Subject: [PATCH 5/7] Fix `darc vmr forwardflow/backflow` commands (#4050) Co-authored-by: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> --- .../VirtualMonoRepo/BackflowOperation.cs | 31 ++++++++++------ .../VirtualMonoRepo/CodeFlowOperation.cs | 35 +++++++++++++++++++ .../VirtualMonoRepo/ForwardFlowOperation.cs | 30 ++++++++++------ .../CodeFlowCommandLineOptions.cs | 10 ++++-- .../DarcLib/ILocalGitClient.cs | 5 +++ .../DarcLib/ILocalGitRepo.cs | 5 +++ .../DarcLib/LocalGitClient.cs | 7 ++++ .../DarcLib/LocalGitRepo.cs | 3 ++ 8 files changed, 101 insertions(+), 25 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/BackflowOperation.cs b/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/BackflowOperation.cs index e8e1de5da6..4316460299 100644 --- a/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/BackflowOperation.cs +++ b/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/BackflowOperation.cs @@ -1,9 +1,11 @@ // 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.Threading; using System.Threading.Tasks; using Microsoft.DotNet.Darc.Options.VirtualMonoRepo; +using Microsoft.DotNet.DarcLib; using Microsoft.DotNet.DarcLib.Helpers; using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.Extensions.Logging; @@ -15,23 +17,30 @@ internal class BackflowOperation( BackflowCommandLineOptions options, IVmrBackFlower vmrBackFlower, IVmrInfo vmrInfo, + IVmrDependencyTracker dependencyTracker, + ILocalGitRepoFactory localGitRepoFactory, ILogger logger) - : CodeFlowOperation(options, vmrInfo, logger) + : CodeFlowOperation(options, vmrInfo, dependencyTracker, localGitRepoFactory, logger) { private readonly BackflowCommandLineOptions _options = options; + private readonly IVmrInfo _vmrInfo = vmrInfo; protected override async Task FlowAsync( string mappingName, NativePath targetDirectory, - string? shaToFlow, + string? targetRepoPath, CancellationToken cancellationToken) - => await vmrBackFlower.FlowBackAsync( - mappingName, - new NativePath(targetDirectory), - shaToFlow, - _options.Build, - _options.BranchName, - _options.BranchName, - _options.DiscardPatches, - cancellationToken); + { + targetRepoPath ??= Environment.CurrentDirectory!; + var targetRepo = new NativePath(targetRepoPath); + return await vmrBackFlower.FlowBackAsync( + mappingName, + targetRepo, + null, + _options.Build, + await GetBaseBranch(targetRepo), + await GetTargetBranch(_vmrInfo.VmrPath), + _options.DiscardPatches, + cancellationToken); + } } diff --git a/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/CodeFlowOperation.cs b/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/CodeFlowOperation.cs index 54c7919a7d..65b59592a6 100644 --- a/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/CodeFlowOperation.cs +++ b/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/CodeFlowOperation.cs @@ -7,6 +7,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.DotNet.Darc.Options.VirtualMonoRepo; +using Microsoft.DotNet.DarcLib; using Microsoft.DotNet.DarcLib.Helpers; using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.Extensions.Logging; @@ -18,15 +19,21 @@ internal abstract class CodeFlowOperation : VmrOperationBase { private readonly ICodeFlowCommandLineOptions _options; private readonly IVmrInfo _vmrInfo; + private readonly IVmrDependencyTracker _dependencyTracker; + private readonly ILocalGitRepoFactory _localGitRepoFactory; protected CodeFlowOperation( ICodeFlowCommandLineOptions options, IVmrInfo vmrInfo, + IVmrDependencyTracker dependencyTracker, + ILocalGitRepoFactory localGitRepoFactory, ILogger logger) : base(options, logger) { _options = options; _vmrInfo = vmrInfo; + _dependencyTracker = dependencyTracker; + _localGitRepoFactory = localGitRepoFactory; } protected override async Task ExecuteInternalAsync( @@ -54,6 +61,8 @@ protected override async Task ExecuteInternalAsync( throw new ArgumentException("Cannot specify both --build and --commit"); } + await _dependencyTracker.InitializeSourceMappings(); + await FlowAsync( repoName, new NativePath(targetDirectory), @@ -66,4 +75,30 @@ protected abstract Task FlowAsync( NativePath targetDirectory, string? shaToFlow, CancellationToken cancellationToken); + + protected async Task GetBaseBranch(NativePath repoPath) + { + var localRepo = _localGitRepoFactory.Create(repoPath); + + if (!string.IsNullOrEmpty(_options.BaseBranch)) + { + await localRepo.CheckoutAsync(_options.BaseBranch); + return _options.BaseBranch; + } + + return await localRepo.GetCheckedOutBranchAsync(); + } + + protected async Task GetTargetBranch(NativePath repoPath) + { + if (!string.IsNullOrEmpty(_options.TargetBranch)) + { + return _options.TargetBranch; + } + + var localRepo = _localGitRepoFactory.Create(repoPath); + var targetSha = await localRepo.GetShaForRefAsync(DarcLib.Constants.HEAD); + + return $"codeflow/{targetSha}"; + } } diff --git a/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ForwardFlowOperation.cs b/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ForwardFlowOperation.cs index 314e9ff5d1..8fdb710b9c 100644 --- a/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ForwardFlowOperation.cs +++ b/src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ForwardFlowOperation.cs @@ -1,9 +1,11 @@ // 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.Threading; using System.Threading.Tasks; using Microsoft.DotNet.Darc.Options.VirtualMonoRepo; +using Microsoft.DotNet.DarcLib; using Microsoft.DotNet.DarcLib.Helpers; using Microsoft.DotNet.DarcLib.VirtualMonoRepo; using Microsoft.Extensions.Logging; @@ -15,23 +17,29 @@ internal class ForwardFlowOperation( ForwardFlowCommandLineOptions options, IVmrForwardFlower vmrForwardFlower, IVmrInfo vmrInfo, + IVmrDependencyTracker dependencyTracker, + ILocalGitRepoFactory localGitRepoFactory, ILogger logger) - : CodeFlowOperation(options, vmrInfo, logger) + : CodeFlowOperation(options, vmrInfo, dependencyTracker, localGitRepoFactory, logger) { private readonly ForwardFlowCommandLineOptions _options = options; protected override async Task FlowAsync( string mappingName, NativePath targetDirectory, - string? shaToFlow, + string? sourceRepoPath, CancellationToken cancellationToken) - => await vmrForwardFlower.FlowForwardAsync( - mappingName, - new NativePath(targetDirectory), - shaToFlow, - _options.Build, - _options.BranchName, - _options.BranchName, - _options.DiscardPatches, - cancellationToken); + { + var sourceRepo = new NativePath(sourceRepoPath ?? Environment.CurrentDirectory!); + + return await vmrForwardFlower.FlowForwardAsync( + mappingName, + sourceRepo, + null, + _options.Build, + await GetBaseBranch(new NativePath(_options.VmrPath)), + await GetTargetBranch(sourceRepo), + _options.DiscardPatches, + cancellationToken); + } } diff --git a/src/Microsoft.DotNet.Darc/Darc/Options/VirtualMonoRepo/CodeFlowCommandLineOptions.cs b/src/Microsoft.DotNet.Darc/Darc/Options/VirtualMonoRepo/CodeFlowCommandLineOptions.cs index 7c2692b30a..4d5387e61c 100644 --- a/src/Microsoft.DotNet.Darc/Darc/Options/VirtualMonoRepo/CodeFlowCommandLineOptions.cs +++ b/src/Microsoft.DotNet.Darc/Darc/Options/VirtualMonoRepo/CodeFlowCommandLineOptions.cs @@ -9,11 +9,12 @@ namespace Microsoft.DotNet.Darc.Options.VirtualMonoRepo; internal interface ICodeFlowCommandLineOptions : IBaseVmrCommandLineOptions { - string BranchName { get; set; } int? Build { get; set; } string Commit { get; set; } bool DiscardPatches { get; set; } string RepositoryDirectory { get; set; } + public string BaseBranch { get; set; } + public string TargetBranch { get; set; } } internal abstract class CodeFlowCommandLineOptions : VmrCommandLineOptions, IBaseVmrCommandLineOptions, ICodeFlowCommandLineOptions where T : Operation @@ -39,6 +40,9 @@ internal abstract class CodeFlowCommandLineOptions : VmrCommandLineOptions [Option("commit", Required = false, HelpText = "If specified, flows the given commit. Cannot be used with --build.")] public string Commit { get; set; } - [Option("branch-name", Required = false, HelpText = "Name of the new branch that will be created in the target repository. Defaults to codeflow/backflow/SHA1-SHA2")] - public string BranchName { get; set; } + [Option("base-branch", Required = false, HelpText = "Name of the branch of the target repository to apply changes on top of. Defaults to the checked out branch")] + public string BaseBranch { get; set; } + + [Option("target-branch", Required = false, HelpText = "Name of the new branch that will be created in the target repository. Defaults to codeflow/SHA1-SHA2")] + public string TargetBranch { get; set; } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/ILocalGitClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/ILocalGitClient.cs index ccda984313..7ce693e4ba 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/ILocalGitClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/ILocalGitClient.cs @@ -184,4 +184,9 @@ Task RunGitCommandAsync( string repoPath, string[] args, CancellationToken cancellationToken = default); + + /// + /// Gets the current checked out branch. + /// + Task GetCheckedOutBranchAsync(NativePath path); } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/ILocalGitRepo.cs b/src/Microsoft.DotNet.Darc/DarcLib/ILocalGitRepo.cs index bf59837d05..46d8205d62 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/ILocalGitRepo.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/ILocalGitRepo.cs @@ -119,6 +119,11 @@ Task CommitAsync( /// Git reference to resolve or HEAD when null Task GetShaForRefAsync(string? gitRef = null); + /// + /// Gets the current checked out branch. + /// + Task GetCheckedOutBranchAsync(); + /// /// Gets the type of a git object (e.g. commit, tree..). /// diff --git a/src/Microsoft.DotNet.Darc/DarcLib/LocalGitClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/LocalGitClient.cs index 9a64839ec1..aba81a5dd5 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/LocalGitClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/LocalGitClient.cs @@ -193,6 +193,13 @@ public async Task GetShaForRefAsync(string repoPath, string? gitRef = nu return result.StandardOutput.Trim(); } + public async Task GetCheckedOutBranchAsync(NativePath repoPath) + { + var result = await _processManager.ExecuteGit(repoPath, "rev-parse", "--abbrev-ref", "HEAD"); + result.ThrowIfFailed($"Failed to get the current branch for {repoPath}"); + return result.StandardOutput.Trim(); + } + public async Task GetObjectTypeAsync(string repoPath, string objectSha) { var args = new[] diff --git a/src/Microsoft.DotNet.Darc/DarcLib/LocalGitRepo.cs b/src/Microsoft.DotNet.Darc/DarcLib/LocalGitRepo.cs index 23df18d68d..a37e91b0de 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/LocalGitRepo.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/LocalGitRepo.cs @@ -62,6 +62,9 @@ public async Task GetRootDirAsync(CancellationToken cancellationToken = public async Task GetShaForRefAsync(string? gitRef = null) => await _localGitClient.GetShaForRefAsync(Path, gitRef); + public async Task GetCheckedOutBranchAsync() + => await _localGitClient.GetCheckedOutBranchAsync(Path); + public async Task FetchAllAsync(IReadOnlyCollection remoteUris, CancellationToken cancellationToken = default) => await _localGitClient.FetchAllAsync(Path, remoteUris, cancellationToken); From c36f05821b751c5d46047c9c58eea8fa4fdf6400 Mon Sep 17 00:00:00 2001 From: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com> Date: Tue, 15 Oct 2024 12:55:23 +0200 Subject: [PATCH 6/7] Add PCS prod environment (#4040) --- .vault-config/product-construction-int.yaml | 21 ------- .vault-config/product-construction-prod.yaml | 28 +++++++++ ...pipelines-product-construction-service.yml | 32 +++++++++- .../container-environment.bicep | 12 ++++ .../managed-identities.bicep | 11 ++++ .../production.bicepparam | 55 ++++++++++++++++ .../provision.bicep | 63 ++++++++++--------- .../ProductConstructionService/provision.ps1 | 7 ++- .../ProductConstructionService/redis.bicep | 2 +- .../staging.bicepparam | 57 +++++++++++++++++ eng/templates/jobs/e2e-pcs-tests.yml | 28 ++++++--- src/Maestro/Client/src/MaestroApiOptions.cs | 2 + .../appsettings.Production.json | 28 +++++++++ .../appsettings.Staging.json | 4 -- .../ProductConstructionServiceApiOptions.cs | 6 +- .../Deployer.cs | 39 +++++++----- .../appsettings.Production.json | 9 +++ .../appsettings.Production.json | 8 +++ .../appsettings.Production.json | 11 ++++ src/ProductConstructionService/Readme.md | 17 +++-- 20 files changed, 345 insertions(+), 95 deletions(-) create mode 100644 .vault-config/product-construction-prod.yaml create mode 100644 eng/service-templates/ProductConstructionService/production.bicepparam create mode 100644 eng/service-templates/ProductConstructionService/staging.bicepparam create mode 100644 src/ProductConstructionService/ProductConstructionService.Api/appsettings.Production.json create mode 100644 src/ProductConstructionService/ProductConstructionService.FeedCleaner/appsettings.Production.json create mode 100644 src/ProductConstructionService/ProductConstructionService.LongestBuildPathUpdater/appsettings.Production.json create mode 100644 src/ProductConstructionService/ProductConstructionService.SubscriptionTriggerer/appsettings.Production.json diff --git a/.vault-config/product-construction-int.yaml b/.vault-config/product-construction-int.yaml index 7d597b7356..47f8f384a1 100644 --- a/.vault-config/product-construction-int.yaml +++ b/.vault-config/product-construction-int.yaml @@ -4,28 +4,7 @@ storageLocation: subscription: e6b5f9f5-0ca4-4351-879b-014d78400ec2 name: ProductConstructionInt -references: - helixkv: - type: azure-key-vault - parameters: - subscription: a4fc5514-21a9-4296-bfaf-5c7ee7fa35d1 - name: helixkv - - engkeyvault: - type: azure-key-vault - parameters: - subscription: a4fc5514-21a9-4296-bfaf-5c7ee7fa35d1 - name: engkeyvault - secrets: - BotAccount-dotnet-bot-repo-PAT: - type: github-access-token - parameters: - gitHubBotAccountSecret: - location: engkeyvault - name: BotAccount-dotnet-bot - gitHubBotAccountName: dotnet-bot - github: type: github-app-secret parameters: diff --git a/.vault-config/product-construction-prod.yaml b/.vault-config/product-construction-prod.yaml new file mode 100644 index 0000000000..52c2d7ac65 --- /dev/null +++ b/.vault-config/product-construction-prod.yaml @@ -0,0 +1,28 @@ +storageLocation: + type: azure-key-vault + parameters: + subscription: fbd6122a-9ad3-42e4-976e-bccb82486856 + name: ProductConstructionProd + +references: + engkeyvault: + type: azure-key-vault + parameters: + subscription: a4fc5514-21a9-4296-bfaf-5c7ee7fa35d1 + name: engkeyvault + +secrets: + BotAccount-dotnet-bot-repo-PAT: + type: github-access-token + parameters: + gitHubBotAccountSecret: + location: engkeyvault + name: BotAccount-dotnet-bot + gitHubBotAccountName: dotnet-bot + + github: + type: github-app-secret + parameters: + hasPrivateKey: true + hasWebhookSecret: false + hasOAuthSecret: true diff --git a/azure-pipelines-product-construction-service.yml b/azure-pipelines-product-construction-service.yml index a2c16709c6..2cb6a712a9 100644 --- a/azure-pipelines-product-construction-service.yml +++ b/azure-pipelines-product-construction-service.yml @@ -3,6 +3,7 @@ trigger: branches: include: - main + - production pr: branches: @@ -47,10 +48,35 @@ variables: value: "Darc: Maestro Staging" - name: MaestroAppId value: $(MaestroStagingAppClientId) - - ${{ if eq(variables['Build.SourceBranch'], 'refs/heads/main') }}: + - name: redisConnectionString + value: "product-construction-service-redis-int.redis.cache.windows.net:6380,ssl=true" +- ${{ else }}: + - name: subscriptionId + value: fbd6122a-9ad3-42e4-976e-bccb82486856 + - name: containerappName + value: product-construction-prod + - name: containerjobNames + value: sub-triggerer-twicedaily-prod,sub-triggerer-daily-prod,sub-triggerer-weekly-prod,longest-path-updater-job-prod,feed-cleaner-prod + - name: containerRegistryName + value: productconstructionprod + - name: containerappEnvironmentName + value: product-construction-service-env-prod + - name: containerappWorkspaceName + value: product-construction-service-workspace-prod + - name: dockerRegistryUrl + value: productconstructionprod.azurecr.io + - name: serviceConnectionName + value: ProductConstructionServiceDeploymentProd + - name: authServiceConnection + value: "Darc: Maestro Production" + - name: MaestroAppId + value: $(MaestroAppClientId) + - name: redisConnectionString + value: "product-construction-service-redis-prod.redis.cache.windows.net,ssl=true" +- ${{ if eq(variables['Build.SourceBranch'], 'refs/heads/main') }}: - name: devBranchSuffix value: - - ${{ else }}: +- ${{ else }}: - name: devBranchSuffix value: -dev @@ -199,7 +225,7 @@ stages: --azCliPath "$(azCliPath)" ` --isCi true ` --entraAppId $(MaestroAppId) ` - --redisConnectionString "product-construction-service-redis-int.redis.cache.windows.net:6380,ssl=true" + --redisConnectionString $(redisConnectionString) displayName: Deploy container app - task: AzureCLI@2 diff --git a/eng/service-templates/ProductConstructionService/container-environment.bicep b/eng/service-templates/ProductConstructionService/container-environment.bicep index 9bd3fd48f7..ade013c8c1 100644 --- a/eng/service-templates/ProductConstructionService/container-environment.bicep +++ b/eng/service-templates/ProductConstructionService/container-environment.bicep @@ -4,6 +4,8 @@ param containerEnvironmentName string param productConstructionServiceSubnetId string param infrastructureResourceGroupName string param applicationInsightsName string +param containerAppsManagedEnvironmentsContributor string +param deploymentIdentityPrincipalId string resource logAnalytics 'Microsoft.OperationalInsights/workspaces@2021-12-01-preview' = { name: logAnalyticsName @@ -57,5 +59,15 @@ resource applicationInsights 'Microsoft.Insights/components@2020-02-02' = { } } +resource deploymentSubscriptionTriggererContributor 'Microsoft.Authorization/roleAssignments@2022-04-01' = { + scope: containerEnvironment + name: guid(subscription().id, resourceGroup().id, containerAppsManagedEnvironmentsContributor) + properties: { + roleDefinitionId: containerAppsManagedEnvironmentsContributor + principalType: 'ServicePrincipal' + principalId: deploymentIdentityPrincipalId + } + } + output applicationInsightsConnectionString string = applicationInsights.properties.ConnectionString output containerEnvironmentId string = containerEnvironment.id diff --git a/eng/service-templates/ProductConstructionService/managed-identities.bicep b/eng/service-templates/ProductConstructionService/managed-identities.bicep index e7fca31dca..3b3be0133e 100644 --- a/eng/service-templates/ProductConstructionService/managed-identities.bicep +++ b/eng/service-templates/ProductConstructionService/managed-identities.bicep @@ -4,6 +4,7 @@ param pcsIdentityName string param subscriptionTriggererIdentityName string param longestBuildPathUpdaterIdentityName string param feedCleanerIdentityName string +param contributorRole string resource deploymentIdentity 'Microsoft.ManagedIdentity/userAssignedIdentities@2023-01-31' = { name: deploymentIdentityName @@ -30,6 +31,16 @@ resource feedCleanerIdentity 'Microsoft.ManagedIdentity/userAssignedIdentities@2 location: location } +resource pcsIdentityContributorRole 'Microsoft.Authorization/roleAssignments@2022-04-01' = { + scope: pcsIdentity + name: guid(subscription().id, resourceGroup().id, contributorRole) + properties: { + roleDefinitionId: contributorRole + principalType: 'ServicePrincipal' + principalId: deploymentIdentity.properties.principalId + } +} + output pcsIdentityPrincipalId string = pcsIdentity.properties.principalId output pcsIdentityId string = pcsIdentity.id output deploymentIdentityPrincipalId string = deploymentIdentity.properties.principalId diff --git a/eng/service-templates/ProductConstructionService/production.bicepparam b/eng/service-templates/ProductConstructionService/production.bicepparam new file mode 100644 index 0000000000..54245b02bd --- /dev/null +++ b/eng/service-templates/ProductConstructionService/production.bicepparam @@ -0,0 +1,55 @@ +using 'provision.bicep' + +param location = 'westus2' + +param containerRegistryName = 'productconstructionprod' + +param containerCpuCoreCount = '1.0' + +param containerMemory = '2Gi' + +param aspnetcoreEnvironment = 'Production' + +param applicationInsightsName = 'product-construction-service-ai-prod' + +param keyVaultName = 'ProductConstructionProd' + +param azureCacheRedisName = 'product-construction-service-redis-prod' + +param logAnalyticsName = 'product-construction-service-workspace-prod' + +param containerEnvironmentName = 'product-construction-service-env-prod' + +param productConstructionServiceName = 'product-construction-prod' + +param storageAccountName = 'productconstructionprod' + +param pcsIdentityName = 'ProductConstructionServiceProd' + +param deploymentIdentityName = 'ProductConstructionServiceDeploymentProd' + +param containerImageName = 'mcr.microsoft.com/azuredocs/containerapps-helloworld:latest' + +param virtualNetworkName = 'product-construction-service-vnet-prod' + +param productConstructionServiceSubnetName = 'product-construction-service-subnet' + +param subscriptionTriggererIdentityName = 'SubscriptionTriggererProd' + +param subscriptionTriggererWeeklyJobName = 'sub-triggerer-weekly-prod' + +param subscriptionTriggererTwiceDailyJobName = 'sub-triggerer-twicedaily-prod' + +param subscriptionTriggererDailyJobName = 'sub-triggerer-daily-prod' + +param longestBuildPathUpdaterIdentityName = 'LongestBuildPathUpdaterProd' + +param longestBuildPathUpdaterJobName = 'longest-path-updater-job-prod' + +param feedCleanerJobName = 'feed-cleaner-prod' + +param feedCleanerIdentityName = 'FeedCleanerProd' + +param networkSecurityGroupName = 'product-construction-service-nsg-prod' + +param infrastructureResourceGroupName = 'product-construction-service-ip-prod' diff --git a/eng/service-templates/ProductConstructionService/provision.bicep b/eng/service-templates/ProductConstructionService/provision.bicep index 2dad5d0f7f..ecf2ecf22e 100644 --- a/eng/service-templates/ProductConstructionService/provision.bicep +++ b/eng/service-templates/ProductConstructionService/provision.bicep @@ -1,17 +1,17 @@ @minLength(1) @description('Primary location for all resources') -param location string = 'westus2' +param location string @minLength(5) @maxLength(50) @description('Name of the Azure Container Registry resource into which container images will be published') -param containerRegistryName string = 'productconstructionint' +param containerRegistryName string @description('CPU cores allocated to a single container instance') -param containerCpuCoreCount string = '1.0' +param containerCpuCoreCount string @description('Memory allocated to a single container instance') -param containerMemory string = '2Gi' +param containerMemory string @description('aspnetcore environment') @allowed([ @@ -19,76 +19,76 @@ param containerMemory string = '2Gi' 'Staging' 'Production' ]) -param aspnetcoreEnvironment string = 'Staging' +param aspnetcoreEnvironment string @description('Name of the application insights resource') -param applicationInsightsName string = 'product-construction-service-ai-int' +param applicationInsightsName string @description('Key Vault name') -param keyVaultName string = 'ProductConstructionInt' +param keyVaultName string @description('Dev Key Vault name') -param devKeyVaultName string = 'ProductConstructionDev' +param devKeyVaultName string = '' @description('Azure Cache for Redis name') -param azureCacheRedisName string = 'product-construction-service-redis-int' +param azureCacheRedisName string @description('Log analytics workspace name') -param logAnalyticsName string = 'product-construction-service-workspace-int' +param logAnalyticsName string @description('Name of the container apps environment') -param containerEnvironmentName string = 'product-construction-service-env-int' +param containerEnvironmentName string @description('Product construction service API name') -param productConstructionServiceName string = 'product-construction-int' +param productConstructionServiceName string @description('Storage account name') -param storageAccountName string = 'productconstructionint' +param storageAccountName string @description('Name of the MI used for the PCS container app') -param pcsIdentityName string = 'ProductConstructionServiceInt' +param pcsIdentityName string @description('Name of the identity used for the PCS deployment') -param deploymentIdentityName string = 'ProductConstructionServiceDeploymentInt' +param deploymentIdentityName string @description('Bicep requires an image when creating a containerapp. Using a dummy image for that.') -param containerImageName string = 'mcr.microsoft.com/azuredocs/containerapps-helloworld:latest' +param containerImageName string @description('Virtual network name') -param virtualNetworkName string = 'product-construction-service-vnet-int' +param virtualNetworkName string @description('Product construction service subnet name') -param productConstructionServiceSubnetName string = 'product-construction-service-subnet' +param productConstructionServiceSubnetName string @description('Subscription Triggerer Identity name') -param subscriptionTriggererIdentityName string = 'SubscriptionTriggererInt' +param subscriptionTriggererIdentityName string @description('Subscription Triggerer Weekly Job name') -param subscriptionTriggererWeeklyJobName string = 'sub-triggerer-weekly-int' +param subscriptionTriggererWeeklyJobName string @description('Subscription Triggerer Twice Daily Job name') -param subscriptionTriggererTwiceDailyJobName string = 'sub-triggerer-twicedaily-int' +param subscriptionTriggererTwiceDailyJobName string @description('Subscription Triggerer Daily Job name') -param subscriptionTriggererDailyJobName string = 'sub-triggerer-daily-int' +param subscriptionTriggererDailyJobName string @description('Longest Build Path Updater Identity Name') -param longestBuildPathUpdaterIdentityName string = 'LongestBuildPathUpdaterInt' +param longestBuildPathUpdaterIdentityName string @description('Longest Build Path Updater Job Name') -param longestBuildPathUpdaterJobName string = 'longest-path-updater-job-int' +param longestBuildPathUpdaterJobName string @description('Feed Cleaner Job name') -param feedCleanerJobName string = 'feed-cleaner-int' +param feedCleanerJobName string @description('Feed Cleaner Identity name') -param feedCleanerIdentityName string = 'FeedCleanerInt' +param feedCleanerIdentityName string @description('Network security group name') -param networkSecurityGroupName string = 'product-construction-service-nsg-int' +param networkSecurityGroupName string @description('Resource group where PCS IP resources will be created') -param infrastructureResourceGroupName string = 'product-construction-service-ip-int' +param infrastructureResourceGroupName string // azure system role for setting up acr pull access var acrPullRole = subscriptionResourceId('Microsoft.Authorization/roleDefinitions', '7f951dda-4ed3-4680-a7ca-43fe172d538d') @@ -104,6 +104,10 @@ var contributorRole = subscriptionResourceId('Microsoft.Authorization/roleDefini var blobContributorRole = subscriptionResourceId('Microsoft.Authorization/roleDefinitions', 'ba92f5b4-2d11-453d-a403-e96b0029c9fe') // Key Vault Crypto User role var kvCryptoUserRole = subscriptionResourceId('Microsoft.Authorization/roleDefinitions', '12338af0-0e69-4776-bea7-57ae8d297424') +// Reader role +var readerRole = subscriptionResourceId('Microsoft.Authorization/roleDefinitions', 'acdd72a7-3385-48ef-bd42-f606fba81ae7') +// Container Apps ManagedEnvironments Contributor Role +var containerAppsManagedEnvironmentsContributor = subscriptionResourceId('Microsoft.Authorization/roleDefinitions', '57cc5028-e6a7-4284-868d-0611c5923f8d') module networkSecurityGroupModule 'nsg.bicep' = { name: 'networkSecurityGroupModule' @@ -132,6 +136,8 @@ module containerEnvironmentModule 'container-environment.bicep' = { productConstructionServiceSubnetId: virtualNetworkModule.outputs.productConstructionServiceSubnetId infrastructureResourceGroupName: infrastructureResourceGroupName applicationInsightsName: applicationInsightsName + containerAppsManagedEnvironmentsContributor: containerAppsManagedEnvironmentsContributor + deploymentIdentityPrincipalId: managedIdentitiesModule.outputs.deploymentIdentityPrincipalId } } @@ -144,6 +150,7 @@ module managedIdentitiesModule 'managed-identities.bicep' = { subscriptionTriggererIdentityName: subscriptionTriggererIdentityName longestBuildPathUpdaterIdentityName: longestBuildPathUpdaterIdentityName feedCleanerIdentityName: feedCleanerIdentityName + contributorRole: contributorRole } } diff --git a/eng/service-templates/ProductConstructionService/provision.ps1 b/eng/service-templates/ProductConstructionService/provision.ps1 index c75893135e..ff4b2ac6fb 100644 --- a/eng/service-templates/ProductConstructionService/provision.ps1 +++ b/eng/service-templates/ProductConstructionService/provision.ps1 @@ -1,5 +1,6 @@ param( - [Parameter(Mandatory=$true)][string]$subscriptionName + [Parameter(Mandatory=$true)][string]$subscriptionName, + [Parameter(Mandatory=$true)][string]$bicepparamFileName ) az account set --subscription $subscriptionName @@ -7,5 +8,5 @@ az account set --subscription $subscriptionName # creates a resource group `product-construction-service` in West US 2 az group create --name product-construction-service --location "West US 2" -$provisionFilePath = Join-Path -Path $PSScriptRoot -ChildPath "provision.bicep" -az deployment group create --resource-group product-construction-service --template-file $provisionFilePath --name deploy \ No newline at end of file +$paramFile = Join-Path -Path $PSScriptRoot -ChildPath $bicepparamFileName +az deployment group create --resource-group product-construction-service --parameters $paramFile --name deploy \ No newline at end of file diff --git a/eng/service-templates/ProductConstructionService/redis.bicep b/eng/service-templates/ProductConstructionService/redis.bicep index 214dcf80a9..b36f9100c0 100644 --- a/eng/service-templates/ProductConstructionService/redis.bicep +++ b/eng/service-templates/ProductConstructionService/redis.bicep @@ -34,7 +34,7 @@ resource pcsRedisDataContributorRoleAssignment 'Microsoft.Cache/redis/accessPoli // allow redis cache read / write access to the service's identity resource deploymentRedisDataContributorRoleAssignment 'Microsoft.Cache/redis/accessPolicyAssignments@2024-03-01' = { - name: guid(subscription().id, resourceGroup().id, 'pcsDataContributor') + name: guid(subscription().id, resourceGroup().id, 'deploymentDataContributor') parent: redisCache properties: { accessPolicyName: 'Data Contributor' diff --git a/eng/service-templates/ProductConstructionService/staging.bicepparam b/eng/service-templates/ProductConstructionService/staging.bicepparam new file mode 100644 index 0000000000..d0cfeb23af --- /dev/null +++ b/eng/service-templates/ProductConstructionService/staging.bicepparam @@ -0,0 +1,57 @@ +using 'provision.bicep' + +param location = 'westus2' + +param containerRegistryName = 'productconstructionint' + +param containerCpuCoreCount = '1.0' + +param containerMemory = '2Gi' + +param aspnetcoreEnvironment = 'Staging' + +param applicationInsightsName = 'product-construction-service-ai-int' + +param keyVaultName = 'ProductConstructionInt' + +param devKeyVaultName = 'ProductConstructionDev' + +param azureCacheRedisName = 'product-construction-service-redis-int' + +param logAnalyticsName = 'product-construction-service-workspace-int' + +param containerEnvironmentName = 'product-construction-service-env-int' + +param productConstructionServiceName = 'product-construction-int' + +param storageAccountName = 'productconstructionint' + +param pcsIdentityName = 'ProductConstructionServiceInt' + +param deploymentIdentityName = 'ProductConstructionServiceDeploymentInt' + +param containerImageName = 'mcr.microsoft.com/azuredocs/containerapps-helloworld:latest' + +param virtualNetworkName = 'product-construction-service-vnet-int' + +param productConstructionServiceSubnetName = 'product-construction-service-subnet' + +param subscriptionTriggererIdentityName = 'SubscriptionTriggererInt' + +param subscriptionTriggererWeeklyJobName = 'sub-triggerer-weekly-int' + +param subscriptionTriggererTwiceDailyJobName = 'sub-triggerer-twicedaily-int' + +param subscriptionTriggererDailyJobName = 'sub-triggerer-daily-int' + +param longestBuildPathUpdaterIdentityName = 'LongestBuildPathUpdaterInt' + +param longestBuildPathUpdaterJobName = 'longest-path-updater-job-int' + +param feedCleanerJobName = 'feed-cleaner-int' + +param feedCleanerIdentityName = 'FeedCleanerInt' + +param networkSecurityGroupName = 'product-construction-service-nsg-int' + +param infrastructureResourceGroupName = 'product-construction-service-ip-int' diff --git a/eng/templates/jobs/e2e-pcs-tests.yml b/eng/templates/jobs/e2e-pcs-tests.yml index 1750a1750b..05ead7ae11 100644 --- a/eng/templates/jobs/e2e-pcs-tests.yml +++ b/eng/templates/jobs/e2e-pcs-tests.yml @@ -17,13 +17,22 @@ jobs: # https://dev.azure.com/dnceng/internal/_library?itemType=VariableGroups&view=VariableGroupView&variableGroupId=20&path=Publish-Build-Assets # Required for MaestroAppClientId, MaestroStagingAppClientId - group: Publish-Build-Assets - - group: MaestroInt KeyVault - - name: PcsTestEndpoint - value: https://product-construction-int.delightfuldune-c0f01ab0.westus2.azurecontainerapps.io - - name: ScenarioTestSubscription - value: "Darc: Maestro Staging" - - name: MaestroAppId - value: $(MaestroStagingAppClientId) + - ${{ if ne(variables['Build.SourceBranch'], 'refs/heads/production') }}: + - group: MaestroInt KeyVault + - name: PcsTestEndpoint + value: https://product-construction-int.delightfuldune-c0f01ab0.westus2.azurecontainerapps.io + - name: ScenarioTestSubscription + value: "Darc: Maestro Staging" + - name: MaestroAppId + value: $(MaestroStagingAppClientId) + - ${{ else }}: + - group: MaestroProd KeyVault + - name: PcsTestEndpoint + value: https://product-construction-prod.wittysky-0c79e3cc.westus2.azurecontainerapps.io + - name: ScenarioTestSubscription + value: "Darc: Maestro Production" + - name: MaestroAppId + value: $(MaestroAppClientId) steps: - download: current displayName: Download Darc @@ -49,8 +58,11 @@ jobs: displayName: Install .NET - powershell: | + $darcNupkg = Get-ChildItem -Path $(Pipeline.Workspace)\PackageArtifacts -Filter Microsoft.DotNet.Darc* + $darcNupkg.Name -match "Microsoft.DotNet.Darc.(.*).nupkg" + $darcVersion = $Matches[1] mkdir darc - .\.dotnet\dotnet tool install Microsoft.DotNet.Darc --prerelease --tool-path .\darc --add-source $(Pipeline.Workspace)\PackageArtifacts + .\.dotnet\dotnet tool install Microsoft.DotNet.Darc --tool-path .\darc --add-source $(Pipeline.Workspace)\PackageArtifacts --version $darcVersion displayName: Install Darc - task: AzureCLI@2 diff --git a/src/Maestro/Client/src/MaestroApiOptions.cs b/src/Maestro/Client/src/MaestroApiOptions.cs index d6d549c4ba..e7385abe87 100644 --- a/src/Maestro/Client/src/MaestroApiOptions.cs +++ b/src/Maestro/Client/src/MaestroApiOptions.cs @@ -22,6 +22,7 @@ public partial class MaestroApiOptions public const string StagingMaestroUri = "https://maestro.int-dot.net/"; public const string OldPcsStagingUri = "https://maestro-int.westus2.cloudapp.azure.com/"; + public const string PcsProdUri = "https://product-construction-prod.wittysky-0c79e3cc.westus2.azurecontainerapps.io/"; public const string PcsStagingUri = "https://product-construction-int.delightfuldune-c0f01ab0.westus2.azurecontainerapps.io/"; public const string PcsLocalUri = "https://localhost:53180/"; @@ -34,6 +35,7 @@ public partial class MaestroApiOptions [PcsStagingUri.TrimEnd('/')] = MaestroStagingAppId, [PcsLocalUri.TrimEnd('/')] = MaestroStagingAppId, + [PcsProdUri.TrimEnd('/')] = MaestroProductionAppId, [ProductionMaestroUri.TrimEnd('/')] = MaestroProductionAppId, [OldProductionMaestroUri.TrimEnd('/')] = MaestroProductionAppId, }; diff --git a/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Production.json b/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Production.json new file mode 100644 index 0000000000..3291ac6785 --- /dev/null +++ b/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Production.json @@ -0,0 +1,28 @@ +{ + "KeyVaultName": "ProductConstructionProd", + "ConnectionStrings": { + "queues": "https://productconstructionprod.queue.core.windows.net", + "redis": "product-construction-service-redis-prod.redis.cache.windows.net:6380,ssl=true" + }, + "ManagedIdentityClientId": "e49bf24a-ec75-490b-803b-6fad99d19159", + "VmrUri": "https://github.com/maestro-auth-test/dnceng-vmr", + "BuildAssetRegistrySqlConnectionString": "Data Source=tcp:maestro-prod.database.windows.net,1433; Initial Catalog=BuildAssetRegistry; Authentication=Active Directory Managed Identity; Persist Security Info=False; MultipleActiveResultSets=True; Connect Timeout=30; Encrypt=True; TrustServerCertificate=False; User Id=USER_ID_PLACEHOLDER", + "Kusto": { + "Database": "engineeringdata", + "KustoClusterUri": "https://engsrvprod.westus.kusto.windows.net" + }, + "DataProtection": { + "KeyBlobUri": "https://productconstructionprod.blob.core.windows.net/dataprotection/keys.xml", + "DataProtectionKeyUri": "https://productconstructionprod.vault.azure.net/keys/data-protection-encryption-key/" + }, + "EntraAuthentication": { + // https://ms.portal.azure.com/#view/Microsoft_AAD_IAM/ManagedAppMenuBlade/~/Overview/objectId/caf36d9b-2940-4270-9a1d-c494eda6ea18/appId/54c17f3d-7325-4eca-9db7-f090bfc765a8/preferredSingleSignOnMode~/null/servicePrincipalType/Application/fromNav/ + "ClientId": "54c17f3d-7325-4eca-9db7-f090bfc765a8", + "Scope": [ "api://54c17f3d-7325-4eca-9db7-f090bfc765a8/Maestro.User" ] + }, + "AzureDevOps": { + "default": { + "ManagedIdentityId": "e49bf24a-ec75-490b-803b-6fad99d19159" + } + } +} diff --git a/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Staging.json b/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Staging.json index 4c3b8bbf6d..4ece3ccc3e 100644 --- a/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Staging.json +++ b/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Staging.json @@ -6,10 +6,6 @@ }, "ManagedIdentityClientId": "1d43ba8a-c2a6-4fad-b064-6d8c16fc0745", "VmrUri": "https://github.com/maestro-auth-test/dnceng-vmr", - "Maestro": { - "Uri": "https://maestro.int-dot.net/", - "Token": "[vault(product-construction-service-int-token)]" - }, "BuildAssetRegistrySqlConnectionString": "Data Source=tcp:maestro-int-server.database.windows.net,1433; Initial Catalog=BuildAssetRegistry; Authentication=Active Directory Managed Identity; Persist Security Info=False; MultipleActiveResultSets=True; Connect Timeout=30; Encrypt=True; TrustServerCertificate=False; User Id=USER_ID_PLACEHOLDER", "Kusto": { "Database": "engineeringdata", diff --git a/src/ProductConstructionService/ProductConstructionService.Client/ProductConstructionServiceApiOptions.cs b/src/ProductConstructionService/ProductConstructionService.Client/ProductConstructionServiceApiOptions.cs index f4259736cb..ca4f0a75a5 100644 --- a/src/ProductConstructionService/ProductConstructionService.Client/ProductConstructionServiceApiOptions.cs +++ b/src/ProductConstructionService/ProductConstructionService.Client/ProductConstructionServiceApiOptions.cs @@ -21,7 +21,8 @@ public partial class ProductConstructionServiceApiOptions public const string OldProductionMaestroUri = "https://maestro-prod.westus2.cloudapp.azure.com/"; public const string StagingMaestroUri = "https://maestro.int-dot.net/"; - public const string OldPcsStagingUri = "https://maestro-int.westus2.cloudapp.azure.com/"; + public const string OldStagingMaestroUri = "https://maestro-int.westus2.cloudapp.azure.com/"; + public const string PcsProdUri = "https://product-construction-prod.wittysky-0c79e3cc.westus2.azurecontainerapps.io/"; public const string PcsStagingUri = "https://product-construction-int.delightfuldune-c0f01ab0.westus2.azurecontainerapps.io/"; public const string PcsLocalUri = "https://localhost:53180/"; @@ -30,12 +31,13 @@ public partial class ProductConstructionServiceApiOptions private static readonly Dictionary EntraAppIds = new Dictionary { [StagingMaestroUri.TrimEnd('/')] = MaestroStagingAppId, - [OldPcsStagingUri.TrimEnd('/')] = MaestroStagingAppId, + [OldStagingMaestroUri.TrimEnd('/')] = MaestroStagingAppId, [PcsStagingUri.TrimEnd('/')] = MaestroStagingAppId, [PcsLocalUri.TrimEnd('/')] = MaestroStagingAppId, [ProductionMaestroUri.TrimEnd('/')] = MaestroProductionAppId, [OldProductionMaestroUri.TrimEnd('/')] = MaestroProductionAppId, + [PcsProdUri.TrimEnd('/')] = MaestroProductionAppId }; /// diff --git a/src/ProductConstructionService/ProductConstructionService.Deployment/Deployer.cs b/src/ProductConstructionService/ProductConstructionService.Deployment/Deployer.cs index de647ceef2..f72620ce82 100644 --- a/src/ProductConstructionService/ProductConstructionService.Deployment/Deployer.cs +++ b/src/ProductConstructionService/ProductConstructionService.Deployment/Deployer.cs @@ -41,8 +41,8 @@ public Deployer( ArmClient client = armClient; SubscriptionResource subscription = client.GetSubscriptionResource(new ResourceIdentifier($"/subscriptions/{_options.SubscriptionId}")); - _resourceGroup = subscription.GetResourceGroups().Get("product-construction-service"); - _containerApp = _resourceGroup.GetContainerApp("product-construction-int").Value; + _resourceGroup = subscription.GetResourceGroups().Get(_options.ResourceGroupName); + _containerApp = _resourceGroup.GetContainerApp(_options.ContainerAppName).Value; _redisCacheFactory = redisCacheFactory; _serviceProvider = serviceProvider; _logger = logger; @@ -60,24 +60,33 @@ public async Task DeployAsync() var activeRevisionTrafficWeight = trafficWeights.FirstOrDefault(weight => weight.Weight == 100) ?? throw new ArgumentException("Container app has no active revision, please investigate manually"); - var activeRevision = (await _containerApp.GetContainerAppRevisionAsync(activeRevisionTrafficWeight.RevisionName)).Value; - var replicas = activeRevision.GetContainerAppReplicas().ToList(); + string inactiveRevisionLabel; + // When we create the ACA, the first revision won't have a name + if (activeRevisionTrafficWeight.RevisionName == null) + { + inactiveRevisionLabel = "blue"; + } + else + { + var activeRevision = (await _containerApp.GetContainerAppRevisionAsync(activeRevisionTrafficWeight.RevisionName)).Value; + var replicas = activeRevision.GetContainerAppReplicas().ToList(); - _logger.LogInformation("Currently active revision {revisionName} with label {label}", - activeRevisionTrafficWeight.RevisionName, - activeRevisionTrafficWeight.Label); + _logger.LogInformation("Currently active revision {revisionName} with label {label}", + activeRevisionTrafficWeight.RevisionName, + activeRevisionTrafficWeight.Label); - // Determine the label of the inactive revision - string inactiveRevisionLabel = activeRevisionTrafficWeight.Label == "blue" ? "green" : "blue"; + // Determine the label of the inactive revision + inactiveRevisionLabel = activeRevisionTrafficWeight.Label == "blue" ? "green" : "blue"; - _logger.LogInformation("Next revision will be deployed with label {inactiveLabel}", inactiveRevisionLabel); - _logger.LogInformation("Removing label {inactiveLabel} from inactive revision", inactiveRevisionLabel); + _logger.LogInformation("Next revision will be deployed with label {inactiveLabel}", inactiveRevisionLabel); + _logger.LogInformation("Removing label {inactiveLabel} from inactive revision", inactiveRevisionLabel); - // Cleanup all revisions except the currently active one - await CleanupRevisionsAsync(trafficWeights.Where(weight => weight != activeRevisionTrafficWeight)); + // Cleanup all revisions except the currently active one + await CleanupRevisionsAsync(trafficWeights.Where(weight => weight != activeRevisionTrafficWeight)); - // Tell the active revision to finish current work items and stop processing new ones - await StopProcessingNewJobs(activeRevisionTrafficWeight.RevisionName); + // Tell the active revision to finish current work items and stop processing new ones + await StopProcessingNewJobs(activeRevisionTrafficWeight.RevisionName); + } var newRevisionName = $"{_options.ContainerAppName}--{_options.NewImageTag}"; var newImageFullUrl = $"{_options.ContainerRegistryName}.azurecr.io/{_options.ImageName}:{_options.NewImageTag}"; diff --git a/src/ProductConstructionService/ProductConstructionService.FeedCleaner/appsettings.Production.json b/src/ProductConstructionService/ProductConstructionService.FeedCleaner/appsettings.Production.json new file mode 100644 index 0000000000..9b74b68cbf --- /dev/null +++ b/src/ProductConstructionService/ProductConstructionService.FeedCleaner/appsettings.Production.json @@ -0,0 +1,9 @@ +{ + "BuildAssetRegistrySqlConnectionString": "Data Source=tcp:maestro-prod.database.windows.net,1433; Initial Catalog=BuildAssetRegistry; Authentication=Active Directory Managed Identity; Persist Security Info=False; MultipleActiveResultSets=True; Connect Timeout=30; Encrypt=True; TrustServerCertificate=False; User Id=USER_ID_PLACEHOLDER", + "AzureDevOps": { + "default": { + "ManagedIdentityId": "520f92da-8df7-4bdf-afcd-400caf2c23b6" + } + }, + "ManagedIdentityClientId": "520f92da-8df7-4bdf-afcd-400caf2c23b6" +} diff --git a/src/ProductConstructionService/ProductConstructionService.LongestBuildPathUpdater/appsettings.Production.json b/src/ProductConstructionService/ProductConstructionService.LongestBuildPathUpdater/appsettings.Production.json new file mode 100644 index 0000000000..c12beb83c9 --- /dev/null +++ b/src/ProductConstructionService/ProductConstructionService.LongestBuildPathUpdater/appsettings.Production.json @@ -0,0 +1,8 @@ +{ + "BuildAssetRegistrySqlConnectionString": "Data Source=tcp:maestro-prod.database.windows.net,1433; Initial Catalog=BuildAssetRegistry; Authentication=Active Directory Managed Identity; Persist Security Info=False; MultipleActiveResultSets=True; Connect Timeout=30; Encrypt=True; TrustServerCertificate=False; User Id=USER_ID_PLACEHOLDER", + "Kusto": { + "Database": "engineeringdata", + "KustoClusterUri": "https://engsrvprod.westus.kusto.windows.net" + }, + "ManagedIdentityClientId": "f15e5ffe-fe05-4430-8226-a8ab343d0487" +} diff --git a/src/ProductConstructionService/ProductConstructionService.SubscriptionTriggerer/appsettings.Production.json b/src/ProductConstructionService/ProductConstructionService.SubscriptionTriggerer/appsettings.Production.json new file mode 100644 index 0000000000..46da7041cb --- /dev/null +++ b/src/ProductConstructionService/ProductConstructionService.SubscriptionTriggerer/appsettings.Production.json @@ -0,0 +1,11 @@ +{ + "ConnectionStrings": { + "queues": "https://productconstructionprod.queue.core.windows.net" + }, + "ManagedIdentityClientId": "3a1499d3-24dc-4e87-960b-3cff340f0fd4", + "BuildAssetRegistrySqlConnectionString": "Data Source=tcp:maestro-prod.database.windows.net,1433; Initial Catalog=BuildAssetRegistry; Authentication=Active Directory Managed Identity; Persist Security Info=False; MultipleActiveResultSets=True; Connect Timeout=30; Encrypt=True; TrustServerCertificate=False; User Id=USER_ID_PLACEHOLDER", + "Kusto": { + "Database": "engineeringdata", + "KustoClusterUri": "https://engsrvprod.westus.kusto.windows.net" + } +} diff --git a/src/ProductConstructionService/Readme.md b/src/ProductConstructionService/Readme.md index 6f11e52f70..017d3e35df 100644 --- a/src/ProductConstructionService/Readme.md +++ b/src/ProductConstructionService/Readme.md @@ -138,22 +138,19 @@ If the service is being recreated and the same Managed Identity name is reused, Once the resources are created and configured: - Go to the newly created User Assigned Managed Identity (the one that's assigned to the container app, not the deployment one) - - Copy the Client ID, and paste it in the correct appconfig.json, under `ManagedIdentityClientId` - - Add this identity as a user to AzDO so it can get AzDO tokens (you'll need a saw for this). You might have to remove the old user identity before doing this - - It needs to be able to manage code / pull requests and manage feeds (this is done in the artifact section). + - Copy the Client ID, and paste it in the correct appconfig.json, under `ManagedIdentityClientId`. Do this for all services (PCS, Subscription Triggerer, LongestBuildPathUpdater and FeedCleaner) + - Add the PCS and FeedCleaner identity as a user to AzDO so it can get AzDO tokens (you'll need a saw for this). You might have to remove the old user identity before doing this + - The PCS identity needs to be able to manage code / pull requests and manage feeds (this is done in the artifact section). + - The FeedCleaner identity needs to be in the `Feed Managers` permissions group in `dnceng/internal` - Update the `ProductConstructionServiceDeploymentProd` (or `ProductConstructionServiceDeploymentInt`) Service Connection with the new MI information (you'll also have to create a Federated Credential in the MI) - - Update the default PCS URI in `ProductConstructionServiceApiOptions`. + - Update the appropriate PCS URI in `ProductConstructionServiceApiOptions` and `MaestroApiOptions`. We're not able to configure a few Kusto things in bicep: - Give the PCS Managed Identity the permissions it needs: - Go to the Kusto Cluster, and select the database you want the MI to have access to - Go to permissions -> Add -> Viewer and select the newly created PCS Managed Identity - - Create a private endpoint between the Kusto cluster and PCS - - Go to the Kusto cluster -> Networking -> Private endpoint connections -> + Private endpoint - - Select the appropriate subscription and resource group. Name the private endpoint something meaningful, like `pcs-kusto-private-connection` - - On the Resource page, set the `Target sub-resource` to `cluster` - - On the Virtual Network page, select the product-construction-service-vntet-int/prod, and the private-endpoints-subnet, leave the rest as default - - leave the rest of the settings as default + +Give the Deployment Identity Reader role for the whole subscription. The last part is setting up the pipeline: - Make sure all of the resources referenced in the yaml have the correct names From df35cbe489bc7454654213b05c9cdb150e46f7bb Mon Sep 17 00:00:00 2001 From: Oleksandr Didyk <106967057+oleksandr-didyk@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:05:38 +0200 Subject: [PATCH 7/7] Add multiple work item consumers (#4049) Co-authored-by: dkurepa --- .../appsettings.Development.json | 2 +- .../appsettings.json | 3 ++- .../WorkItemConfiguration.cs | 15 ++++++++++++++- .../WorkItemConsumer.cs | 5 ++++- .../WorkItemProcessorState.cs | 2 +- .../WorkItemScopeManager.cs | 9 ++++++++- .../WorkItemsProcessorScopeManagerTests.cs | 4 ++-- 7 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Development.json b/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Development.json index 956a0bf33d..8beb2f6a65 100644 --- a/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Development.json +++ b/src/ProductConstructionService/ProductConstructionService.Api/appsettings.Development.json @@ -10,7 +10,7 @@ "WorkItemQueueName": "pcs-workitems", "QueuePollTimeout": "00:00:05", "MaxWorkItemRetries": 3, - "QueueMessageInvisibilityTime": "00:05:00" + "QueueMessageInvisibilityTime": "00:15:00" }, "Maestro": { "Uri": "http://localhost:8088/", diff --git a/src/ProductConstructionService/ProductConstructionService.Api/appsettings.json b/src/ProductConstructionService/ProductConstructionService.Api/appsettings.json index c0803ea3ac..2d14aa38fd 100644 --- a/src/ProductConstructionService/ProductConstructionService.Api/appsettings.json +++ b/src/ProductConstructionService/ProductConstructionService.Api/appsettings.json @@ -13,10 +13,11 @@ }, "AllowedHosts": "*", "WorkItemQueueName": "pcs-workitems", + "WorkItemConsumerCount": 5, "WorkItemConsumerOptions": { "QueuePollTimeout": "00:01:00", "MaxWorkItemRetries": 3, - "QueueMessageInvisibilityTime": "00:01:00" + "QueueMessageInvisibilityTime": "00:15:00" }, "EntraAuthentication": { "Instance": "https://login.microsoftonline.com/", diff --git a/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemConfiguration.cs b/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemConfiguration.cs index 8cb6c0dc5c..e71f2b0285 100644 --- a/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemConfiguration.cs +++ b/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemConfiguration.cs @@ -14,6 +14,7 @@ namespace ProductConstructionService.WorkItems; public static class WorkItemConfiguration { public const string WorkItemQueueNameConfigurationKey = "WorkItemQueueName"; + public const string WorkItemConsumerCountConfigurationKey = "WorkItemConsumerCount"; public const string ReplicaNameKey = "CONTAINER_APP_REPLICA_NAME"; public const int PollingRateSeconds = 10; @@ -40,7 +41,19 @@ public static void AddWorkItemQueues(this IHostApplicationBuilder builder, Defau builder.Configuration.GetRequiredValue(WorkItemQueueNameConfigurationKey); builder.Services.Configure( builder.Configuration.GetSection(WorkItemConsumerOptions.ConfigurationKey)); - builder.Services.AddHostedService(); + + var consumerCount = int.Parse( + builder.Configuration.GetRequiredValue(WorkItemConsumerCountConfigurationKey)); + + for (int i = 0; i < consumerCount; i++) + { + var consumerId = $"WorkItemConsumer_{i}"; + + // https://github.com/dotnet/runtime/issues/38751 + builder.Services.AddSingleton( + p => ActivatorUtilities.CreateInstance(p, consumerId)); + } + builder.Services.AddTransient(); } diff --git a/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemConsumer.cs b/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemConsumer.cs index 904c85c239..655afc25f2 100644 --- a/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemConsumer.cs +++ b/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemConsumer.cs @@ -12,6 +12,7 @@ namespace ProductConstructionService.WorkItems; internal class WorkItemConsumer( + string consumerId, ILogger logger, IOptions options, WorkItemScopeManager scopeManager, @@ -19,6 +20,7 @@ internal class WorkItemConsumer( IMetricRecorder metricRecorder) : BackgroundService { + private readonly string _consumerId = consumerId; private readonly ILogger _logger = logger; private readonly IOptions _options = options; private readonly WorkItemScopeManager _scopeManager = scopeManager; @@ -31,7 +33,8 @@ protected override async Task ExecuteAsync(CancellationToken cancellationToken) await Task.Yield(); QueueClient queueClient = queueServiceClient.GetQueueClient(_options.Value.WorkItemQueueName); - _logger.LogInformation("Starting to process PCS queue {queueName}", _options.Value.WorkItemQueueName); + _logger.LogInformation("Consumer {consumerId} starting to process PCS queue {queueName}", _consumerId, _options.Value.WorkItemQueueName); + while (!cancellationToken.IsCancellationRequested) { await using (WorkItemScope workItemScope = await _scopeManager.BeginWorkItemScopeWhenReadyAsync()) diff --git a/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemProcessorState.cs b/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemProcessorState.cs index 4840f5c6ec..42188ef738 100644 --- a/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemProcessorState.cs +++ b/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemProcessorState.cs @@ -64,7 +64,7 @@ public async Task ReturnWhenWorkingAsync(int pollingRateSeconds) do { status = await _cache.GetAsync(); - } while (_autoResetEvent.WaitIfTrue(() => status == Stopped, pollingRateSeconds)); + } while (_autoResetEvent.WaitIfTrue(() => status != Working, pollingRateSeconds)); } public async Task SetStoppedIfStoppingAsync() diff --git a/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemScopeManager.cs b/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemScopeManager.cs index cdffdcb9d5..15632d52ce 100644 --- a/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemScopeManager.cs +++ b/src/ProductConstructionService/ProductConstructionService.WorkItems/WorkItemScopeManager.cs @@ -13,6 +13,8 @@ public class WorkItemScopeManager private readonly WorkItemProcessorState _state; private readonly int _pollingRateSeconds; + private int _activeWorkItems = 0; + public WorkItemScopeManager( IServiceProvider serviceProvider, WorkItemProcessorState state, @@ -32,6 +34,8 @@ public async Task BeginWorkItemScopeWhenReadyAsync() await _state.ReturnWhenWorkingAsync(_pollingRateSeconds); var scope = _serviceProvider.CreateScope(); + Interlocked.Increment(ref _activeWorkItems); + return new WorkItemScope( scope.ServiceProvider.GetRequiredService>(), WorkItemFinishedAsync, @@ -41,7 +45,10 @@ public async Task BeginWorkItemScopeWhenReadyAsync() private async Task WorkItemFinishedAsync() { - await _state.SetStoppedIfStoppingAsync(); + if (Interlocked.Decrement(ref _activeWorkItems) == 0) + { + await _state.SetStoppedIfStoppingAsync(); + } } public async Task InitializationFinished() diff --git a/test/ProductConstructionService.WorkItem.Tests/WorkItemsProcessorScopeManagerTests.cs b/test/ProductConstructionService.WorkItem.Tests/WorkItemsProcessorScopeManagerTests.cs index 858aaf8e60..a9d97c53cd 100644 --- a/test/ProductConstructionService.WorkItem.Tests/WorkItemsProcessorScopeManagerTests.cs +++ b/test/ProductConstructionService.WorkItem.Tests/WorkItemsProcessorScopeManagerTests.cs @@ -55,9 +55,9 @@ public async Task WorkItemsProcessorStatusNormalFlow() TaskCompletionSource workItemCompletion1 = new(); TaskCompletionSource workItemCompletion2 = new(); - Thread t = new(() => + Thread t = new(async () => { - using (_scopeManager.BeginWorkItemScopeWhenReadyAsync()) { } + await using (await _scopeManager.BeginWorkItemScopeWhenReadyAsync()) { } workItemCompletion1.SetResult(); }); t.Start();