lxddoc: a go-swagger like documentation tool

[gabrielmougard]

No description provided.

[lxc-jenkins]

Documentation preview available at: https://linuxcontainers.org/lxd/docs/pr.11652/

[monstermunchkin]

I just did a quick review as this is still a draft, and I have a few notes:

• Is there any reason we aren't using cobra/cli here? Is it to reduce dependencies?
• Please replace interface{} with any as that's a lot nicer to read

[gabrielmougard]

I just did a quick review as this is still a draft, and I have a few notes:

• Is there any reason we aren't using cobra/cli here? Is it to reduce dependencies?
• Please replace interface{} with any as that's a lot nicer to read

@monstermunchkin cobra seemed like a big dependency for such a simple tool (only one flag for now). Regarding the interface{} thing, sure, I can change that.

[gabrielmougard]

@ru-fu for now, the generated .txt file is unique and will contain all the doc. Also, the file name is hardcoded. Ideally, where would you like it to be generated and how would you integrate it with the rest of the sphinx documentation ?

[gabrielmougard]

@ru-fu shall I introduce an other optional metadata key called docfile for example (alongside the existing group and key ones) to specify in which .txt file we want this piece of doc to be generated ? Maybe having a .txt doc file per group will be easier to reference in sphinx no ?

[monstermunchkin]

cobra seemed like a big dependency for such a simple tool (only one flag for now).

I don't have too strong of an opinion about this, but I believe we should be consistent. Both lxc-to-lxd and lxd-migrate use cobra as well (and have no flags).

[gabrielmougard]

cobra seemed like a big dependency for such a simple tool (only one flag for now).

I don't have too strong of an opinion about this, but I believe we should be consistent. Both lxc-to-lxd and lxd-migrate use cobra as well (and have no flags).

ok I understand. I'll change that then.

[ru-fu]

A bit of nitpicking and a few questions and clarifications.
I have only reviewed the docs though, not the code.

[ru-fu]

@ru-fu for now, the generated .txt file is unique and will contain all the doc. Also, the file name is hardcoded. Ideally, where would you like it to be generated and how would you integrate it with the rest of the sphinx documentation ?
@ru-fu shall I introduce an other optional metadata key called docfile for example (alongside the existing group and key ones) to specify in which .txt file we want this piece of doc to be generated ? Maybe having a .txt doc file per group will be easier to reference in sphinx no ?

I think we should have one file for all generated output - we can then include the content in different places using "start-after" and "end-before" tags.
The file should be generated in the doc/ folder, and it should have a name and ending that make it clear that this is a generated file and not a normal Markdown file. Maybe config_options.gen (or is there some convention for generated files)?

Having a docfile option would give us more flexibility, but I think for now we don't need it.

[gabrielmougard]

@ru-fu for now, the generated .txt file is unique and will contain all the doc. Also, the file name is hardcoded. Ideally, where would you like it to be generated and how would you integrate it with the rest of the sphinx documentation ?
@ru-fu shall I introduce an other optional metadata key called docfile for example (alongside the existing group and key ones) to specify in which .txt file we want this piece of doc to be generated ? Maybe having a .txt doc file per group will be easier to reference in sphinx no ?

I think we should have one file for all generated output - we can then include the content in different places using "start-after" and "end-before" tags. The file should be generated in the doc/ folder, and it should have a name and ending that make it clear that this is a generated file and not a normal Markdown file. Maybe config_options.gen (or is there some convention for generated files)?

Having a docfile option would give us more flexibility, but I think for now we don't need it.

thanks !

[gabrielmougard]

be generated in the doc/ folder, and it should ha

@ru-fu usually, in Swagger, there is always a comment at the top of the file like: // Code generated by go-swagger; DO NOT EDIT. Maybe we could prepend a comment like this one in the generated .txt file ?

[ru-fu]

be generated in the doc/ folder, and it should ha

@ru-fu usually, in Swagger, there is always a comment at the top of the file like: // Code generated by go-swagger; DO NOT EDIT. Maybe we could prepend a comment like this one in the generated .txt file ?

Sure, that makes sense. :)

[ru-fu]

Some first comments before I start testing.

[ru-fu]

Is there a way to include the tool as a binary?
It feels a bit like overkill to require Go to be installed just to build the documentation.

[ru-fu]

Some issues I found:

• When I specify a group as device-tpm or device_tpm, it gets cut to just device.
• If a key value starts with ` (for example, default: `all` ), it must be surrounded with " or Sphinx will give an error. So we actually need default: "`all`". We can either require it to be written correctly in the comments (in which case the readme must be fixed) or insert the quotes automatically for such values. The latter might make more sense to keep the YAML file clean?
• In the long description, we might have nested markup like ```{note}, which starts and ends with three backticks. If we have this, the generated ```{config:option} command must actually start and end with four backticks, or the markup will not be valid. In theory, we might even have nested markup inside the long description, so the surrounding ```{config:option} might have to use five or six backticks.
  So I guess we'd need to check the long description and check for the longest sequence of backticks and then start and end the generated markup with one more backtick.

[ru-fu]

Also, we should add doc/config_options.txt/yaml to the .gitignore file.

[ru-fu]

Also, we should add doc/config_options.txt/yaml to the .gitignore file.

[gabrielmougard]

Is there a way to include the tool as a binary? It feels a bit like overkill to require Go to be installed just to build the documentation.

It's usually not a good idea to include a binary in the source. The reason being that this project might be built on different system architecture like x86, ARM, etc. If I were to provide a binary, this would only work for my own architecture. If we would still want to do that, we'd need the binary to be sent to a package manager with all the arch we want to support.. Maybe we are not at that stage yet.

[gabrielmougard]

@ru-fu now the github build should be fine. But now, as a requirement of building the documentation, one has to also have go installed (we reuse some of the go compiler parsing libs to build lxd-doc). One easy way to install it is with snap : sudo snap install --classic --channel=1.18/stable go

[monstermunchkin]

Looks good, Just a few nits.

[ru-fu]

Can't review the code, but it works as expected now. Thanks!
Still one remaining nitpick. ;)

[stgraber]

We should probably move this to lxd/config/generate or something like this to mirror lxd/db/generate.

Having this be under .sphinx/ feels wrong when we intend for this tool to also generate JSON documentation, bash completion profiles, ... none of which are related to Sphinx.

[tomponline]

@gabrielmougard @stgraber @ru-fu is this one ready to be merged?

[tomponline]

jenkins: test this please

[gabrielmougard]

@gabrielmougard @stgraber @ru-fu is this one ready to be merged?

On my side yes

[tomponline]

Thanks @ru-fu and @stgraber have approved. So I'll merge if jenkins is clear.