-
Notifications
You must be signed in to change notification settings - Fork 547
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
EOL sphinx extension #2251
Changes from 4 commits
506093e
ede97c7
e7b74f5
d9961cb
e30debf
59ec15b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) { | ||||||||||
return str.slice(0, needle.length) == needle | ||||||||||
} | ||||||||||
|
@@ -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>' : ''; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this is an accidental change?
Suggested change
|
||||||||||
banner += msg; | ||||||||||
banner += important ? '<br>' : ''; | ||||||||||
banner += '</div>'; | ||||||||||
document.write(banner); | ||||||||||
{% endif %} | ||||||||||
</script> | ||||||||||
{% endif %} | ||||||||||
</script> |
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>. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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'] |
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 | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe 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 | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pro tip: try to use |
||||||||
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}") | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't just print stuff. Use logging. You can get it via Better yet — raise a proper error: https://github.com/sphinx-contrib/sphinxcontrib-towncrier/blob/7f57120/src/sphinxcontrib/towncrier/ext.py#L148C13-L151C14
Suggested change
|
||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh... That is better. Thanks! I'll actually put |
||||||||
|
||||||||
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, | ||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idiomatic way is to inject the
Suggested change
|
||||||||||
|
||||||||||
# We want sphinx to document the ansible modules contained in this repository, | ||||||||||
# not those that may happen to be installed in the version | ||||||||||
|
@@ -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. Choose a reason for hiding this commentThe 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
|
||||||||||
] | ||||||||||
|
||||||||||
# Later on, add 'sphinx.ext.viewcode' to the list if you want to have | ||||||||||
|
@@ -227,7 +229,6 @@ | |||||||||
html_context = { | ||||||||||
'display_github': 'True', | ||||||||||
'show_sphinx': False, | ||||||||||
'is_eol': False, | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This name is rather misleading. Both are packages. |
||
active: | ||
- devel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
- stable-2.18 | ||
- stable-2.17 | ||
- stable-2.16 | ||
eol: | ||
- stable-2.15 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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.