Encode container image manifests in ImageBuilder via Encoder Fixes #47399 #48255
Conversation
|
From the PR title, I assumed this was about graphics images and something like JPEG encoding. Could clarify with "Encode container image manifests". |
| @@ -91,7 +94,7 @@ internal BuiltImage Build() | |||
| Config = imageJsonStr, | |||
| ImageDigest = imageDigest, | |||
| ImageSha = imageSha, | |||
| Manifest = JsonSerializer.SerializeToNode(newManifest)?.ToJsonString() ?? "", | |||
| Manifest = JsonSerializer.SerializeToNode(newManifest, serializerOptions)?.ToJsonString() ?? "", | |||
| ManifestDigest = newManifest.GetDigest(), | |||
There was a problem hiding this comment.
GetDigest seems to be using JsonSerializer.SerializeToNode with the default encoder so the digest might not match after this change
There was a problem hiding this comment.
I can change that one to use the same serializer.
I'm a little concerned because I don't really understand how these are deserialized, and I'd want to make sure we are able to read them properly. Do you know more about that?
(Sorry I have stupid questions; I don't know very much about containers 🙂)
There was a problem hiding this comment.
my main comment was going to be that all json (de)serialization should happen via the same set of serializer options, so I'm glad @KalleOlaviNiemitalo touched on this.
for other places we deserialize - we do this in a few places, but prominently in Registry.cs, where we take opaque JSON blobs from HTTP APIs and convert them to various structured representations.
There was a problem hiding this comment.
Perhaps consider using the BannedApiAnalyzers for this purpose? It's probably a bit more complicated that it ought to be, since you'll need to ban APIs one-at-a-time, but it's worth at least considering so that usages without the options don't creep in.
There was a problem hiding this comment.
As far as I know, the only advantage to using JsonSerializer.Serialize(obj) over JsonSerializer.Serialize(obj, JsonSerializerOptions) is brevity. Even if you just want the standard, default JsonSerializerOptions, you can pass in JsonSerializerOptions.Default, and I believe it works the same way. For that reason, I think adding it to our BannedApiAnalyzers might be a reasonable approach.
Any concerns @baronfel?
There was a problem hiding this comment.
Bring down the banhammer :) I'm happy preventing the sue of the convenience API throughout this component.
There was a problem hiding this comment.
Since we don't seem to have infrastructure for this set up at the moment, I think I wouldn't set it up in this PR regardless, so I may as well make an issue to record the suggestion for discussion.
|
Note: Requires additional testing and a review CC @dotnet/sdk-container-builds-maintainers Who should I assign this to? |
|
@surayya-MS would be the go-to |
Fixes #47399
@MattKotsenas @baronfel
I wasn't really able to test this (see here) as I'd like, but does this look like the kind of fix you'd expect?