-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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: editorial improvements, typo fixes #4712
Conversation
@thaJeztah let me see if I can carry a couple follow-ups from the previous refresh as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looking good; left some comments/suggestions, and recalled some "related" changes that were never finished.
> TLS version 1.0 and higher is supported. Protocols SSLv3 and below are not | ||
> supported for security reasons. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remind me; we need to verify what TLS versions we support now (it's very likely we no longer support 1.0)
@@ -687,7 +685,7 @@ To set the DNS search domain for all Docker containers, use: | |||
$ sudo dockerd --dns-search example.com | |||
``` | |||
|
|||
### Allow push of nondistributable artifacts | |||
### Allow push of non-distributable artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remind me; we need to check the status on this one. I know the "foreign layers" feature was deprecated by the OCI, and we may be enabling this by default now (or are planning to)
- config (`config=<name or id>`) | ||
- container (`container=<name or id>`) | ||
- daemon (`daemon=<name or id>`) | ||
- event (`event=<event action>`) | ||
- image (`image=<repository or tag>`) | ||
- label (`label=<key>` or `label=<key>=<value>`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminds me I had a PR to rewrite this to use tables, but some of the combinations in my examples were wrong, as combining some did not produce the same as --type
🤔 (IIRC); perhaps you're interested in carrying that PR though;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me carry this one, but I'll do it in a follow-up as not to block this one since there's an open technical question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to merge the events
and system_events
pages. Currently information is spread between them (and they may be out of sync).
I recall doing that as part of my "WIP use canonical URLs" but that got bit-rot.
ae456a8
to
08f3aff
Compare
@thaJeztah I pushed an additional update as a fixup (to make it easier to review), see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM (thanks!)
found a couple of minor issues, and one or two that may need checking (sudo
and env-var, and perhaps update that redirected URL)
`tar` UNIX format and can be compressed with any one of the 'xz', 'bzip2', | ||
'gzip' or 'identity' (no compression) formats. | ||
`tar` Unix format and can be compressed with any one of the `xz`, `bzip2`, | ||
`gzip` or `identity` (no compression) formats. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think zstd
may now also work, but we'd have to check if it does (so no need to change for this PR)
@@ -274,7 +272,7 @@ $ docker build - < context.tar.gz | |||
``` | |||
|
|||
This will build an image for a compressed context read from `STDIN`. Supported | |||
formats are: bzip2, gzip and xz. | |||
formats are: `bzip2`, `gzip` and `xz`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here (also for follow-ups)
$ export DOCKER_TMPDIR=/mnt/disk2/tmp | ||
$ /usr/local/bin/dockerd --data-root /var/lib/docker -H unix:// > /var/lib/docker-machine/docker.log 2>&1 | ||
$ sudo dockerd --data-root /var/lib/docker -H unix:// > /var/lib/docker-machine/docker.log 2>&1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh; does this work? sudo
may be omitting the DOCKER_TMPDIR
env-var (either requires sudo -E
, or we should inline the env-var instead of exporting.
Also wondering if we should leave the > /var/lib/docker-machine/docker.log 2>&1
as an exercise to the reader (managing detached processes can be fiddly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't even know what the redirect is doing in this case so I am happy to drop it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2b252a8
to
db7c7ec
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4712 +/- ##
==========================================
- Coverage 59.68% 59.65% -0.03%
==========================================
Files 287 285 -2
Lines 24865 24757 -108
==========================================
- Hits 14841 14770 -71
+ Misses 9138 9100 -38
- Partials 886 887 +1 |
Signed-off-by: David Karlsson <[email protected]>
db7c7ec
to
6fd4cff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: David Karlsson [email protected]
q4 freshness updates
carries followups:
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)