Map SourceBranchName from sourcelink to RepositoryBranch for NuGet pack by MattKotsenas · Pull Request #5923 · NuGet/NuGet.Client

MattKotsenas · 2024-07-15T17:51:45Z

Bug

Description

This is a companion PR for dotnet/sourcelink#1248. Once that change lands and sourcelink is emitting the source branch name, this PR consumes that value and sets it to RepositoryBranch as part of NuGet pack.

PR Checklist

Meaningful title, helpful description and a linked NuGet/Home issue
Added tests
Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

baronfel

This is pretty much exactly what I was hoping to see 👍

MattKotsenas · 2024-07-15T21:21:49Z

I'm going to wait to publish this until the SourceLink PR lands in case things change, specifically of interest are:

The property name
If the symbolic-ref prefixes are kept

Edit: Published to get eyes / feedback on this; it's still a chance that I'll need to update this if the SourceLink PR change's its property name.

nkolev92

Just a question about the potential risk here.

nkolev92 · 2024-07-23T23:01:22Z

For the NuGet shepherd, let's wait for a review from @tmat before merging.

tmat · 2024-07-26T19:24:34Z

Is there a potential for private info leak if we publish the branch name by default?

For Repository URL we require the project to explicitly set PublishRepositoryUrl.

NuGet.Client/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets

Lines 292 to 293 in 60974f1

    
           <!-- The project must specify PublishRepositoryUrl=true in order to publish the URL, in order to prevent inadvertent leak of internal URL. --> 
        
           <RepositoryUrl Condition="'$(RepositoryUrl)' == '' and '$(PublishRepositoryUrl)' == 'true'">$(PrivateRepositoryUrl)</RepositoryUrl>

Branch name might potentially include code names, etc.

baronfel · 2024-07-26T19:27:06Z

Good point - the commit sha is unique enough to not worry, but the branch could be behind the existing '$(PublishRepositoryUrl)' check (or a similarly named and documented check for the branch).

MattKotsenas · 2024-07-26T20:05:09Z

@baronfel / @tmat thoughts on re-using PublishRepositoryUrl or creating a new flag? I defer to the experts here.

baronfel · 2024-07-26T20:21:44Z

Happy to re-use the existing flag, mostly was bringing up the possibility of a new flag for completeness.

nkolev92 · 2024-07-26T20:29:27Z

Would pack even set the repository details if the repository url was never provided?
Like if there's no repository url, but there's a branch.

MattKotsenas · 2024-07-26T20:54:30Z

~~Good reminder @nkolev92! I verified both here and with a quick local experiment that when $(PublishRepositoryUrl) is false the whole <repository /> node is omitted from the .nuspec.~~

~~@tmat / @baronfel, I believe this means the concern is already addressed? Let me know if there's anything I missed, or if there are any other issues. Thanks!~~

Edit: This comment is incorrect; see below.

baronfel · 2024-07-26T21:04:54Z

Perfect, that works for me. That's also the pattern we've used for the SDK Container source control information linkages.

tmat · 2024-07-26T21:05:37Z

Sounds good. Do we have a test?

tmat · 2024-07-26T21:07:05Z

Should also add a note to the documentation: https://github.com/dotnet/sourcelink/blob/669979bf371a587c908f732499df0ed883cd5d6c/docs/README.md?plain=1#L44

MattKotsenas · 2024-07-26T21:59:13Z

We don't have a test covering this part, and when I add one I'm not seeing the same behavior I see from the command line. I'm looking into it, but let's hold off on merging until we understand the difference.

Edit: OK I at least understand the difference. NuGet Package Explorer doesn't show the raw nuspec XML and seems to hide the <repository /> node if the url attribute is missing. Thus, my command line tests were invalid.

MattKotsenas · 2024-07-26T22:17:58Z

I'm going to go with gating the branch name on the repository URL as suggested above and we can take a look at how that feels.

Edit: Pushed. @tmat, @nkolev92, @baronfel; mind taking another look? Thanks!

…yUrl This is a follow up PR based on NuGet/NuGet.Client#5923. Update the README to specify that publishing `$(SourceBranchName)` into `$(RepositoryBranch)' is also controlled by the `$(PublishRepositoryUrl)` property.

MattKotsenas · 2024-07-26T23:03:01Z

Should also add a note to the documentation: https://github.com/dotnet/sourcelink/blob/669979bf371a587c908f732499df0ed883cd5d6c/docs/README.md?plain=1#L44

Available @ dotnet/sourcelink#1252

…yUrl (#1252) This is a follow up PR based on NuGet/NuGet.Client#5923. Update the README to specify that publishing `$(SourceBranchName)` into `$(RepositoryBranch)' is also controlled by the `$(PublishRepositoryUrl)` property.

baronfel · 2024-08-03T19:05:02Z

Ok, we've got sourcelink merged and flowing to the dotnet/sdk again, so I think this is good to review+merge!

Map SourceBranchName from sourcelink to RepositoryBranch for NuGet pack

60974f1

dotnet-policy-service Bot added the Community PRs created by someone not in the NuGet team label Jul 15, 2024

MattKotsenas mentioned this pull request Jul 15, 2024

Populate RepositoryBranch dotnet/sourcelink#1248

Merged

2 tasks

baronfel approved these changes Jul 15, 2024

View reviewed changes

MattKotsenas marked this pull request as ready for review July 22, 2024 17:45

MattKotsenas requested a review from a team as a code owner July 22, 2024 17:45

nkolev92 reviewed Jul 22, 2024

View reviewed changes

Comment thread test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs

nkolev92 previously approved these changes Jul 22, 2024

View reviewed changes

MattKotsenas requested a review from tmat July 26, 2024 20:55

Only map SourceBranchName if PublishRepositoryUrl is also set

d8ed323

MattKotsenas dismissed nkolev92’s stale review via d8ed323 July 26, 2024 22:45

MattKotsenas requested a review from nkolev92 July 26, 2024 22:48

baronfel approved these changes Jul 26, 2024

View reviewed changes

MattKotsenas mentioned this pull request Jul 26, 2024

Document that RepositoryBranch is controlled by PublishRepositoryUrl dotnet/sourcelink#1252

Merged

dotnet-policy-service Bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 3, 2024

dotnet-policy-service Bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 3, 2024

zivkan approved these changes Aug 6, 2024

View reviewed changes

zivkan merged commit e9b7d1c into NuGet:dev Aug 6, 2024

MattKotsenas mentioned this pull request Aug 21, 2024

RepositoryBranch logic conflicts with .NET 9 SDK dotnet/reproducible-builds#46

Closed

Conversation

MattKotsenas commented Jul 15, 2024

Bug

Description

PR Checklist

Uh oh!

baronfel left a comment

Choose a reason for hiding this comment

Uh oh!

MattKotsenas commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nkolev92 commented Jul 23, 2024

Uh oh!

tmat commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

baronfel commented Jul 26, 2024

Uh oh!

MattKotsenas commented Jul 26, 2024

Uh oh!

baronfel commented Jul 26, 2024

Uh oh!

nkolev92 commented Jul 26, 2024

Uh oh!

MattKotsenas commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

baronfel commented Jul 26, 2024

Uh oh!

tmat commented Jul 26, 2024

Uh oh!

tmat commented Jul 26, 2024

Uh oh!

MattKotsenas commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattKotsenas commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattKotsenas commented Jul 26, 2024

Uh oh!

baronfel commented Aug 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MattKotsenas commented Jul 15, 2024 •

edited

Loading

tmat commented Jul 26, 2024 •

edited

Loading

MattKotsenas commented Jul 26, 2024 •

edited

Loading

MattKotsenas commented Jul 26, 2024 •

edited

Loading

MattKotsenas commented Jul 26, 2024 •

edited

Loading