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

🤖 Set build config via Sphinx ext #2360

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

webknjaz
Copy link
Member

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 #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.
@webknjaz webknjaz force-pushed the maintenance/extension-driven-build-config branch from 8ef3352 to ab0e151 Compare January 17, 2025 03:53
@oraNod oraNod added doc builds Relates to building the documentation tooling This PR affects tooling (CI, pr_labeler, noxfile, linters, etc.) but not the docs builds themselves. labels Jan 17, 2025
def from_dict(cls, raw_dist: dict[str, list[str]]) -> 'Distribution':
return cls(
**{
kind.replace('-', '_'): versions
Copy link
Member Author

Choose a reason for hiding this comment

The 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 end-of-life in the TOML config to end_of_life.

Comment on lines +14 to +25
[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',
]
Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we should do it separately, if at all

@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 ansible vs. ansible-core thing — both list the same list of EOL entries. What's that about? Did you anticipate these having different values under some circumstances? Any more info?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 ansible vs. ansible-core there is a period during the release lifecycle where the core version reaches EOL before the Ansible package version. @samccann probably knows those details the best but we do need to mark both of those distributions as EOL at different times.

Copy link
Member Author

Choose a reason for hiding this comment

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

@oraNod but ansible has a different versioning scheme. Why does it list the ansible-core branches?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't fully understand the mapping to ansible then.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 stable-2.18 branch. In the ansible/ansible repo this corresponds to version of 2.18 for core. The ansible/ansible-documentation repo also has a stable-2.18 branch that maps to 2.18 for the core docs build and 11 for the package docs build. It is one branch that maps to two builds that EOL at different times. (I feel like I'm explaining something you already know here. I don't mean to be patronizing.)

Would it be worthwhile to comment the toml so it's clear which branch names map to which ansible versions?

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 VERSION constant instead of the branch name?

For example on the stable-2.18 branch we have this:

You can see the version for the package and the version for core more clearly.

I know the version remains devel until right up until release time. That shouldn't be too relevant for the EOL use case though. However it's worth pointing out that VERSION doesn't change when the branch gets cut.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, FYI, we discussed the missing version on docs.ansible.com. I forgot about this issue: #2211

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if it would be better to use the VERSION constant instead of the branch name?

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...

DOCSITE_EOL_CONFIG_PATH = DOCSITE_ROOT_DIR / 'end_of_life.toml'


@dataclass(frozen=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a nice default to have.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

from dataclasses import dataclass
from functools import cache
from pathlib import Path
from tomllib import loads as parse_toml_string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need to support Python 3.10?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The stable-2.13 is the only branch where we'd need to support Python 3.10. Initially I was thinking we'd want to merge this back to every branch for consistency. However it that requires extra dependencies for this extension I don't think it is worth it.

We can still declare the stable-2.13 branch in the toml file for completeness. And it would also be good to document the process to EOL docs in the README. We could just mention the "legacy" eol flag on those older branches. It's already set there anyway and there's no reason to ever revert it.

from dataclasses import dataclass
from functools import cache
from pathlib import Path
from tomllib import loads as parse_toml_string
Copy link
Contributor

Choose a reason for hiding this comment

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

The stable-2.13 is the only branch where we'd need to support Python 3.10. Initially I was thinking we'd want to merge this back to every branch for consistency. However it that requires extra dependencies for this extension I don't think it is worth it.

We can still declare the stable-2.13 branch in the toml file for completeness. And it would also be good to document the process to EOL docs in the README. We could just mention the "legacy" eol flag on those older branches. It's already set there anyway and there's no reason to ever revert it.

Comment on lines +14 to +25
[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',
]
Copy link
Contributor

Choose a reason for hiding this comment

The 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 VERSION constant instead of the branch name?

For example on the stable-2.18 branch we have this:

You can see the version for the package and the version for core more clearly.

I know the version remains devel until right up until release time. That shouldn't be too relevant for the EOL use case though. However it's worth pointing out that VERSION doesn't change when the branch gets cut.

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.

Comment on lines +14 to +25
[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',
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, FYI, we discussed the missing version on docs.ansible.com. I forgot about this issue: #2211

Copy link
Collaborator

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

LINT_FILES: tuple[str, ...] = (
so the repo's Python linters (black, ruff, isort) are applied?

Copy link
Member Author

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.


@cache
def _read_eol_data() -> EOLConfigType:
raw_config_dict = parse_toml_string(DOCSITE_EOL_CONFIG_PATH.read_text())
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 tomllib.load pattern instead of loading the whole text into memory first?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I also thought that it's probably not worth it.

) from proc_err


def _set_global_j2_context(app, config):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type annotation?

Copy link
Member Author

@webknjaz webknjaz Mar 7, 2025

Choose a reason for hiding this comment

The 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
def _set_global_j2_context(app, config):
def _set_global_j2_context(app: Sphinx, config: object) -> None:

Comment on lines +10 to +11
'stable-2.17',
'stable-2.16',
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
'stable-2.17',
'stable-2.16',

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Co-authored-by: Maxwell G <maxwell@gtmx.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc builds Relates to building the documentation tooling This PR affects tooling (CI, pr_labeler, noxfile, linters, etc.) but not the docs builds themselves.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants