Add 2.1 Alpine images #480
Conversation
|
|
||
| private static string GetRid(ImageDescriptor imageDescriptor) | ||
| { | ||
| string rid; |
There was a problem hiding this comment.
If we go by most common condition first, then would the following code be better?
string rid = "debian.8-x64";
if (imageDescriptor.IsArm)
{
rid = "linux-arm";
}
else if (string.Equals(imageDescriptor.OsVariant, "alpine", StringComparison.OrdinalIgnoreCase))
{
rid = "alpine.3.6-x64";
}
return rid;There was a problem hiding this comment.
Leaving as it - I prefer to not initialize variables like this. I prefer the else pattern in general as it allows the compiler warning to catch any code paths that may have mistakenly left the variable undefined.
| { | ||
| VerifySdkImage_RunApp(imageDescriptor, appSdkImage); | ||
| } | ||
|
|
||
| VerifyRuntimeImage_FrameworkDependentApp(imageDescriptor, appSdkImage); | ||
|
|
||
| if (DockerHelper.IsLinuxContainerModeEnabled) | ||
| if (DockerHelper.IsLinuxContainerModeEnabled | ||
| && !string.Equals(imageDescriptor.OsVariant, "alpine", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Consider moving string.Equals(imageDescriptor.OsVariant, "alpine", StringComparison.OrdinalIgnoreCase) to a separate method -
private static bool IsAlpine(ImageDescriptor imageDescriptor)
{
return string.Equals(imageDescriptor.OsVariant, "alpine", StringComparison.OrdinalIgnoreCase))
}
2.1/runtime/alpine/amd64/Dockerfile
Outdated
| ENV DOTNET_DOWNLOAD_SHA 2c7ff8a63c03354b6c3f563f662e8820108be7995b2d72db7d5446ad579977a37db46bae7c5de918b4423a1c37bcfd505df514248f253be1ed2d9a84a982aa98 | ||
|
|
||
| RUN apk add --no-cache --virtual .build-deps \ | ||
| openssl \ |
There was a problem hiding this comment.
Is the openssl really needed? Isn't the libssl1.0 in the runtime-deps image sufficient? Just asking...
There was a problem hiding this comment.
It is needed with wget. Without it wget fails with
wget: can't execute 'ssl_helper': No such file or directory.
I was surprised by this.
There was a problem hiding this comment.
Ok, thank you for the explanation.
|
@janvorli - Can you point me to the issue tracking Alpine support for self-contained apps or should I log one? I would like to track it so that I can enable the unit tests for the runtime-deps image once support is in place. Thanks |
|
@MichaelSimons there is none. I was first wondering what's the actual problem, but then I've remembered that @eerhardt has asked me to revert part of the core-setup change until we have the official corefx / coreclr builds and then I've completely forgotten about it. |
|
@dotnet-bot test Debian - 1. Dockerfiles please |
|
Linking to dotnet/core-setup#3395 which will enable support for self-contained apps. |
| { | ||
| VerifySdkImage_RunApp(imageDescriptor, appSdkImage); | ||
| } | ||
|
|
||
| VerifyRuntimeImage_FrameworkDependentApp(imageDescriptor, appSdkImage); | ||
|
|
||
| if (DockerHelper.IsLinuxContainerModeEnabled) | ||
| if (DockerHelper.IsLinuxContainerModeEnabled && !imageDescriptor.IsAlpine) |
There was a problem hiding this comment.
Do you want to stick a comment here for the tracking bug for self-contained and alpine?
There was a problem hiding this comment.
Ah, I just read the comments - @janvorli was making one...
There was a problem hiding this comment.
If that dotnet/core-setup#3395 gets merged soon I will hold off on merging this PR. If not I will add a comment.
There was a problem hiding this comment.
Merged. I'll kick off an official build to get a new build out sooner.
|
A decision was made to have the Alpine images make use of the Globalization Invariant Mode. The belief is the majority of Docker usage scenarios would prefer this mode. This eliminates the need for icu-lib which will reduce the runtime-deps image by 30.3 MB (also applies to the runtime image). Users can turn this option off and add the icu-lib package if needed. |
|
The Globalization Invariant Mode changes are dependent on https://github.com/dotnet/coreclr/issues/14927. The Docker images need a way to enable the mode globally so that the out of box experience works since icu-lib will not be included. |
| string rid; | ||
| if (imageDescriptor.IsArm) | ||
| { | ||
| rid = "linux-arm"; |
There was a problem hiding this comment.
(This existed before this change)
What is the reasoning for using linux-arm (i.e. a 'portable' RID) for arm architectures, but using debian.8-x64 (i.e. a 'non-portable' RID) for x64? It seems like it would be consistent to always use the portable RID or always use the non-portable RID.
Is it to get more test coverage?
There was a problem hiding this comment.
The reason for it is legacy related - the same tests run over all our image variants (1.* and 2.). Arm images exist only for 2.. 1.* obviously doesn't support 'portable' RIDs.
That being said, there is no reason why we can't add conditional logic to use a 'portable' rid where supported. I will log an issue for this.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, that makes sense to me. Thanks for the explanation.
|
When is this going to be live? |
|
@ThatDeveloper - waiting on the https://github.com/dotnet/coreclr/issues/14927 to make it into Core-Setup and picked up by the CLI. The images won't be available in microsoft/dotnet until 2.1 is released. |
|
@MichaelSimons - FYI that change should be in the latest core-setup build: Microsoft.NETCore.App |
|
@richlander - I think this is ready to merge. Are you good with that? |
|
LGTM We should post (after merge) on our call to action / ask for feedback. I'll talk to you about that. |
Related to dotnet/dotnet-docker#22
Image size on disk (MB):
alpine:3.6 3.97
2.1-runtime-deps-alpine
43.913.82.1-runtime-alpine
11282.5Note: self-contained apps are not yet working e2e therefore the test case for the runtime-deps is disabled.
cc @janvorli, @Petermarcu