Skip to content

Commit

Permalink
Simplify PR updater, improve logging and cache clean-up (#4240)
Browse files Browse the repository at this point in the history 
Co-authored-by: Oleksandr Didyk <[email protected]>
  • Loading branch information
@premun @oleksandr-didyk
premun and oleksandr-didyk authored Dec 12, 2024
1 parent c9836e5 commit a7d4c92
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 125 deletions.
11 changes: 10 additions & 1 deletion ...tructionService/ProductConstructionService.DependencyFlow/NonBatchedPullRequestUpdater.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ internal class NonBatchedPullRequestUpdater : PullRequestUpdater
private readonly NonBatchedPullRequestUpdaterId _id;
private readonly BuildAssetRegistryContext _context;
private readonly IPullRequestPolicyFailureNotifier _pullRequestPolicyFailureNotifier;
private readonly ILogger<NonBatchedPullRequestUpdater> _logger;

public NonBatchedPullRequestUpdater(
NonBatchedPullRequestUpdaterId id,
Expand Down Expand Up @@ -57,21 +58,29 @@ public NonBatchedPullRequestUpdater(
_id = id;
_context = context;
_pullRequestPolicyFailureNotifier = pullRequestPolicyFailureNotifier;
_logger = logger;
}

public Guid SubscriptionId => _id.SubscriptionId;

private async Task<Subscription?> RetrieveSubscription()
{
Subscription? subscription = await _context.Subscriptions.FindAsync(SubscriptionId);

// This can mainly happen during E2E tests where we delete a subscription
// while some PRs have just been closed and there's a reminder on those still
if (subscription == null)
{
_logger.LogInformation(
$"Failed to find a subscription {SubscriptionId}. " +
"Possibly it was deleted while an existing PR is still tracked. Untracking PR...");

// We don't know if the subscription was a code flow one, so just unset both
await _pullRequestState.TryDeleteAsync();
await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow: true);
await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow: false);
await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow: true);
await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow: false);

return null;
}

Expand Down
1 change: 0 additions & 1 deletion ...ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStatus.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ namespace ProductConstructionService.DependencyFlow;
public enum PullRequestStatus
{
Invalid = 0,
UnknownPR = 1,
Completed = 2,
InProgressCanUpdate = 3,
InProgressCannotUpdate = 4,
Expand Down
177 changes: 55 additions & 122 deletions ...roductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Net;
using Maestro.Contracts;
using Maestro.Data.Models;
using Maestro.MergePolicyEvaluation;
Expand Down Expand Up @@ -91,6 +90,44 @@ public PullRequestUpdater(

protected abstract Task<IReadOnlyList<MergePolicyDefinition>> GetMergePolicyDefinitions();

/// <summary>
/// Applies or queues asset updates for the target repository and branch from the given build and list of assets.
/// </summary>
/// <param name="subscriptionId">The id of the subscription the update comes from</param>
/// <param name="buildId">The build that the updated assets came from</param>
/// <param name="sourceSha">The commit hash that built the assets</param>
/// <param name="assets">The list of assets</param>
/// <remarks>
/// This function will queue updates if there is a pull request and it is currently not-updateable.
/// A pull request is considered "not-updateable" based on merge policies.
/// If at least one merge policy calls <see cref="IMergePolicyEvaluationContext.Pending" /> and
/// no merge policy calls <see cref="IMergePolicyEvaluationContext.Fail" /> then the pull request is considered
/// not-updateable.
///
/// PRs are marked as non-updateable so that we can allow pull request checks to complete on a PR prior
/// to pushing additional commits.
/// </remarks>
public async Task<bool> UpdateAssetsAsync(
Guid subscriptionId,
SubscriptionType type,
int buildId,
string sourceRepo,
string sourceSha,
List<Asset> assets)
{
return await ProcessPendingUpdatesAsync(new()
{
UpdaterId = Id.ToString(),
SubscriptionId = subscriptionId,
SubscriptionType = type,
BuildId = buildId,
SourceSha = sourceSha,
SourceRepo = sourceRepo,
Assets = assets,
IsCoherencyUpdate = false,
});
}

/// <summary>
/// Process any pending pull request updates.
/// </summary>
Expand All @@ -111,23 +148,24 @@ public async Task<bool> ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up
}
else
{
switch (await GetPullRequestStatusAsync(pr, isCodeFlow))
var prStatus = await GetPullRequestStatusAsync(pr, isCodeFlow);
switch (prStatus)
{
case PullRequestStatus.Completed:
case PullRequestStatus.Invalid:
// If the PR is completed, we will open a new one
pr = null;
await _pullRequestState.TryDeleteAsync();
await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow);
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);
case PullRequestStatus.InProgressCannotUpdate:
_logger.LogInformation("PR {url} for subscription {subscriptionId} cannot be updated at this time. Deferring update..", pr.Url, update.SubscriptionId);
await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow);
await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow);
return false;
default:
throw new NotImplementedException($"Unknown PR status {prStatus}");
}
}

Expand All @@ -141,7 +179,6 @@ public async Task<bool> ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up
if (pr != null)
{
await UpdatePullRequestAsync(pr, update);
_logger.LogInformation("Pull request {url} for subscription {subscriptionId} was updated", pr.Url, update.SubscriptionId);
await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow);
return true;
}
Expand All @@ -168,7 +205,8 @@ public async Task<bool> CheckPullRequestAsync(PullRequestCheck pullRequestCheck)
if (inProgressPr == null)
{
_logger.LogInformation("No in-progress pull request found for a PR check");
await ClearAllStateAsync();
await ClearAllStateAsync(isCodeFlow: true);
await ClearAllStateAsync(isCodeFlow: false);
return false;
}

Expand Down Expand Up @@ -203,12 +241,13 @@ protected virtual Task TagSourceRepositoryGitHubContactsIfPossibleAsync(InProgre
// If the PR is currently open, then evaluate the merge policies, which will potentially
// merge the PR if they are successful.
case PrStatus.Open:
var mergePolicyResult = await CheckMergePolicyAsync(pr, remote);
MergePolicyCheckResult mergePolicyResult = await CheckMergePolicyAsync(pr, remote);

_logger.LogInformation("Policy check status for pull request {url} is {result}", pr.Url, mergePolicyResult);

switch (mergePolicyResult)
{
// Policies evaluated successfully and the PR was merged just now
case MergePolicyCheckResult.Merged:
await UpdateSubscriptionsForMergedPRAsync(pr.ContainedSubscriptions);
await AddDependencyFlowEventsAsync(
Expand All @@ -218,7 +257,7 @@ await AddDependencyFlowEventsAsync(
mergePolicyResult,
pr.Url);

await ClearAllStateAsync();
await ClearAllStateAsync(isCodeFlow);
return PullRequestStatus.Completed;

case MergePolicyCheckResult.FailedPolicies:
Expand All @@ -237,6 +276,7 @@ await AddDependencyFlowEventsAsync(
return PullRequestStatus.InProgressCannotUpdate;

default:
await SetPullRequestCheckReminder(pr, isCodeFlow);
throw new NotImplementedException($"Unknown merge policy check result {mergePolicyResult}");
}

Expand All @@ -261,7 +301,7 @@ await AddDependencyFlowEventsAsync(

_logger.LogInformation("PR {url} has been manually {action}. Stopping tracking it", pr.Url, status.ToString().ToLowerInvariant());

await ClearAllStateAsync();
await ClearAllStateAsync(isCodeFlow);

// Also try to clean up the PR branch.
try
Expand Down Expand Up @@ -358,8 +398,7 @@ private async Task UpdateSubscriptionsForMergedPRAsync(IEnumerable<SubscriptionP
ISubscriptionTriggerer triggerer = _updaterFactory.CreateSubscriptionTrigerrer(update.SubscriptionId);
if (!await triggerer.UpdateForMergedPullRequestAsync(update.BuildId))
{
_logger.LogInformation("Failed to update subscription {subscriptionId} for merged PR", update.SubscriptionId);
await ClearAllStateAsync();
_logger.LogWarning("Failed to update subscription {subscriptionId} for merged PR", update.SubscriptionId);
}
}
}
Expand All @@ -381,110 +420,6 @@ private async Task AddDependencyFlowEventsAsync(
}
}

/// <summary>
/// Applies or queues asset updates for the target repository and branch from the given build and list of assets.
/// </summary>
/// <param name="subscriptionId">The id of the subscription the update comes from</param>
/// <param name="buildId">The build that the updated assets came from</param>
/// <param name="sourceSha">The commit hash that built the assets</param>
/// <param name="assets">The list of assets</param>
/// <remarks>
/// This function will queue updates if there is a pull request and it is currently not-updateable.
/// A pull request is considered "not-updateable" based on merge policies.
/// If at least one merge policy calls <see cref="IMergePolicyEvaluationContext.Pending" /> and
/// no merge policy calls <see cref="IMergePolicyEvaluationContext.Fail" /> then the pull request is considered
/// not-updateable.
///
/// PRs are marked as non-updateable so that we can allow pull request checks to complete on a PR prior
/// to pushing additional commits.
/// </remarks>
public async Task<bool> UpdateAssetsAsync(
Guid subscriptionId,
SubscriptionType type,
int buildId,
string sourceRepo,
string sourceSha,
List<Asset> assets)
{
// Check if we track an on-going PR already
InProgressPullRequest? pr = await _pullRequestState.TryGetStateAsync();
bool isCodeFlow = type == SubscriptionType.DependenciesAndSources;

bool canUpdate;
if (pr == null)
{
_logger.LogInformation("No existing pull request state found");
canUpdate = true;
}
else
{
var status = await GetPullRequestStatusAsync(pr, isCodeFlow);
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
{
UpdaterId = Id.ToString(),
SubscriptionId = subscriptionId,
SubscriptionType = type,
BuildId = buildId,
SourceSha = sourceSha,
SourceRepo = sourceRepo,
Assets = assets,
IsCoherencyUpdate = false,
};

// Regardless of code flow or regular PR, if the PR are not complete, postpone the update
if (pr != null && !canUpdate)
{
await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow);
await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow);
_logger.LogInformation("Pull request '{prUrl}' cannot be updated, update queued", pr!.Url);
return true;
}

if (type == SubscriptionType.DependenciesAndSources)
{
return await ProcessCodeFlowUpdateAsync(update, pr);
}

try
{
if (pr == null)
{
var prUrl = await CreatePullRequestAsync(update);
if (prUrl == null)
{
_logger.LogInformation("Updates require no changes, no pull request created");
}
else
{
_logger.LogInformation("Pull request '{prUrl}' created", prUrl);
}

return true;
}

await UpdatePullRequestAsync(pr, update);
}
catch (HttpRequestException reqEx) when (reqEx.Message.Contains(((int)HttpStatusCode.Unauthorized).ToString()))
{
// We want to preserve the HttpRequestException's information but it's not serializable
// We'll log the full exception object so it's in Application Insights, and strip any single quotes from the message to ensure
// GitHub issues are properly created.
_logger.LogError(reqEx, "Failure to authenticate to repository");
return false;
}

return true;
}

/// <summary>
/// Creates a pull request from the given updates.
/// </summary>
Expand Down Expand Up @@ -846,13 +781,11 @@ private async Task SetPullRequestCheckReminder(InProgressPullRequest prState, bo
await _pullRequestState.SetAsync(prState);
}

private async Task ClearAllStateAsync()
private async Task ClearAllStateAsync(bool isCodeFlow)
{
await _pullRequestState.TryDeleteAsync();
await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow: true);
await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow: true);
await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow: false);
await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow: false);
await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow);
await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow);
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_SdkUpdate.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ await RunDarcAsync("add-dependency",
repo.Directory,
isCompleted: false,
isUpdated: false,
cleanUp: true,
cleanUp: false,
expectedFeeds: null,
notExpectedFeeds: null);
}
Expand Down

0 comments on commit a7d4c92

Please sign in to comment.