-
Notifications
You must be signed in to change notification settings - Fork 133
Populate RepositoryBranch #1248
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
|
I haven't looked in detail, but to answer the question about where the package metadata mapping should go I'd say a followup PR to NuGet would be the appropriate place - they already have the mappings for the other automatic package properties. |
OK, I can get started on that. Separately but relatedly, do you know the flow for updates? I'm new in this area, but to "ship" this feature end-to-end it sounds like we'd need at least:
Is this a manual process, or are these picked up automatically? Any info greatly appreciated. Thanks! |
|
There's automated code flow of both SourceLink and NuGet targets into the SDK now - if you're working against main branch for all of the repos then you're targeting .NET 9 at this point, which feels fine to me. |
|
Created a draft in NuGet.Client: NuGet/NuGet.Client#5923. @tmat, would you mind taking a look, or letting me know who can review? Happy to make any changes requested. Thanks! |
|
I'll take a look. |
src/Microsoft.Build.Tasks.Git/GitDataReader/GitReferenceResolver.cs
Outdated
Show resolved
Hide resolved
src/SourceLink.Git.IntegrationTests/CloudHostedProvidersTests.cs
Outdated
Show resolved
Hide resolved
|
@MattKotsenas Looks great! Thanks for the PR. Just minor update to the comments and it's good to go. |
|
Thanks @tmat! I believe I fixed all your comments. Would you mind taking another look? |
Fixes dotnet#1251 Now that the SDK includes dotnet#1248, uncomment the integration tests that explicitly use the SDK-provided version. In a few cases the commented-out code was wrong, but those cases should be obvious (e.g. NoCommit tests).
Fixes #1243.
Adds
BranchNameas an output of theLocateRepositorytask and also adds it to the SourceRoot for the main repository. The locate task maps theBranchNameoutput task toSourceBranchNamein the .targets file to follow the naming convention. The branch is the full symbolic-ref name and not the short name to avoid ambiguity in source control systems like GitHub which use therefs/pullnamespace for Pull Requests.This change doesn't add branch name to submodules, as fundamentally submodules only track commits, and the main branch will fully specify any submodule-related metadata.
The corresponding change to map this value into
RepositoryBranchas part of NuGet pack is @ NuGet/NuGet.Client#5923.Open issues in the PR:
CloudHostedProvidersTests.csspecifically use the inbox version of the Git tasks, and as such they can't assert the new functionality. I'm unsure if that's by design and I should not worry about that scenario, or if the tests should be changed?