EOL sphinx extension #2251
EOL sphinx extension #2251
Conversation
oraNod
added
doc builds
|
Thanks for doing this! So the extra 'older version' banner is coming from [line 57 in banner.html](https://github.com/ansible/ansible-documentation/pull/2251/files#diff-1d79a9e5a54a083e8cd65610b77d6983e4d6b75ebb4c230b7d13633701bbd063L57]. This is the banner that shows up in PR builds etc. I can't figure out how to test this locally. I tried adding 'devel' to the core eol list, but since I'm grabbing your PR, I'm guessing the 'branch' isn't devel and thus not working? |
| {# Creates a banner at the top of the page for EOL versions. #} | ||
| <div id="banner" class="Admonition caution"> | ||
| <p> | ||
| You are reading an unmaintained version of the Ansible documentation. Unmaintained Ansible versions can contain unfixed security vulnerabilities (CVE). Please upgrade to a maintained version. See <a href="/ansible/latest/">the latest Ansible documentation</a>. |
There was a problem hiding this comment.
this is an existing bug, but I just realized that href will redirect eol core docsite to ansible latest. I don't know of an easy way to fix it so maybe I should just open a separate issue for this? It should be /ansible/latest for package releases, and ansible-core/devel for core releases probably.
| banner += '<div id="banner_id" class="admonition '; | ||
| banner += important ? 'important' : 'caution'; | ||
| banner += '">'; | ||
| banner += '<div id="banner_id" class="admonition '; banner += important ? 'important' : 'caution'; banner += '">'; |
There was a problem hiding this comment.
I assume this is an accidental change?
| banner += '<div id="banner_id" class="admonition '; banner += important ? 'important' : 'caution'; banner += '">'; | |
| banner += '<div id="banner_id" class="admonition '; | |
| banner += important ? 'important' : 'caution'; | |
| banner += '">'; |
| banner += important ? '<br>' : ''; | ||
| banner += msg; | ||
| banner += important ? '<br>' : ''; | ||
| banner += '</div>'; | ||
| document.write(banner); | ||
| {% endif %} | ||
| </script> | ||
| {% endif %} | ||
| </script> |
There was a problem hiding this comment.
I'd just keep it indented to hide from the diff
| </script> | |
| </script> |
| </div> | ||
| {% else %} | ||
| <script> | ||
| <script> |
There was a problem hiding this comment.
| <script> | |
| <script> |
There was a problem hiding this comment.
No need to have this file. Just reference the extension directly. Re-exporting an ambiguous setup func is going to be confusing. Plus, referencing this entire folder as an extension is not what it exists for. It's already supposed to be injected into sys.path.
| @@ -25,6 +25,7 @@ | |||
| # sys.path.append(os.path.abspath('some/directory')) | |||
| # | |||
| sys.path.insert(0, os.path.join('ansible', 'lib')) | |||
| sys.path.insert(0, os.path.abspath('.')) | |||
There was a problem hiding this comment.
The idiomatic way is to inject the _ext dir so you could have multiple separate extensions loadable. Besides, no need to make things outside this directory importable.
https://github.com/ansible/awx-plugins/blob/0d569b5/docs/conf.py#L34C1-L34C48
| sys.path.insert(0, os.path.abspath('.')) | |
| sys.path.insert(0, os.path.abspath('_ext')) |
| @@ -65,6 +66,7 @@ | |||
| 'notfound.extension', | |||
| 'sphinx_antsibull_ext', # provides CSS for the plugin/module docs generated by antsibull | |||
| 'sphinx_copybutton', | |||
| '_ext.eol_version', | |||
There was a problem hiding this comment.
It's better to be explicit about which extensions are local: https://github.com/ansible/awx-plugins/blob/0d569b5/docs/conf.py#L81C5-L81C26
| '_ext.eol_version', | |
| # In-tree extensions: | |
| 'eol_version', # conditionally injects a banner if EOL |
| @@ -0,0 +1,20 @@ | |||
| package: | |||
| active: | |||
| - devel | |||
There was a problem hiding this comment.
Why have a single-space indent? Most people argue about zero vs. two-space... I'm in the no-indent camp FWIW.
https://github.com/ansible/awx-plugins/blob/0d569b5/.yamllint#L6C3-L8C28
| @@ -0,0 +1,20 @@ | |||
| package: | |||
There was a problem hiding this comment.
This name is rather misleading. Both are packages. ansible-core is a distribution package that does not bundle heavy stuff.
There was a problem hiding this comment.
I wonder if this structure should be a mapping of version to EOL date instead. The Python side could then compute if something is EOL based on that…
| @@ -227,7 +229,6 @@ | |||
| html_context = { | |||
| 'display_github': 'True', | |||
| 'show_sphinx': False, | |||
| 'is_eol': False, | |||
There was a problem hiding this comment.
I'm still in the middle of the review, but it appears to me that a lot of what's happening here is over-engineering. I'd rather compute this dynamically and have this context for the main Jinja rendering mechanism to do its thing.
|
|
||
|
|
||
| class EOLVersionCheck(Transform): | ||
| # pylint: disable=too-few-public-methods |
There was a problem hiding this comment.
pro tip: try to use disable-next or put the suppression onto the exact line. bare disable turns the pylint check off until the end of the document, unlike what flake8 would do.
| with open(yaml_path, "r", encoding="utf-8") as f: | ||
| versions = yaml.safe_load(f) |
There was a problem hiding this comment.
That's a low-level API. Pathlib has a more elegant interface for reading/writing entire files:
| with open(yaml_path, "r", encoding="utf-8") as f: | |
| versions = yaml.safe_load(f) | |
| versions = yaml.safe_load(yaml_path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
Oh... That is better. Thanks! I'll actually put read_text to use in a separate project.
| ["git", "rev-parse", "--abbrev-ref", "HEAD"], text=True | ||
| ).strip() | ||
| except subprocess.CalledProcessError as e: | ||
| print(f"Git error: {e}") |
There was a problem hiding this comment.
Don't just print stuff. Use logging. You can get it via from sphinx.util import logging.
Better yet — raise a proper error: https://github.com/sphinx-contrib/sphinxcontrib-towncrier/blob/7f57120/src/sphinxcontrib/towncrier/ext.py#L148C13-L151C14
| print(f"Git error: {e}") | |
| raise self.error(f"Failed to abbreviate Git HEAD: {e !s}") from None |
|
|
||
| import yaml | ||
| from docutils import nodes | ||
| from jinja2 import Template |
There was a problem hiding this comment.
So my problem with this is that you're going out of the way to have disconnected double use of the templating engine, while previously it was happening in one pass. I'm pretty sure you could throw most of this away and just compute a single boolean that would go into the context that the regular templating engine relies on.
The only thing you'd need would be hooking to different Sphinx lifecycle events. Since this is being computed early, I'd go for the earliest one that receives the Sphinx app as an argument and assign the computed value to the env config object that gets into Jinja, through that very same html_context mapping.
Oh, and since the config values may change, it's important to tell Sphinx that it influences caching. Otherwise, it may refuse to rebuild things that depend on that file.
There was a problem hiding this comment.
Here's how the alabaster theme does this: https://github.com/sphinx-doc/alabaster/blob/fba58a4/alabaster/__init__.py#L15-L34
There was a problem hiding this comment.
"disconnected double use of the templating engine, while previously it was happening in one pass" - now that you say it like that...
| self.document.insert(0, banner) | ||
|
|
||
|
|
||
| def setup(app) -> Dict[str, Any]: |
There was a problem hiding this comment.
Forgot to annotate the arg. Also, what Python version are you targeting? It may be possible to just lowercase dict. Plus, you should avoid using Any, especially since it's dead obvious that it's str | bool.
| - stable-2.17 | ||
| - stable-2.16 | ||
| eol: | ||
| - stable-2.15 |
There was a problem hiding this comment.
if this represents non-core dist, the versioning here would probably be different
This is an initial change allowing for restructuring how the config settings are computed and putting that logic into a dedicated Sphinx extension. The idea is that this extension may have multiple callbacks that set configuration for Sphinx based on tags and the environment state. As an example, this in-tree extension implements setting the `is_eol` variable in Jinja2 context very early in Sphinx life cycle. It does this based on inspecting the state of current Git checkout as well as reading a config file listing EOL and supported versions of `ansible-core`. The configuration format is TOML as it's gained a lot of the ecosystem adoption over the past years and its parser made its way into the standard library of Python, while PyYAML remains a third-party dependency. Supersedes ansible#2251.
This is an initial change allowing for restructuring how the config settings are computed and putting that logic into a dedicated Sphinx extension. The idea is that this extension may have multiple callbacks that set configuration for Sphinx based on tags and the environment state. As an example, this in-tree extension implements setting the `is_eol` variable in Jinja2 context very early in Sphinx life cycle. It does this based on inspecting the state of current Git checkout as well as reading a config file listing EOL and supported versions of `ansible-core`. The configuration format is TOML as it's gained a lot of the ecosystem adoption over the past years and its parser made its way into the standard library of Python, while PyYAML remains a third-party dependency. Supersedes ansible#2251.
|
Closed in favor of #2360 thanks both of you! |
Fixes #1490
This adds a
versions.yamlfile that declares the active and eol versions of core and package docs. A sphinx extension,docs/docsite/rst/_ext/eol_version.py, uses thecoreandansibletags from the sphinxconf.pyfile to set the build type, grabs the branch name from the git repo, then checks if the branch name is in theeollist. If so then the extension injects theeol_bannerinto pages during the build.This PR also separates EOL banner from the
docs/docsite/.templates/banner.htmlfile into a new file calleddocs/docsite/.templates/eol_banner.html.Tested my fork by adding the branch name to the
eollist for package docs and deploying to the test site: https://ansible-community.github.io/package-doc-builds/You can verify the changes locally by adding the branch to the
eoloractivelists for core docs and runningnox -s make.Final note: I think this might be causing an issue with the main banner. I need to check that out but I do see "You are reading an older version of the Ansible documentation." in the devel version of the docs.