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.

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

Add option to enable floating versions in CPM #5440

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Sep 29, 2023

Bug

Fixes: NuGet/Home#10432

Regression? Last working version:

Description

By default, central package management (CPM) does not allow users to specify floating version (i.e. 1.*). CPM is considered an enterprise-level feature and since floating versions can introduce non-deterministic restores we want users to not be able to get situations where restores in different environments could not be identical.

However, the community has left a good amount of feedback that such a feature should be allowed with more details available in NuGet/Home#10432. This change allows a user to enable floating versions by setting an MSBuild property which remains off by default.

The only place the MSBuild property will be documented is at https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu1011 where users will be encouraged to use non floating versions and educated on the potential issues they could encounter.

PR Checklist

@jeffkl jeffkl force-pushed the dev-jeffkl-cpm-allow-floating-versions branch from 0c3055a to 3df7ed6 Compare September 29, 2023 23:13
@jeffkl jeffkl self-assigned this Oct 4, 2023
@jeffkl jeffkl marked this pull request as ready for review October 4, 2023 21:16
@jeffkl jeffkl requested a review from a team as a code owner October 4, 2023 21:16
Copy link
Contributor

@martinrrm martinrrm left a comment

Choose a reason for hiding this comment

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

Should we also enable floating versions in the PM UI?

@jeffkl
Copy link
Contributor Author

jeffkl commented Oct 5, 2023

Should we also enable floating versions in the PM UI?

How much work would it be for PM UI to read the CentralPackageFloatingVersionsEnabled property and then allow floating versions?

@jeffkl jeffkl force-pushed the dev-jeffkl-cpm-allow-floating-versions branch from 3df7ed6 to 1e9f3e1 Compare October 6, 2023 21:00
@martinrrm
Copy link
Contributor

Should we also enable floating versions in the PM UI?

How much work would it be for PM UI to read the CentralPackageFloatingVersionsEnabled property and then allow floating versions?

@jeffkl Probably a day for work and another day for testing, we are already reading the CPM enabled property and that decides if the combobox is going to be editable or not. Floating Versions in PM UI is only a project level feature, since this is more of a "Solution" level feature is it worth enabling it?

@jeffkl
Copy link
Contributor Author

jeffkl commented Oct 11, 2023

Should we also enable floating versions in the PM UI?

How much work would it be for PM UI to read the CentralPackageFloatingVersionsEnabled property and then allow floating versions?

@jeffkl Probably a day for work and another day for testing, we are already reading the CPM enabled property and that decides if the combobox is going to be editable or not. Floating Versions in PM UI is only a project level feature, since this is more of a "Solution" level feature is it worth enabling it?

@martinrrm I guess we might as well read the new property and allow the user to specify a floating version? If its only 1 day of work I think its worth it. I would also be interested in adding telemetry so we know how many users are enabling it.

zivkan
zivkan previously approved these changes Oct 13, 2023
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

You probably already know, but just in case, we'll need to make dotnet/project-system change to have the new property flow through nominations.

CPM is considered an enterprise-level feature

I don't agree, and I wish this would stop being repeated. Given the amount of customer feedback we've gotten, I think it's clear that CPM is broadly interesting, not just for "enterprise" developers. Ever since I first heard about the idea I thought it would be useful even for personal projects with more than 1 project in the repo, so I really don't understand why certain people keep repeating that it's "an enterprise-level feature".

@ghost ghost 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 Oct 20, 2023
@ghost
Copy link

ghost commented Oct 20, 2023

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@jeffkl
Copy link
Contributor Author

jeffkl commented Oct 24, 2023

You probably already know, but just in case, we'll need to make dotnet/project-system change to have the new property flow through nominations.

Pul request created for the property here: dotnet/project-system#9295

@ghost ghost 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 Oct 24, 2023
@jeffkl jeffkl force-pushed the dev-jeffkl-cpm-allow-floating-versions branch from 156032e to b087968 Compare October 24, 2023 16:42
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.

Looks good to me. I think this change is touching a lot of different parts of the code, so I think we should make sure we add the good test coverage.

@jeffkl jeffkl requested review from martinrrm and nkolev92 November 1, 2023 18:33
nkolev92
nkolev92 previously approved these changes Nov 2, 2023
@jeffkl jeffkl force-pushed the dev-jeffkl-cpm-allow-floating-versions branch from ba00ec4 to 4fdc758 Compare November 2, 2023 20:33
Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

Consider filing a techdebt issue for the spaces around the msbuild property values.

@jeffkl jeffkl merged commit 8bc8c9a into dev Nov 6, 2023
1 check passed
@jeffkl jeffkl deleted the dev-jeffkl-cpm-allow-floating-versions branch November 6, 2023 23:12
jeffkl added a commit that referenced this pull request Nov 17, 2023
jeffkl added a commit that referenced this pull request Nov 17, 2023
Nigusu-Allehu pushed a commit that referenced this pull request Nov 21, 2023
jeffkl added a commit that referenced this pull request Dec 4, 2023
jeffkl added a commit that referenced this pull request Dec 11, 2023
jeffkl added a commit that referenced this pull request Dec 12, 2023
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.

[DCR]: Allow floating versions with Central Package Management (CPM)
5 participants