Skip to content
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

Generate role documentation #272

Merged
merged 1 commit into from
Aug 3, 2021
Merged

Conversation

felixfontein
Copy link
Collaborator

@felixfontein felixfontein commented May 21, 2021

With the role argument spec feature of ansible-core 2.11, it is now possible to document roles in collections!

This is a first WIP to implement this. It's also a WIP because it contains #255. There's a lot of special-casing since roles behave quite differently than other plugin types (for example they can have multiple entry points).

You can see the effects here:

  1. The menu contains 'Index of all Roles': https://ansible.fontein.de/
  2. There's a 'Index of all Roles': https://ansible.fontein.de/collections/index_role.html
  3. In the felixfontein.acme collection docs, there's a 'Role Index': https://ansible.fontein.de/collections/felixfontein/acme/index.html#role-index (please ignore the 'Roles' section above, it is created manually with Allow collections to add RST files #255)
  4. There are role documentations:

@felixfontein felixfontein force-pushed the roles branch 3 times, most recently from 2e4b703 to 00dec46 Compare May 24, 2021 22:24
@felixfontein felixfontein changed the title [WIP] Generate role documentation Generate role documentation May 25, 2021
@felixfontein
Copy link
Collaborator Author

I also built documentation for sensu.sensu_go, a collection included in Ansible which has roles with argument specs: https://ansible.fontein.de/collections/sensu/sensu_go/index.html#role-index

@@ -195,6 +229,13 @@ def main(args):
for plugin_type in C.DOCUMENTABLE_PLUGINS:
result['plugins'][plugin_type] = load_all_plugins(plugin_type, basedir, coll_filter)

# Export role docs
RoleMixin = getattr(doc, 'RoleMixin', None)
Copy link

Choose a reason for hiding this comment

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

Just FYI, neither RoleMixin nor any of its methods were ever meant to be publicly accessible. Using this is at-your-own-risk as my intention was to try to make a proper public API for some of this in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually most code in collection-enum.py is using internal stuff from ansible-doc that can break at any moment. I would prefer to have a stable API, but unfortunately there isn't one :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(In case it breaks, you can configure antsibull to use ansible-doc --json to extract all the docs. Unfortunately it's like 1000x slower, that's why this method is the default :) )

@wbclark
Copy link

wbclark commented Jun 8, 2021

Hi @felixfontein ! Thanks for the work here on structured docs for Ansible roles

We would like to use this approach for our roles in the Foreman community collections. We are very interested in your feedback on the following ideas and whether you consider it to be a good approach, feasible as already implemented, or worth implementing here:

  1. Adding a template for README.md in the roles/$role_name directory to be populated from the structured documentation.
  2. A way to include 'Common Role Variables' which are shared across multiple roles but defined in a single location such as parent roles/ directory for the collection. To make things conceptually simple for users, these would ideally be rendered under a separate heading.
  3. A way to define groupings of Role Variables, assign a description to each grouping, and render the groupings separately in the docs. For example we might want separate groupings for required parameters, parameters which have values in the role defaults, parameters which have no default in the role but do have a default value in the target, and parameters which will be omitted in the target if not specified.
  4. A way to define Example Playbooks for the Role within an example_playbooks/ directory, and render each of these along with a description in the documentation.

Most of our roles accept a primary data structure which is usually a single yaml dictionary, or sometimes a list of yaml dictionaries (so the 'groupings' I refer to above would ideally be possible at the level of sub-parameters for the yaml dictionary). My current understanding (please correct me if I am wrong) is that class InnerRoleOptionsSchema allows for handling of nested data structures like yaml lists and dictionaries. If that is correct, it would be great to see a test case / example!

If you like these ideas and consider them to be worthwhile, we would of course be happy to assist with implementation. Looking forward to your feedback.

CC: @evgeni for any additional comments

@felixfontein
Copy link
Collaborator Author

Hi @wbclark, thanks for your interest in this PR!

1. Adding a template for README.md in the `roles/$role_name` directory to be populated from the structured documentation.

This should be doable, but I'm not sure whether it should be part of antsibull (and definitely not part of this PR :) ).

2. A way to include 'Common Role Variables' which are shared across multiple roles but defined in a single location such as parent `roles/` directory for the collection.

This could be solved with something similar to docs fragments, but unfortunately ansible-core does not support that (yet). It would be nice if this could be added for ansible-core 2.12, but this repository is the wrong venue to discuss this (it's the core team's decision, not ours). It's probably a good idea to discuss this with core team members in #ansible-devel and/or a core meeting, and/or creating a proposal (https://github.com/ansible/proposals/) for that.

To make things conceptually simple for users, these would ideally be rendered under a separate heading.

This would require additional metadata in the argument spec. While this is probably more relevant for roles, it can also be of interest for plugins. It's probably best to start discussing this in the documentation working group meeting. This is in particular important since the ansible-doc text output should have a similar amount of information than the generated RST documentation.

3. A way to define groupings of Role Variables, assign a description to each grouping, and render the groupings separately in the docs. For example we might want separate groupings for required parameters, parameters which have values in the role defaults, parameters which have no default in the role but do have a default value in the target, and parameters which will be omitted in the target if not specified.

This is similar to the above: it needs additional metadata, which needs to be defined and standardized first (preferably in the docs meeting and/or as a proposal).

4. A way to define Example Playbooks for the Role within an `example_playbooks/` directory, and render each of these along with a description in the documentation.

This also needs to be defined and standardized first, so that both ansible-doc and antsibull-docs process data the same way. (I cannot say whether reading examples from external files would be acceptable for ansible-doc though.)

Most of our roles accept a primary data structure which is usually a single yaml dictionary, or sometimes a list of yaml dictionaries (so the 'groupings' I refer to above would ideally be possible at the level of sub-parameters for the yaml dictionary).

That's not so easy to implement with the way module, plugin and role options are presented currently (as a table). I'm planning to work on a different representation eventually (which will not use <table>), with that this is probably easier to implement. It still needs additional metadata though.

My current understanding (please correct me if I am wrong) is that class InnerRoleOptionsSchema allows for handling of nested data structures like yaml lists and dictionaries. If that is correct, it would be great to see a test case / example!

Yes, nested data is supported, see the specification: https://docs.ansible.com/ansible/devel/user_guide/playbooks_reuse_roles.html#specification-format It looks like suboptions: is the wrong key though (i.e. the documentation is wrong about this, and also this PR until 8e92ab3) - the correct key seems to be options: according to ansible-core's code and its integration tests. I haven't seen an example of its use outside the integration tests (https://github.com/ansible/ansible/tree/devel/test/integration/targets/roles_arg_spec) yet, though.

Copy link
Contributor

@samccann samccann left a comment

Choose a reason for hiding this comment

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

just a few questions since I can't follow the code....;-)

The output so far LGTM.


{% if collection == 'ansible.builtin' -%}
.. note::
This module is part of ``ansible-base`` and included in all Ansible
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be saying it's part of 'ansible-core' at this point? Also, since I'm not really following the code, is it a module that triggers this note, or is it some future role included in ansible.builtin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I cut this paragraph together to

    This role is part of ``ansible-core`` and included in all Ansible
    installations.

This case will probably not happen in the short term anyway (and maybe never in the long term as well); I think ansible-core right now doesn't even have a mechanism by which it can contain roles.

antsibull/data/docsite/role.rst.j2 Outdated Show resolved Hide resolved
.. seealso::

{% for item in ep_doc['seealso'] %}
{% if item.module is defined and item.description %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that any of the example roles are exercising this bit of the code yet. Is there a way to 'dummy up' some See Also sections so we can be sure this part works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@acozine acozine left a comment

Choose a reason for hiding this comment

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

Thanks for implementing role documentation @felixfontein! This is a big addition, and I think could be useful. I have a few questions, see below.

antsibull/data/collection-enum.py Outdated Show resolved Hide resolved
@@ -29,20 +29,35 @@ Collection version @{ collection_version }@

{% endfor %}

{% if 'role' in plugin_maps %}
Role Index
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is the correct order. For a majority of collections, most users are looking for modules. In the docs survey results, we had at least one person say that they were confused by the Plugin Index heading and thought they were on the wrong page. For a collection that includes modules, other plugins, and roles, I would probably expect that to be the order of presentation (modules first, then plugins, then roles).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it to the bottom in c28c8e4.

Maybe we should rename "Plugin Index" to "Module and Plugin Index"?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea. Or "Module and Other Plugin Index"? Modules really are just a type of plugin . . . but I don't know how important it is for people to know that . . .

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea. Or "Module and Other Plugin Index"? Modules really are just a type of plugin . . . but I don't know how important it is for people to know that . . .

+1 to changing to this.

antsibull/data/docsite/role.rst.j2 Show resolved Hide resolved

.. Collection note

{% if collection == 'ansible.builtin' -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we ever have roles in ansible core?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No idea. I don't think this will happen short-term, and probably never long-term either. But if it does happen, the description will be less wrong than if this isn't specially handled :)

antsibull/data/docsite/role.rst.j2 Show resolved Hide resolved
antsibull/data/docsite/role.rst.j2 Show resolved Hide resolved
^^^^^^^

{% for author_name in ep_doc['author'] %}
- @{ author_name }@
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we. have this metadata?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This section is only added when author is present. Some roles do provide this.

error_tmpl = env.get_template('plugin-error.rst.j2')

writers = []
lib_ctx = app_context.lib_ctx.get()
async with asyncio_pool.AioPool(size=lib_ctx.thread_max) as pool:
for collection_name, plugins_by_type in collection_to_plugin_info.items():
for plugin_type, plugins in plugins_by_type.items():
plugin_type_tmpl = plugin_tmpl
if plugin_type == 'role':
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably the simplest way to extend the existing docs, but . . . are roles actually plugins? If they are, then this is fine. If they aren't, this seems like adding confusion for Future Coders. If roles aren't plugins, then could we use a different name? Like "elements" or something - where all plugins would be elements, and roles would also be elements . . . ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good question, and nothing I can easily answer. I would see it similar to "are modules plugins?". This is also something @abadger wanted to look at.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the way that the ansible-core code works, modules really are a type of plugin. roles are not. I'm working up an alternate structure for felixfontein's code to see if it looks like it will be easier to maintain as a separate code path rather than pretending like they're a different type of plugin. I'm not sure yet if it will be... have to see them side by side to decide.

"Felix Fontein (@felixfontein)"
],
"description": [
"This is a role which can use any CA supporting the ACME protocol, such as L(Let's Encrypt,https://letsencrypt.org/), L(Buypass,https://www.buypass.com/ssl/products/acme>) or L(ZeroSSL,https://zerossl.com/features/acme/>), to rekey ACME account keys.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Way to slip the L(label,link) syntax into the test!

Copy link
Contributor

@samccann samccann left a comment

Choose a reason for hiding this comment

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

LGTM

@samccann
Copy link
Contributor

Once @acozine gives it a thumbs up we should be good to go! Thanks for all the work on this one.

@samccann
Copy link
Contributor

samccann commented Aug 3, 2021

Generated the docs locally and it all looks good!

@abadger abadger merged commit f009a2f into ansible-community:main Aug 3, 2021
@felixfontein felixfontein deleted the roles branch August 3, 2021 20:31
@felixfontein
Copy link
Collaborator Author

@Shrews @wbclark @abadger @briantist @samccann @acozine thanks a lot for your reviews, testing and feedback! :)

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.

7 participants