Check NuGet.org for SiteExtensions #2568

EricSten-MSFT · 2017-09-18T20:10:35Z

No description provided.

dnfclas · 2017-09-18T20:10:39Z

@EricSten-MSFT,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

dnfclas · 2017-09-18T20:33:00Z

@EricSten-MSFT, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

davidebbo · 2017-09-18T21:34:49Z

Kudu.Core/SiteExtensions/SiteExtensionManager.cs

+                        packages = packages.Concat(foundUIPackages);
+                    }
+                }
+                packages.OrderByDescending(p => p.DownloadCount);


I assume you meant to assign it back? If you don't it's a no op!

Originally I had:
packages.Concat(await remoteRepo.Search(string.IsNullOrWhiteSpace(filter) ? string.Empty : filter, filterOptions: filterOptions));
but it turns out that .Concat doesn't actually concat to packages; it simply returns the concatination of packages + 1st arg. This was the cause of the last bug fix.
From the description of foo.Concat(bar) , I foolishly thought that it would concatenate bar to foo in-place. I was wrong; foo is not modified by .Concat().

None of the Linq methods ever have side effects. It's all about building new enumerations over existing ones.

davidebbo · 2017-09-18T21:37:42Z

Kudu.Core/SiteExtensions/SiteExtensionManager.cs

+                    var foundUIPackages = await remoteRepo.Search(string.IsNullOrWhiteSpace(filter) ? string.Empty : filter, filterOptions: filterOptions);
+                    if (null != foundUIPackages)
+                    {
+                        packages = packages.Concat(foundUIPackages);


How does that deal with packages that are on both feeds? I think we need find duplicates and only keep the latest.

I thought the mission was to find the ones on the NuGet.org feed first, and add the ones from siteextensions.org second. I didn't know there was a requirement to eliminate duplicates.

I suppose we could tell people that once they transition to nuget.org, they should hide all the packages from siteextensions.net.

davidebbo · 2017-09-19T01:36:15Z

Kudu.Core/SiteExtensions/SiteExtensionManager.cs

-                    package = await remoteRepo.GetLatestPackageByIdFromSrcRepo(id);
+                    foreach (SourceRepository remoteRepo in remoteRepos)
+                    {
+                        package = await remoteRepo.GetLatestPackageByIdFromSrcRepo(id);


Not sure this is quite correct, as this will return any package from nuget.org, even if it doesn't have our AzureSiteExtension tag.

Ah, you're right. Only .Search() has the special logic to automagically add the AzureSiteExtension magic tag. I'll have to figure out the best place to put this magic to avoid duplicating code everywhere.

Side Question: is "AzureSiteExtension" case sensitive? And do we need to worry about collisions?
e.g. "DefinitelyNotAnAzureSiteExtension" vs. "AzureSiteExtension"

Not sure about the casing, but I highly doubt that it is sensitive (you'll have to try it on NuGet.org with the one package we have there).

No, I'm not worried about those collisions. The tag is unique enough that it won't happen accidentally.

davidebbo · 2017-09-19T01:37:08Z

Kudu.Core/SiteExtensions/SiteExtensionManager.cs

-                    package = await remoteRepo.GetPackageByIdentity(id, version);
+                    foreach (SourceRepository remoteRepo in remoteRepos)
+                    {
+                        package = await remoteRepo.GetPackageByIdentity(id, version);


Same as previous comment.

davidebbo · 2017-09-19T01:41:04Z

Kudu.Core/SiteExtensions/SiteExtensionManager.cs

-                            repoPackage = await remoteRepo.GetLatestPackageByIdFromSrcRepo(id);
+                            foreach (SourceRepository rr in remoteRepos)
+                            {
+                                repoPackage = await rr.GetLatestPackageByIdFromSrcRepo(id);


Same AzureSiteExtension issue

davidebbo · 2017-09-19T01:41:53Z

Kudu.Core/SiteExtensions/SiteExtensionManager.cs

-                            repoPackage = await remoteRepo.GetPackageByIdentity(id, version);
+                            foreach (SourceRepository rr in remoteRepos)
+                            {
+                                repoPackage = await rr.GetPackageByIdentity(id, version);


Same AzureSiteExtension issue

davidebbo · 2017-09-19T01:55:30Z

A good way to see the problem is to try to install the NewRelic.Azure.WebSites package. It exists in both gallery, but the one on NuGet is really unrelated, and doesn't have the tag. But we still end up trying to install it.

EricSten-MSFT · 2017-09-25T15:17:16Z

Kudu.Core/SiteExtensions/FeedExtensions.cs

                    // is the best we can do based on how nuget.org works
-                    searchTerm = $"tags:AzureSiteExtension title:\"{searchTerm}\"";
+                    searchTerm = $"tags:AzureSiteExtension id:\"{searchTerm}\"";


I'm switching this to id:, since the functional tests pass a package Id instead of a title in places. If this is too much of a breaking change, I can change the functional tests to pass package Titles instead.

@EricSten-MSFT you can assume that no one is using the nuget.org branch of the current code, so don't worry about breaking changes

@EricSten-MSFT as I'm trying to finally make this transition happen, this change from title to id seems problematic, because it cause search to only work for exac match. e.g. compare:

Search by 'counter' successfully finds filecounter package when using title:counter: https://www.nuget.org/packages?q=tags%3AAzureSiteExtension+title%3Acounter

The same doesn't find it when using id:counter: https://www.nuget.org/packages?q=tags%3AAzureSiteExtension+id%3Acounter

If the only issue here was a functional test, I think we should revert back to title. Do you know what test was failing?

Ok, I moved back to title and updated the tests. 59d0283

EricSten-MSFT · 2017-09-25T15:24:16Z

@davidebbo Re: NewRelic.Azure.WebSites, I added a functional test per your comment, above. See: SiteExtensionNugetOrgTest() in SiteExtensionApiFacts.cs

davidebbo · 2017-09-27T18:16:30Z

Kudu.Core/SiteExtensions/SiteExtensionManager.cs

+
+            if (!string.IsNullOrEmpty(info.FeedUrl))
+            {
+                isNuGetPackage = System.Text.RegularExpressions.Regex.IsMatch(


Why not use the helper that does that check to avoid duplication?

Because I was too lazy to refactor IsNuGetRepo to take a string instead of a SourceRepository. I'll fix it.

davidebbo

Let's get it in after fixing the nuget.org check duplication.

Can you also squash/rebase over latest?

davidebbo · 2017-09-27T18:20:33Z

Actually, I think I can get GitHub to do the Squash & Merge.

davidebbo · 2017-10-05T19:58:53Z

Kudu.Core/SiteExtensions/SiteExtensionManager.cs

+                    && version.Equals(localPackageVersion, StringComparison.OrdinalIgnoreCase)
+                    && rr.PackageSource.Source.Equals(localPackageFeedUrl, StringComparison.OrdinalIgnoreCase))
+                {
+                    isInstalled = true;


This code is actually incorrect when the version is blank, as it prevents upgrading from the first feed to the second feed if you already have the latest from the first (but second has newer one).

@davidebbo : Fixed. Submitted a new pull request with the fix.

commit f3f98a6 Merge: 9b5da52 b3f8936 Author: Eric Stenson <ericsten@php.net> Date: Thu Oct 5 16:14:37 2017 -0700 merge in upstream stuff commit 9b5da52 Author: Eric Stenson <ericsten@php.net> Date: Thu Oct 5 15:56:15 2017 -0700 Fix site extension version check across multiple repos commit b3f8936 Author: Ahmed ElSayed <ahmels@microsoft.com> Date: Fri Sep 29 17:37:17 2017 -0700 Reduce size of test_data for ARM requests to 8MB/2 commit 8665f80 Author: Ahmed ElSayed <ahmels@microsoft.com> Date: Fri Sep 29 17:16:41 2017 -0700 Return 409 on functions secret api if the storageType is set to blob. commit 82791e9 Author: Nick Walker <nickwalk@microsoft.com> Date: Tue Oct 3 11:35:42 2017 -0700 Zip push-to-deploy feature (projectkudu#2570) commit 1d5f917 Author: Suwath Ch <suwatch@microsoft.com> Date: Sun Oct 1 12:57:21 2017 -0700 Convert Dropbox v1 to v2 tests commit f0de225 Author: Eric Stenson <ericsten@php.net> Date: Wed Sep 27 13:44:51 2017 -0700 simplify some csproj files commit 75a3839 Author: Eric Stenson (work) <EricSten-MSFT@users.noreply.github.com> Date: Wed Sep 27 13:40:08 2017 -0700 Check NuGet.org for SiteExtensions (projectkudu#2568) Check NuGet.org for SiteExtensions commit 60f1c5d Author: Eric Stenson <ericsten@php.net> Date: Wed Sep 27 11:48:41 2017 -0700 Eliminate regex duplication for IsNuGet commit 319dec8 Author: Eric Stenson <ericsten@php.net> Date: Tue Sep 19 15:57:22 2017 -0700 even more fixes to get functional tests passing commit 0891d0e Author: Eric Stenson <ericsten@php.net> Date: Mon Sep 18 13:07:46 2017 -0700 more fixes to get functional tests passing commit 5e3102e Author: Eric Stenson <ericsten@php.net> Date: Fri Sep 15 19:03:54 2017 -0700 bug fixes for functional tests commit 150dab5 Author: Nick Walker <nickwalk@microsoft.com> Date: Fri Sep 15 12:33:43 2017 -0700 Assorted Linux fixes/updates (projectkudu#2563) - Open the first 10 Docker logfiles before reading their FileInfos to update the CIFS metadata. CIFS metadata on Linux "lags", and fresh data (esp. size) may be needed for some callers. - Fix "Location" header URL value in FetchHandler - Put link to Docker logs (both API and zip endpoints) on Kudu homepage on Linux - Restore VFS link on Kudu homepage on Linux commit ed2c183 Author: David Ebbo <david.ebbo@gmail.com> Date: Thu Sep 14 17:51:14 2017 -0700 Version 66 commit 9676026 Author: bala16 <balag@outlook.com> Date: Thu Sep 14 10:03:39 2017 -0700 Fix ArgumentNullException when enumerating WebHooks commit dc41d86 Merge: dbbd3b4 6152ca9 Author: Eric Stenson <ericsten@php.net> Date: Wed Sep 13 17:17:27 2017 -0700 Merge remote-tracking branch 'upstream/master' commit dbbd3b4 Author: Eric Stenson <ericsten@php.net> Date: Wed Sep 13 17:14:16 2017 -0700 Because VS cares... commit 00dc549 Author: Eric Stenson <ericsten@php.net> Date: Wed Sep 13 17:12:04 2017 -0700 Check NuGet.org for SiteExtensions commit 6152ca9 Author: Nick Walker <nickwalk@microsoft.com> Date: Fri Sep 8 16:24:50 2017 -0700 Fix for projectkudu#2547 occasional null HttpContext on Mono (projectkudu#2556) commit bb683db Author: bala16 <balag@outlook.com> Date: Thu Sep 7 17:13:17 2017 -0700 Add git/mingw32/bin to path commit af2029b Author: shucai <shucai@microsoft.com> Date: Thu Sep 7 09:31:57 2017 -0700 remove checks for host.json when deploying a functionApp

EricSten-MSFT added 5 commits September 13, 2017 17:12

Check NuGet.org for SiteExtensions

00dc549

Because VS cares...

dbbd3b4

Merge remote-tracking branch 'upstream/master'

dc41d86

bug fixes for functional tests

5e3102e

more fixes to get functional tests passing

0891d0e

dnfclas added the cla-required label Sep 18, 2017

dnfclas added cla-signed and removed cla-required labels Sep 18, 2017

davidebbo reviewed Sep 19, 2017

View reviewed changes

even more fixes to get functional tests passing

319dec8

EricSten-MSFT commented Sep 25, 2017

View reviewed changes

davidebbo reviewed Sep 27, 2017

View reviewed changes

davidebbo approved these changes Sep 27, 2017

View reviewed changes

Eliminate regex duplication for IsNuGet

60f1c5d

davidebbo merged commit 75a3839 into projectkudu:master Sep 27, 2017

davidebbo reviewed Oct 5, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check NuGet.org for SiteExtensions #2568

Check NuGet.org for SiteExtensions #2568

EricSten-MSFT commented Sep 18, 2017

dnfclas commented Sep 18, 2017

dnfclas commented Sep 18, 2017

davidebbo Sep 18, 2017

EricSten-MSFT Sep 19, 2017

davidebbo Sep 19, 2017 •

edited

davidebbo Sep 18, 2017

EricSten-MSFT Sep 19, 2017

davidebbo Sep 19, 2017

davidebbo Sep 19, 2017

EricSten-MSFT Sep 19, 2017

EricSten-MSFT Sep 19, 2017

davidebbo Sep 19, 2017

davidebbo Sep 19, 2017

davidebbo Sep 19, 2017

davidebbo Sep 19, 2017

davidebbo commented Sep 19, 2017

EricSten-MSFT Sep 25, 2017

davidebbo Sep 25, 2017

davidebbo Feb 27, 2018

davidebbo Feb 28, 2018

EricSten-MSFT commented Sep 25, 2017

davidebbo Sep 27, 2017

EricSten-MSFT Sep 27, 2017

davidebbo left a comment

davidebbo commented Sep 27, 2017

davidebbo Oct 5, 2017

EricSten Oct 5, 2017

Check NuGet.org for SiteExtensions #2568

Check NuGet.org for SiteExtensions #2568

Conversation

EricSten-MSFT commented Sep 18, 2017

dnfclas commented Sep 18, 2017

dnfclas commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidebbo Sep 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidebbo commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricSten-MSFT commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidebbo left a comment

Choose a reason for hiding this comment

davidebbo commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidebbo Sep 19, 2017 •

edited