-
Notifications
You must be signed in to change notification settings - Fork 78
Persist the base commit of subscription update for diff links #4870
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
base: main
Are you sure you want to change the base?
Conversation
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 ensures that the original base commit SHA for a subscription is persisted across multiple updates on the same pull request, enabling correct diff link generation.
- Adds a
BaseSourceSha
property to the subscription update model. - Populates
BaseSourceSha
when creating and updating code‐flow pull requests. - Passes
BaseSourceSha
into the PR title/description builder for accurate diff URLs.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/ProductConstructionService/.../SubscriptionPullRequestUpdate.cs | Added BaseSourceSha DataMember to track original commit |
src/ProductConstructionService/.../PullRequestUpdater.cs | Computes and assigns BaseSourceSha in create/update flows |
Comments suppressed due to low confidence (1)
src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs:1114
- Consider adding unit tests to verify that
BaseSourceSha
is correctly retrieved for existing subscriptions and falls back topreviousSourceSha
when absent.
string? baseSourceSha= pullRequest.ContainedSubscriptions.FirstOrDefault(u => u.SubscriptionId.Equals(update.SubscriptionId))?.BaseSourceSha
...nstructionService/ProductConstructionService.DependencyFlow/SubscriptionPullRequestUpdate.cs
Show resolved
Hide resolved
@@ -20,4 +20,7 @@ public class SubscriptionPullRequestUpdate | |||
|
|||
[DataMember] | |||
public string CommitSha { 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.
[nitpick] Add XML documentation for BaseSourceSha
explaining that it holds the original base commit SHA used for generating diffs.
/// <summary> | |
/// Gets or sets the original base commit SHA used for generating diffs. | |
/// </summary> |
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.
This would be nice to add
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.
It's a bit difficult to describe. I'm thinking "The earliest commit in the source repository from the changes contained in the current PR".
While writing this, I realize that this property is a bit out of place - technically, SubscriptionPullRequestUpdate shouldn't depend on any PR state - it only represents a single subscription update (i.e. changes from one subscription trigger), while BaseSourceSha
only makes sense in the context of the PR
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.
That's a good point. Why isn't it in the InProgressPullRequest
?
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 change it
#4777 (comment)
Persist the original base commit throughout subsequent subscription updates on the same open PR in order to generate the right commit diff url.
Example PR: maestro-auth-test/maestro-test-vmr#1850