Added save for PackageSourceMappingsProvider #4709

mcnallyella · 2022-07-01T19:07:23Z

Bug

Fixes: NuGet/Home#11935

Regression? Last working version:

Description

I added a method in PackageSourceMappingProvider.cs that saves changes in package source mapping to the config. The save method calls the existing AddOrUpdatePackageSourceMappingSourceItem and Remove methods. This will be useful for implementing the options page UI. I also added 4 tests for the save method. There is also a draft PR with the changes to the options UI for package source mapping: #4711

PR Checklist

src/NuGet.Clients/NuGet.VisualStudio.Common/OptionsPage.cs

kartheekp-ms · 2022-07-02T02:01:17Z

Are there any UI related changes in this PR? If yes, please consider adding screenshots or a link to spec for reference.

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs

zivkan · 2022-07-08T14:15:13Z

src/NuGet.Core/NuGet.Configuration/PublicAPI.Unshipped.txt

+NuGet.Configuration.PackageSourceMappingProvider.SavePackageSourceMappings(System.Collections.Generic.IReadOnlyList<NuGet.Configuration.PackageSourceMappingSourceItem> mappings) -> void
+NuGet.Configuration.PackageSourceMappingProvider.SavePackageSourcesMappings(System.Collections.Generic.IReadOnlyList<NuGet.Configuration.PackageSourceMappingSourceItem> packageSourceMappingsSourceItems, NuGet.Configuration.PackageSourceUpdateOptions sourceUpdateSettings) -> void


How will customers using the package NuGet.Configuration in their own apps use the two different overloads of SavePackageSourceMappings?

Once APIs become public and we ship to customers, it becomes very difficult in the future to change/remove because we have no way to investigate how many (if any) developers we will break who use these APIs. See also our "changes to APIs" section of the NuGet SDK docs: https://github.com/NuGet/NuGet.Client/blob/dev/docs/nuget-sdk.md#changes-to-apis

I agree that there needs to be public APIs to modify/save PackageSourceMappings. I think it was a significant oversight from an SDK point of view that this was not done when the feature was added in the first place. That has the very unfortunate side-effect that the person who ends up making these public then "bears the responsibility" of considering public API design, rather than the people who implemented the feature/methods in the first place. But once these become public, we have to "support" them for a long time, so I don't want to be reckless and mechanically make existing internal APIs public without considering public API design.

Thanks for pointing this out. Turns out I don't need two save methods. I was copying the method from PackageSourceProvider.cs (which has two save methods) and didn't look closely enough to see if I should just do one instead.

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs

erdembayar · 2022-07-11T20:26:23Z

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs

+            }
+
+            //Remove all old mappings not in new mappings
+            if (existingSettingsLookup != null)


I believe you may need to reload this value.
Because existingSettingsLookup is stale now, so you may not see new values here.

Yeah I won't see any new values, but I don't think I need to see the mappings that were just added/updated. I really only need to see the ones that are in the config currently but aren't the current list of mappings displayed in the UI. These will still be in existingSettingsLookup even if I add new ones

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs

nkolev92

You have some pretty good tests and code is pretty easy to follow. 👏

I think we have some duplication in what the newly public methods provider.

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMapping.cs

src/NuGet.Core/NuGet.Configuration/PublicAPI.Unshipped.txt

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

src/NuGet.Core/NuGet.Configuration/PublicAPI.Unshipped.txt

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs

dominoFire

Good job. Just comments. Thank you!

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

dominoFire · 2022-07-19T01:08:42Z

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

+            tempMappings.Add(testMappingItemAdd);
+            tempMappings.Remove(testMappingItem2);
+
+            sourceMappingProvider.SavePackageSourceMappings(tempMappings);


Would this call be the 'act' part of the test?

Suggested change

sourceMappingProvider.SavePackageSourceMappings(tempMappings);

// Act

sourceMappingProvider.SavePackageSourceMappings(tempMappings);

Yeah there is a comment on line 602 labeling it "Act & Assert". That is how the other tests in this file seem to be set up, so I just copied how everything else was labeled.

src/NuGet.Core/NuGet.Configuration/PublicAPI.Unshipped.txt

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

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs

erdembayar · 2022-07-22T00:19:25Z

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs

+            IReadOnlyList<PackageSourceMappingSourceItem> existingSettingsLookup = GetPackageSourceMappingItems();
+            if (existingSettingsLookup == null)
+            {
+                return;


Does this mean packageSourceMapping should exist before this action?
Technically prevent from onboarding. Instead, should it create a new one?

I removed this if statement because it was a bit redundant. GetPackageSourceMappingItems() will return an empty list of mappings if Package source mappings has not yet been enabled, so existingSettingsLookup will never be null. The Remove() and AddorUpdate() methods have logic that will create a new config if there are no previous mappings

erdembayar · 2022-07-22T00:30:33Z

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs

+                }
+            }
+
+            if (removeMappings != null && removeMappings.Count > 0)


Suggested change

if (removeMappings != null && removeMappings.Count > 0)

if (removeMappings.Count > 0)

You already initialized the list at line 75 so it's not going to be null.

This one is not resolved, so I reverted back to unresolved state.

Oops sorry. I clicked the commit suggestion button, so I thought it was resolved. I guess the commit suggestion button does not actually work? I changed it manually

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

erdembayar

Great work, almost ready if you address my comment regarding reduce number of new public apis.

src/NuGet.Core/NuGet.Configuration/PublicAPI.Unshipped.txt

erdembayar · 2022-07-22T18:53:39Z

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

+    <packageSourceMapping>
+        <clear />
+        <packageSource key=""nuget.org"">
+            <package pattern=""stuff"" />


Do you have test just adding new pattern into existing nuget.org package sourceKey section?
Before:

<packageSource key=""nuget.org""> <package pattern=""stuff1"" /> </packageSource>

After: Add stufff2

<packageSource key=""nuget.org""> <package pattern=""stuff1"" /> <package pattern=""stuff2"" /> </packageSource>

Just made a test. SavePackageSourceMappings_NewPatternExistingSource_Add()

Let's wait fingers crossed for CI test results. If CI tests are passing, then I'm ready to approve the PR.

It looks CI tests for your old commit "Remove WhiteSpace" still running, probably cancelling it manually better, because it'll trigger another CI build with latest changes.

@mcnallyella
Few End2end tests are failing due to some reason, please rebase your branch with latest dev branch. Those failing tests are skipped here this morning.

Yes I rebased it. I had a green build a moment ago but had to change a test, so hopefully the next build will be green as well

erdembayar · 2022-07-22T18:54:58Z

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

+    <packageSourceMapping>
+        <clear />
+        <packageSource key=""nuget.org"">
+            <package pattern=""stuff"" />


Do you have test just removing 1 pattern from existing nuget.org package sourceKey section?
Before:

<packageSource key=""nuget.org""> <package pattern=""stuff1"" /> <package pattern=""stuff2"" /> </packageSource>

After: Remove stuff1

<packageSource key=""nuget.org""> <package pattern=""stuff2"" /> </packageSource>

Just made a test for that. SavePackageSourceMappings_RemovePatternExistingSource_Remove()

nkolev92

Implementation looks great!

I really like how detailed the tests are.

One idea for another test case, and some suggestions for removing duplicate tests.

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

nkolev92 · 2022-07-22T20:48:16Z

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

+            result.Replace("\r\n", "\n")
+                .Should().BeEquivalentTo(
+                File.ReadAllText(configPath1).Replace("\r\n", "\n"));
+        }


I think we need to do one or two more tests.
Multi config tests, where say you have 2 configs, A & B, A is further and has the source mapping clear, but B has a source mapping config for one source, but A has for another one.

I don't expect you to do the same matrix of tests, that'd be an overkill, but at least have a smoke test that ensures the correct one is being updated.

Okay I added a new test called SavePackageSourceMappings_WithTwoConfigs_UseCorrectMapping() to test with two configs.

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

nkolev92

👏
Great job

zivkan

Thank you for optimizing the public API surface area.

erdembayar

Great work. 👏 👏 👏

mcnallyella requested a review from a team as a code owner July 1, 2022 19:07

nkolev92 reviewed Jul 1, 2022

View reviewed changes

src/NuGet.Clients/NuGet.VisualStudio.Common/OptionsPage.cs Show resolved Hide resolved

kartheekp-ms reviewed Jul 2, 2022

View reviewed changes

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs Outdated Show resolved Hide resolved

zivkan reviewed Jul 8, 2022

View reviewed changes

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs Outdated Show resolved Hide resolved

erdembayar reviewed Jul 11, 2022

View reviewed changes

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs Outdated Show resolved Hide resolved

erdembayar reviewed Jul 11, 2022

View reviewed changes

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs Outdated Show resolved Hide resolved

erdembayar reviewed Jul 11, 2022

View reviewed changes

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs Outdated Show resolved Hide resolved

zivkan mentioned this pull request Jul 13, 2022

Package Source Mapping Options UI #4711

Closed

6 tasks

nkolev92 reviewed Jul 13, 2022

View reviewed changes

dominoFire reviewed Jul 15, 2022

View reviewed changes

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs Outdated Show resolved Hide resolved

mcnallyella requested review from dominoFire, erdembayar, kartheekp-ms, nkolev92 and zivkan July 18, 2022 23:25

dominoFire previously approved these changes Jul 19, 2022

View reviewed changes

nkolev92 requested changes Jul 19, 2022

View reviewed changes

mcnallyella dismissed dominoFire’s stale review via d7ce61c July 19, 2022 16:30

erdembayar reviewed Jul 22, 2022

View reviewed changes

mcnallyella force-pushed the dev-mcnallyella-PackageSourceMappingOptionsAPI branch from cbc26b2 to 6b3ae6d Compare July 22, 2022 17:02

erdembayar reviewed Jul 22, 2022

View reviewed changes

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

erdembayar reviewed Jul 22, 2022

View reviewed changes

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

erdembayar reviewed Jul 22, 2022

View reviewed changes

nkolev92 reviewed Jul 22, 2022

View reviewed changes

Added save for PackageSourceMappingsProvider

e9f2ca9

mcnallyella added 21 commits July 25, 2022 12:11

removed obsolete save method

624a0af

Changing ObservableCollections to Lists

01e55e9

remove var

acf211a

Change test names and remove stuff from PublicAPI

8d0790e

add to api

04a69af

changed api

7487c36

make PackageSourceMapping.cs public

1ee6d4d

add to api

7cfe101

move if statement

4deea43

edit save in PackageSourceMappingsProvider.cs

b226c3b

changed class from internal to public

fa6fce2

checkout publicAPI.Unshipped.txt from dev

9d74e15

Made PackageSourceMappingProvider.cs public

7fce173

Add new test

21fcd3d

Change logic on remove test for SavePackageSourceMappings

79b2f70

Remove whitespace

c7d2f7a

Make Remove() AddorUpdate() and GetPackageSourceMappings() internal

9e21dcd

change if statement in PackageSourceMappingProvider

ffafe1e

Add tests

5fd90a0

Change SavePackageSourceMappings_WithUpdatedMappings_Update() test

c0ffbfd

Add SavePackageSourceMappings_WithTwoConfigs_UseCorrectMapping() test

5739717

mcnallyella force-pushed the dev-mcnallyella-PackageSourceMappingOptionsAPI branch from baa6076 to 5739717 Compare July 25, 2022 19:11

nkolev92 reviewed Jul 25, 2022

View reviewed changes

mcnallyella requested a review from nkolev92 July 25, 2022 22:33

Change SavePackageSourceMappings_WithTwoConfigs_UseCorrectMapping test

307cf0c

nkolev92 approved these changes Jul 25, 2022

View reviewed changes

zivkan approved these changes Jul 26, 2022

View reviewed changes

erdembayar approved these changes Jul 26, 2022

View reviewed changes

mcnallyella merged commit 1ad7503 into dev Jul 26, 2022

mcnallyella deleted the dev-mcnallyella-PackageSourceMappingOptionsAPI branch July 26, 2022 17:25

		NuGet.Configuration.PackageSourceMappingProvider.SavePackageSourceMappings(System.Collections.Generic.IReadOnlyList<NuGet.Configuration.PackageSourceMappingSourceItem> mappings) -> void
		NuGet.Configuration.PackageSourceMappingProvider.SavePackageSourcesMappings(System.Collections.Generic.IReadOnlyList<NuGet.Configuration.PackageSourceMappingSourceItem> packageSourceMappingsSourceItems, NuGet.Configuration.PackageSourceUpdateOptions sourceUpdateSettings) -> void

	sourceMappingProvider.SavePackageSourceMappings(tempMappings);
	// Act
	sourceMappingProvider.SavePackageSourceMappings(tempMappings);

	if (removeMappings != null && removeMappings.Count > 0)
	if (removeMappings.Count > 0)

Added save for PackageSourceMappingsProvider #4709

Added save for PackageSourceMappingsProvider #4709

Uh oh!

Conversation

mcnallyella commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

PR Checklist

Uh oh!

Uh oh!

kartheekp-ms commented Jul 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dominoFire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erdembayar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erdembayar Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erdembayar Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mcnallyella commented Jul 1, 2022 •

edited

Loading

kartheekp-ms commented Jul 2, 2022 •

edited

Loading

erdembayar Jul 22, 2022 •

edited

Loading

erdembayar Jul 22, 2022 •

edited

Loading

nkolev92 left a comment •

edited

Loading