(GH-1397) Improved choco outdated #1495

DamianMaslanka5 · 2018-02-19T19:45:03Z

Added get_outdated in NugetService to make it more readable.
Used
packageManager.SourceRepository.GetPackages().Where(x => x.Id.ToLower() == packageName && x.IsLatestVersion).FirstOrDefault();
to get the latest version of package instead of
packageManager.SourceRepository.FindPackage(packageName, version, config.Prerelease, allowUnlisted: false);
which downloads data about all versions of a package.

#1397

ferventcoder · 2018-02-20T04:31:04Z

Related to #994

ferventcoder

This looks good, but I'd like some testing on all types of repository sources. I think that this could possibly be helpful with upgrade in cases where it isn't using a particular --version.

ferventcoder · 2018-02-20T04:33:13Z

src/chocolatey/infrastructure.app/services/NugetService.cs

-        /// <param name="packageInfo">The package information.</param>
-        /// <returns>The original unmodified configuration, so it can be reset after upgrade</returns>
-        private ChocolateyConfiguration set_package_config_for_upgrade(ChocolateyConfiguration config, ChocolateyPackageInformation packageInfo)
+		public ConcurrentDictionary<string, PackageResult> get_outdated(ChocolateyConfiguration config)


XML comments here might be helpful 👍

They are lower with the method Here It just showed up oddly.

I meant, it might be beneficial to add xml comments to this method. I also noticed the spacing looks funny, how many spaces over is that?

These are 2 tabs, they look odd on github.

This is in INugetService

/// <summary> /// Get outdated packages /// </summary> /// <param name="config">The configuration.</param> ConcurrentDictionary<string, PackageResult> get_outdated(ChocolateyConfiguration config);

INuGetService, there we go!

ferventcoder · 2018-02-20T04:36:06Z

src/chocolatey/infrastructure.app/services/NugetService.cs

+
+			foreach (var packageName in packageNames)
+			{
+				var latestPackage = packageManager.SourceRepository.GetPackages().Where(x => x.Id.ToLower() == packageName && x.IsLatestVersion).FirstOrDefault();


I like it! How does it hold up against different sources though? Specifically I think this works great when it is hitting an OData feed like the community repository, but how does it work when it hits a file share or a NuGet.Server (Chocolatey.Server is a good one to test)?

ferventcoder · 2018-02-20T04:42:01Z

If this query is 👍 , it probably makes sense to use it for installs and upgrades as well when a specific version hasn't been passed - that would relieve some pressure against the community repository.

ferventcoder · 2018-02-20T14:15:21Z

src/chocolatey/infrastructure.app/services/NugetService.cs

@@ -862,11 +862,12 @@ public void remove_rollback_directory_if_exists(string packageName)

 			foreach (var packageName in packageNames)
 			{
-				var latestPackage = packageManager.SourceRepository.GetPackages().Where(x => x.Id.ToLower() == packageName && x.IsLatestVersion).FirstOrDefault();
+				// LastOrDefault to get the latest package from local repository, because all packages have set IsLatestVersion to true
+				var latestPackage = packageManager.SourceRepository.GetPackages().Where(x => x.Id.ToLower() == packageName && x.IsLatestVersion).LastOrDefault();


That's a weird change there.

ferventcoder · 2018-02-20T14:15:59Z

src/chocolatey/infrastructure.app/services/NugetService.cs

+			foreach (var packageName in packageNames)
+			{
+				// LastOrDefault to get the latest package from local repository, because all packages have set IsLatestVersion to true
+				var latestPackage = packageManager.SourceRepository.GetPackages().Where(x => x.Id.ToLower() == packageName && x.IsLatestVersion).LastOrDefault();


Nevermind, that makes sense.

ferventcoder · 2018-02-20T14:42:26Z

src/chocolatey/infrastructure.app/services/NugetService.cs

+
+				if(latestPackages.Count > 1)
+				{
+					latestPackage = latestPackages.Last();


There is actually a way to see if you are hitting a file repo or an OData repo. I'd take a look at that as this is not quite right.

What do you suggest?

Apologies meant to link it. Take a look at

choco/src/chocolatey/infrastructure.app/nuget/NugetList.cs

Line 40 in cdf0938

private static IQueryable<IPackage> execute_package_search(ChocolateyConfiguration configuration, ILogger nugetLogger)

Particularly

choco/src/chocolatey/infrastructure.app/nuget/NugetList.cs

Lines 50 to 59 in cdf0938

bool isRemote;

var aggregateRepo = packageRepository as AggregateRepository;

if (aggregateRepo != null)

{

isRemote = aggregateRepo.Repositories.All(repo => repo is IServiceBasedRepository);

}

else

{

isRemote = packageRepository is IServiceBasedRepository;

}

DamianMaslanka5 · 2018-02-20T16:57:48Z

src/chocolatey/infrastructure.app/services/NugetService.cs

 				{
-					latestPackage = latestPackages.Last();
+					latestPackage = repository.GetPackages().Where(x => x.Id.ToLower() == packageName && x.IsLatestVersion).SingleOrDefault();


If there is no package in a remote source nuget produces warning [NuGet] Object reference not set to an instance of an object. Is there a way to suppress it? If not we can call .ToList().SingleOrDefault().

Since we are acting on that list immediately, you can .ToList() it. No need to defer results any further.

ferventcoder · 2018-02-20T19:55:28Z

src/chocolatey/infrastructure.app/services/NugetService.cs

+			return outdatedPackages;
+		}
+
+		private bool IsRepositoryRemote(IPackageRepository repository)


naming conventions - this should be repository_is_remote

ferventcoder · 2018-02-20T19:56:25Z

src/chocolatey/infrastructure.app/services/NugetService.cs

+			{
+				var repository = packageManager.SourceRepository;
+
+				bool isRemote = IsRepositoryRemote(repository);


This call and the repository seem like they are not a per iteration kind of call. Might be best not to recheck that each time.

ferventcoder · 2018-02-20T20:01:35Z

src/chocolatey/infrastructure.app/services/NugetService.cs

+				}
+				else
+				{
+					latestPackage = repository.FindPackage(packageName);


What happened to the method you had before? It was legit, I mentioned it was weird but then said nevermind.

I changed it in 4fa560f because LastOrDefault is not supported in remote repository and in this commit updated to use isRemote.
So if we query remote repository it gets only the latest package and when we call local repository we use the same method that old outdated used without additional parameters.

I meant repository.FindPackage() for a repository of packages, which would also have multiple versions. Note the method I linked earlier shows you how to deal with this -

choco/src/chocolatey/infrastructure.app/nuget/NugetList.cs

Lines 130 to 138 in cdf0938

if (!isRemote)

{

results =

results

.Where(PackageExtensions.IsListed)

.Where(p => configuration.Prerelease || p.IsReleaseVersion())

.distinct_last(PackageEqualityComparer.Id, PackageComparer.Version)

.AsQueryable();

}

Are you talking about supporting Prelease and IsListed like in this call packageManager.SourceRepository.FindPackage(packageName, version, config.Prerelease, allowUnlisted: false);
Or are you talking about repository.FindPackage(packageName); not be able to get the latest version of package if there are multiple versions.

ferventcoder · 2018-02-20T21:14:53Z

This is looking really good so far!

ferventcoder · 2018-02-20T21:38:43Z

Just got to play with this, wow, that is lightning fast. I'm not fully sure on the urls it is generating yet:

https://chocolatey.org/api/v2/Packages()?$filter=(tolower(Id)%20eq%20'1password')%20and%20IsLatestVersion&$skip=0&$top=30

The bit about $filter=tolower(Id) - if it works, great! I'm going to test some more.

DamianMaslanka5 · 2018-02-20T21:50:41Z

@ferventcoder
It works good. tolower(Id) prevents from not getting package when id is not lowercase, for example WebStorm or ChocolateyGUI.

What the about output. This shows up when calling repository that doesn't have package.

IMO it should show up only with verbose switch. At the end should be some kind of summary, how many packages where actually checked in the source, number of all installed packages and number of outdated packages

ferventcoder · 2018-02-20T22:02:43Z

@DamianMaslanka5 add --ignore-unfound to your query or set the feature ignoreUnfoundPackagesOnUpgradeOutdated. #1398

ferventcoder · 2018-02-20T22:03:53Z

It's not working, which means you may have missed something there

DamianMaslanka5 · 2018-02-20T22:04:31Z

tolower(Id) prevents from not getting package when id is not lowercase, for example WebStorm or ChocolateyGUI.

nvm apparently tolower(Id) is not needed

ferventcoder · 2018-02-20T22:05:58Z

It works good. tolower(Id) prevents from not getting package when id is not lowercase, for example WebStorm or ChocolateyGUI.

Chocolatey.org packages (the community repo) represents only a tiny fraction of what's out there. Most folks are using ProGet, Artifactory, Nexus, Chocolatey Simple Server or some derivative that is at https://chocolatey.org/docs/how-to-host-feed.

ferventcoder · 2018-02-20T22:25:29Z

We can't change the output like that (the way you have) - that would be a break in format for all of the tools using Chocolatey. I prefer to do this as a non-breaking change versus waiting for a long time to move a major version.

ferventcoder · 2018-02-20T22:48:45Z

This is where I'm at with a format that doesn't incur a breaking change:

 public ConcurrentDictionary<string, PackageResult> get_outdated(ChocolateyConfiguration config)
        {
            var packageManager = NugetCommon.GetPackageManager(
              config,
              _nugetLogger,
              _packageDownloader,
              installSuccessAction: null,
              uninstallSuccessAction: null,
              addUninstallHandler: false);

            var repository = packageManager.SourceRepository;
            bool isRemote = repository_is_remote(repository);

            var outdatedPackages = new ConcurrentDictionary<string, PackageResult>();

            set_package_names_if_all_is_specified(config, () => { config.IgnoreDependencies = true; });
            var packageNames = config.PackageNames.Split(new[] { ApplicationParameters.PackageNamesSeparator }, StringSplitOptions.RemoveEmptyEntries).or_empty_list_if_null().ToList();
            foreach (var packageName in packageNames)
            {
                var installedPackage = packageManager.LocalRepository.FindPackage(packageName);
                var pkgInfo = _packageInfoService.get_package_information(installedPackage);
                bool isPinned = pkgInfo.IsPinned;

                // if the package is pinned and we are skipping pinned, 
                // move on quickly
                if (isPinned && config.OutdatedCommand.IgnorePinned)
                {
                    string pinnedLogMessage = "{0} is pinned. Skipping pinned package.".format_with(packageName);
                    var pinnedPackageResult = outdatedPackages.GetOrAdd(packageName, new PackageResult(installedPackage, _fileSystem.combine_paths(ApplicationParameters.PackagesLocation, installedPackage.Id)));
                    pinnedPackageResult.Messages.Add(new ResultMessage(ResultType.Debug, pinnedLogMessage));
                    pinnedPackageResult.Messages.Add(new ResultMessage(ResultType.Inconclusive, pinnedLogMessage));

                    continue;
                }

                IQueryable<IPackage> results = repository.GetPackages().Where(x => x.Id == packageName);
              
                if (config.Prerelease && repository.SupportsPrereleasePackages)
                {
                    results = results.Where(p => p.IsAbsoluteLatestVersion);
                }
                else
                {
                    results = results.Where(p => p.IsLatestVersion);
                }

                if (!isRemote)
                {
                    results = results
                        .Where(PackageExtensions.IsListed)
                        .Where(p => config.Prerelease || p.IsReleaseVersion())
                        .distinct_last(PackageEqualityComparer.Id, PackageComparer.Version)
                        .AsQueryable();
                }
                
                // get only one result, should be the latest
                var latestPackage = results.ToList().SingleOrDefault();

                if (latestPackage == null)
                {
                    if (config.Features.IgnoreUnfoundPackagesOnUpgradeOutdated) continue;

                    string unfoundLogMessage = "{0} was not found with the source(s) listed.{1} Source(s): \"{2}\"".format_with(packageName, Environment.NewLine, config.Sources);
                    var unfoundResult = outdatedPackages.GetOrAdd(packageName, new PackageResult(installedPackage, _fileSystem.combine_paths(ApplicationParameters.PackagesLocation, installedPackage.Id)));
                    unfoundResult.Messages.Add(new ResultMessage(ResultType.Warn, unfoundLogMessage));
                    unfoundResult.Messages.Add(new ResultMessage(ResultType.Inconclusive, unfoundLogMessage));

                    this.Log().Warn("{0}|{1}|{1}|{2}".format_with(installedPackage.Id, installedPackage.Version, isPinned.to_string().to_lower()));
                    continue;
                }

                if (latestPackage.Version <= installedPackage.Version) continue;
                
                var packageResult = outdatedPackages.GetOrAdd(packageName, new PackageResult(latestPackage, _fileSystem.combine_paths(ApplicationParameters.PackagesLocation, latestPackage.Id)));

                string logMessage = "You have {0} v{1} installed. Version {2} is available based on your source(s).{3} Source(s): \"{4}\"".format_with(installedPackage.Id, installedPackage.Version, latestPackage.Version, Environment.NewLine, config.Sources);
                packageResult.Messages.Add(new ResultMessage(ResultType.Note, logMessage));

                this.Log().Info("{0}|{1}|{2}|{3}".format_with(installedPackage.Id, installedPackage.Version, latestPackage.Version, isPinned.to_string().to_lower()));
            }

            return outdatedPackages;
        }

DamianMaslanka5 · 2018-02-20T22:49:33Z

@ferventcoder I will add

ignoreUnfoundPackagesOnUpgradeOutdated

and change the output tomorrow. I tested Chocolatey Simple Server, the community repo and local folder. If you have configured sources to any of

ProGet, Artifactory, Nexus

Let me know if it works

ferventcoder · 2018-02-20T22:50:47Z

pulling this away from the upgrade command gave us some options, we can check for pinned and ignore and not even reach out to search.

ferventcoder · 2018-02-20T22:54:11Z

I updated the code above. If you can squash your commits down tomorrow and maybe look at flipping over to the above, I think we'll be set. If not, I can take care of squashing and then add an additional commit to use this format.

gep13

Other than a few whitespace changes, and assuming we are using a combination of both pieces of work, this LGTM! 😄

gep13 · 2018-02-21T08:39:28Z

src/chocolatey/infrastructure.app/services/INugetService.cs

-    using configuration;
+	using chocolatey.infrastructure.results;
+	using configuration;
+	using System.Collections.Concurrent;


Minor nitpick... System namespaces should appear first, before any other namespaces.

I thought I was nitpicky ;)

gep13 · 2018-02-21T08:40:22Z

src/chocolatey/infrastructure.app/services/INugetService.cs


-    public interface INugetService : ISourceRunner
+	public interface INugetService : ISourceRunner


Looks like there is a whitespace issue here, where the main definition is bumped over an extra tab.

gep13 · 2018-02-21T08:40:47Z

src/chocolatey/infrastructure.app/services/INugetService.cs

@@ -49,5 +51,11 @@ public interface INugetService : ISourceRunner
        /// </summary>
        /// <param name="packageName">Name of the package.</param>
        void remove_rollback_directory_if_exists(string packageName);
+
+		/// <summary>


Looks like this method is bumped over an extra tab as well, i.e. it isn't inline with the other methods.

GitHub by default shows tabs as 8 spaces instead of 4 add ?ts=4 to file view url, it doesn't work with diffs.
Anyway I will convert tabs to spaces.

Sounds good, thanks!

DamianMaslanka5 · 2018-02-21T16:28:24Z

src/chocolatey/infrastructure.app/services/NugetService.cs

+                }
+
+                // Call .ToList() to prevent warning from NuGet if package is not found in a remote repository
+                var latestPackage = results.ToList().SingleOrDefault();


This throws exceptions, if we use multiple sources, because we get 2 packages if they exists in both sources. Should we just move .distinct_last(PackageEqualityComparer.Id, PackageComparer.Version) outside if (!isRemote) and take package with the highest version? Or we shouldn't support multiple sources at all?

@DamianMaslanka5 that's a good observation. Are you on Gitter? https://gitter.im/chocolatey/choco - we could discuss in more realtime.

Added get_outdated to NugetService for simplifiation. "packageManager.SourceRepository.FindPackage" downloaded data about all versions of a package instead of only the latest. Now "packageManager.SourceRepository.GetPackages()" is used to get only the latest version of a package.

DamianMaslanka5 · 2018-02-22T11:29:44Z

@gep13 @ferventcoder

ferventcoder · 2018-10-02T23:14:43Z

Howdy, this has been rebased onto stable, rebased on merge conflicts, then rebased to squash down the commits and merged into stable at 31fe5e3. Thanks for the contribution, this is going to be great!!

ferventcoder reviewed Feb 20, 2018

View reviewed changes

DamianMaslanka5 force-pushed the improve_choco_outdated branch from e2c74bd to cb0c781 Compare February 20, 2018 14:03

ferventcoder reviewed Feb 20, 2018

View reviewed changes

ferventcoder requested a review from gep13 February 20, 2018 14:17

ferventcoder reviewed Feb 20, 2018

View reviewed changes

DamianMaslanka5 commented Feb 20, 2018

View reviewed changes

ferventcoder reviewed Feb 20, 2018

View reviewed changes

gep13 requested changes Feb 21, 2018

View reviewed changes

DamianMaslanka5 commented Feb 21, 2018

View reviewed changes

DamianMaslanka5 force-pushed the improve_choco_outdated branch from ecc88e4 to 3266258 Compare February 22, 2018 11:23

ferventcoder closed this Oct 2, 2018

DamianMaslanka5 deleted the improve_choco_outdated branch October 3, 2018 11:23

	bool isRemote;
	var aggregateRepo = packageRepository as AggregateRepository;
	if (aggregateRepo != null)
	{
	isRemote = aggregateRepo.Repositories.All(repo => repo is IServiceBasedRepository);
	}
	else
	{
	isRemote = packageRepository is IServiceBasedRepository;
	}

	if (!isRemote)
	{
	results =
	results
	.Where(PackageExtensions.IsListed)
	.Where(p => configuration.Prerelease \|\| p.IsReleaseVersion())
	.distinct_last(PackageEqualityComparer.Id, PackageComparer.Version)
	.AsQueryable();
	}


		public interface INugetService : ISourceRunner
		public interface INugetService : ISourceRunner

(GH-1397) Improved choco outdated #1495

(GH-1397) Improved choco outdated #1495

Conversation

DamianMaslanka5 commented Feb 19, 2018

ferventcoder commented Feb 20, 2018 • edited Loading

ferventcoder left a 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

ferventcoder commented Feb 20, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferventcoder commented Feb 20, 2018

ferventcoder commented Feb 20, 2018

DamianMaslanka5 commented Feb 20, 2018

ferventcoder commented Feb 20, 2018

ferventcoder commented Feb 20, 2018

DamianMaslanka5 commented Feb 20, 2018 • edited Loading

ferventcoder commented Feb 20, 2018

ferventcoder commented Feb 20, 2018

ferventcoder commented Feb 20, 2018 • edited Loading

DamianMaslanka5 commented Feb 20, 2018

ferventcoder commented Feb 20, 2018

ferventcoder commented Feb 20, 2018

gep13 left a 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

DamianMaslanka5 Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DamianMaslanka5 commented Feb 22, 2018

ferventcoder commented Oct 2, 2018

ferventcoder commented Feb 20, 2018 •

edited

Loading

DamianMaslanka5 commented Feb 20, 2018 •

edited

Loading

ferventcoder commented Feb 20, 2018 •

edited

Loading

DamianMaslanka5 Feb 21, 2018 •

edited

Loading