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

Allow collections to add RST files #255

Merged
merged 2 commits into from
May 25, 2021

Conversation

felixfontein
Copy link
Collaborator

@felixfontein felixfontein commented Mar 6, 2021

The RST files have to be in docs/docsite/rst/ (and subdirs), need to use labels of the form ansible_collections.{namespace}.{name}.docsite. (RST files not satisfying that are ignored), and there needs to be a index.yml which allows to add sections to the main collection page and allows to say which files show up in the toctree for that section (also allows to directly reference things).

Example branches:

These result in:

There's a lint tool for collection owners (antsibull-lint collection-docs /path/to/collection), it checks RST labels and also runs rstcheck.

The PR also contains a commit which ensures that "empty" collections (which have no documentable plugins) are shown in the index, and will obtain docs. Example: https://ansible.fontein.de/collections/felixfontein/acme/ (that collection only has roles; I'll add docsite RST files for these eventually, currently I only have MD files it now has role docs as well)

@briantist
Copy link
Contributor

I don't feel qualified to actually review this but it looks great. I could definitely see myself using this for c.hv!

@felixfontein
Copy link
Collaborator Author

(I've just rebased so this PR includes all other docs building PRs that were merged today. Makes it easier to test for me :) )

@felixfontein
Copy link
Collaborator Author

As discussed at the docs meeting, I've renamed index.yml to extra-docs.yml, and removed the currently not used entries code.

I've also updated the example ansible-collections/community.docker@main...felixfontein:scenario-guide, which (still) results in https://ansible.fontein.de/collections/community/docker/.

ready_for_review

antsibull/docsite_docs.py Outdated Show resolved Hide resolved
antsibull/docsite_docs.py Outdated Show resolved Hide resolved
@abadger
Copy link
Contributor

abadger commented May 24, 2021

I think we should use a name other than docsite for the names of these functions and variables. We're already using docsite to refer to building docs.ansible.com as a whole. How about one of the following:

  • extra_docs
  • manual_docs
  • static_docs

raise DocsiteIndexError('extra-docs.yml does not exist')

try:
index = load_yaml_file(index_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

I note that this is synchronous code rather than async code. The majority of time is going to be spent reading and writing the actual page data rather than the extra-docs.yml file so it's not the end of the world but it is a point where we could be doing things in parallel. I assume you coded it as synchronous for easy reuse with antsibull-lint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main reason is that the IO part of this function is done by load_yaml_file, which is not async either. It is also used to load routing data for collections, so eventually it should be converted to async, but once that's done adjusting the calling code is also not so hard, so I would defer adjusting this function until then. Is that ok for you?

antsibull/docsite_docs.py Outdated Show resolved Hide resolved
antsibull/docsite_docs.py Outdated Show resolved Hide resolved
antsibull/docsite_docs.py Outdated Show resolved Hide resolved
@felixfontein
Copy link
Collaborator Author

I think we should use a name other than docsite for the names of these functions and variables. We're already using docsite to refer to building docs.ansible.com as a whole. How about one of the following:

* extra_docs

I renamed docsite to this in a3aa61d.

@felixfontein felixfontein force-pushed the extra-docsite-files branch from 4f22297 to c7e7a3b Compare May 24, 2021 22:23
@abadger
Copy link
Contributor

abadger commented May 25, 2021

Everything looks good! Talked with felixfontein earlier and he will squash this manually so we can look at the saving-content-in-memory implementation later if we need to, then go ahead and merge it.

Thanks @felixfontein !

@felixfontein felixfontein force-pushed the extra-docsite-files branch from c7e7a3b to c2bcd06 Compare May 25, 2021 16:12
@felixfontein felixfontein force-pushed the extra-docsite-files branch from c2bcd06 to 93a7073 Compare May 25, 2021 16:14
@felixfontein felixfontein merged commit 78fa7be into ansible-community:main May 25, 2021
@felixfontein felixfontein deleted the extra-docsite-files branch May 25, 2021 16:29
@felixfontein
Copy link
Collaborator Author

@abadger @briantist thanks a lot for reviewing this!
@acozine @samccann @lmalivert thanks a lot for discussing this! :)

ansible-zuul bot pushed a commit to ansible-collections/amazon.aws that referenced this pull request Jul 2, 2021
Migrates scenario guide from ansible/ansible repo

SUMMARY
Related to ansible-community/antsibull-build#255.
Do we need to rename the page index.rst? Or add an index.rst file?
Also needs a PR against ansible/ansible removing this content from there.
ISSUE TYPE

Docs Pull Request

COMPONENT NAME
docs.ansible.com
Depends-On: ansible/ansible-zuul-jobs#962

Reviewed-by: Mark Chappell <None>
Reviewed-by: Alicia Cozine <None>
Reviewed-by: Jill R <None>
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.

3 participants