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

docs: set the READTHEDOCS context variable #1410

Merged

Conversation

tonyandrewmeyer
Copy link
Contributor

As of October 7th, the html_context dictionary is meant to contain a READTHEDOCS key set to True if that's in the environment variables, so this change does that.

We may see small changes in the visual look of the docs but everything should keep working with this small adjustment.

Instructions from Read the Docs

Fixes #1393

@james-garner-canonical
Copy link
Contributor

I read the linked page on read the docs and had a quick look at our custom_conf.py.

I'm guessing we need this change that Read the Docs suggests in "the most common changes you may need to do in your Sphinx's configuration file" (adding the READTHEDOCS environment variable to our html_context dict) because Spinx uses Jinja internally -- the change is commented in the article with the note "Tell Jinja2 templates the build is running on Read the Docs"?

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks!

@tonyandrewmeyer
Copy link
Contributor Author

I'm guessing we need this change that Read the Docs suggests in "the most common changes you may need to do in your Sphinx's configuration file" (adding the READTHEDOCS environment variable to our html_context dict) because Spinx uses Jinja internally -- the change is commented in the article with the note "Tell Jinja2 templates the build is running on Read the Docs"?

Yes, I think so. My understanding is that rtd does some post processing (for the PR banner for example) that's moving to addons and so we need to have this signal there. I couldn't find anything more specific than what's in the linked post (although I admittedly didn't look for ages), and it's tricky to dig into since I'm expecting to see no changes and that could also be from doing something that's ignored...

Copy link
Contributor

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

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

Thanks for the info, Tony. The change makes sense to me, and I don't think there's any harm in including the environment variable in the html_context if it's not used. At the very least it should future proof us to some extent if we lean on more rtd features in future.

@tonyandrewmeyer tonyandrewmeyer merged commit 52c91cb into canonical:main Oct 8, 2024
30 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the rtd-addons-by-default-1393 branch October 8, 2024 00:14
tonyandrewmeyer added a commit to tonyandrewmeyer/operator that referenced this pull request Oct 9, 2024
As of October 7th, the `html_context` dictionary is meant to contain a
`READTHEDOCS` key set to True if that's in the environment variables, so
this change does that.

We may see small changes in the visual look of the docs but everything
should keep working with this small adjustment.

[Instructions from Read the
Docs](https://about.readthedocs.com/blog/2024/07/addons-by-default/)

Fixes canonical#1393
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update docs to handle readthedocs addons change (by October 7, 2024)
3 participants