Skip to content
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

Merged
merged 4 commits into from
Dec 10, 2024
Merged

Add Alpine 3.21 Dockerfile #1284

merged 4 commits into from
Dec 10, 2024

Conversation

mthalman
Copy link
Member

@mthalman mthalman commented Dec 6, 2024

Related to dotnet/core#9577

I didn't continue the pattern of defining both alpine/3.21/amd64/Dockerfile and alpine/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

&& rm -f /tmp/powershell.tar.gz

# Install azurecli from PIP
RUN azureEnv="/usr/local/share/azure-cli-env" && \
Copy link
Member

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.

Copy link
Member Author

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" ]
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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

@richlander
Copy link
Member

richlander commented Dec 8, 2024

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.

#1276 (comment)

@mthalman
Copy link
Member Author

mthalman commented Dec 9, 2024

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.

@richlander
Copy link
Member

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.

@mthalman
Copy link
Member Author

When you say "validation", does that involve building the code or just testing it?

It's source building the VMR on Alpine and running tests in that environment with the built output.

And why does Alpine have a "WithNode" variant and the other distros do not?

I don't have the historical context of that. It was introduced a long time ago: #66.

My goal is to (A) ensure we understand the model, and (B) simplify it.

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.

@richlander
Copy link
Member

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 WithNode image references. They will definitely pop up.

@mthalman
Copy link
Member Author

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.

@richlander
Copy link
Member

That's fine. Let's start by getting rid of the naming scheme.

@MichaelSimons
Copy link
Member

Interestingly Fedora had them removed by @MichaelSimons in #1226, but I don't think that was intentional.

It was intentional as aspnet just added them for source-build. Cleaning this up wholesale and eliminating the special naming is good.

@mthalman mthalman changed the title Add Alpine 3.21 Node Dockerfile Add Alpine 3.21 Dockerfile Dec 10, 2024
@mthalman mthalman merged commit 62395ff into dotnet:main Dec 10, 2024
10 checks passed
@mthalman mthalman deleted the alpine3.21 branch December 10, 2024 19:19
@ellahathaway
Copy link
Member

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

@MichaelSimons
Copy link
Member

Having node is a requirement for containerizing jobs using non-glibc-based containers: Container jobs in YAML - Azure Pipelines | Microsoft Learn

That is reason enough to add it back IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants