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

(GH-490) Exception if no source is enabled #547

Closed
wants to merge 2 commits into from
Closed

(GH-490) Exception if no source is enabled #547

wants to merge 2 commits into from

Conversation

MikeFoden
Copy link
Contributor

Previously if no sources were available during installation of a
package, installation would fail with an Exception. This fix will make
the choco client check the source list to make sure it is not null
before attempting to install a package.

As per my discussion with @ferventcoder this fixes #490.

The exception itself occurs at \src\chocolatey\infrastructure.app\services\NugetService.cs:313 when checking the source list contains an extension without confirming there is something in the source list first. I've added in a check before showing the licenses to confirm that there is sources defined.

This PR supersedes #546.

Previously if no sources were available during installation of a
package, installation would fail with an Exception. This fix will make
the choco client check the source list to make sure it is not null
before attempting to install a package.
}

this.Log().Info(@"By installing you accept licenses for the packages.");

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is also necessary for upgrade_run.

@ferventcoder
Copy link
Member

Please also add a few tests that will show this working properly. That can be done in the install/upgrade scenarios if you want.

@ferventcoder
Copy link
Member

Overall 👍 so far!

There have been messages added for when an attempt to install, update
or list a package occurs without any sources being available.

Tests have also been added to show the code successfully catches these
scenarios.
@MikeFoden
Copy link
Contributor Author

@ferventcoder I have added it in for upgrade and for list, and set up tests for all three to show that if there is no sources available that it will show a message to the user explaining the issue.

MockLogger.contains_message("Installation was NOT successful. There are no sources enabled for packages.", LogLevel.Error).ShouldBeTrue();
Results.Count().ShouldEqual(0);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@ferventcoder
Copy link
Member

The exception itself occurs at \src\chocolatey\infrastructure.app\services\NugetService.cs:313 when checking the source list contains an extension without confirming there is something in the source list first. I've added in a check before showing the licenses to confirm that there is sources defined.

Do we have the ability to fix the exception at the source?

@MikeFoden
Copy link
Contributor Author

It's anything that references config.Sources causing the exception. The messages I have added in I guess would be considered friendly error messages.

There is a couple of things I can do:

  • We could patch each usage of config.Sources to confirm there is at least one source defined first.
  • Modify it so when config.Sources is loaded from the configuration file, if there is no valid sources have it get stored as an empty string, rather than null.

Which one works best for you @ferventcoder? I would assume the scenario that no sources are available would be relatively small as by default the chocolately feed is configured - a user would have to actively play with the configuration to get this situation to occur.

@ferventcoder
Copy link
Member

All good - I'll take a look at this and merge it soon.

@ferventcoder
Copy link
Member

Thanks for the contribution!

@MikeFoden
Copy link
Contributor Author

Thanks for helping me get involved!

@ferventcoder
Copy link
Member

I fixed up a little and squashed the commits. Thanks again!

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.

2 participants