lxddoc: Add a readthedoc pre-build hook to build and generate codebase doc #12043

gabrielmougard · 2023-07-19T12:33:55Z

@ru-fu Now lxd-doc can be built from source and executed entirely from the conf.py file which means that we no longer have to use the makefile and it should directly integrates with the readthedoc pipeline.

ru-fu · 2023-07-19T16:44:05Z

I tested it locally now, and I think it works - but we should remove it from the Makefile. I'm actually not sure when the generation happens now. ;)

Also, I tried including our example in the docs now, with the following markup:

```{include} config_options.txt
    :start-after: <!-- config group cluster start -->
    :end-before: <!-- config group cluster end -->
```

This gives an error though, because the - (default value for the second option) must be enclosed in quotes.
This isn't related to this PR, but must also be fixed.

gabrielmougard · 2023-07-19T17:01:30Z

@ru-fu you're right, the lxd-doc tool is called twice now. I'll remove it from the make doc target. But maybe we can keep a separate makefile target to call lxd-doc without generating the readthedoc pages (maybe it could be of some use for "preview" purposes)

ru-fu · 2023-07-20T07:31:53Z

@ru-fu you're right, the lxd-doc tool is called twice now. I'll remove it from the make doc target. But maybe we can keep a separate makefile target to call lxd-doc without generating the readthedoc pages (maybe it could be of some use for "preview" purposes)

Sure, a separate target makes sense. :)

tomponline · 2023-07-20T07:33:23Z

What is causing the lxd-doc tool to be called twice now (for my understanding)?

ru-fu · 2023-07-20T07:39:33Z

What is causing the lxd-doc tool to be called twice now (for my understanding)?

It's in the Makefile as a requirement of the doc build (

lxd/Makefile

Line 129 in fc1c870

doc: lxd-doc doc-setup doc-incremental

), and in addition, the Sphinx config now contains an extension that calls it during the doc build (this PR).

tomponline · 2023-07-20T07:46:23Z

And sphinx gets called as part of make doc?

ru-fu · 2023-07-20T07:58:14Z

And sphinx gets called as part of make doc?

Yes, it's in doc-incremental.
And actually, that's also where the generation happens - the lxd-doc target only builds the tool.

tomponline · 2023-07-20T08:00:31Z

And sphinx gets called as part of make doc?

Yes, it's in doc-incremental. And actually, that's also where the generation happens - the lxd-doc target only builds the tool.

Ah in that case we shouldn't have a lxd-doc target, the lxd-doc tool should be build as part of the normal "make" command target.

ru-fu · 2023-07-20T08:08:01Z

Ah in that case we shouldn't have a lxd-doc target, the lxd-doc tool should be build as part of the normal "make" command target.

I think we should have a target that can be used to run the lxd-doc tool manually. In the future, we'll need this to generate the YAML file that the UI can consume.

But having the tool built as part of the main make command surely makes sense.

tomponline · 2023-07-20T08:10:55Z

Ah in that case we shouldn't have a lxd-doc target, the lxd-doc tool should be build as part of the normal "make" command target.

I think we should have a target that can be used to run the lxd-doc tool manually. In the future, we'll need this to generate the YAML file that the UI can consume.

But having the tool built as part of the main make command surely makes sense.

Yep, happy to have a target that runs the tool, but building should take place in main target so we catch any issues as part of the main build. Ta

ru-fu · 2023-07-20T11:44:31Z

Makefile

@@ -120,18 +114,25 @@ endif

 .PHONY: doc-setup
 doc-setup:
+	@go version > /dev/null 2>&1 || { echo "go is not installed for lxd-doc installation."; exit 1; }


I think @tomponline meant to build the tool as part of the general Go build, not as part of the doc setup.

Also, this here would mean that we build the tool twice when building the docs - once here in doc-setup and once during the actual doc build (conf.py).

We don't need this. But we want to see if the tool build breaks because of some code change, so it makes sense to add the tool build to the general build.

Makefile

ru-fu · 2023-07-21T07:31:40Z

Makefile

@@ -29,7 +29,9 @@ ifeq "$(TAG_SQLITE3)" ""
 	@echo "Missing dqlite, run \"make deps\" to setup."
 	exit 1
 endif
-
+	@go version > /dev/null 2>&1 || { echo "go is not installed for lxd-doc installation."; exit 1; }


I don't know enough about Go to really review this part, but the other steps for this target require Go as well. Do they already have a check whether it's installed? Or if not, should this be made more generic?

You're right, there is already an implicit check (line 10 with GOPATH ?= $(shell go env GOPATH)). I'll remove my useless check.

tomponline · 2023-07-21T14:59:25Z

Makefile

@@ -30,6 +30,8 @@ ifeq "$(TAG_SQLITE3)" ""
 	exit 1
 endif

+	cd lxd/config/generate && CGO_ENABLED=0 go build -o $(GOPATH)/bin/lxd-doc


Can we use the same sort of invocation we do for the other commands, e.g.

CGO_ENABLED=0 go install -v -tags netgo ./lxd-migrate

@gabrielmougard Can you update this?

tomponline · 2023-07-21T14:59:39Z

Makefile

@@ -30,6 +30,8 @@ ifeq "$(TAG_SQLITE3)" ""
 	exit 1
 endif

+	cd lxd/config/generate && CGO_ENABLED=0 go build -o $(GOPATH)/bin/lxd-doc
+	@echo "LXD-DOC built successfully"


no need for this line

tomponline · 2023-07-24T11:18:39Z

@ru-fu please let me know if/when you would like this merged. Thanks

ru-fu · 2023-07-24T12:25:46Z

@ru-fu please let me know if/when you would like this merged. Thanks

I've tested it and it seems to work, so I'm fine with merging it.
However, this PR is based on #12062, so that should probably go in first.
And there's some open comments from you on the Makefile in this PR here, which should probably be addressed.

tomponline · 2023-07-24T12:27:32Z

@gabrielmougard needs a rebase now.

ru-fu

Thanks!

tomponline · 2023-08-01T07:57:43Z

Needs a rebase

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

github-actions bot added the Documentation Documentation needs updating label Jul 19, 2023

gabrielmougard marked this pull request as draft July 19, 2023 12:37

gabrielmougard force-pushed the feat/readthedoc branch 3 times, most recently from f6b3996 to 7a06337 Compare July 19, 2023 12:58

gabrielmougard marked this pull request as ready for review July 19, 2023 13:01

gabrielmougard force-pushed the feat/readthedoc branch 2 times, most recently from 97be63d to b7f8755 Compare July 19, 2023 14:05

tomponline requested a review from ru-fu July 19, 2023 14:07

gabrielmougard force-pushed the feat/readthedoc branch 2 times, most recently from bb03740 to 221b98b Compare July 19, 2023 14:18

gabrielmougard force-pushed the feat/readthedoc branch 2 times, most recently from d56d3ef to dfa89fc Compare July 20, 2023 08:44

ru-fu reviewed Jul 20, 2023

View reviewed changes

gabrielmougard force-pushed the feat/readthedoc branch from dfa89fc to 569089b Compare July 21, 2023 07:00

gabrielmougard requested a review from ru-fu July 21, 2023 07:00

ru-fu reviewed Jul 21, 2023

View reviewed changes

gabrielmougard force-pushed the feat/readthedoc branch from 569089b to a1eeb81 Compare July 21, 2023 09:01

tomponline reviewed Jul 21, 2023

View reviewed changes

gabrielmougard force-pushed the feat/readthedoc branch from a1eeb81 to f752719 Compare July 23, 2023 15:37

ru-fu mentioned this pull request Jul 24, 2023

lxd-doc improvements #12062

Merged

gabrielmougard force-pushed the feat/readthedoc branch from f752719 to 415552e Compare July 24, 2023 14:11

tomponline changed the title ~~doc: Add a readthedoc pre-build hook to build and generate codebase doc~~ lxddoc: Add a readthedoc pre-build hook to build and generate codebase doc Jul 24, 2023

gabrielmougard force-pushed the feat/readthedoc branch 2 times, most recently from 317691d to def8ca2 Compare July 31, 2023 09:42

ru-fu approved these changes Jul 31, 2023

View reviewed changes

gabrielmougard force-pushed the feat/readthedoc branch from def8ca2 to 3230df9 Compare July 31, 2023 14:18

gabrielmougard force-pushed the feat/readthedoc branch from 3230df9 to 109c2b9 Compare August 1, 2023 14:52

gabrielmougard mentioned this pull request Aug 1, 2023

lxddoc: add option comments #12100

Merged

gabrielmougard force-pushed the feat/readthedoc branch 3 times, most recently from 4453888 to 9dd5bd3 Compare August 1, 2023 18:06

gabrielmougard added 2 commits August 1, 2023 20:10

doc: Add a readthedoc pre-build hook to build and generate codebase doc

8ae2a2e

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

makefile: don't use the lxd-doc target in the doc target

51d8706

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

gabrielmougard force-pushed the feat/readthedoc branch from 9dd5bd3 to 51d8706 Compare August 1, 2023 18:11

tomponline approved these changes Aug 21, 2023

View reviewed changes

tomponline merged commit e6c1e94 into canonical:main Aug 21, 2023
24 of 25 checks passed

tomponline mentioned this pull request Aug 25, 2023

lxd: Add generated configuration options to API #12101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lxddoc: Add a readthedoc pre-build hook to build and generate codebase doc #12043

lxddoc: Add a readthedoc pre-build hook to build and generate codebase doc #12043

gabrielmougard commented Jul 19, 2023 •

edited

Loading

ru-fu commented Jul 19, 2023

gabrielmougard commented Jul 19, 2023

ru-fu commented Jul 20, 2023

tomponline commented Jul 20, 2023

ru-fu commented Jul 20, 2023

tomponline commented Jul 20, 2023

ru-fu commented Jul 20, 2023

tomponline commented Jul 20, 2023

ru-fu commented Jul 20, 2023

tomponline commented Jul 20, 2023

ru-fu Jul 20, 2023

ru-fu Jul 20, 2023

ru-fu Jul 21, 2023

gabrielmougard Jul 21, 2023

tomponline Jul 21, 2023

ru-fu Jul 31, 2023

tomponline Jul 21, 2023

tomponline commented Jul 24, 2023

ru-fu commented Jul 24, 2023

tomponline commented Jul 24, 2023

ru-fu left a comment

tomponline commented Aug 1, 2023

lxddoc: Add a readthedoc pre-build hook to build and generate codebase doc #12043

lxddoc: Add a readthedoc pre-build hook to build and generate codebase doc #12043

Conversation

gabrielmougard commented Jul 19, 2023 • edited Loading

ru-fu commented Jul 19, 2023

gabrielmougard commented Jul 19, 2023

ru-fu commented Jul 20, 2023

tomponline commented Jul 20, 2023

ru-fu commented Jul 20, 2023

tomponline commented Jul 20, 2023

ru-fu commented Jul 20, 2023

tomponline commented Jul 20, 2023

ru-fu commented Jul 20, 2023

tomponline commented Jul 20, 2023

ru-fu Jul 20, 2023

Choose a reason for hiding this comment

ru-fu Jul 20, 2023

Choose a reason for hiding this comment

ru-fu Jul 21, 2023

Choose a reason for hiding this comment

gabrielmougard Jul 21, 2023

Choose a reason for hiding this comment

tomponline Jul 21, 2023

Choose a reason for hiding this comment

ru-fu Jul 31, 2023

Choose a reason for hiding this comment

tomponline Jul 21, 2023

Choose a reason for hiding this comment

tomponline commented Jul 24, 2023

ru-fu commented Jul 24, 2023

tomponline commented Jul 24, 2023

ru-fu left a comment

Choose a reason for hiding this comment

tomponline commented Aug 1, 2023

gabrielmougard commented Jul 19, 2023 •

edited

Loading