Support PackageIcon attribute & icon caching#423
Support PackageIcon attribute & icon caching#423iodes wants to merge 5 commits intoloic-sharma:masterfrom
Conversation
| return NotFound(); | ||
| } | ||
|
|
||
| return File(iconStream, "image/png"); |
There was a problem hiding this comment.
The content type image/png may be incorrect. Can we use an empty string "" instead? If not, we can copy nuget.org's logic: https://github.com/NuGet/NuGet.Services.Metadata/blob/91f97be8882ec83c3dfd7521ebe62865ab3adb5d/src/Catalog/Icons/IconProcessor.cs#L170-L278
There was a problem hiding this comment.
Some notes if the empty string "" does not work:
- I would add a
DetermineContentTypeAsyncto BaGet'sStreamExtensions. That way you can do:
var contentType = iconStream.DetermineContentTypeAsync(cancellationToken);
return File(iconStream, contentType);- When copying the
DetermineContentTypemethod from NuGet.org, you can act as ifonlyGallerySupportedistrue:
// Checks are ordered by format popularity among external icons for existing packages
if (ArrayStartsWith(imageData, PngHeader))
{
return "image/png";
}
if (ArrayStartsWith(imageData, JpegHeader))
{
return "image/jpeg";
}
return "";There was a problem hiding this comment.
It turns out using an empty string "" does not work. I will try image/xyz instead.
| routes.MapRoute( | ||
| name: Routes.PackageDownloadIconRouteName, | ||
| template: "v3/package/{id}/{version}/icon", | ||
| defaults: new { controller = "PackageContent", action = "DownloadIconAsync" }); |
There was a problem hiding this comment.
I recently ported BaGet to ASP.NET Core 3. You will need to change this to:
endpoints.MapControllerRoute(
name: Routes.PackageDownloadIconRouteName,
pattern: "v3/package/{id}/{version}/icon",
defaults: new { controller = "PackageContent", action = "DownloadIcon" })Sorry about that! 😅
| <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="$(MicrosoftExtensionsPackageVersion)" /> | ||
| <PackageReference Include="Microsoft.Extensions.Options.ConfigurationExtensions" Version="$(MicrosoftExtensionsPackageVersion)" /> | ||
| <PackageReference Include="NuGet.Packaging" Version="$(NuGetPackageVersion)" /> | ||
| <PackageReference Include="NuGet.Packaging" Version="5.3.1" /> |
There was a problem hiding this comment.
Please revert this change. You can bump up the NuGet.Packaging version here:
BaGet/src/Directory.Build.props
Line 33 in 05bfc61
| lowercasedNormalizedVersion, | ||
| iconPath); | ||
|
|
||
| result = await _storage.PutAsync(iconPath, iconStream, IconContentType, cancellationToken); |
There was a problem hiding this comment.
Same comment as here: let's try to use an empty string "" if possible, otherwise we can detect the content type
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="$(MicrosoftExtensionsPackageVersion)" /> | ||
| <PackageReference Include="Newtonsoft.Json" Version="11.0.2" /> | ||
| <PackageReference Include="NuGet.Protocol" Version="$(NuGetPackageVersion)" /> | ||
| <PackageReference Include="NuGet.Protocol" Version="5.3.1" /> |
There was a problem hiding this comment.
Please revert this change. You can bump up the NuGet.Packaging version here:
BaGet/src/Directory.Build.props
Line 33 in 05bfc61
| <ItemGroup> | ||
| <PackageReference Include="Newtonsoft.Json" Version="11.0.2" /> | ||
| <PackageReference Include="NuGet.Protocol" Version="5.0.0-rtm.5856" /> | ||
| <PackageReference Include="NuGet.Protocol" Version="5.3.1" /> |
There was a problem hiding this comment.
This now uses a variable $(NuGetPackageVersion). That is defined here:
BaGet/tests/Directory.Build.props
Line 18 in 05bfc61
|
I apologize for taking so long to look at this. I've left a few minor comments, I'll merge this in once you've addressed them. Thank you, this is amazing work! 😊 |
|
@loic-sharma you can update the merging branch directly if the changes are so minor, to speed up fix merging. |
| { | ||
| Id = id, | ||
| Version = versionString, | ||
| Id2 = id, |
|
I've updated your changes with the latest master branch here: #469. Feel free to review, and thanks for opening this! |
Summary of the changes (in less than 80 chars)
Addresses #123