Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Map SourceBranchName from sourcelink to RepositoryBranch for NuGet pack #5923

Merged
merged 2 commits into from
Aug 6, 2024
Merged

Map SourceBranchName from sourcelink to RepositoryBranch for NuGet pack #5923

merged 2 commits into from
Aug 6, 2024

Conversation

MattKotsenas
Copy link
Contributor

Bug

Fixes: NuGet/Home#13625

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.

@dotnet-policy-service dotnet-policy-service bot added the Community PRs created by someone not in the NuGet team label Jul 15, 2024
@MattKotsenas MattKotsenas mentioned this pull request Jul 15, 2024
Merged
2 tasks
baronfel
baronfel approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

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

@MattKotsenas
Copy link
Contributor Author

MattKotsenas commented Jul 15, 2024 •
edited
Loading

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.

@MattKotsenas MattKotsenas marked this pull request as ready for review July 22, 2024 17:45
@MattKotsenas MattKotsenas requested a review from a team as a code owner July 22, 2024 17:45
nkolev92
nkolev92 reviewed Jul 22, 2024
View reviewed changes
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Just a question about the potential risk here.

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs Show resolved Hide resolved
nkolev92
nkolev92 previously approved these changes Jul 22, 2024
View reviewed changes
@nkolev92
Copy link
Member

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

@tmat
Copy link
Contributor

tmat commented Jul 26, 2024 •
edited
Loading

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@baronfel
Copy link
Contributor

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

@nkolev92
Copy link
Member

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
Copy link
Contributor Author

MattKotsenas commented Jul 26, 2024 •
edited
Loading

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.

@MattKotsenas MattKotsenas requested a review from tmat July 26, 2024 20:55
@baronfel
Copy link
Contributor

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

@tmat
Copy link
Contributor

tmat commented Jul 26, 2024

Sounds good. Do we have a test?

@tmat
Copy link
Contributor

tmat commented Jul 26, 2024

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

@MattKotsenas
Copy link
Contributor Author

MattKotsenas commented Jul 26, 2024 •
edited
Loading

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
Copy link
Contributor Author

MattKotsenas commented Jul 26, 2024 •
edited
Loading

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!

@MattKotsenas MattKotsenas requested a review from nkolev92 July 26, 2024 22:48
MattKotsenas added a commit to MattKotsenas/sourcelink that referenced this pull request Jul 26, 2024
@MattKotsenas
Document that RepositoryBranch is also controlled by PublishRepositor…
f247e48 
…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 MattKotsenas mentioned this pull request Jul 26, 2024
Merged
@MattKotsenas
Copy link
Contributor Author

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

tmat pushed a commit to dotnet/sourcelink that referenced this pull request Jul 31, 2024
@MattKotsenas
Document that RepositoryBranch is also controlled by PublishRepositor…
744df35 
…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.
@dotnet-policy-service 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
@baronfel
Copy link
Contributor

baronfel commented Aug 3, 2024

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

@dotnet-policy-service 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 zivkan merged commit e9b7d1c into NuGet:dev Aug 6, 2024
27 of 29 checks passed
@MattKotsenas MattKotsenas mentioned this pull request Aug 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zivkan zivkan zivkan approved these changes

@baronfel baronfel baronfel approved these changes

@tmat tmat Awaiting requested review from tmat

@nkolev92 nkolev92 Awaiting requested review from nkolev92

Assignees
No one assigned
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Map branch name from sourcelink to RepositoryBranch for NuGet pack
5 participants
@MattKotsenas @nkolev92 @tmat @baronfel @zivkan