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

EOL sphinx extension #2251

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions docs/docsite/.templates/banner.html
Original file line number Diff line number Diff line change
@@ -1,10 +1,4 @@
{% if is_eol %}
{# 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>.</p>
</div>
{% else %}
<script>
<script>
function startsWith(str, needle) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<script>
<script>

return str.slice(0, needle.length) == needle
}
Expand Down Expand Up @@ -61,14 +55,11 @@
}
msg += '</p>';

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>' : '';
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is an accidental change?

Suggested 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 += msg;
banner += important ? '<br>' : '';
banner += '</div>';
document.write(banner);
{% endif %}
</script>
{% endif %}
</script>
6 changes: 6 additions & 0 deletions docs/docsite/.templates/eol_banner.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{# 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>.
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 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.

</p>
samccann marked this conversation as resolved.
Show resolved Hide resolved
</div>
5 changes: 5 additions & 0 deletions docs/docsite/rst/_ext/__init__.py
Copy link
Member

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from .eol_version import setup

__version__ = '1.0.0'

__all__ = ['setup']
56 changes: 56 additions & 0 deletions docs/docsite/rst/_ext/eol_version.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import subprocess
from pathlib import Path

import yaml
from docutils import nodes
from jinja2 import Template
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"disconnected double use of the templating engine, while previously it was happening in one pass" - now that you say it like that...

from sphinx.transforms import Transform


class EOLVersionCheck(Transform):
# pylint: disable=too-few-public-methods
Copy link
Member

Choose a reason for hiding this comment

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

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.

default_priority = 999

def apply(self):
env = self.document.settings.env
app = env.app

if app.tags.has("core"):
build_type = "core"
elif app.tags.has("ansible"):
build_type = "package"
else:
return

try:
branch = subprocess.check_output(
["git", "rev-parse", "--abbrev-ref", "HEAD"], text=True
).strip()
except subprocess.CalledProcessError as e:
print(f"Git error: {e}")
Copy link
Member

Choose a reason for hiding this comment

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

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

Suggested change
print(f"Git error: {e}")
raise self.error(f"Failed to abbreviate Git HEAD: {e !s}") from None

return

yaml_path = Path(env.srcdir).parent / "versions.yaml"
template_path = Path(env.srcdir).parent / ".templates" / "eol_banner.html"

with open(yaml_path, "r", encoding="utf-8") as f:
versions = yaml.safe_load(f)
Comment on lines +37 to +38
Copy link
Member

Choose a reason for hiding this comment

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

That's a low-level API. Pathlib has a more elegant interface for reading/writing entire files:

Suggested change
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"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... That is better. Thanks! I'll actually put read_text to use in a separate project.


with open(template_path, "r", encoding="utf-8") as f:
template = Template(f.read())

if branch in versions[build_type]["eol"]:
banner_html = template.render(
version=branch, active_versions=versions[build_type]["active"]
)
banner = nodes.raw("", banner_html, format="html")
self.document.insert(0, banner)


def setup(app):
app.add_transform(EOLVersionCheck)
return {
"version": "1.0",
"parallel_read_safe": True,
"parallel_write_safe": True,
}
3 changes: 2 additions & 1 deletion docs/docsite/rst/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -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('.'))
Copy link
Member

Choose a reason for hiding this comment

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

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

Suggested change
sys.path.insert(0, os.path.abspath('.'))
sys.path.insert(0, os.path.abspath('_ext'))


# We want sphinx to document the ansible modules contained in this repository,
# not those that may happen to be installed in the version
Expand Down Expand Up @@ -65,6 +66,7 @@
'notfound.extension',
'sphinx_antsibull_ext', # provides CSS for the plugin/module docs generated by antsibull
'sphinx_copybutton',
'_ext.eol_version',
Copy link
Member

Choose a reason for hiding this comment

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

It's better to be explicit about which extensions are local: https://github.com/ansible/awx-plugins/blob/0d569b5/docs/conf.py#L81C5-L81C26

Suggested change
'_ext.eol_version',
# In-tree extensions:
'eol_version', # conditionally injects a banner if EOL

]

# Later on, add 'sphinx.ext.viewcode' to the list if you want to have
Expand Down Expand Up @@ -227,7 +229,6 @@
html_context = {
'display_github': 'True',
'show_sphinx': False,
'is_eol': False,
Copy link
Member

Choose a reason for hiding this comment

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

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.

'github_user': 'ansible',
'github_repo': 'ansible-documentation',
'github_version': 'devel',
Expand Down
20 changes: 20 additions & 0 deletions docs/docsite/versions.yaml
Copy link
Member

Choose a reason for hiding this comment

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

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…

Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package:
Copy link
Member

Choose a reason for hiding this comment

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

This name is rather misleading. Both are packages. ansible-core is a distribution package that does not bundle heavy stuff.

active:
- devel
Copy link
Member

Choose a reason for hiding this comment

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

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

- stable-2.18
- stable-2.17
- stable-2.16
eol:
- stable-2.15
Copy link
Member

Choose a reason for hiding this comment

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

if this represents non-core dist, the versioning here would probably be different

- stable-2.14
- stable-2.13
core:
active:
- devel
- stable-2.18
- stable-2.17
- stable-2.16
eol:
- stable-2.15
- stable-2.14
- stable-2.13
Loading