Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.51%. Comparing base (91d097e) to head (f0dcb42).
Report is 33 commits behind head on master.

@@           Coverage Diff           @@
##           master    #4228   +/-   ##
=======================================
  Coverage   59.51%   59.51%           
=======================================
  Files         346      346           
  Lines       29379    29379           
=======================================
  Hits        17486    17486           
  Misses      10923    10923           
  Partials      970      970           

I'm confused why the doc building validation is failing with Go compilation errors:

go: cannot find main module, but -modfile was set.
	-modfile cannot be used to set the module root directory.

On my machine, with this patch both make manpages and make -f docker.Makefile manpages work without issues. Same goes for yamldocs and mddocs. I'm also using go1.20.3.

It needs a temporary go.mod, otherwise -modfile=vendor.mod won't work.

But we need to look at this change; I know we split these to separate modules to not add the tool's dependencies as dependencies for the main module / vendor, and because we needed these separate modules for our migration to Hugo for the docs website. ./cc @crazy-max @dvdksn

I don't think my change had any impact in this respect - it was only moving the doc generation to a subdirectory so it wouldn't confuse Hugo when fetching content files from this repo.

The cli-docs-tool module needs to be an external dep since it's used by other projects as well. I don't suppose there's much harm in vendoring them here -- other than 23k lines of bloat.

It needs a temporary go.mod, otherwise -modfile=vendor.mod won't work.

Ah. I can add the temporary copying logic for these builds if it's necessary.

I know we split these to separate modules to not add the tool's dependencies as dependencies for the main module / vendor

Unfortunately that's exactly what I'm trying to revert -- removing this from vendoring means that we cannot build Docker's man pages in openSUSE's OBS (there is no internet access in OBS when building packages).

The other alternative would be to include the generated of the man pages in the source tree (verifying they are kept up-to-date in the CI), so distributions can just copy them without needing to use a tool to build them during packaging.

@thaJeztah I've fixed the go.mod issue, it all works now. 😸

Rebased.

@thaJeztah Did you want me to go the route of making the documentation be pre-built and then verified in CI? I think packaging things this way is better for distributions (it Just Works ™️ with the distributed tarballs) and the bloat here is only to the tarballs not the final binary, but if it is an issue I can go the other route. Rebasing this patchset for every Docker patch release is getting a little tiring. 😅

Gentle ping @thaJeztah @crazy-max 🙇

(having the script that generates the manpages also be responsible for downloading and building the tools necessary is definitely problematic 😭)

If we want to avoid having the tools in the "main" package dependencies/vendoring, perhaps we could have a separate tools directory with a vendor.mod that includes these as a separate vendoring tree? Or within that "manpage generation" code itself (further isolating it in the directory tree if necessary)?

Ah, yes, I tried doing a rebase, and some updates in #4903 some time ago, but now that the cli-docs-tool also should be able to do the man-page generating (without building an go-md2man binary), we may be able to get rid of that part; but I didn't get round to that yet 😓

I think @crazy-max is on PTO this week, but maybe @krissetto or @Benehiko (also on PTO this week 🙈) are interested to have a peek.

I've been rebasing this for the SUSE packages, I can push a new version here if you like.

I've rebased it again (this bump was a bit more complicated than previous ones, fwiw). I didn't remove the build of the go-md2man binary, but I can take a look at that if that's what's needed to get this merged so I can stop rebasing it 😿

Hm.. so @tianon's comment made me try if this works well with an existing go.mod, and it looks like it has some issues there.

⚠️ this is not the "standard" situation, but while we're working on (finally!) getting our repos to have an actual go.mod, a trick I (and others use) is to create symlinks;

ls -la go.*
lrwxr-xr-x 1 root root 10 Jan 10 21:44 go.mod -> vendor.mod
lrwxr-xr-x 1 root root 10 Jan 10 21:44 go.sum -> vendor.sum

With the above symlinks present, I ran make mdddocs (tried both inside the make dev dev-container and on my host), and it looks like it doesn't handle that gracefully;

make mddocs
scripts/docs/generate-md.sh
go: inconsistent vendoring in /go/src/github.com/docker/cli:
	dario.cat/[email protected]: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod
	github.com/Azure/[email protected]: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod
...

After the above, I had modifications in my git, and it looks like it overwrote vendor.mod, emptying it (which would explain the error above);

git status
On branch docs-no-internet
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   vendor.mod

diff --git a/vendor.mod b/vendor.mod
index 3badee9b8..a8ea3c017 100644
--- a/vendor.mod
+++ b/vendor.mod
@@ -1,109 +1,3 @@
 module github.com/docker/cli

-// 'vendor.mod' enables use of 'go mod vendor' to managed 'vendor/' directory.
-// There is no 'go.mod' file, as that would imply opting in for all the rules
-// around SemVer, which this repo cannot abide by as it uses CalVer.
-
 go 1.22.0
-
-require (
-	dario.cat/mergo v1.0.1
-	github.com/containerd/platforms v0.2.1
-	github.com/creack/pty v1.1.21
-	github.com/distribution/reference v0.6.0
-	github.com/docker/cli-docs-tool v0.8.0

It also removed the go.mod symlink, but kept the go.sum one;

ls -la go.*
lrwxr-xr-x 1 root root 10 Jan 10 21:44 go.sum -> vendor.sum

Curious if we could use something like the script that's in moby/moby, which seemed to detect the "already have a go.mod" situation; it will produce a warning ("you shouldn't do this!" - I know), but otherwise handle it gracefully;
https://github.com/moby/moby/blob/d34a5f5d72ace96c2fbd2a7484ee4a9ddfa36e42/hack/with-go-mod.sh

Thanks for updating! ❤️

Overall, this looks good; I tried it in a "standard" situation (no go.mod / go.mod symlinks), and it all works well. It would be good if it could handle the "existing go.mod" either gracefully (like moby; warn, but continue), or hard failure ("can't have a go.mod present).