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

Update samples to Alpine #4171

Merged
merged 11 commits into from
Oct 31, 2022

Conversation

ashnaga
Copy link
Member

@ashnaga ashnaga commented Oct 26, 2022

Fixes #2167

@dnfadmin
Copy link

dnfadmin commented Oct 26, 2022

CLA assistant check
All CLA requirements met.

@ashnaga ashnaga marked this pull request as draft October 26, 2022 20:58
@ashnaga ashnaga marked this pull request as ready for review October 26, 2022 20:58
@mthalman mthalman self-requested a review October 26, 2022 21:02
@mthalman
Copy link
Member

Because the tag names have changed, you'll also need to update this file to reference those new names: https://github.com/dotnet/dotnet-docker/blob/main/eng/mcr-tags-metadata-templates/samples-tags.yml. After updating that file, run .\eng\readme-templates\Get-GeneratedReadmes.ps1 to update the readme to reflect the new tags.

FROM mcr.microsoft.com/dotnet/runtime-deps:7.0-alpine
WORKDIR /app
COPY --from=build /app .
ENTRYPOINT ["./aspnetapp"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a newline at the end of the file. Applies to the other Dockerfile too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ashnaga
Copy link
Member Author

ashnaga commented Oct 26, 2022

Because the tag names have changed, you'll also need to update this file to reference those new names: https://github.com/dotnet/dotnet-docker/blob/main/eng/mcr-tags-metadata-templates/samples-tags.yml. After updating that file, run .\eng\readme-templates\Get-GeneratedReadmes.ps1 to update the readme to reflect the new tags.

Done

@ashnaga ashnaga closed this Oct 26, 2022
@ashnaga ashnaga reopened this Oct 26, 2022
@mthalman mthalman changed the base branch from main to nightly October 27, 2022 18:27
@mthalman
Copy link
Member

FYI: I rebased this to nightly so we can get it merged there first. Then we'll merge it to main and get the new images published as part of the November release.

@mthalman
Copy link
Member

I'm investigating an issue with the tests where they're not testing what we expect to be tested. This was apparently an issue prior to these changes but I just discovered it today.

@mthalman
Copy link
Member

I have fixed the tests locally but that's uncovered a product issue which I've logged at dotnet/sdk#28809.

@mthalman
Copy link
Member

I have fixed the tests locally but that's uncovered a product issue which I've logged at dotnet/sdk#28809.

Looks like this won't be fixed in GA. Our choices:

  1. Hold off on making these changes until it is fixed (hopefully 7.0.1)
  2. Get rid of the Dockerfile currently defined in this PR and replace it with Dockerfile.alpine-arm64-slim and Dockerfile.alpine-arm32-slim (we already have Dockerfile.alpine-x64-slim). Then when it is fixed we can add the multi-arch Dockerfile.alpine-slim file and probably delete those two Arm-based Dockerfiles.

@ashnaga
Copy link
Member Author

ashnaga commented Oct 28, 2022

2. Get rid of the Dockerfile currently defined in this PR and replace it with Dockerfile.alpine-arm64-slim and Dockerfile.alpine-arm32-slim (we already have Dockerfile.alpine-x64-slim). Then when it is fixed we can add the multi-arch Dockerfile.alpine-slim file and probably delete those two Arm-based Dockerfiles.

We should pick the option 2 as the goal is to have reduced size samples (using arch specific Dockerfiles) for better customer experience. But I am not sure on the benefit of deleting and adding the multi-arch file later.

@mthalman
Copy link
Member

We should pick the option 2 as the goal is to have reduced size samples (using arch specific Dockerfiles) for better customer experience.

Option 1 gives us the reduced size as well, just a month later.

But I am not sure on the benefit of deleting and adding the multi-arch file later.

The multi-arch file is the canonical and more maintainable solution that we would want to guide customers to use.

@MichaelSimons
Copy link
Member

I am in favor of #2. There will be marketing buzz at 7.0.0 GA that we should take advantage of by making our published samples quicker to pull therefore leaving a more favorable impression.

@mthalman mthalman merged commit fb07c27 into dotnet:nightly Oct 31, 2022
mthalman pushed a commit to mthalman/dotnet-docker that referenced this pull request Nov 7, 2022
mthalman pushed a commit to mthalman/dotnet-docker that referenced this pull request Nov 16, 2022
@ashnaga ashnaga deleted the ashitanagar/docker-sample-alpine branch January 4, 2023 00:52
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.

Consider using Alpine for .NET Samples
6 participants