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

Create tracking file when user-profile nuget.config does not exist #4338

Merged

zivkan merged 7 commits into dev from dev-zivkan-default-package-source

Nov 24, 2021

Member

zivkan commented Nov 15, 2021 •

edited

Loading

Tip for reviewer: remove the first commit when viewing the diff.

Bug

Fixes: NuGet/Home#11387

Regression? No

Description

Revert Respect empty user-wide NuGet.Config file, no matter if there is a track file or not #3907
When default nuget.config does not exist, create both the fie with the default contents and tracking file.

PR Checklist

zivkan requested a review from a team as a code owner

November 15, 2021 23:01

Member Author

zivkan commented Nov 16, 2021

I just realized this PR will cause problems for customers who started from a clean machine since #3907 shipped, and do not want to use nuget.org. If this PR is merged as-is, they will have nuget.org added to their user-profile one time.

Since chocolatey claim that their fix has been released, perhaps we should just close this PR. I believe that Windows PowerShell (5.1)'s Install-Module will still create an empty nuget.config, but PowerShell 7 (and I believe 6, but that's already out of support) use an updated NuGet.Configuration which adds nuget.org as a source.

So, this will be an on-going issue for anyone who uses PowerShell 5.1's Install-Module before using nuget.exe, dotnet CLI or Visual Studio, but the number of customers doing this appear to be small. Therefore merging this PR might negatively affect more customers than the number of customers who will be helped.

@JonDouglas thoughts?

Contributor

JonDouglas commented Nov 17, 2021

This seems like a net positive PR. I don't really see the downsides here as this issue crops up time from time and looks like this may help future proof it.

dominoFire reviewed

View reviewed changes

Contributor

dominoFire left a comment

Some comments. Looks good.

src/NuGet.Core/NuGet.Configuration/PublicAPI.Shipped.txt Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Configuration/PackageSource/NuGetConstants.cs Outdated Show resolved Hide resolved

test/NuGet.Core.Tests/NuGet.Configuration.Test/PackageSourceProviderTests.cs Outdated Show resolved Hide resolved

dominoFire approved these changes

View reviewed changes

zivkan force-pushed the dev-zivkan-default-package-source branch 3 times, most recently from bd0da6d to 3b33e14 Compare

November 20, 2021 01:42

donnie-msft reviewed

View reviewed changes

Contributor

donnie-msft left a comment

A consideration is simply telling the user when these files are written or overwritten.
That may require piping the ILogger all the way down from places like RestoreArgs.cs. But as a user, might I like to know when files are written, at least at certain verbosities?

src/NuGet.Core/NuGet.Configuration/Settings/Settings.cs Outdated Show resolved Hide resolved

test/NuGet.Core.Tests/NuGet.Configuration.Test/SettingsTests.cs Outdated Show resolved Hide resolved

test/NuGet.Core.Tests/NuGet.Configuration.Test/SettingsTests.cs Outdated Show resolved Hide resolved

kartheekp-ms reviewed

View reviewed changes

src/NuGet.Core/NuGet.Configuration/Settings/Settings.cs Outdated Show resolved Hide resolved

zivkan added 6 commits

November 23, 2021 08:44


          Revert "Respect empty user-wide NuGet.Config file, no matter if there…

b487a39

… is a track file or not (#3907)"

This reverts commit 6b91678.


          Save default nuget.config and trk file when it doesn't exist

16176e5


          Handle when user settings directory does not exist

e2db0d2


          feedback

a3db7a4


          Fix error

b582ea7


          feedback

abcb5b0

zivkan force-pushed the dev-zivkan-default-package-source branch from 3b33e14 to abcb5b0 Compare

November 23, 2021 17:42

zivkan requested review from donnie-msft and kartheekp-ms

November 23, 2021 17:42

donnie-msft approved these changes

View reviewed changes

test/NuGet.Core.Tests/NuGet.Configuration.Test/SettingsTests.cs

               <configuration>
               </configuration>";
-                              var nugetConfigPath = "NuGet.Config";
-                              SettingsTestUtils.CreateConfigurationFile(nugetConfigPath, Path.Combine(mockBaseDirectory, "TestingGlobalPath"), config);
+                          string nugetConfigPath = "NuGet.Config";

Contributor

donnie-msft Nov 23, 2021

Literals should be implicit types

test/NuGet.Core.Tests/NuGet.Configuration.Test/SettingsTests.cs

+                          // Settings.LoadSettings' useTestingGlobalPath will use a subdirectory, instead of machine configuration, to allow for testing.
+                          string mockUserProfileDirectory = Path.Combine(mockSolutionDirectory, "TestingGlobalPath");
+                          string emptyConfig = @"<?xml version=""1.0"" encoding=""utf-8""?>

Contributor

donnie-msft Nov 23, 2021

Literals should be implicit types

test/NuGet.Core.Tests/NuGet.Configuration.Test/SettingsTests.cs

-                              File.Exists(nugetConfigPath).Should().BeTrue();
-                              var actual = SettingsTestUtils.RemoveWhitespace(File.ReadAllText(nugetConfigPath));
-                              var expected = SettingsTestUtils.RemoveWhitespace(@"<?xml version=""1.0"" encoding=""utf-8""?>
+                          string emptyConfig = @"<?xml version=""1.0"" encoding=""utf-8""?>

Contributor

donnie-msft Nov 23, 2021

Literals should be implicit types

test/NuGet.Core.Tests/NuGet.Configuration.Test/SettingsTests.cs

-                              actual.Should().Be(expected);
-                          }
+                          string nugetConfigPath = "NuGet.Config";

Contributor

donnie-msft Nov 23, 2021

Literals should be implicit types

test/NuGet.Core.Tests/NuGet.Configuration.Test/SettingsTests.cs Outdated Show resolved Hide resolved


          Update comments

ff4c17b

kartheekp-ms approved these changes

View reviewed changes

zivkan merged commit 35617e6 into dev

zivkan deleted the dev-zivkan-default-package-source branch

November 24, 2021 01:57

jeffkl mentioned this pull request

The SDK resolver "NuGetSdkResolver" failed to run. The process cannot access the file 'nugetorgadd.trk' because it is being used by another process. NuGet/Home#7503

Closed

kartheekp-ms mentioned this pull request

[Bug]: The process cannot access the file 'NuGet.Config' because it is being used by another process. NuGet/Home#11607

Closed

zivkan added a commit that referenced this pull request


          Revert "Create tracking file when user-profile nuget.config does not …

f6e1671

…exist (#4338)"

This reverts commit 35617e6.

zivkan mentioned this pull request

Revert "Create tracking file when user-profile nuget.config does not exist" #4473

Merged

8 tasks

zivkan added a commit that referenced this pull request


          Revert "Create tracking file when user-profile nuget.config does not …

7a5b0aa

…exist (#4338)" (#4473)

This reverts commit 35617e6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet