Skip to content

Deprecation: remove "use system packages" (python.system_packages config key and UI checkbox) #10562

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Aug 22, 2023
Merged

Deprecation: remove "use system packages" (python.system_packages config key and UI checkbox) #10562

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9275a56
Docs: do not mention `python.system_packages`
humitos Jul 24, 2023
c513568
Build: remove "use system package" option
humitos Aug 3, 2023
82b5ec4
Merge branch 'main' into humitos/config-system-packages
humitos Aug 3, 2023
6b0230c
Do not remove the DB field for now
humitos Aug 3, 2023
75bcc32
Remove `use_system_site_packages` leftovers
humitos Aug 3, 2023
4561971
Display custom error messages based on the config key
humitos Aug 3, 2023
26d0748
Apply suggestions from code review
humitos Aug 7, 2023
cebb79e
Minor code style changes
humitos Aug 7, 2023
71904ad
Update readthedocs/config/config.py
humitos Aug 9, 2023
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
53 changes: 36 additions & 17 deletions readthedocs/config/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

"""Build configuration for rtd."""

import collections
import copy
import os
import re
Expand Down Expand Up @@ -128,14 +129,43 @@ class InvalidConfig(ConfigError):

"""Error for a specific key validation."""

message_template = 'Invalid "{key}": {error}'
# Define the default message to show on ``InvalidConfig``
default_message_template = 'Invalid configuration option "{key}"'

# Create customized message for based on each particular ``key``
message_templates = collections.defaultdict(lambda: "{default_message}: {error}")

# Redirect the user to the blog post when using
# `python.system_packages` or `python.use_system_site_packages`
message_templates.update(
{
"python.system_packages": "{default_message}. "
"This was an old config key that's not supported anymore. "
"Please, refer to https://blog.readthedocs.com/use-system-packages-deprecated/ to read more about it and how to upgrade your config file." # noqa
}
)
message_templates.update(
{
"python.use_system_site_packages": message_templates.get(
"python.system_packages"
)
}
)

def __init__(self, key, code, error_message, source_file=None):
self.key = key
self.code = code
self.source_file = source_file
message = self.message_template.format(
key=self._get_display_key(),

display_key = self._get_display_key()
default_message = self.default_message_template.format(
key=display_key,
code=code,
error=error_message,
)
message = self.message_templates[display_key].format(
default_message=default_message,
key=display_key,
code=code,
error=error_message,
)
Expand Down Expand Up @@ -211,18 +241,10 @@ def __init__(self, env_config, raw_config, source_file, base_path=None):

def error(self, key, message, code):
"""Raise an error related to ``key``."""
if not os.path.isdir(self.source_file):
source = os.path.relpath(self.source_file, self.base_path)
error_message = '{source}: {message}'.format(
source=source,
message=message,
)
else:
error_message = message
raise InvalidConfig(
key=key,
code=code,
error_message=error_message,
error_message=message,
source_file=self.source_file,
)

Expand Down Expand Up @@ -1257,18 +1279,15 @@ def validate_keys(self):
This should be called after all the validations are done and all keys
are popped from `self._raw_config`.
"""
msg = (
'Invalid configuration option: {}. '
'Make sure the key name is correct.'
)
msg = "Make sure the key name is correct."
# The version key isn't popped, but it's
# validated in `load`.
self.pop_config('version', None)
wrong_key = '.'.join(self._get_extra_key(self._raw_config))
if wrong_key:
self.error(
wrong_key,
msg.format(wrong_key),
msg,
code=INVALID_KEY,
)

Expand Down
7 changes: 5 additions & 2 deletions readthedocs/config/tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ def test_correct_error_when_source_is_dir(self, tmpdir):
build.error(key='key', message='Message', code='code')
# We don't have any extra information about
# the source_file.
assert str(excinfo.value) == 'Invalid "key": Message'
assert str(excinfo.value) == 'Invalid configuration option "key": Message'

def test_formats_check_valid(self):
build = self.get_build_config({'formats': ['htmlzip', 'pdf', 'epub']})
Expand Down Expand Up @@ -1471,7 +1471,10 @@ def test_python_install_requirements_error_msg(self, tmpdir):
with raises(InvalidConfig) as excinfo:
build.validate()

assert str(excinfo.value) == 'Invalid "python.install[0].requirements": expected string'
assert (
str(excinfo.value)
== 'Invalid configuration option "python.install[0].requirements": expected string'
)

def test_python_install_requirements_does_not_allow_empty_string(self, tmpdir):
build = self.get_build_config(
Expand Down
8 changes: 7 additions & 1 deletion readthedocs/templates/builds/build_detail.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,13 @@ <h3>{% trans "Build Errors" %}</h3>
<h3>{% trans "Error" %}</h3>
<p class="build-error"
data-bind="text: error">
{{ build.error }}
{#
I'd like to use ``build.error|urlize`` here, so we can have nice links.
However, this is not possible because we are using `data-bind="text: error"`
which means that Knockout.js will use the `.error` attribute to fill
Copy link
Member

Choose a reason for hiding this comment

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

What error attribute? On the build itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

If you would like this, we should make the APIv3 response match what we want, perhaps applying the filter on the field in the response there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm.. I don't think the API should return blob HTML tags on its response.

Copy link
Contributor

@agjohnson agjohnson Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Similar to templates and HTMX, the front end should only have to display the data from the backend application. I don't think we should have a front end reimplementation of urlize, which is required here to show build.error in the same way the templates would render it.

On the implementation side, I think we should be authoring error message to be a bit more rich than what we're doing now. Relying on urlize results in error message HTML like:

Something happened. See our documentation on webhooks at <a href="https://docs.readthedocs.io/page/something.html">https://docs.readthedocs.io/page/something.html</a>

This is not super user friendly, but is also not accessible either. Using HTML in the error would be better, but does require some translation to HTML at some point:

Something happened. <a href="https://docs.readthedocs.io/page/something.html">Read our documentation on webhooks</a> for more information

This is perhaps another good point for the notification redesign.

Copy link
Member Author

Choose a reason for hiding this comment

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

Similar to templates and HTMX, the front end should only have to display the data from the backend application

I think this is out of discussion. We are not using HTMX which requires specific endpoints for its purpose. Besides, here we are talking about a resource API that exposes data to our users. If we want to start exposing "data to be rendered by the frontend without any modification" that's a whole different use case than what the API was thought for.

In the case we want move in that direction,we will need to add _display fields (or similar) to all the things we want to expose to user as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know we're not using HTMX, but we've talked about this approach specifically. Templates are more applicable then, and maintain a similar opinionated approach. My point is that we should do what we can at the backend in order to keep our frontend footprint low.

I think we talk about this more as part of the notification refactor either way. I just brought this up here to note that replicating urlize in front end code is not a great path forward.

If we want to start exposing "data to be rendered by the frontend without any modification"

I don't feel I'm talking about the same thing here. I am only referring to text with HTML links embedded, not any more structure than that.

Besides, here we are talking about a resource API that exposes data to our users

Well, I'd certainly say our API is dual function. The API needs to be useful for our own application too, it's not just a user resource API.

that's a whole different use case than what the API was thought for

Perhaps, but our application is also a consumer of this API. We don't need all the features exposed to end users though.

But if not the current API, it should be something on the application side. This can be separate, a different API or API-like endpoint, but it should be part of the application. I guess I don't see a strong reason for avoiding APIv3 though.

In the case we want move in that direction,we will need to add _display fields (or similar)

That seems fine, I don't have strong opinions on structure. We're not solving this problem here, so it's worth thinking about more. I'm just noting here as I think we should change our patterns, and the pattern noted in the PR here, in our notification refactor.

the content of this tag dynamically.
#}
{{ build.error|urlize }}
</p>
<p>
{% block github_issue_link %}
Expand Down