-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add basic ability to get latest version and install to installation library #51374
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far! I definitely think logging/telemetry is a top priority for us after this, as well as adding even more tests. I'll leave this feedback now. Working on adding a little more verification to the e2e tests, then ill help merge your PR into that pipeline change so we have a working PR CI flow.
| IEnumerable<string> GetAvailableChannels(); | ||
|
|
||
| ReleaseVersion GetLatestVersion(InstallComponent component, string channel); | ||
| ReleaseVersion? GetLatestVersion(InstallComponent component, string channel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It might be better to fail if we can't find a release version - that would likely be cleaner than requiring invokers to use !. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As is, the caller should probably check for a null return value and either throw an error or handle appropriately if there is one. It might be better to just throw in the implementation. I think we'll want to clean up a lot of the ReleaseManifestcode, probably we can consider when we do that.
we can chat if we want this to be internal - though it might be helpful for consumers to be able to marshal the architecture from the runtime into our architecture type that we use in our install objects, considering it's part of dotnet install root, which is also public
| namespace Microsoft.Dotnet.Installation.Internal; | ||
|
|
||
| internal class ArchiveDotnetInstaller : IDotnetInstaller, IDisposable | ||
| internal class ArchiveDotnetExtractor : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about the name DotnetArchiveExtractor? It feels more consistent with other types like DotnetInstaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this name! Agree it's an improvement.
| { | ||
| internal class DotnetInstaller : IDotnetInstaller | ||
| { | ||
| public void Install(DotnetInstallRoot dotnetRoot, InstallComponent component, ReleaseVersion version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: DotnetInstaller and DotnetReleaseInfoProvider have brank lines between the members.
|
|
||
| namespace Microsoft.DotNet.Tools.Dnup.Tests; | ||
|
|
||
| public class LibraryTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider splitting these out into a separate test assembly? It would help discoverability and help separate areas of concerns.
Move enough implementation into the library to implement the following methods:
IDotnetInstaller.InstallIDotnetReleaseInfoProvider.GetLatestVersionUsage:
Other interface methods are not hooked up and will throw
NotImplementedException.