Skip to content

generate reference docs with cli-docs-tool #62

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

Merged
merged 4 commits into from
May 21, 2025
Merged

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented May 15, 2025

Generate reference docs similar to other CLI plugins (buildx, compose, scout, ...):

  • Docs generation is sandboxed using a Dockerfile
  • Can be tested with make docs
  • To validate docs you can use Bake: docker buildx bake validate-docs
  • A GHA workflow has been added to validate docs as well

As follow-up docs team can:

  • Start adding examples for each command and flag.
  • Create hugo module on the docs website to vendor model-cli with reference docs.

cc @ArthurFlag

Keeping in draft for now until I meet with Arthur tomorrow

github.com/docker/docker v28.0.1+incompatible
github.com/docker/go-connections v0.5.0
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is not related, seems go mod is not tidy on main. Also don't see any kind of validation for go.mod and vendor in CI.

@@ -0,0 +1,49 @@
# syntax=docker/dockerfile:1

ARG GO_VERSION=1.24
Copy link
Member Author

@crazy-max crazy-max May 15, 2025

Choose a reason for hiding this comment

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

I use Go latest stable for generating docs but I don't see any Go version set on this repo.

Seems when model-cli is released on CI it uses whatever version is shipped in the runner atm:

- uses: actions/setup-go@v5
with:
go-version-file: go.mod
cache: true

This should be fixed imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. We'll find some spare cycles to address the build stuff in 4.43.

@crazy-max crazy-max marked this pull request as ready for review May 16, 2025 08:31
Signed-off-by: CrazyMax <[email protected]>
push:
branches:
- 'main'
- 'v[0-9]*'
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what branching strategy is in place as I don't see any release branch but just in case, I match it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The tags are all coming off of main for now (and we're generally backporting those changes to recent releases via modules). We're planning a proper release branch structure starting with 4.43.

@ArthurFlag
Copy link
Contributor

@crazy-max I don't seem to be able to merge the docs update into your branch. This might be something only you can do.

@crazy-max
Copy link
Member Author

@crazy-max I don't seem to be able to merge the docs update into your branch. This might be something only you can do.

It should be as follow-up when this one is merged and then you could pick your commits from https://github.com/crazy-max/docker-model-cli/pull/1/commits and open a new PR on this repo. Better to keep separation of concerns with this PR generating the docs and yours for docs copy.

Copy link
Collaborator

@doringeman doringeman left a comment

Choose a reason for hiding this comment

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

I remember I saw this before in another repo.
Will these be used by docker/docs?

@crazy-max
Copy link
Member Author

I remember I saw this before in another repo. Will these be used by docker/docs?

Yes the go module will be vendored on docs repo similar to buildx, compose cli plugins.

push:
branches:
- 'main'
- 'v[0-9]*'
Copy link
Contributor

Choose a reason for hiding this comment

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

The tags are all coming off of main for now (and we're generally backporting those changes to recent releases via modules). We're planning a proper release branch structure starting with 4.43.

@@ -0,0 +1,49 @@
# syntax=docker/dockerfile:1

ARG GO_VERSION=1.24
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. We'll find some spare cycles to address the build stuff in 4.43.

- docker_model_tag.yaml
- docker_model_uninstall-runner.yaml
- docker_model_version.yaml
options:
Copy link
Contributor

Choose a reason for hiding this comment

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

These top-level options are only added to the Model CLI's root command if not running as a CLI plugin (i.e. if invoking docker-model directly). We mostly do this for standalone debugging. We should probably exclude them from the docs, because in official usage as a CLI plugin (i.e. docker model) they'd be options on the top-level docker command (e.g. docker --context mycloudengine model status).

Copy link
Member Author

@crazy-max crazy-max May 19, 2025

Choose a reason for hiding this comment

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

If that's the case they should be marked as hidden similar to https://github.com/docker/buildx/blob/57a1c97c9d9bc2e610364e9da4948c492d4254fe/commands/install.go#L50

Although version should be kept imo

Can you list what commands should be hidden?

@@ -0,0 +1,26 @@
command: docker model compose
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like docker_model_compose.yaml files have been excluded from the cname/clink sections in docker_model.yaml. Does that mean they'll be hidden in the generated output? If so, that's perfect, I think we definitely want to exclude the hidden Compose provider commands. (Same question for markdown, but it looks like they're excluded there too 👍 ?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes seems root compose command is hidden:

c.Hidden = true

$ docker model --help
Usage:  docker model COMMAND

Docker Model Runner

Commands:
  inspect     Display detailed information on one model
  list        List the available models that can be run with the Docker Model Runner
  logs        Fetch the Docker Model Runner logs
  pull        Download a model
  push        Upload a model
  rm          Remove models downloaded from Docker Hub
  run         Run a model with the Docker Model Runner
  status      Check if the Docker Model Runner is running
  tag         Tag a model
  version     Show the Docker Model Runner version

Run 'docker model COMMAND --help' for more information on a command.

But doesn't look inherited:

$ docker model compose --help
Usage:  docker model compose [OPTIONS] COMMAND



Options:
      --project-name string   compose project name

Commands:
  down
  up

Run 'docker model compose COMMAND --help' for more information on a command.

Because Hidden is set after subcommands are added:

c := &cobra.Command{
Use: "compose EVENT",
}
c.AddCommand(newUpCommand())
c.AddCommand(newDownCommand())
c.Hidden = true

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok seems cli-docs-tool doesn't handle inheritance for hidden commands so subcommands docs is still generated. I marked all subcommands as hidden in the meantime.

@thaJeztah Wonder if we should handle this, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Hm... good one; so ISTR we had some discussion around hidden commands (and to generate things regardless, leaving it to the docs.docker.com repository to decide whether to publish docs for them?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@crazy-max crazy-max May 19, 2025

Choose a reason for hiding this comment

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

Updated to cli-docs-tool v0.10.0 to fix this issue

Comment on lines 29 to 38
| `--config` | `string` | `/root/.docker` | Location of client config files |
| `-c`, `--context` | `string` | | Name of the context to use to connect to the daemon (overrides DOCKER_HOST env var and default context set with "docker context use") |
| `-D`, `--debug` | `bool` | | Enable debug mode |
| `-H`, `--host` | `list` | | Daemon socket to connect to |
| `-l`, `--log-level` | `string` | `info` | Set the logging level ("debug", "info", "warn", "error", "fatal") |
| `--tls` | `bool` | | Use TLS; implied by --tlsverify |
| `--tlscacert` | `string` | `/root/.docker/ca.pem` | Trust certs signed only by this CA |
| `--tlscert` | `string` | `/root/.docker/cert.pem` | Path to TLS certificate file |
| `--tlskey` | `string` | `/root/.docker/key.pem` | Path to TLS key file |
| `--tlsverify` | `bool` | | Use TLS and verify the remote |
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also exclude all of these top-level docker flags. If the generation is invoked via docker model instead of docker-model, do they disappear?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes docker global flags are registered in standalone mode:

// Initialize client options and register their flags if running in
// standalone mode.
if plugin.RunningStandalone() {
globalOptions = flags.NewClientOptions()
globalOptions.InstallFlags(rootCmd.Flags())
}

I will look if we can omit them for our docs

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be good now

Copy link
Member

Choose a reason for hiding this comment

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

@crazy-max do we want the yaml docs to be separate from the markdown ones? (or even; could we generate them dynamically as we did for buildx?).

Mostly considering that these unlikely are useful for readers on GitHub (and the reverse; docs.docker.com doesn't use the markdown ones directly).

Copy link
Member Author

@crazy-max crazy-max May 19, 2025

Choose a reason for hiding this comment

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

I preferred to keep yaml generation in this repo so it's easier to vendor changes on docs repo. So if we want to vendor specific commit for the module we don't need extra work for docs team. Same as compose repo.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha; ISTR for compose this caused some complication with other markdown files that still had to be used? Wondering if the YAML should be in a separate directory.

Not a blocker though; mostly something we should probably look at to separate these 🤔

Copy link
Member Author

@crazy-max crazy-max May 19, 2025

Choose a reason for hiding this comment

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

I think I would do the same on buildx repo at some point. Otherwise people need to first clone plugin project, run make docs, copy over generated yaml files in docs repo and update vendor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's a bit why I was considering that as well, because docs only uses the YAML files? But because they're in the same directory would now pull in both; was wondering if putting it separate would allow only pulling in the yaml 🤔 (although possibly it only looks as the module as a whole, and all markdown in all paths?)

@crazy-max
Copy link
Member Author

crazy-max commented May 21, 2025

@doringeman @xenoscopic Are we good to merge this so Docs team can start working on examples? Thanks!

@kiview
Copy link
Member

kiview commented May 21, 2025

Synced with @xenoscopic, good to merge 👍

Copy link
Contributor

@xenoscopic xenoscopic left a comment

Choose a reason for hiding this comment

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

LGTM!

@kiview kiview merged commit 0ef53e8 into docker:main May 21, 2025
1 check passed
This was referenced May 21, 2025
ArthurFlag pushed a commit to ArthurFlag/model-cli that referenced this pull request May 21, 2025
* fix go.mod

Signed-off-by: CrazyMax <[email protected]>

* generate reference docs with cli-docs-tool

Signed-off-by: CrazyMax <[email protected]>

* mark command as experimental

Signed-off-by: CrazyMax <[email protected]>

* docs: don't register global docker flags

Signed-off-by: CrazyMax <[email protected]>

---------

Signed-off-by: CrazyMax <[email protected]>
@crazy-max crazy-max deleted the ref-docs branch May 22, 2025 12:44
ArthurFlag added a commit to docker/docs that referenced this pull request Jun 2, 2025
## Description

Adds model-cli reference docs.

## Related issues or tickets

* docker/model-cli#62
* docker/model-cli#69

## Reviews

<!-- Notes for reviewers here -->
<!-- List applicable reviews (optionally @tag reviewers) -->

- [ ] Technical review
- [ ] Editorial review
- [ ] Product review

---------

Signed-off-by: CrazyMax <[email protected]>
Co-authored-by: ArthurFlag <[email protected]>
sarahsanders-docker pushed a commit to sarahsanders-docker/docs that referenced this pull request Jun 4, 2025
## Description

Adds model-cli reference docs.

## Related issues or tickets

* docker/model-cli#62
* docker/model-cli#69

## Reviews

<!-- Notes for reviewers here -->
<!-- List applicable reviews (optionally @tag reviewers) -->

- [ ] Technical review
- [ ] Editorial review
- [ ] Product review

---------

Signed-off-by: CrazyMax <[email protected]>
Co-authored-by: ArthurFlag <[email protected]>
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.

6 participants