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

Changing the structure of the section #15164

Merged
merged 9 commits into from
Jul 27, 2022

Conversation

dockertopia
Copy link
Contributor

Proposed changes

Changes as previously discussed in #14910 (comment)

Related issues (optional)

#14910

@dockertopia dockertopia requested a review from jedevc July 19, 2022 16:19
@dockertopia dockertopia changed the title Changing structure of the section Changing the structure of the section Jul 19, 2022
@netlify
Copy link

netlify bot commented Jul 19, 2022

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 0c69399
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/62e166e24dc3fe0009c699dd
😎 Deploy Preview https://deploy-preview-15164--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.

@jedevc
Copy link
Contributor

jedevc commented Jul 20, 2022

Personally, I would rather we keep the pages together. If we measure pages in screen heights, then for me, then it works out to only around 1-1.5 screens, so I have to click between them to navigate instead of scrolling. I also think most of this information makes sense together in a broad overview - when we dive into the details of how each of these individual parts work, I think we should definitely split then, but maybe not here?

_data/toc.yaml Outdated Show resolved Hide resolved
@dockertopia
Copy link
Contributor Author

Personally, I would rather we keep the pages together. If we measure pages in screen heights, then for me, then it works out to only around 1-1.5 screens, so I have to click between them to navigate instead of scrolling. I also think most of this information makes sense together in a broad overview - when we dive into the details of how each of these individual parts work, I think we should definitely split then, but maybe not here?

We're going to have more content for these pages, and we can get useful analytics if they are sitting in different pages. I honestly think this improves discoverability of content even if we need tweak the placement of some sections. Let's discuss in our next sync.

@dockertopia
Copy link
Contributor Author

As discussed in sync last week.

@dockertopia dockertopia marked this pull request as ready for review July 25, 2022 17:18
@dockertopia dockertopia requested a review from jedevc July 25, 2022 17:18
description: Working with Docker Buildx
keywords: build, buildx, buildkit
keywords: build, buildx, BuildKit
Copy link
Member

Choose a reason for hiding this comment

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

keywords should be lowercased to me (cc @usha-mandya @thaJeztah)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this was a suggestion by the IDE. Makes sense to have a standard. Just out of curiosity, does it need to be lowercase, i.e., otherwise something is broken?

Copy link
Member

@thaJeztah thaJeztah Jul 26, 2022

Choose a reason for hiding this comment

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

Good question; I think either way may work, as long as we're consistently using the same.

Looks like the search auto-complete uses them case-insensitive, but it may be that they're used different elsewhere;

Screenshot 2022-07-26 at 11 04 03

"elsewhere" could be as part of an URL and/or anchor, in which case it's possible that they're treated as "different tags". I guess "lowercase" would be the safest option for these 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Ah indeed there is the auto-complete search but I was thinking about search engines and the meta name="keywords" that is case-insensitive. See also #14642 (comment) about keywords meta tag abuse. So might only be relevant for our auto-complete.

Copy link
Contributor Author

@dockertopia dockertopia Jul 26, 2022

Choose a reason for hiding this comment

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

So, in your previous example all of them are like Docker, BuildKit, etc.
image

Comment: I agree that making them all lowercase allows less room for creating multiple like (docker, Docker, dockerfile, Dockerfile) in terms of metadata, in terms of the presentation (they are visible) they make more sense with case. It would be great to decouple that. As in, write them in suitable case (rendering) and them being always interpretated in a case-insensitive way(metadata).

Question for @usha-mandya do we then change all keywords to be lowercase in our docs sets?

Copy link
Member

Choose a reason for hiding this comment

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

@dockertopia Agree on having a standard for keywords. Suggest we use lowercase going forward.

We can create a ticket in the backlog to retrospectively update all keywords to be lowercase. It's not a high priority as the search results are case-insensitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @usha-mandya. Sounds good.

build/buildx/multiarch-images.md Outdated Show resolved Hide resolved
build/buildx/multiple-builders.md Outdated Show resolved Hide resolved
_data/toc.yaml Outdated Show resolved Hide resolved
build/buildx/multiarch-images.md Outdated Show resolved Hide resolved
---
title: Building multi-platform images
description: Different strategies for building multiplatform images
keywords: build, buildx, BuildKit, multiplatform images
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
keywords: build, buildx, BuildKit, multiplatform images
keywords: build, buildx, buildkit, multi-platform images

_data/toc.yaml Outdated
title: Working with Buildx
- path: /build/buildx/multiple-builders/
title: Using multiple builders
- path: /build/buildx/multiarch-images/
Copy link
Member

Choose a reason for hiding this comment

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

We should be consistent:

Suggested change
- path: /build/buildx/multiarch-images/
- path: /build/buildx/multi-platform-images/

Needs to rename multiarch-images.md to multi-platform-images.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true. It's called "multi-arch" in Desktop docs, and in conversation we do say multi-arch all the time.
I think most users would equate multi-platform with macOs/Linux/Windows.
And architectures is clear that it refers to HW such as arm, mips, ppc64le, and even s390x.

Also, taking for example this sentence in the original content:
"When you invoke a build, you can set the --platform flag to specify the target platform for the build output, (for example, linux/amd64, linux/arm64, or darwin/amd64)" or this command example "docker buildx build --platform linux/amd64,linux/arm64 "?
We also say "An experimental package is available for arch-based distributions." in Engine doc set.

Just trying to point out that it seems to be a combination of both. Platform is not enough to determine the arch (following the above interpretation): imho multi-architecture being more specific is a better candidate.

Copy link
Member

@crazy-max crazy-max Jul 26, 2022

Choose a reason for hiding this comment

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

Hum right, WDYT @tonistiigi @jedevc @thaJeztah?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed this inconsistency as well, agreed it would be good to clarify this, but maybe out of scope for this PR, there's a lot of build docs that mix-and-match between the two, so I'd propose to pick something here that works, and then discuss separately later.

Personally, I think multi-platform is better when talking about building images. A couple reasons:

  • This is what the OCI spec refers to - the CPU architecture is part of the platform (the OS is the other main component).
  • Buildkit will eventually (hopefully) support building windows images. Say we have an amd64 linux image and a amd64 windows image - in this case, it's not as accurate to talk about multi-arch images, even though the underlying mechanism is the same. Multi-platform is a nice catch-all.
  • The buildx/buildkit flags are --platform - calling it multi-arch seems like it can be confusing, multi-platform seems more inline with existing behavior.

Maybe there are still use-cases for talking about multi-arch though, for example, maybe around QEMU, when we're using linux throughout, but emulating different CPU architectures.

An experimental package is available for arch-based distributions

I think this is probably unrelated, I think this refers to ArchLinux, not multi-architecture/platform support.

Copy link
Contributor Author

@dockertopia dockertopia Jul 26, 2022

Choose a reason for hiding this comment

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

Agreed on the Engine example. I revisited it and @jedevc is right, but a simple docs search renders this
image
image
( some are community hits)
And searching multi-platform results in a very different search results list.

It seems for the discussion there's the case for both in content, when platform is the same and we do want to distinguish architectures, but that we'd like to introduce the feature as multi-platform as it's seen as capturing both.

But it leaves the case of harmonizing the remaining docs content.

_data/toc.yaml Outdated
- path: /build/buildx/install/
title: Install Buildx

- path: /build/buildx/
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about the order, looks confusing to me to have the parent page after a subpage.

As I can see with compose it's the first page: https://docs.docker.com/compose/ so we should be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we discussed I wasn't aware you saw this whole page as a "Build Overview", sure let's rename that page and reorder the pages, then, so 1) Buildx Overview, 2) Install Buildx for consistency.

Copy link
Member

@crazy-max crazy-max Jul 26, 2022

Choose a reason for hiding this comment

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

Maybe Overview and Install is enough as it's already subpages of Buildx section. Don't think we need to repeat ourselves?

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 see what you mean, that there's hierarchy providing context. But when we need to derive it from the folder context and there's multiple in the doc set, I think it's best for any "just" 'overview' or 'install' to be the ones for the main section (so doc set root index or install) for clear disambiguation in a glance.

Copy link
Contributor Author

@dockertopia dockertopia left a comment

Choose a reason for hiding this comment

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

Left a couple of comments and committed line break removals and lowercased keywords.

description: Working with Docker Buildx
keywords: build, buildx, buildkit
keywords: build, buildx, BuildKit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this was a suggestion by the IDE. Makes sense to have a standard. Just out of curiosity, does it need to be lowercase, i.e., otherwise something is broken?

_data/toc.yaml Outdated
- path: /build/buildx/install/
title: Install Buildx

- path: /build/buildx/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we discussed I wasn't aware you saw this whole page as a "Build Overview", sure let's rename that page and reorder the pages, then, so 1) Buildx Overview, 2) Install Buildx for consistency.

_data/toc.yaml Outdated
title: Working with Buildx
- path: /build/buildx/multiple-builders/
title: Using multiple builders
- path: /build/buildx/multiarch-images/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true. It's called "multi-arch" in Desktop docs, and in conversation we do say multi-arch all the time.
I think most users would equate multi-platform with macOs/Linux/Windows.
And architectures is clear that it refers to HW such as arm, mips, ppc64le, and even s390x.

Also, taking for example this sentence in the original content:
"When you invoke a build, you can set the --platform flag to specify the target platform for the build output, (for example, linux/amd64, linux/arm64, or darwin/amd64)" or this command example "docker buildx build --platform linux/amd64,linux/arm64 "?
We also say "An experimental package is available for arch-based distributions." in Engine doc set.

Just trying to point out that it seems to be a combination of both. Platform is not enough to determine the arch (following the above interpretation): imho multi-architecture being more specific is a better candidate.


- path: /build/buildx/multiple-builders/
title: Using multiple builders
- path: /build/buildx/multiplatform-images/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- path: /build/buildx/multiplatform-images/
- path: /build/buildx/multi-platform-images/


* **Creating build-once, run-anywhere with multi-architecture builds**
Collaborate across platforms with one build artifact.
See [Build multi-platform images](buildx/multiplatform-images.md).
Copy link
Member

@crazy-max crazy-max Jul 27, 2022

Choose a reason for hiding this comment

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

Suggested change
See [Build multi-platform images](buildx/multiplatform-images.md).
See [Build multi-platform images](buildx/multi-platform-images.md).

(needs to also change filename)

Copy link
Contributor Author

@dockertopia dockertopia Jul 27, 2022

Choose a reason for hiding this comment

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

I'm keeping the filename as two words for simplicity in xrefs. I think you'll find it's OK to not reflect the preferred spelling in the name for the file.
We can revisit this later both in terms of style guide (preferred spelling for hyphenated words so it's applied across doc sets) or simple shared guidelines for naming doc source files (even if just recording anything that's been implicit so far).
The idea is whatever points of standardization coming up to be recorded and elevated so there's alignment across teams/doc sets and everyone on the docs teams has an opportunity to be clear on the matter. Again, the is not to dismiss the comments but to address them out of the scope of the PR.

Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

As discussed let's decide what do to for multiplatform vs multiarch vs multi-platform wording and fix references about this in our docs in a follow-up.

@dockertopia dockertopia merged commit 511c0a0 into docker:master Jul 27, 2022
@dockertopia dockertopia deleted the buildx-newia2 branch September 1, 2022 11:06
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.

5 participants