Skip to content

Allow some PR checks to be skipped #4717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 28, 2025

Conversation

adamzip
Copy link
Contributor

@adamzip adamzip commented Apr 23, 2025

public IImmutableList<MergePolicyEvaluationResult> Results { get; }
public string Id { get; set; }

public IEnumerable<MergePolicyEvaluationResult> Results { get; set; }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This IEnumerable can maybe be reverted back to IImmutableList as it was - this change was in order to attempt to fix some redis serialization error I'd had previously.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to keep it like this too. There's also IReadOnlyCollection<> which is a bit easier to work with than IImmutable*

@adamzip adamzip force-pushed the avoid-rerunning-pr-checks branch from c7bd078 to 24347ed Compare April 24, 2025 13:16
@adamzip adamzip marked this pull request as ready for review April 25, 2025 08:01
@adamzip
Copy link
Contributor Author

adamzip commented Apr 25, 2025

Some tests are failing because when they check that there's no InProgressPullRequests in the cache, it actually picks up all cache objects to do that, not just InProgressPullRequests. Will fix this

@@ -964,6 +973,11 @@ private static PullRequestUpdateSummary CreatePrSummaryFromInProgressPr(
pr.CodeFlowDirection);
}

private static string GetMergePolicyEvaluationResultsCacheId(UpdaterId updaterId)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this - the prefix is already added automatically based on the object type

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

@@ -37,7 +37,8 @@ public override Task<MergePolicyEvaluationResult> EvaluateAsync(PullRequestUpdat
.Append("\n\nThe documentation can be found at this location: ")
.Append("https://github.com/dotnet/arcade/blob/main/Documentation/Darc.md#coherent-parent-dependencies");

return Task.FromResult(Fail("Coherency check failed.", description.ToString()));
//todo Should this be a transient failure?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so - we will need a new commit to fix coherency.

public IImmutableList<MergePolicyEvaluationResult> Results { get; }
public string Id { get; set; }

public IEnumerable<MergePolicyEvaluationResult> Results { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to keep it like this too. There's also IReadOnlyCollection<> which is a bit easier to work with than IImmutable*

@@ -964,6 +973,11 @@ private static PullRequestUpdateSummary CreatePrSummaryFromInProgressPr(
pr.CodeFlowDirection);
}

private static string GetMergePolicyEvaluationResultsCacheId(UpdaterId updaterId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

@adamzip adamzip requested a review from premun April 25, 2025 15:09
@adamzip adamzip self-assigned this Apr 25, 2025
@adamzip adamzip requested review from a team, dkurepa and Copilot and removed request for a team April 25, 2025 15:32
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors several components to allow selected PR checks to be skipped by updating merge policy evaluation statuses and related data structures. Key changes include:

  • Renaming cache state variables and updating test assertions for clarity.
  • Changing merge policy evaluation methods and status enums (e.g., replacing Success/Failure with DecisiveSuccess/TransientSuccess and DecisiveFailure/TransientFailure).
  • Updating interfaces and method signatures to use IReadOnlyCollection instead of IReadOnlyList, as well as minor adjustments for improved consistency across merge policy evaluations.

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/ProductConstructionService.DependencyFlow.Tests/UpdaterTests.cs Renamed cache state properties and updated test assertions.
test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs Adjusted merge status updates with ImmutableList conversion.
src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs Updated merge policy evaluation logic and added new parameters to improve accuracy.
src/ProductConstructionService/ProductConstructionService.DependencyFlow/MergePolicyEvaluator.cs Modified evaluation method to return IEnumerable and updated caching logic.
src/Microsoft.DotNet.Darc/DarcLib/Remote.cs Changed method signature for merge status update from IReadOnlyList to IReadOnlyCollection.
src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs Updated interface parameter types to reflect the new collection type.
src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs Modified merge policy check run updates to use updated naming properties.
src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs Updated merge status info creation with revised ordering and status checks.
src/Maestro/Maestro.MergePolicyEvaluation/* and Maestro.MergePolicies/* Updated merge policy evaluation statuses and helper methods to support decisive and transient outcomes.

Comment on lines 409 to 410
string TargetBranchSha = prInfo.HeadBranch;

Copy link
Preview

Copilot AI Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable 'TargetBranchSha' is assigned from 'prInfo.HeadBranch', which appears to be a branch name rather than a commit SHA. Consider using 'prInfo.TargetBranchCommitSha' or the appropriate property to ensure the correct commit identifier is used.

Suggested change
string TargetBranchSha = prInfo.HeadBranch;
string TargetBranchSha = prInfo.TargetBranchCommitSha;

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @copilot ! 🙏

premun
premun previously approved these changes Apr 28, 2025
{
var results = new List<MergePolicyEvaluationResult>();
IDictionary<string, MergePolicyEvaluationResult> cachedResultsByPolicyName =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: technically these are the actual results, not cached ones, so the method reads a bit confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I'll fix the naming

@adamzip adamzip enabled auto-merge (squash) April 28, 2025 09:49
@adamzip adamzip merged commit a78a06e into dotnet:main Apr 28, 2025
7 of 9 checks passed
premun pushed a commit that referenced this pull request Apr 30, 2025
adamzip added a commit that referenced this pull request Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants