New Microsoft.Deployment.DotNet.Releases package #75

joeloff · 2020-12-07T23:45:20Z

Preliminary checking for the releases.json API (An immutable DOM for reading release.json data an query .NET releases)

net452 has to be supported for now, thus the dependency on Newtonsoft.Json
While the releases.json data refers to items such as channels, during the API reviews it was decided to disambiguate some of these overloaded terms.
- For example, .NET Core 3.1 is considered a Product (in releases-index.json this is called a channel).
- Each product can have multiple releases, e.g. 3.1.8
- Each release is composed of multiple components, e.g. runtime, SDKs

joeloff · 2020-12-07T23:54:04Z

@NikolaMilosavljevic can you or @dleeapho provide me with perms to add reviewers so I can loop in additional folks that were involved?

joeloff · 2020-12-08T04:43:41Z

@leecow, @terrajobst, @KathleenDollard as an FYI

src/dotnetreleases/Directory.Build.props

src/dotnetreleases/src/Microsoft.Deployment.DotNet.Releases/AspNetCoreReleaseComponent.cs

src/dotnetreleases/src/Microsoft.Deployment.DotNet.Releases/Cve.cs

src/dotnetreleases/src/Microsoft.Deployment.DotNet.Releases/JsonExtensions.cs

src/dotnetreleases/src/Microsoft.Deployment.DotNet.Releases/Product.cs

src/dotnetreleases/src/Microsoft.Deployment.DotNet.Releases/ProductCollection.cs

src/dotnetreleases/src/Microsoft.Deployment.DotNet.Releases/ReleaseComponent.cs

src/dotnetreleases/src/Microsoft.Deployment.DotNet.Releases/ReleaseVersion.cs

joeloff · 2021-01-13T04:52:36Z

@NikolaMilosavljevic @MichaelSimons I've finally got round to moving the code/renaming. I had to settle for naming the subset dotnet_releases as '-' is used to remove subsets and '.' are used to refer to sub-subsets it seems.

MichaelSimons · 2021-01-13T15:03:22Z

@joeloff, are the changes ready to be reviewed again?

joeloff · 2021-01-13T16:50:52Z

@joeloff, are the changes ready to be reviewed again?

I believe so

eerhardt · 2021-01-13T21:06:57Z

src/Microsoft.Deployment.DotNet.Releases/Directory.Build.props

+  </PropertyGroup>
+
+  <PropertyGroup>
+    <LibPrefix Condition="'$(TargetOS)' != 'Windows_NT'">lib</LibPrefix>


Is all the stuff in this props file necessary? I don't see any usages of these properties from a quick look.

It looks to be duplicated with https://github.com/dotnet/deployment-tools/blob/master/src/installer/Directory.Build.props. Can it be refactored?

I don't think it's being used right now. @NikolaMilosavljevic when looking at the build pipeline, it seems only WindowsNT debug/release and x86/x64 legs are running, so it should be fine to trim Directory.build.props?

We don't build any Linux specific native binaries today and don't have a Linux build leg. Feel free to remove this stuff from your props file as you are not building any native projects.

I've cleaned up the props file

MichaelSimons

The changes are looking nice. I have a few comments.

src/Microsoft.Deployment.DotNet.Releases/src/ProductCollection.cs

MichaelSimons · 2021-01-15T19:59:08Z

src/Microsoft.Deployment.DotNet.Releases/src/Product.cs

+            }
+
+            using (HttpClient client = new HttpClient())
+            using (var stream = new MemoryStream(await client.GetByteArrayAsync(address)))


The use of var here feels inconsistent with the two surrounding lines which aren't using var. I see this pattern copied in a few other places.

These should now be addressed

This is still an issue. The use of var here is permitted. What I am pointing out is the inconsistency with the preceding and following lines.

src/Microsoft.Deployment.DotNet.Releases/src/Product.cs

MichaelSimons · 2021-01-15T20:56:23Z

src/Microsoft.Deployment.DotNet.Releases/src/ReleaseFile.cs

+
+            if (!string.Equals(Hash, actualHash, StringComparison.OrdinalIgnoreCase))
+            {
+                File.Delete(destinationPath);


The following scenario does not seem to have a desirable behavior.

FileX exists

User calls DownloadAsync with FileX's path to get a newer version.

For some reason the download fails the hash check.

Results: The original FileX was overwritten and deleted.

ExpectedResults: This method should not have any side effects if it failed to download the file. In other words, the original FileX should be preserved.

As per our conversation, I've added an additional method that will download, but not verify and one that will download to a temp file, verify the hash and then either delete or move the temp file depending on the hash results.

I wasn't sure about adding a test because it could turn out to be flaky if there is a problem downloading the file at all

src/Microsoft.Deployment.DotNet.Releases/tests/ProductCollectionTests.cs

MichaelSimons · 2021-01-22T15:34:48Z

src/Microsoft.Deployment.DotNet.Releases/src/Product.cs

+            }
+
+            using (HttpClient client = new HttpClient())
+            using (var stream = new MemoryStream(await client.GetByteArrayAsync(address)))


This is still an issue. The use of var here is permitted. What I am pointing out is the inconsistency with the preceding and following lines.

MichaelSimons · 2021-01-22T15:46:26Z

src/Microsoft.Deployment.DotNet.Releases/src/Product.cs

+        /// <param name="address">The URL pointing to the releases.json file to use.</param>
+        /// <param name="product">The <see cref="Product"/> to link to the releases.</param>
+        /// <returns></returns>
+        public static async Task<ReadOnlyCollection<ProductRelease>> GetReleasesAsync(Uri address, Product product)


What is the reason this is static? It feels like it should be an product instance method overload which takes an address parameter.

Same comment applies to the next overload.

The non-static one uses the default URL. The static one was intended if users want to mirror the releases.json files on their own server, e.g. an intranet.

I understand that. It is very strange to have a static method of Product that has a Product parameter. What is the reason to not make it an instance method?

The way I thought about it is that the instance method would use the default URLs and felt like good symmetry given the static method we have on the ProductCollection

Assuming you're hosting the files on a different server, the releases-index would likely point to local copies of the releases.json.

Maybe we should just remove it for now. The only other use case would be if you knew where a specific release lived and didn't care about the top level products, but only wanted to get the releases information.

I see the merit in the scenario. I see this as different than the ProductCollection static method because instance state is not required.

Another option is perhaps the ReleasesJson setter should be public. For local copy scenarios, the user would tweak the ReleasesJson property before calling the instance GetReleasesAsync method.

@terrajobst, can you provide guidance here?

The ReleasesJson property reflects the URL associated with the product as defined in the releases-index.json. Overriding that seems to defeat the main case of an application working its way through the ProductCollection->Product->Releases.

I'd prefer changing the static to an instance method instead of making the setter public

@terrajobst this is the last issue we need to close on. Feel free to ping me if you need any detail.

I agree wit @MichaelSimons. It seems more natural to have them regular instance overloads. It makes sense to me so see some with a URL and some without; this would indicate that URL is optional and if not specified a default is being used.

Thanks @terrajobst I'll update the PR

This has now been addressed

src/Microsoft.Deployment.DotNet.Releases/src/ReleaseFile.cs

MichaelSimons · 2021-02-01T15:18:53Z

src/Microsoft.Deployment.DotNet.Releases/src/Product.cs

        /// <param name="product">The <see cref="Product"/> to link to the releases.</param>
        /// <returns></returns>
-        public static async Task<ReadOnlyCollection<ProductRelease>> GetReleasesAsync(Uri address, Product product)
+        public async Task<ReadOnlyCollection<ProductRelease>> GetReleasesAsync(Uri address, Product product)


I was expecting the product parameter would be removed with the change to make this an instance method.

Consider grouping with the other GetReleasesAsync overloads.

You are correct. I've fixed this in the last commit.

IEvangelist

I had a few nits, and a few larger concerns.

Why not use System.Text.Json instead of Newtonsoft.Json, it would remove a 3rd party dependency - I suggest switching
Are you able to use the IHttpClientFactory instead of newing up an HttpClient and disposing of it right away, I have concerns about socket exhaustion. Also, what if there is no internet connection - these won't work will it?
I have concerns about the static functionality hanging off of what initially look like POCOs, it might be better to separate these into services. For example, instead of a Product class with static Task-turning methods on it, have an IProductService that gets you a collection of products. Also, as a consumer of this library it would be nice if it exposed its services through DI. I'd expect to see an extension method on IServiceCollection that adds the services, so consumers could require an IProductService or IReleaseService or whatever, into the .ctor of their consuming classes.

IEvangelist · 2021-02-01T21:00:35Z

src/Microsoft.Deployment.DotNet.Releases/src/AspNetCoreReleaseComponent.cs

+
+using System;
+using System.Collections.ObjectModel;
+using Newtonsoft.Json.Linq;


Why not use System.Text.Json instead of an external 3rd party package?

We have to support 4.5.2 for some enterprise scenarios, which takes System.Text.Json off the table for us. If you want the latest copy you can only get that by going, but if you downloaded the files and laid them out on an internal machine you can use the local set of files without having to go online.

How do you know to download them, that is somewhat unintuitive? How does a consumer know when there is a "latest copy", what if they never have internet connection. The idea of relying on parsing these JSON files is a bit fragile, there are inconsistencies. What about codifying this metadata in the package itself, so that it doesn't need to relying on parsing JSON and making HTTP requests. All the past releases are not changing, maybe this would be a use-case for code generators?

IEvangelist · 2021-02-01T21:06:15Z

src/Microsoft.Deployment.DotNet.Releases/src/Product.cs

+        /// The date of the latest release for this product.
+        /// </summary>
+        [JsonProperty(PropertyName = "latest-release-date")]
+        public DateTime? LatestReleaseDate


I'm curious why this is nullable, I'd hope that there is ALWAYS a latest release date? If not, you might want to add something in the property comments as to when consumers might expect that to be null.

Some of the older data, especially around 1.0 and 1.1 is not always consistent or complete. There's been instances where fields were either absent, empty or explicitly set to null or "null".

I see what you're saying, maybe that old data should be cleaned up? If these are JSON files sitting around somewhere, we should update them rather than making fragile models that represent them. Consumers are going to want to know these things.

1.0/1.1 released before the releases.json existed. Some issues in the manifest have been resolved. We could do a scan to see if any quirks remain. @leecow to comment on what our options are around updating the .json files. The other thing to note is that we know there are external consumers that have likely worked around some of the data that is incorrect, so there is risk in fixing the older entries.

Yes, backwards compatibility is always important. This is why I believe relying on the JSON makes this entire approach fragile.

@joeloff - I don't have any material objection to fixing up old data, particularly where it may be leading you to implement problematic workarounds.

Thanks @leecow I'll remove the nullable datetime and do a pass over the current data.

I've fixed this now.

IEvangelist · 2021-02-01T21:07:54Z

src/Microsoft.Deployment.DotNet.Releases/src/Product.cs

+        /// The version of the product, e.g "5.0" or "1.1".
+        /// </summary>
+        [JsonProperty(PropertyName = "channel-version")]
+        public string ProductVersion


The product version is always {major}.{minor} format, but the other versions follow semantic versioning - is that correct? If so, might be worth calling that out in the comments.

Correct. A specific release e.g. 5.0.0-rc2 will follow semver and will be part of the 5.0 product.

IEvangelist · 2021-02-01T21:08:51Z

src/Microsoft.Deployment.DotNet.Releases/src/Product.cs

+        /// The name of the product.
+        /// </summary>
+        [JsonProperty(PropertyName = "product")]
+        public string ProductName


I was curious if this should be an enum instead of a string, thoughts on that?

We already rebranded between .NET Core and .NET, and it's certainly possible that could happen again. This data is typically used to drive UIs/ human readable text. If it were an enum, any consumer would need to know what the proper product branding is.

Yeah, that makes sense - thanks.

IEvangelist · 2021-02-01T21:11:32Z

src/Microsoft.Deployment.DotNet.Releases/src/Product.cs

+        public async Task<ReadOnlyCollection<ProductRelease>> GetReleasesAsync()
+        {
+            return await GetReleasesAsync(ReleasesJson, this);
+        }


Suggested change

public async Task<ReadOnlyCollection<ProductRelease>> GetReleasesAsync()

{

return await GetReleasesAsync(ReleasesJson, this);

}

public Task<ReadOnlyCollection<ProductRelease>> GetReleasesAsync() =>

GetReleasesAsync(ReleasesJson, this);

You can omit the async and await keywords for single Task returning methods like this. Also, expression bodied member is nice here.

This should also be fixed now

IEvangelist · 2021-02-02T13:11:39Z

src/Microsoft.Deployment.DotNet.Releases/src/ReleaseVersion.cs

+        {
+            get;
+            private set;
+        } = 0;


Suggested change

} = 0;

}

I've addressed this.

IEvangelist · 2021-02-02T13:11:51Z

src/Microsoft.Deployment.DotNet.Releases/src/ReleaseVersion.cs

+        {
+            get;
+            private set;
+        } = 0;


Suggested change

} = 0;

}

This should be fixed

IEvangelist · 2021-02-02T13:17:25Z

src/Microsoft.Deployment.DotNet.Releases/src/Utils.cs

+            using (var httpClient = new HttpClient())
+            {
+                HttpRequestMessage httpRequest = new HttpRequestMessage(HttpMethod.Head, address);
+                HttpResponseMessage httpResponse = await httpClient.SendAsync(httpRequest);
+
+                httpResponse.EnsureSuccessStatusCode();
+
+                DateTime? onlineLastModified = httpResponse.Content.Headers.LastModified?.DateTime;
+                FileInfo fileInfo = new FileInfo(fileName);
+
+                return fileInfo.LastWriteTime >= onlineLastModified;
+            }


Suggested change

using (var httpClient = new HttpClient())

{

HttpRequestMessage httpRequest = new HttpRequestMessage(HttpMethod.Head, address);

HttpResponseMessage httpResponse = await httpClient.SendAsync(httpRequest);

httpResponse.EnsureSuccessStatusCode();

DateTime? onlineLastModified = httpResponse.Content.Headers.LastModified?.DateTime;

FileInfo fileInfo = new FileInfo(fileName);

return fileInfo.LastWriteTime >= onlineLastModified;

}

using HttpClient httpClient = new();

HttpRequestMessage httpRequest = new(HttpMethod.Head, address);

var httpResponse = await httpClient.SendAsync(httpRequest);

httpResponse.EnsureSuccessStatusCode();

var onlineLastModified = httpResponse.Content.Headers.LastModified?.DateTime;

FileInfo fileInfo = new(fileName);

return fileInfo.LastWriteTime >= onlineLastModified;

IEvangelist · 2021-02-02T13:19:19Z

src/Microsoft.Deployment.DotNet.Releases/src/Utils.cs

+            using (var httpClient = new HttpClient())
+            {
+                string directory = Path.GetDirectoryName(fileName);
+
+                if (!Directory.Exists(directory))
+                {
+                    Directory.CreateDirectory(directory);
+                }
+
+                HttpResponseMessage httpResponse = await httpClient.GetAsync(address);
+                httpResponse.EnsureSuccessStatusCode();
+
+                using (FileStream fileStream = File.Create(fileName))
+                {
+                    await httpResponse.Content.CopyToAsync(fileStream);
+                }
+            }


Suggested change

using (var httpClient = new HttpClient())

{

string directory = Path.GetDirectoryName(fileName);

if (!Directory.Exists(directory))

{

Directory.CreateDirectory(directory);

}

HttpResponseMessage httpResponse = await httpClient.GetAsync(address);

httpResponse.EnsureSuccessStatusCode();

using (FileStream fileStream = File.Create(fileName))

{

await httpResponse.Content.CopyToAsync(fileStream);

}

}

var directory = Path.GetDirectoryName(fileName);

if (!Directory.Exists(directory))

{

Directory.CreateDirectory(directory);

}

using HttpClient httpClient = new();

var httpResponse = await httpClient.GetAsync(address);

httpResponse.EnsureSuccessStatusCode();

using FileStream fileStream = File.Create(fileName);

await httpResponse.Content.CopyToAsync(fileStream);

That's C# 8.0, which we can't use because of 4.5.2

IEvangelist · 2021-02-02T13:20:33Z

src/Microsoft.Deployment.DotNet.Releases/src/Utils.cs

+            using (FileStream stream = File.OpenRead(fileName))
+            {
+                byte[] checksum = hashAlgorithm.ComputeHash(stream);
+
+                return BitConverter.ToString(checksum).Replace("-", "").ToLowerInvariant();
+            }


Suggested change

using (FileStream stream = File.OpenRead(fileName))

{

byte[] checksum = hashAlgorithm.ComputeHash(stream);

return BitConverter.ToString(checksum).Replace("-", "").ToLowerInvariant();

}

using FileStream stream = File.OpenRead(fileName);

var checksum = hashAlgorithm.ComputeHash(stream);

return BitConverter.ToString(checksum).Replace("-", "").ToLowerInvariant();

marcpopMSFT · 2021-02-05T22:50:45Z

As we have a few folks waiting to get their hands on this package, should we consider checking in with the latest updates from Jacques and filing a bug for any remaining open questions? We should ensure that we aren't going to change the api surface after it's checked in if we can help it but I think most of the feedback has shifted away from the API type changes to coding patterns. @dleeapho, thoughts?

dleeapho · 2021-02-10T21:50:05Z

should we consider checking in with the latest updates from Jacques and filing a bug for any remaining open questions? We should ensure that we aren't going to change the api surface after it's checked in if we can help it but I think most of the feedback has shifted away from the API type changes to coding patterns.

@marcpopMSFT , I agree. We are in good shape API surface-wise and can iterate on follow up issues subsequently.

IEvangelist · 2021-02-12T19:26:15Z

Yes, @marcpopMSFT and @dleeapho - I would love this, even a preview. 😬

joeloff · 2021-02-12T19:29:45Z

I'm going to file issues for the HTTPClient concerns @IEvangelist raised. @NikolaMilosavljevic already opened an issue for Linux builds (we need those for tests).

@MichaelSimons do you have any outstanding concerns or are we ready to sign off for Preview 1 and merge?

MichaelSimons · 2021-02-12T20:29:04Z

@MichaelSimons do you have any outstanding concerns or are we ready to sign off for Preview 1 and merge?

I am good with merging. I think we should track the remaining work with issues, gather feedback, and refactor as appropriate.

IEvangelist · 2021-02-12T21:12:41Z

One thing that @joeloff and I were just discussing, is that fact that this package doesn't account for or consider .NET Standard releases. For example, it has no knowledge of .NET Standard releases, instead it only knows about .NET Core, .NET and I believe .NET Framework - but .NET Standard is missing. I was somewhat expecting this package to provide instances of ReleaseComponent for each release of .NET Standard 1.0, 1.2, ... 2.1. Does that make sense?

This table is useful, and it would be great if this was somehow codified into the release package - so that consumers could use it to determine compatibility.

New Microsoft.Deployment.DotNet.Releases package

812e612

joeloff requested a review from NikolaMilosavljevic December 8, 2020 04:42

joeloff requested review from dsplaisted, sfoslund and wli3 December 8, 2020 04:44

MichaelSimons reviewed Dec 8, 2020

View reviewed changes

joeloff added 4 commits December 10, 2020 20:59

Address code review feedback

19634c6

Rename dotnetreleases to DotNetReleases

6b5d42b

Fix up doc comments, minor clean up

f409adf

Rename subset to 'dotnet_releases'

623339a

eerhardt reviewed Jan 13, 2021

View reviewed changes

Clean up props file

9bf23c0

MichaelSimons reviewed Jan 15, 2021

View reviewed changes

Code cleanup

04d9960

MichaelSimons reviewed Jan 22, 2021

View reviewed changes

joeloff added 2 commits January 22, 2021 10:54

PR cleanup and feedback

4e17e38

Make GetReleasesAsync overload an instance method

b0d51a0

MichaelSimons reviewed Feb 1, 2021

View reviewed changes

IEvangelist reviewed Feb 2, 2021

View reviewed changes

IEvangelist mentioned this pull request Feb 3, 2021

Tech debt: replace custom implementation with release deployment package dotnet/versionsweeper#7

Closed

Replace var with explicit types

0730530

joeloff added 2 commits February 10, 2021 20:59

Make latest release data non-nullable, fix var usage

b4348b4

Reacting to more feedback

6e405d3

joeloff merged commit b6fd308 into dotnet:master Feb 12, 2021

marcpopMSFT mentioned this pull request Feb 13, 2021

Developers can easily parse release metadata for .NET Core updates for use in their deployment process dotnet/core#5440

Closed

5 tasks

joeloff deleted the dotnetreleases branch May 6, 2021 03:10

New Microsoft.Deployment.DotNet.Releases package #75

New Microsoft.Deployment.DotNet.Releases package #75

Uh oh!

Conversation

joeloff commented Dec 7, 2020

Uh oh!

joeloff commented Dec 7, 2020

Uh oh!

joeloff commented Dec 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joeloff commented Jan 13, 2021

Uh oh!

MichaelSimons commented Jan 13, 2021

Uh oh!

joeloff commented Jan 13, 2021

Uh oh!

eerhardt Jan 13, 2021 • 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!

MichaelSimons 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!

Choose a reason for hiding this comment

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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

eerhardt Jan 13, 2021 •

edited

Loading

IEvangelist left a comment •

edited

Loading

IEvangelist Feb 2, 2021 •

edited

Loading