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

docs: remove unnecessary -itd from examples #5214

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dvdksn
Copy link
Contributor

@dvdksn dvdksn commented Jul 1, 2024

- What I did

The -itd options, probably used to keep the tty container running in the
background, is not necessary for these examples. Updated the examples to
remove the unnecessary flags. Also, changed the example image from
busybox to nginx.

Relates to:

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

The -itd options, probably used to keep the tty container running in the
background, is not necessary for these examples. Updated the examples to
remove the unnecessary flags. Also, changed the example image from
busybox to nginx.

Signed-off-by: David Karlsson <[email protected]>
@dvdksn dvdksn requested review from thaJeztah and a team as code owners July 1, 2024 11:49
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.43%. Comparing base (4270341) to head (a80c7c4).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5214   +/-   ##
=======================================
  Coverage   61.43%   61.43%           
=======================================
  Files         298      298           
  Lines       20799    20799           
=======================================
  Hits        12777    12777           
  Misses       7109     7109           
  Partials      913      913           

$ docker run -itd --network=my-net busybox
$ docker run --network=my-net nginx
Copy link
Member

Choose a reason for hiding this comment

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

we can probably use the nginx:alpine image here (a bit smaller); or even the httpd server built-into to busybox 😂

I'm a bit on the fence on running the container in the foreground if we use the nginx (or nginx:alpine) container, as it produces a lot of output that may not be relevant to the user, or was the intent to run it with -d / --detach ?

If the concern is for the container to stick around, at least we could add --rm, but also wondering if there would be some example we can run that makes it clear from the docker run that "it worked"? I don't think anything inside the container would allow seeing what networks we're attached to (@robmry ?) but I guess we could still have the container running in the background, and then docker inspect or something to see it's connected to the network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes in terms of terminal output, the -d helps reduce the verbosity. But in my mind, the purpose of these examples is to demonstrate the command syntax: the commands and flags you would use to achieve the thing that the documentation is discussing. For that purpose, any additional flags that aren't necessary can be confusing. So for that reason I prefer to remove those flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same goes for nginx vs nginx:alpine. Yes if you run these examples, then it incurs additional size. But for reading, nginx is less for the reader to parse.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think anything inside the container would allow seeing what networks we're attached to

Agreed - there's no way for the container itself to tell which networks it's connected to.

To prove that it's actually connected to a network, network inspect or container inspect would do it. Or, could run a second container on the same network to fetch from the http server. But, that's probably not really the point in this command-ref.

(Without testing the http server though, is there any advantage in using nginx over busybox? Maybe a new user would try a curl http://localhost to see if it worked - which it wouldn't without a -p 80:80.)

Copy link
Member

Choose a reason for hiding this comment

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

If we don't expect the example to be executed, we can just as well stick with busybox. If, on the other hand, we want the example to show something, then we should look at making the example do something that shows that it did something.

If running the example doesn't do anything, then we can just as well remove them, and just show --network=<name of your network. The network must pre-exist (see docker network create).

Copy link
Member

Choose a reason for hiding this comment

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

(Without testing the http server though, is there any advantage in using nginx over busybox? Maybe a new user would try a curl http://localhost to see if it worked - which it wouldn't without a -p 80:80.)

Yeah, that can become a sliding slope; that's why I was looking if we can provide the most minimal to illustrate 'it worked'.

For the custom network example, we'd quickly end up with;

  • create network
  • start container with some http server on the network
  • start a second container attached to the same network
  • make network-connection from the second container

And, now we'd end up (re)writing the networking manual / section :grimace:

Copy link
Contributor

Choose a reason for hiding this comment

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

Right - showing it worked / how to use it probably belongs on tutorial page, rather than in this command-ref.

But I think the concrete examples are worthwhile, some of the later ones are a bit involved like ...

docker run --network=name=my-net,\"driver-opt=com.docker.network.endpoint.sysctls=net.ipv4.conf.IFNAME.log_martians=1,net.ipv4.conf.IFNAME.forwarding=0\",ip=192.0.2.42 nginx

... so, the equivalent of --network <name of your network> might not get the point across very well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To prove that it's actually connected to a network, network inspect or container inspect would do it. Or, could run a second container on the same network to fetch from the http server. But, that's probably not really the point in this command-ref.

Yeah I think it's out of scope for the reference doc, this is why I changed it. In its current form, we don't include information about how to inspect the network. We only give them half the solution (here you go, a container running in the background) but we don't tell them why that's useful. Experienced users can figure it out, but to someone new it's just a bit cryptic.

For the custom network example, we'd quickly end up with;

  • create network
  • start container with some http server on the network
  • start a second container attached to the same network
  • make network-connection from the second container

We have many sections in the reference documentation that read like this, unfortunately. It's very useful, but it also means that this content is not exactly a "reference" document, but more of an explanation.

However, currently these command examples are neither, or both. We just need to pick a strategy and go for it.

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.

4 participants