Skip to content

Conversation

@ChrisChinchilla
Copy link
Contributor

@ChrisChinchilla ChrisChinchilla commented Apr 6, 2023

- What I did

Added a link to --networking flag as requested in this issue docker/docs#16890

Made it an "external" link for better usage in multiple locations.

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2023

Codecov Report

Merging #4166 (64e9cad) into master (21682eb) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4166   +/-   ##
=======================================
  Coverage   59.64%   59.64%           
=======================================
  Files         287      287           
  Lines       24771    24771           
=======================================
  Hits        14774    14774           
  Misses       9111     9111           
  Partials      886      886           

Copy link
Member

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

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

LGTM

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.

left a comment (I can catch up with you if needed)

| `-m`, `--memory` | `bytes` | `0` | Memory limit |
| `--memory-swap` | `bytes` | `0` | Swap limit equal to memory plus swap: -1 to enable unlimited swap |
| `--network` | `string` | `default` | Set the networking mode for the RUN instructions during build |
| `--network` | `string` | `default` | Set [the networking mode](https://docs.docker.com/network/) for the RUN instructions during build |
Copy link
Member

Choose a reason for hiding this comment

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

This table is actually generated from the source code (I thought we had a verify step for this one to check it, but looks like it doesn't flag this 🤔). The table in markdown is only used on GitHub (and generated from the YAML properties on docs.docker.com).

What I think we need here instead is to either add a section for this option further down (having the right anchor tag to match the flag name), and add the link for details in there, or perhaps we need to add a link to the buildx build documentation as alternative link (if that page does have a description).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, there don't actually appear to be many sections for more explanation in this doc. Is there any reason we can't add links to code comments? Or we'd just rather not?

@thaJeztah
Copy link
Member

Looks like buildx build also doesn't have a flag description for this, so we probably need to add one on this page (or both, if there's differences between BuildKit's support for --network and non-buildkit (see moby/moby#40379))

@crazy-max do you know from the top of your head if BuildKit now has full feature-parity on --network ?

@ChrisChinchilla
Copy link
Contributor Author

@thaJeztah and @crazy-max Turns out my last change was linking people to the wrong place anyway and digging more into this it seems that --network is basically the only non-experimental flag with the build/buildx disparity. So I think this latest change is correct now

@thaJeztah thaJeztah modified the milestones: 24.0.0, 25.0.0 May 5, 2023
Co-authored-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Chris Chinchilla <[email protected]>
@thaJeztah thaJeztah force-pushed the chrisward/network-context branch from aba7d62 to 64e9cad Compare December 20, 2023 23:31
@thaJeztah
Copy link
Member

Rebased, and squashed the commits

@thaJeztah thaJeztah self-assigned this Dec 20, 2023
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 if CI is happy

@thaJeztah thaJeztah merged commit dd3ba73 into docker:master Dec 20, 2023
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.

5 participants