Skip to content

Log warning when using central packagement management but not package source mapping#4505

Merged
jeffkl merged 4 commits intodevfrom
dev-jeffkl-package-source-mapping-requirement-cpm
Mar 25, 2022
Merged

Log warning when using central packagement management but not package source mapping#4505
jeffkl merged 4 commits intodevfrom
dev-jeffkl-package-source-mapping-requirement-cpm

Conversation

@jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Mar 16, 2022

Bug

Fixes: NuGet/Home#11505

Regression? Last working version:

Description

Log a warning during restore if central package management is being used, there is more than one feed configured, and package source mapping is not enabled.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@jeffkl jeffkl requested a review from a team as a code owner March 16, 2022 15:40
@jeffkl jeffkl changed the title Log warning when using central packagement management but not package… Log warning when using central packagement management but not package source mapping Mar 16, 2022
<packageSource key=""https://feed2"">
<package pattern=""baz"" />
</packageSource>
</packageSourceMapping>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is having more than 1 source is bad practice with central package management?
Still trying to understand why it's the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to make NuGet secure by default and in this case since the customer is using CPM, they should at least be told they can use package source mapping to be more secure.

@jeffkl jeffkl force-pushed the dev-jeffkl-package-source-mapping-requirement-cpm branch from 79ea854 to 1281247 Compare March 16, 2022 17:29
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.

Change looks great, the test probably need cleaned up.

@jeffkl jeffkl force-pushed the dev-jeffkl-package-source-mapping-requirement-cpm branch from 1281247 to 7f7c115 Compare March 21, 2022 14:54
@JonDouglas JonDouglas self-requested a review March 22, 2022 16:38
@jeffkl
Copy link
Contributor Author

jeffkl commented Mar 22, 2022

Team Review: Decide if we want to allow the feeds that Visual Studio configures so that people don't get the warning from feeds configured not by them

@jeffkl
Copy link
Contributor Author

jeffkl commented Mar 24, 2022

@NuGet/nuget-client please review again with the final wording of the warning, @aortiz-msft does this look good to you?

nkolev92
nkolev92 previously approved these changes Mar 24, 2022
@jeffkl jeffkl force-pushed the dev-jeffkl-package-source-mapping-requirement-cpm branch from a02f29b to 860b793 Compare March 24, 2022 17:23
@jeffkl
Copy link
Contributor Author

jeffkl commented Mar 24, 2022

@NuGet/nuget-client Had to rebase to get a green build which reset approvals, @nkolev92 please approve again

Copy link
Contributor

@JonDouglas JonDouglas left a comment

Choose a reason for hiding this comment

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

Ship it!

@jeffkl jeffkl merged commit cc31dd6 into dev Mar 25, 2022
@jeffkl jeffkl deleted the dev-jeffkl-package-source-mapping-requirement-cpm branch March 25, 2022 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Require package source mapping when using CPM

5 participants