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

desktop: refresh containerd examples #17106

Merged
merged 2 commits into from
Apr 27, 2023
Merged

Conversation

dvdksn
Copy link
Contributor

@dvdksn dvdksn commented Apr 13, 2023

Signed-off-by: David Karlsson david.karlsson@docker.com

Proposed changes

This refreshes the containerd examples.

This removes some docker info and docker run examples, which IMO don't add a lot of useful information.

I also rewrote the section no multi-platform builds in an attempt to improve it.

Related issues (optional)

This update was actually triggered by the fact that we should remove the Registry: field from docker info.

See: docker/cli#4204

@netlify
Copy link

netlify bot commented Apr 13, 2023

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 43de7a9
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/64463e2546b86e0008ea6419
😎 Deploy Preview https://deploy-preview-17106--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@dvdksn
Copy link
Contributor Author

dvdksn commented Apr 13, 2023

@thaJeztah @rumpl ptal

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

👍 on removing some of that content (agreed that it's not very useful).

Left some comments / thoughts on what to show on some of the output

desktop/containerd/index.md Outdated Show resolved Hide resolved
desktop/containerd/index.md Outdated Show resolved Hide resolved
desktop/containerd/index.md Outdated Show resolved Hide resolved
Signed-off-by: David Karlsson <david.karlsson@docker.com>
Signed-off-by: David Karlsson <david.karlsson@docker.com>

Docker Engine already uses containerd for container lifecycle management, which
includes creating, starting, and stopping containers. This page describes the
next step of containerd integration for Docker Engine: the image store.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can remove the whole "image store" wording and say : image management?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better to keep the term "image store" for now, since it also appears in the Docker Desktop settings

image

$ docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
93b4d60dfd08 nginx "/docker-entrypoint.…" 3 seconds ago Up 3 seconds 0.0.0.0:8080->80/tcp stoic_mccarthy
$ docker buildx create --bootstrap
Copy link
Member

Choose a reason for hiding this comment

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

Wondering now if we need to show the docker buildx create step at all; would the error-message that's shown above be enough to illustrate what doesn't work without the integration enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking to keep it, for those who might be familiar with multi-platform builds using the docker-container driver, to illustrate the limitation with --load

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha. Yeah, was wondering if users would potentially run the example (and it's only there to illustrate "this is no longer needed")

@dvdksn
Copy link
Contributor Author

dvdksn commented Apr 26, 2023

We good with this @rumpl @thaJeztah ?

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@dvdksn @rumpl we should also clean up the "known issues" section (for old Docker Desktop versions), and where needed add new known issues (still to be fixed)

@dvdksn
Copy link
Contributor Author

dvdksn commented Apr 27, 2023

yes, I have a feeling we have more to say in the limitations section.

@dvdksn dvdksn merged commit bda1d65 into docker:main Apr 27, 2023
8 checks passed
@dvdksn dvdksn deleted the desktop/refresh-c8d branch May 3, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants