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

Multi-Architecture for Containers Documentation #5174

Closed
wants to merge 7 commits into from

Conversation

KanishkT123
Copy link

Adding documentation changes for the multi-architecture update here: https://github.com/microsoft/vscode-remote-containers/pull/187

@@ -11,6 +11,26 @@ DateApproved: 12/17/2021

Given the growing number of use cases for dev containers, there is a companion `devcontainer` command line interface (CLI) that can be used independent of the [Remote - Containers extension](https://marketplace.visualstudio.com/items?itemName=ms-vscode-remote.remote-containers) or GitHub Codespaces. This article will walk you through its installation and how to use it in different scenarios.

## Table of Contents
Copy link

@gregvanl gregvanl Feb 11, 2022

Choose a reason for hiding this comment

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

Hi @KanishkT123 We don't typically add a table of contents to the beginning of the VS Code topics but instead rely on the auto-generated right navigation, which displays the H2 headers.

image

If you think a section needs to be elevated to an H2 for discoverability, we can change the header level. For example, your new section "Building Multi-Architecture builds" may make sense as an H2.
I also think "Adding automation" and "devcontainer CLI build options" are discrete enough sections to be at their own H2 level.

Copy link
Author

Choose a reason for hiding this comment

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

Makes perfect sense, I've made the changes you suggested. How should we track the WIP-ness of this PR? Would you like me to ping you when the WIP feature merges into the Remote repository?

Choose a reason for hiding this comment

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

We would time the merge and publishing to when the extension is finally released. You can ping me when the feature is merged and I can coordinate with the extension publisher

@gregvanl
Copy link

Added don't merge label as this PR documents a WIP feature

Changes as per reviewer comments
Copy link

@bderusha bderusha left a comment

Choose a reason for hiding this comment

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

A few requested changes, but overall this reads really well. Seems like a great way to get started and I wish I had it for my current feature ;)

docs/remote/devcontainer-cli.md Outdated Show resolved Hide resolved
docs/remote/devcontainer-cli.md Outdated Show resolved Hide resolved
docs/remote/devcontainer-cli.md Outdated Show resolved Hide resolved
docs/remote/devcontainer-cli.md Outdated Show resolved Hide resolved
docs/remote/devcontainer-cli.md Outdated Show resolved Hide resolved
Changes per reviewer comments.
Copy link

@bderusha bderusha left a comment

Choose a reason for hiding this comment

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

@KanishkT123 One minor fix to add. Preapproving. The rest looks great. Thanks for the solid work!


1. `devcontainer build`: As before, this invokes the devcontainer CLI build command.
2. `--platform linux/amd64,linux/arm64`: This creates Docker images for the `linux/amd64` and `linux/arm64` architectures. Find other supported platforms [here](https://docs.docker.com/engine/install/) (all Docker supported platforms are supported) and [here](https://github.com/docker/buildx/blob/master/docs/reference/buildx_build.md#platform) on the Docker Buildx documentation page.
4. `--image-name hubusername/multiarch-test:v1`: **This is mandatory when using `--platform` and optional otherwise.** This will tag the image and needs to follow container registry naming conventions to upload.

Choose a reason for hiding this comment

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

Suggested change
4. `--image-name hubusername/multiarch-test:v1`: **This is mandatory when using `--platform` and optional otherwise.** This will tag the image and needs to follow container registry naming conventions to upload.
3. `--image-name hubusername/multiarch-test:v1`: **This is mandatory when using `--platform` and optional otherwise.** This will tag the image and needs to follow container registry naming conventions to upload.


By default, the Devcontainer CLI **will push** to the container registry when multi-platform builds are specified. To run a multi-paltform build for local development purposes, append `--no-push` to the command as shown below:

```bash

Choose a reason for hiding this comment

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

overall LGTM -- I did notice something appears to be affecting the '3-backtick-formatting' in this "No Push" section, since I see the 3-backtick chars displayed, even with the text fully "MD rendered" ?

image

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Seems good for Multi-Architecture. Nothing else to say, but it's having don't merge label, but I'll still approve it as it seems fine.

@gregvanl
Copy link

Closing this PR as the devcontainer CLI topic will shortly be updated to reflect the new devcontainer CLI.
Now that devcontainers/cli#24 is merged, a new PR could be created once the /docs/remote/devcontainer-cli.md is updated.

@gregvanl gregvanl closed this Jul 19, 2022
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.

4 participants