-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add Alpine 3.21 Dockerfile #1284
Conversation
&& rm -f /tmp/powershell.tar.gz | ||
|
||
# Install azurecli from PIP | ||
RUN azureEnv="/usr/local/share/azure-cli-env" && \ |
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.
Can you make this Dockerfile consistent with respect to the '&&` formatting? The previous section uses a prefix pattern where this section uses a suffix pattern. I have been trying to standardize around the prefix pattern.
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.
Fixed in 01d0447
ENV NO_UPDATE_NOTIFIER=true | ||
|
||
# Set node as a default command | ||
CMD [ "node" ] |
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 requires this? It feels strange to have this set for a general build container.
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.
@wtgodbe - Does your infrastructure require this?
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.
Yes, historically we've relied on it
I'd like to understand why this non-helix Dockerfile is needed at all. As I mentioned on the Debian 12 PR, we need to understand why anyone needs non-Azure Linux builder images. |
It's used by source build for validation of Alpine environment: https://github.com/dotnet/sdk/blob/3f26c6e94d9bc3ee87b88bc81c0606f619579ec4/eng/pipelines/templates/variables/vmr-build.yml#L17-L18 As for the other repos, I can't say for sure. It's used by sdk, aspnetcore, diagnostics, and installer. |
When you say "validation", does that involve building the code or just testing it? And why does Alpine have a "WithNode" variant and the other distros do not? My goal is to (A) ensure we understand the model, and (B) simplify it. |
It's source building the VMR on Alpine and running tests in that environment with the built output.
I don't have the historical context of that. It was introduced a long time ago: #66.
I'm all for that. Maybe we just work with the assumption that existing usages outside of source build should be using Helix for testing or Azure Linux for building. So then we make this a source build-tagged image. |
I propose that we switch this PR to be the same as the other images you need, like the Ubuntu one, with the same naming scheme and the same components. I assume you are not using it with Azure DevOps so no node is needed. We can then figure out what the use case is for repos with the |
Node was initially added to images used by source build here: #949 That requirement has since gone away but that may end up being temporary as we will likely need a way to build Node sources in the future. Most of the images used by source build still have Node in them. Interestingly Fedora had them removed by @MichaelSimons in #1226, but I don't think that was intentional. |
That's fine. Let's start by getting rid of the naming scheme. |
It was intentional as aspnet just added them for source-build. Cleaning this up wholesale and eliminating the special naming is good. |
Having node is a requirement for containerizing jobs using non-glibc-based containers: Container jobs in YAML - Azure Pipelines | Microsoft Learn Removing node blocks my ability to move forward with dotnet/source-build#4547 |
That is reason enough to add it back IMO. |
Related to dotnet/core#9577
I didn't continue the pattern of defining both
alpine/3.21/amd64/Dockerfile
andalpine/3.21/withnode/amd64/Dockerfile
as has been done for previous Alpine versions, because the non-Node Dockerfile is no longer being used by any builds. So I've consolidated it into a single Dockerfile.This also only defines an architecture-specific tag. Previous Alpine versions had a
alpine-<version>-withnode
but that doesn't follow the new tagging guidelines. So it no longer provides that tag for backwards compatibility. Related: https://github.com/dotnet/dotnet-buildtools-prereqs-docker/pull/1262/files#diff-0983c53ccd3b14ab97c1b4d7dec524efc89e51c56ebc90d96ce1b8ba5d4fa100