-
Notifications
You must be signed in to change notification settings - Fork 818
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
Docs module generation from YAML instead of python meta dicts #5276
Conversation
9699919
to
5efafc2
Compare
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.
Overall, a big +1 to the changes here. I left a few inline comments, but I think this is a really good direction.
@@ -0,0 +1,29 @@ | |||
cc_ansible: |
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.
Think it's sensible to enforce that any yaml in this directory contain name, title, description, and examples?
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.
agreed will do.
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.
This can be removed now, right?
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.
The top-level key cc_ansible
within this data.yaml file could be removed or moved to a module_id
key, but it is currently sourced as the mod_id
parameter in https://github.com/canonical/cloud-init/pull/5276/files#diff-6d95c2666c03bf7fc99cbc86be09b58f01d8478d9f92e8dc81e99ce0026d76ccR2 as it is presented as the data
dict for the datatemplate plugin. I think for this pass it may make sense to retain that key structure as it also allows for all data.yaml files to be processed and combined without worries of colliding with other data.yaml files because each separate file is scoped below an unique key.
Oops you mean this is a vestigial file. Yes, this can be droppped. removing it now! thanks.
@@ -649,7 +649,7 @@ | |||
"distro", | |||
"pip" | |||
], | |||
"description": "The type of installation for ansible. It can be one of the following values:\n\n - ``distro``\n - ``pip``" | |||
"description": "The type of installation for ansible. It can be one of the following values: ``distro``, ``pip``" |
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.
Does this (and other changes here) mean that we can no longer control the formatting of any of the schema descriptions? That could be fairly significant for some of our longer descriptions.
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.
That could be fairly significant for some of our longer descriptions.
+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.
What it means is that we no longer control the leading white-space indentation as it'll get collapsed during render. We can still insert newlines, but not also attempt to expect retained format in nested RST within those descriptions. This is a good point. I'll look to what we can do during rendering to ensure we can appropriately embed formatted text in the description.
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.
@TheRealFalcon I've reworked the jinja template in doc/rtd/templates/module_property to properly split out any multi-line schema property description and provide the right indent prefix for each line after the first. So, one-line descriptions will be on the same line as the property declaration and any subsequent lines will be indented properly (check ansible/schema references for the install_method
key. What this does mean is any description in JSON schema that tried to properly set an indent across a newline will actually need to redact the leading white-space across each \n
@@ -0,0 +1,5 @@ | |||
{% macro print_prop(name, types, description, prefix) -%} |
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.
This is a much nicer change. These templates aren't very pretty, but much better to manage here than trying to format via the python code directly.
9cf2b6f
to
c14ee14
Compare
c14ee14
to
5566525
Compare
ok this PR is ready for review. Best way to test the branch is to either:
--- OR ---
|
e1bea99
to
4801ddb
Compare
565c5e8
to
1ce87a3
Compare
@@ -0,0 +1,29 @@ | |||
cc_ansible: |
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.
This can be removed now, right?
This module provides ``ansible`` integration for | ||
augmenting cloud-init's configuration of the local | ||
node. | ||
} # type: ignore |
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'm assuming that at the end of all of the conversions you plan to update the MetaSchema and remove this comment?
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.
Correct, once the final branch drops all examples
, title
and name
keys we can update MetaSchema and ensure typing is upheld.
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.
Inline comment about an extra file, but otherwise LGTM!
Add a new doc/rtd/templates and doc/module-docs directory for which use sphinxcontrib.datatemplates to render schema documentation instead of automodule documentation. The refactor allows moving doc-related keys out of each cc_* module's meta dictionary: examples, descripton, name and title. Move doc-related metadata keys into doc/module-docs/<module_id>/data.yaml and supporting examples into doc/module-docs/<module_id>/example*.yaml. This allows us to simplify cc_<module> documentation by allowing doc authors to manipulate either sphinx jinja templates, RST files or YAML example files instead of having to dedent and encode RST or MD markup in python "meta" dictionarties. To support doc generation for both readthedocs(sphinx) and cloud-init schema --docs command, schema.py grows a get_module_docs function to source the specific module-docs/*/data.yaml files for examples, names and titles.
1ce87a3
to
12c3548
Compare
rebase individual commits
Primary functional commit with cc_ansible doc template migration followed by minor integration test helper and doc migrations of apt_configure/apt_pipelining, bootcmd and byobu. The intent and migration will be basically the same for every cc_ module once this lands. I expect 3 more branches of just cc_ refactors to templates, the thing to be wary of is that our json schema inline RST syntax doesn't get rendered with poor formatting in the generated docs, or disappear from output of the
cloud-init schema --docs cc_<mod_name>
The intent with this draft is to start a conversation about relocating RST templates out of
cloudinit.config.schema
and into official templates in doct/rtd/templates that could be more easily updated if we change skins.This only sets up common logic for generating RTD docs and sharing the module-docs/*.yaml with
cloud-init schema --doc
command line. If we decide this is a reasonable approach, this branch will likely adapt a few cc_ at a time to ease reviewing.Benefits:
__doc__ = get_meta_doc()
on module load which reads cloud-config-schema.json