-
Notifications
You must be signed in to change notification settings - Fork 12
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
Man support #14
Man support #14
Conversation
Use the generate.sh script instead of md2man directly. Update Dockerfile for generating man pages. Signed-off-by: Daniel Nephin <dnephin@docker.com>
Also consolidate the leftover packages under cli. Remove pkg/mflag. Make manpage generation work with new cobra layout. Remove remaining mflag and fix tests after rebase with master. Signed-off-by: Daniel Nephin <dnephin@docker.com>
Using gomvpkg -from github.com/docker/docker/api/client -to github.com/docker/docker/cli/command -vcs_mv_cmd 'git mv {{.Src}} {{.Dst}}' Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Remove the global variable used. Allows easier unit testing. Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Refactor content_trust cli/flags handling
Previously our man pages included the current time each time they were generated. This causes an issue for reproducible builds, since each re-build of a package that includes the man pages will have different times listed in the man pages. To fix this, add support for SOURCE_DATE_EPOCH (which is a standardised packaging environment variable, designed to be used specifically for this purpose[1]). spf13/cobra doesn't support this natively yet (though I will push a patch for that as well), but it's simpler to fix it directly in docker/cli. [1]: https://reproducible-builds.org/specs/source-date-epoch/ Signed-off-by: Aleksa Sarai <asarai@suse.de>
… this removes a whole lot of dependencies from people depending on docker/cli… Signed-off-by: Vincent Demeester <vincent@sbr.pm>
man: obey SOURCE_DATE_EPOCH when generating man pages
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
…truction Introduce functional arguments to NewDockerCli for a more stable API.
This was set in our manually written markdowns, but not in the man pages generated through Cobra. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
man-pages: fix missing manual title in heading
07bf100
to
9d3997c
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
==========================================
+ Coverage 67.36% 70.58% +3.22%
==========================================
Files 4 5 +1
Lines 576 459 -117
==========================================
- Hits 388 324 -64
+ Misses 132 77 -55
- Partials 56 58 +2 ☔ View full report in Codecov by Sentry. |
@@ -59,6 +60,12 @@ func gen(opts *options) error { | |||
SourceDir: opts.source, | |||
TargetDir: opts.target, | |||
Plugin: true, | |||
ManHeader: &doc.GenManHeader{ | |||
Title: "BUILDX", | |||
Section: "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.
Wondering if we will have cases where not all man-pages must be in the same section https://man7.org/linux/man-pages/man7/man-pages.7.html
e.g. for the cli, we also currently have Dockerfile (5)
and dockerd (8)
. I think in most/all cases a binary would only be in a single section, it's possible that, e.g. a man page describing things like --config
could also be in section 5 ("config")
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.
Yes indeed we should take care of it. Will make some changes related to that.
func (c *Client) loadLongDescription(parentCmd *cobra.Command, generator string) error { | ||
for _, cmd := range parentCmd.Commands() { | ||
if cmd.HasSubCommands() { | ||
if err := c.loadLongDescription(cmd, generator); err != nil { |
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.
Is the intent to use the same markdown files for man pages and online docs?
I know we had some discussions about that in the past, and while this would be "great!", it can be somewhat difficult (and I wasn't able to come up with a one-size-fits-all solution). Conventions in man pages can be quite different from conventions in online docs. For example, the online docs would refer to another command using something like "for details on other command
, refer to the <link>other command reference documentation</link>
", whereas in a man page it would be "see other-command(1)
".
For the cli, that lead us to maintain two separate sets of markdown files (https://github.com/docker/cli/tree/3fb4fb83dfb5db0c0753a8316f21aea54dab32c5/man), but that situation is also kinda horrible, as they're not as well maintained, and often not as up-to-date as the equivalent "online docs" man pages.
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.
Hum yeah it will be hard to be generic with the library. I will think about it and keep you in touch.
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 wonder if https://github.com/muesli/mango could solve this issue. I will take a look.
😬 looks like this needs a rebase now |
couple of tests failing, @crazy-max |
6d55ae3
to
d8d8f3e
Compare
@thaJeztah rebased |
d8d8f3e
to
732dcce
Compare
732dcce
to
ebc2fad
Compare
example/go.mod
Outdated
@@ -27,6 +27,7 @@ require ( | |||
github.com/containerd/continuity v0.2.2 // indirect | |||
github.com/containerd/ttrpc v1.1.0 // indirect | |||
github.com/containerd/typeurl v1.0.2 // indirect | |||
github.com/cpuguy83/go-md2man/v2 v2.0.1 // indirect |
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.
Ah let me bump to latest stable 2.0.3
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.
Actually this is an indirect dependency, should we bump cobra @thaJeztah ?
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.
Hm.. good one. Honestly, I think all our CLI's are already on "current" versions of Cobra, so perhaps it's "fine".
OTOH, we don't need "latest" Cobra for this tool, but we DO (likely) want to have the latest go-md2man
because we know it has fixes in man-page generating.
So I would be fine starting with updating the dependency here (even if it's indirect); for the "paranoid", you could add a comment, e.g. (// indirect // updated to latest, which has some bug fixes for man-pages
)
Perhaps update to v1.6.0 or v1.7.0? v1.6.0 removes the old YAML v2 dependency, and we should update mousetrap to v1.1.0 to remove some legacy code.
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.
Looks like the oldest branch still maintained in docker/cli (23.0) is at v1.6.1, so that could work without disrupting anything; https://github.com/docker/cli/blob/23.0/vendor.mod#L35
clidocstool_test.go
Outdated
//tmpdir, err := os.MkdirTemp("", "clidocstool") | ||
//require.NoError(t, err) | ||
//t.Log(tmpdir) |
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.
Looks like some debugging code you forgot to remove
.SH DESCRIPTION | ||
.PP | ||
Stop builder instance |
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.
Do we need a fixture / example with a long description (taken from markdown)? (I think that's implemented in this PR as well, correct?)
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.
Added "attach" and "buildx dial-stdio" that contains long description for testing
ebc2fad
to
bce3e1e
Compare
Rebase time! 😂 ❤️ |
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
bce3e1e
to
4ecc2f2
Compare
@thaJeztah Ok should be good now 😇 |
@@ -10,7 +10,9 @@ require ( | |||
) | |||
|
|||
require ( | |||
github.com/cpuguy83/go-md2man/v2 v2.0.2 // indirect |
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.
Did we still want to push this up a nudge, to v2.0.3? https://github.com/cpuguy83/go-md2man/releases/tag/v2.0.3
I should also check with @cpuguy83 if he's planning to do v2.0.4, as some fixes were merged; cpuguy83/go-md2man@v2.0.3...c2c0656
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.
Can we do this as follow-up? We can still vendor cli-docs-tool main branch in the meantime
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.
Ah, yes, sure; just recalled we were discussing it last week or so 😂
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
Add man page support with import of
man/generate.go
with history fromdocker/cli
.