-
Notifications
You must be signed in to change notification settings - Fork 2k
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 more instructions #4413
Add more instructions #4413
Conversation
|
||
In addition, Docker [Buildkit exposes multiple environment variables](https://github.com/dotnet/dotnet-docker/pull/4387#issuecomment-1416565213) that can be used to further conditionalize behavior. These environment variables can be controlled with the pattern demonstrated in [Dockerfile](https://github.com/mthalman/dredge/blob/main/src/Valleysoft.Dredge/Dockerfile). As mentioned, .NET doesn't support being run in emulation. The pattern in that Dockerfile results in the SDK always being run natively while the final image is affected by the `--platform` switch. This model also has the best performance since the bulk of computation is run natively. | ||
|
||
Note: We are enabling the following model in a future release: https://github.com/dotnet/dotnet-docker/issues/4388#issuecomment-1421401384. |
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.
Why not make these changes as part of this PR? I think it'd be more appropriate to reference the Dockerfile in this repo rather than the Dredge Dockerfile.
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.
I plan to switch that PR to use the new format and then block on merging it until it is ready.
Co-authored-by: Matt Thalman <mthalman@microsoft.com>
samples/enable-globalization.md
Outdated
docker run --rm -it -e TZ=$(cat /etc/timezone) app | ||
``` | ||
|
||
This approach enables a container image to be launched matching the host, as opposed to setting the information as part of `docker build`. |
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.
enables a container image to be launched matching the host
Is that a common practice? In the world of cloud computing, what time zone the host is in seems irrelevant. I did a quick search and couldn't find any examples that use the host time zone. All are either hardcoding it or have it set to an environment variable that can be set dynamically. That approach seems better to me.
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.
See this PR: #4414
What do you think @Clockwork-Muse?
COPY aspnetapp/. . | ||
RUN dotnet publish --use-current-runtime -c Release --self-contained false --no-restore -o /app | ||
|
||
# To enable globalization support, see |
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.
The wording To enable globalization
is a bit confusing because globalization is already enabled. Maybe something like To read more about globalization support, see...
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.
Good call.
samples/build-for-a-platform.md
Outdated
|
||
## Dockerfiles that build everywhere | ||
|
||
The default scenario is using multi-arch tags, which will work in multiple environments. |
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.
Will the reader understand the multi-arch
nomenclature here? The earlier link referred to these are multi-platform images.
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.
Yup.
samples/build-for-a-platform.md
Outdated
docker build -t app . | ||
``` | ||
|
||
It can be built on any supported operating system and architecture. For example, if built on an Apple M1 machine, Docker will produce a `linux/arm64` image, while on a Windows x64 machine, it will produce a `linux/amd64` image. |
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.
while on a Windows x64 machine, it will produce a
linux/amd64 image
Not if your windows machine is configured for Windows containers :)
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. I was being lazy.
samples/build-for-a-platform.md
Outdated
|
||
Docker Desktop uses [QEMU](https://www.qemu.org/) for emulation, for example running x64 code on an Arm64 machine. [.NET doesn't support being run in QEMU](https://github.com/dotnet/core/blob/main/release-notes/7.0/supported-os.md#qemu). That means that the SDK needs to always be run natively, to enable [multi-stage build](https://docs.docker.com/build/building/multi-stage/). Multi-stage build is used by all of our samples. | ||
|
||
As a result, we need a reliable pattern that enables produces multiple variants of images on one machine, but that doesn't use emulation. |
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.
Broken wording - that enables produces multiple variants
Co-authored-by: Michael Simons <msimons@microsoft.com>
I believe I have addressed all feedback. |
samples/enable-globalization.md
Outdated
|
||
### Alpine | ||
|
||
Install `tzdata`: |
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.
This section is formatted differently than the previous ICU section.
The previous section provided a Dockerfile snippet - e.g. a RUN instruction in a Dockerfile code block. It also didn't have an Install xxx
prefix.
samples/build-for-a-platform.md
Outdated
For Linux containers on an x64 machine: | ||
|
||
```bash | ||
$ docker inspect app | jq .[0].Architecture |
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.
The docker CLI will do this for you and the nice thing is the command is platform agnostic.
docker inspect app -f "{{ .Architecture }}"
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.
Thanks!
I submit to being a jq
addict.
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.
How do you like translating that into PowerShell 🥲
Co-authored-by: Michael Simons <msimons@microsoft.com>
I believe I have now addressed the feedback. Great feedback, BTW. |
Please review @tarekgh