-
Notifications
You must be signed in to change notification settings - Fork 614
🤖 Set build config via Sphinx ext #2360
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
base: devel
Are you sure you want to change the base?
Changes from all commits
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 | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,110 @@ | ||||||
"""Sphinx extension for setting up the build settings.""" | ||||||
|
||||||
import subprocess | ||||||
from dataclasses import dataclass | ||||||
from functools import cache | ||||||
from pathlib import Path | ||||||
from tomllib import loads as parse_toml_string | ||||||
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. Do we still need to support Python 3.10? 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. Could add an extra optional dependency if needed. Though given that 3.11 is hardcoded in the automation, I'd not bother. 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 We can still declare the |
||||||
from typing import Literal | ||||||
|
||||||
from sphinx.application import Sphinx | ||||||
from sphinx.util import logging | ||||||
|
||||||
|
||||||
logger = logging.getLogger(__name__) | ||||||
|
||||||
|
||||||
DOCSITE_ROOT_DIR = Path(__file__).parents[1].resolve() | ||||||
DOCSITE_EOL_CONFIG_PATH = DOCSITE_ROOT_DIR / 'end_of_life.toml' | ||||||
|
||||||
|
||||||
@dataclass(frozen=True) | ||||||
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. What's the point of a frozen dataclass if it has mutable attributes? 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 just a nice default to have. 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. Also, the point of immutable data structures is that it's harder to get into a situation where their contents would be changed suddenly by functions where you pass them. Of course, this doesn't shield us from the mutable nested pieces of data, but it's the first step. It might be good to use some marshalling library in the future. |
||||||
class Distribution: | ||||||
end_of_life: list[str] | ||||||
supported: list[str] | ||||||
|
||||||
@classmethod | ||||||
def from_dict(cls, raw_dist: dict[str, list[str]]) -> 'Distribution': | ||||||
return cls( | ||||||
**{ | ||||||
kind.replace('-', '_'): versions | ||||||
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 dataclass has to have snake_case attrs so this is doing that conversion. It effectively maps |
||||||
for kind, versions in raw_dist.items() | ||||||
}, | ||||||
) | ||||||
|
||||||
|
||||||
EOLConfigType = dict[str, Distribution] | ||||||
|
||||||
|
||||||
@cache | ||||||
def _read_eol_data() -> EOLConfigType: | ||||||
raw_config_dict = parse_toml_string(DOCSITE_EOL_CONFIG_PATH.read_text()) | ||||||
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. Not that the file is so large, but why not use the usual 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. Ah, tomllib.load reads the whole thing into memory and passes it to tomllib.loads anyways (https://github.com/python/cpython/blob/0c088e44428d74d701fe1fc80a4cb4fe124c43f0/Lib/tomllib/_parser.py#L57). 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. Yeah, I also thought that it's probably not worth it. |
||||||
|
||||||
return { | ||||||
dist_name: Distribution.from_dict(dist_data) | ||||||
for dist_name, dist_data in raw_config_dict['distribution'].items() | ||||||
} | ||||||
|
||||||
|
||||||
@cache | ||||||
def _is_eol_build(git_branch: str, kind: str) -> bool: | ||||||
return git_branch in _read_eol_data()[kind].end_of_life | ||||||
|
||||||
|
||||||
@cache | ||||||
def _get_current_git_branch() -> str: | ||||||
git_branch_cmd = 'git', 'rev-parse', '--abbrev-ref', 'HEAD' | ||||||
|
||||||
try: | ||||||
return subprocess.check_output(git_branch_cmd, text=True).strip() | ||||||
except subprocess.CalledProcessError as proc_err: | ||||||
raise LookupError( | ||||||
f'Failed to locate current Git branch: {proc_err !s}', | ||||||
) from proc_err | ||||||
|
||||||
|
||||||
def _set_global_j2_context(app, config): | ||||||
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. Type annotation? 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. Thanks! This was meant as a demo, so I didn't pay attention at all the places, I suppose. Looks like the config type isn't imported. Will have to look it up later.
Suggested change
|
||||||
if 'is_eol' in config.html_context: | ||||||
raise ValueError( | ||||||
'`is_eol` found in `html_context` unexpectedly. ' | ||||||
'It should not be set in `conf.py`.', | ||||||
) from None | ||||||
|
||||||
dist_name = ( | ||||||
'ansible-core' if app.tags.has('core') | ||||||
else 'ansible' if app.tags.has('ansible') | ||||||
else None | ||||||
) | ||||||
|
||||||
if dist_name is None: | ||||||
return | ||||||
|
||||||
try: | ||||||
git_branch = _get_current_git_branch() | ||||||
except LookupError as lookup_err: | ||||||
logger.info(str(lookup_err)) | ||||||
return | ||||||
|
||||||
config.html_context['is_eol'] = _is_eol_build( | ||||||
oraNod marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
git_branch=git_branch, kind=dist_name, | ||||||
) | ||||||
|
||||||
|
||||||
def setup(app: Sphinx) -> dict[str, bool | str]: | ||||||
"""Initialize the extension. | ||||||
|
||||||
:param app: A Sphinx application object. | ||||||
:returns: Extension metadata as a dict. | ||||||
""" | ||||||
|
||||||
# NOTE: `config-inited` is used because it runs once as opposed to | ||||||
# NOTE: `html-page-context` that runs per each page. The data we | ||||||
# NOTE: compute is immutable throughout the build so there's no need | ||||||
# NOTE: to have a callback that would be executed hundreds of times. | ||||||
app.connect('config-inited', _set_global_j2_context) | ||||||
|
||||||
return { | ||||||
'parallel_read_safe': True, | ||||||
'parallel_write_safe': True, | ||||||
'version': app.config.release, | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,25 @@ | ||||||||||||||||
[distribution.ansible] | ||||||||||||||||
end-of-life = [ | ||||||||||||||||
'stable-2.15', | ||||||||||||||||
'stable-2.14', | ||||||||||||||||
'stable-2.13', | ||||||||||||||||
] | ||||||||||||||||
supported = [ | ||||||||||||||||
oraNod marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
'devel', | ||||||||||||||||
'stable-2.18', | ||||||||||||||||
'stable-2.17', | ||||||||||||||||
'stable-2.16', | ||||||||||||||||
Comment on lines
+10
to
+11
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 community package is only built for the latest ansible-core version. Should these be removed?
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. Dunno, I'm yet to fully understand the context. I'd leave this to Don. Or maybe we'll get to doing something together. 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 community package is only built for the latest core version but the guideline for backporting and rebuilding docs is to maintain devel, the latest version, and the previous two versions. In any case we should drop the "supported" portion of the toml since it is not used. I put that in my POC and it got copied over here. The point was to have a declarative approach to the versions that are EOL and the versions that are actively maintained. I was also thinking of having a workflow that would periodically compare the list of "supported / active" versions and compare with the core branch and then notify the docs channel in Matrix when a new stable branch hits the core repo so we'd know to cut a new branch for docs. |
||||||||||||||||
] | ||||||||||||||||
Comment on lines
+7
to
+12
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.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
[distribution.ansible-core] | ||||||||||||||||
end-of-life = [ | ||||||||||||||||
'stable-2.15', | ||||||||||||||||
'stable-2.14', | ||||||||||||||||
'stable-2.13', | ||||||||||||||||
] | ||||||||||||||||
supported = [ | ||||||||||||||||
'devel', | ||||||||||||||||
'stable-2.18', | ||||||||||||||||
'stable-2.17', | ||||||||||||||||
'stable-2.16', | ||||||||||||||||
] | ||||||||||||||||
Comment on lines
+14
to
+25
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. @oraNod things here are the same as in the other mapping. Did you expect them to differ at some point? What's the semantic meaning you were going for? 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. Hey @webknjaz Sorry for the slow response. I've been focused on prep work for cfgmgmtcamp. My thinking was to use the "supported" list as a way to detect when the core repo creates a new stable branch. We could then get an announcement in matrix that it's time to cut a new stable branch for docs. Here's a link to my comment where I mentioned this: #1695 (comment) It was just an idea. And since it's not strictly needed as part of this PR maybe we should do it separately, if at all. 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.
@oraNod yes, it's a good idea to exclude things that are out of the scope right now. However, it's also good to think about the desired data structure overall so it could be designed taking that into account right away. I'm also confused about this 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. @webknjaz Makes sense to narrow the scope to only what is necessary. For 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. @oraNod but 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 don't fully understand the mapping to 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. OK, maybe there is also a disconnect in my understanding. I'll try but my explanation could be off so bear with me. It would probably be easier to discuss on a call. That will have to be next week, I'm afraid. Take the Would it be worthwhile to comment the toml so it's clear which branch names map to which I don't think that is documented anywhere and is kind of tribal knowledge. Part of my goal was indeed to have a central, declarative place where we could clearly see which doc builds are EOL and which are still active. 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. @webknjaz Hey, so revisiting this after the chat yesterday. I wonder if it would be better to use the For example on the
You can see the version for the package and the version for core more clearly. I know the version remains I guess this doesn't address my other comment about the branch names to package version mappings. I think we can just put that into the maintainers file somewhere rather than shoehorning it into the toml. 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. Also, FYI, we discussed the missing version on 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.
Perhaps. Depending on what we need to have in the config vs. in the command invocation. I already forgot half of that context :) We may need to hack on this together sometime...
Comment on lines
+20
to
+25
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.
Suggested change
|
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.
Do you want to add this to
ansible-documentation/noxfile.py
Line 13 in c671fca
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.
I despise black, so I'd rather not. Maybe, when somebody else takes over. I don't mind some of the others, though. I don't really use a full development environment with this repo, so whatever doesn't stand in the way of my workflow is good.