-
Notifications
You must be signed in to change notification settings - Fork 80
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
Conversation
public IImmutableList<MergePolicyEvaluationResult> Results { get; } | ||
public string Id { get; set; } | ||
|
||
public IEnumerable<MergePolicyEvaluationResult> Results { get; set; } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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*
c7bd078
to
24347ed
Compare
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
src/Maestro/Maestro.MergePolicies/NoRequestedChangesMergePolicy.cs
Outdated
Show resolved
Hide resolved
@@ -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? |
There was a problem hiding this comment.
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.
src/Maestro/Maestro.MergePolicyEvaluation/MergePolicyEvaluationResult.cs
Outdated
Show resolved
Hide resolved
public IImmutableList<MergePolicyEvaluationResult> Results { get; } | ||
public string Id { get; set; } | ||
|
||
public IEnumerable<MergePolicyEvaluationResult> Results { get; set; } |
There was a problem hiding this comment.
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*
src/Maestro/Maestro.MergePolicyEvaluation/MergePolicyEvaluationResults.cs
Outdated
Show resolved
Hide resolved
src/Maestro/Maestro.MergePolicyEvaluation/MergePolicyEvaluationStatus.cs
Outdated
Show resolved
Hide resolved
@@ -964,6 +973,11 @@ private static PullRequestUpdateSummary CreatePrSummaryFromInProgressPr( | |||
pr.CodeFlowDirection); | |||
} | |||
|
|||
private static string GetMergePolicyEvaluationResultsCacheId(UpdaterId updaterId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
...ProductConstructionService/ProductConstructionService.DependencyFlow/MergePolicyEvaluator.cs
Outdated
Show resolved
Hide resolved
...ProductConstructionService/ProductConstructionService.DependencyFlow/MergePolicyEvaluator.cs
Outdated
Show resolved
Hide resolved
src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs
Outdated
Show resolved
Hide resolved
... and more minor fixes after CR
There was a problem hiding this 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. |
string TargetBranchSha = prInfo.HeadBranch; | ||
|
There was a problem hiding this comment.
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.
string TargetBranchSha = prInfo.HeadBranch; | |
string TargetBranchSha = prInfo.TargetBranchCommitSha; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @copilot ! 🙏
{ | ||
var results = new List<MergePolicyEvaluationResult>(); | ||
IDictionary<string, MergePolicyEvaluationResult> cachedResultsByPolicyName = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
#4610 (comment)