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

Hosting: manual integrations via build contract #10127

Merged
merged 43 commits into from
Mar 20, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
5b28071
Hosting: manual integrations via build contract
humitos Mar 8, 2023
e18b40f
Use a single script to load everything
humitos Mar 8, 2023
7754d5f
Include Read the Docs analytics to integrations
humitos Mar 8, 2023
2925ed9
Initial work for hosting features
humitos Mar 8, 2023
3385649
External version banner and doc-diff integration
humitos Mar 8, 2023
1e391b8
Old version warning
humitos Mar 8, 2023
6afca0b
Do not inject doc-diff on search page
humitos Mar 8, 2023
7ce98a4
Inject old version warning only for non-external versions
humitos Mar 8, 2023
3ca96ec
Comments!
humitos Mar 8, 2023
f4f1268
More comments
humitos Mar 8, 2023
a596512
Build: pass `PATH` environment variable to Docker container
humitos Mar 9, 2023
33fdb2b
Lint: for some reason fails at CircleCI otherwise
humitos Mar 9, 2023
4ced5c3
Merge branch 'humitos/build-cmd-environment' of github.com:readthedoc…
humitos Mar 9, 2023
ea2af4c
Feature flag for new hosting integrations
humitos Mar 10, 2023
79b7393
Load `readthedocs-build.yaml` and generate `readthedocs-data.html`
humitos Mar 10, 2023
17effaf
Load READTHEDOCS_DATA async
humitos Mar 10, 2023
2b9cdbf
Absolute proxied API path
humitos Mar 10, 2023
0116f41
Remove duplicated code
humitos Mar 10, 2023
d5130cc
New approach using `readthedocs-client.js` and `/_/readthedocs-config…
humitos Mar 11, 2023
761e3b6
Do not require `readthedocs-build.YAML` for now
humitos Mar 11, 2023
bd9f70e
Expand the JSON response with more data
humitos Mar 13, 2023
842a228
Remove non-required files and rely on `readthedocs-client.js` only
humitos Mar 13, 2023
2ad30cc
Improve helper text
humitos Mar 13, 2023
89662fa
Builds: save `readthedocs-build.yaml` into database
humitos Mar 13, 2023
f83eee6
Use `Version.build_data` from the endpoint
humitos Mar 13, 2023
d14115a
Flyout: return data required to generate flyout dynamically
humitos Mar 13, 2023
067bb4c
Updates to the API
humitos Mar 14, 2023
17c1af3
Minor updates
humitos Mar 15, 2023
53366aa
Update the javascript client compiled version
humitos Mar 15, 2023
0f89186
doc-diff object returned
humitos Mar 16, 2023
48de597
Build: check if the YAML file exists before trying to open it
humitos Mar 16, 2023
4b05e77
Proxito: don't inject the header if the feature is turned off
humitos Mar 16, 2023
e90af75
Merge branch 'main' of github.com:readthedocs/readthedocs.org into hu…
humitos Mar 16, 2023
364de9c
Test: add hosting integrations tests
humitos Mar 16, 2023
c1cf8cb
Remove JS
humitos Mar 20, 2023
3930915
Load the javascript from a local server for now
humitos Mar 20, 2023
2d7b6fe
Update URL to remove .json from it
humitos Mar 20, 2023
ab43635
Remove non-required f-string
humitos Mar 20, 2023
85ab2e7
Allow saving `build_data` via API endpoint
humitos Mar 20, 2023
3439e24
Merge branch 'main' of github.com:readthedocs/readthedocs.org into hu…
humitos Mar 20, 2023
6154c7f
Lint
humitos Mar 20, 2023
0e8f245
Migrate nodejs installation to asdf
humitos Mar 20, 2023
a6c5977
Change port to match `npm run dev` from readthedocs-client
humitos Mar 20, 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
4 changes: 4 additions & 0 deletions readthedocs/doc_builder/director.py
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,10 @@ def store_readthedocs_build_yaml(self):
"readthedocs-build.yaml",
)

if not os.path.exists(yaml_path):
log.debug("Build output YAML file (readtehdocs-build.yaml) does not exist.")
return
humitos marked this conversation as resolved.
Show resolved Hide resolved

try:
with open(yaml_path, "r") as f:
Copy link
Member

Choose a reason for hiding this comment

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

We should use readthedocs.core.utils.filesystem.safe_open here

data = yaml.safe_load(f)
Expand Down
33 changes: 19 additions & 14 deletions readthedocs/doc_builder/environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -305,25 +305,30 @@ def run(self):
# Create a copy of the environment to update PATH variable
environment = self._environment.copy()
# Default PATH variable
environment[
"PATH"
] = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
# Add asdf extra paths
environment["PATH"] += (
":/home/{settings.RTD_DOCKER_USER}/.asdf/shims"
":/home/{settings.RTD_DOCKER_USER}/.asdf/bin"
# This default comes from our Docker image:
#
# $ docker run --user docs -it --rm readthedocs/build:ubuntu-22.04 /bin/bash
# docs@bfe702e31cdd:~$ echo $PATH
# /home/docs/.asdf/shims:/home/docs/.asdf/bin
# :/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
# docs@bfe702e31cdd:~$
asdf_paths = (
f"/home/{settings.RTD_DOCKER_USER}/.asdf/shims"
f":/home/{settings.RTD_DOCKER_USER}/.asdf/bin"
)

if settings.RTD_DOCKER_COMPOSE:
environment["PATH"] += ":/root/.asdf/shims:/root/.asdf/bin"
asdf_paths += ":/root/.asdf/shims:/root/.asdf/bin"

default_paths = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
environment["PATH"] = f"{asdf_paths}:{default_paths}"

# Prepend the BIN_PATH if it's defined
if self.bin_path:
path = environment.get("PATH")
bin_path = self._escape_command(self.bin_path)
environment["PATH"] = bin_path
if path:
environment["PATH"] += f":{path}"
original_path = environment.get("PATH")
escaped_bin_path = self._escape_command(self.bin_path)
environment["PATH"] = escaped_bin_path
if original_path:
environment["PATH"] = f"{escaped_bin_path}:{original_path}"

try:
exec_cmd = client.exec_create(
Expand Down
2 changes: 2 additions & 0 deletions readthedocs/projects/tests/test_build_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ def test_build_updates_documentation_type(self, load_yaml_config):
assert self.requests_mock.request_history[7]._request.method == "PATCH"
assert self.requests_mock.request_history[7].path == "/api/v2/version/1/"
assert self.requests_mock.request_history[7].json() == {
"build_data": None,
"built": True,
"documentation_type": "mkdocs",
"has_pdf": True,
Expand Down Expand Up @@ -457,6 +458,7 @@ def test_successful_build(
assert self.requests_mock.request_history[7]._request.method == "PATCH"
assert self.requests_mock.request_history[7].path == "/api/v2/version/1/"
assert self.requests_mock.request_history[7].json() == {
"build_data": None,
"built": True,
"documentation_type": "sphinx",
"has_pdf": True,
Expand Down
2 changes: 0 additions & 2 deletions readthedocs/proxito/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,6 @@ def add_hosting_integrations_headers(self, request, response):
project = Project.objects.get(slug=project_slug)
if project.has_feature(Feature.HOSTING_INTEGRATIONS):
response["X-RTD-Hosting-Integrations"] = "true"
else:
response["X-RTD-Hosting-Integrations"] = "false"

def process_response(self, request, response): # noqa
self.add_proxito_headers(request, response)
Expand Down
123 changes: 123 additions & 0 deletions readthedocs/proxito/tests/test_hosting.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
"""Test hosting views."""

import django_dynamic_fixture as fixture
import pytest
from django.conf import settings
from django.contrib.auth.models import User
from django.test import TestCase, override_settings
from django.urls import reverse

from readthedocs.builds.constants import EXTERNAL, INTERNAL, LATEST
from readthedocs.builds.models import Build
from readthedocs.projects.constants import PUBLIC
from readthedocs.projects.models import Project


@override_settings(
PUBLIC_DOMAIN="dev.readthedocs.io",
PUBLIC_DOMAIN_USES_HTTPS=True,
)
@pytest.mark.proxito
class TestReadTheDocsConfigJson(TestCase):
def setUp(self):
self.user = fixture.get(User, username="user")
self.user.set_password("user")
self.user.save()

self.project = fixture.get(
Project,
slug="project",
language="en",
privacy_level=PUBLIC,
external_builds_privacy_level=PUBLIC,
repo="git://10.10.0.1/project",
programming_language="words",
single_version=False,
users=[self.user],
main_language_project=None,
)
self.project.versions.update(
privacy_level=PUBLIC,
built=True,
active=True,
type=INTERNAL,
identifier="1a2b3c",
)
self.version = self.project.versions.get(slug=LATEST)
self.build = fixture.get(
Build,
version=self.version,
)

def test_get_config(self):
r = self.client.get(
reverse("proxito_readthedocs_config_json"),
{"url": "https://project.dev.readthedocs.io/en/latest/"},
secure=True,
HTTP_HOST="project.dev.readthedocs.io",
)
assert r.status_code == 200

expected = {
"comment": "THIS RESPONSE IS IN ALPHA FOR TEST PURPOSES ONLY AND IT'S GOING TO CHANGE COMPLETELY -- DO NOT USE IT!",
"project": {
"slug": self.project.slug,
"language": self.project.language,
"repository_url": self.project.repo,
"programming_language": self.project.programming_language,
},
"version": {
"slug": self.version.slug,
"external": self.version.type == EXTERNAL,
},
"build": {
"id": self.build.pk,
},
"domains": {
"dashboard": settings.PRODUCTION_DOMAIN,
},
"readthedocs": {
"analytics": {
"code": None,
}
},
"features": {
"analytics": {
"code": None,
},
"external_version_warning": {
"enabled": True,
"query_selector": "[role=main]",
},
"non_latest_version_warning": {
"enabled": True,
"query_selector": "[role=main]",
"versions": [
"latest",
],
},
"doc_diff": {
"enabled": True,
"base_url": "https://project.dev.readthedocs.io/en/latest/index.html",
"root_selector": "[role=main]",
"inject_styles": True,
"base_host": "",
"base_page": "",
},
"flyout": {
"translations": [],
"versions": [
{"slug": "latest", "url": "/en/latest/"},
],
"downloads": [],
"vcs": {
"url": "https://github.com",
"username": "readthedocs",
"repository": "test-builds",
"branch": self.version.identifier,
"filepath": "/docs/index.rst",
},
},
},
}
assert r.json() == expected
39 changes: 32 additions & 7 deletions readthedocs/proxito/views/hosting.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from readthedocs.builds.constants import EXTERNAL
from readthedocs.core.mixins import CDNCacheControlMixin
from readthedocs.core.resolver import resolver
from readthedocs.core.unresolver import unresolver

log = structlog.get_logger(__name__) # noqa
Expand All @@ -15,21 +16,29 @@
class ReadTheDocsConfigJson(CDNCacheControlMixin, View):
def get(self, request):

url = request.GET.get("url")
if not url:
return JsonResponse(
{"error": "'url' GET attribute is required"},
status=400,
)

unresolved_domain = request.unresolved_domain
project = unresolved_domain.project

# TODO: why the UnresolvedURL object is not injected in the `request` by the middleware.
# Is is fine to calculate it here?
unresolved_url = unresolver.unresolve_url(request.headers.get("Referer"))
unresolved_url = unresolver.unresolve_url(url)
version = unresolved_url.version

# TODO: use Referrer header or GET arguments for Version / Build
project.get_default_version()
build = version.builds.last()

# TODO: define how it will be the exact JSON object returned here
# NOTE: we could use the APIv3 serializers for some of these objects if we want to keep consistency.
# However, those may require some extra db calls that we probably want to avoid.
# NOTE: we could use the APIv3 serializers for some of these objects
# if we want to keep consistency. However, those may require some
# extra db calls that we probably want to avoid.
data = {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we're re-creating the footer API, but with JSON instead of HTML. That seems reasonable, but I do think we'll want to think more about what this API is returning.

It seems like we actually want the client to be able to tell us exactly what data points they want? Something similar to what we're doing with apiv3 with the expand and fields parameters? That way we give the caller control over how much data they want...

That might be an over-optimization at this stage, but I think we want to design this with expandability and performance in mind.

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 do think we'll want to think more about what this API is returning.

Yes. This is one part of the design decisions I mentioned we have to sit down, talk together, think about and decide what's the best structure.

It seems like we actually want the client to be able to tell us exactly what data points they want? Something similar to what we're doing with apiv3 with the expand and fields parameters? That way we give the caller control over how much data they want...

This is a good point to start considering and thinking about, yeah. However, I don't think we will require that complexity because:

  • the API endpoint will be cached
  • most of the values are static
  • there are a few db queries only (Project and Project.versions(active=True))
  • we could decide whether or not return some specific data based on the feature being enabled. If non_latest_version_warning is disabled, we don't calculate/query nor return the list of active versions this feature consumes in the client.

In any case, I think we will arrive at the best answer by exploration, experimentation and brainstorming. Also, once we deploy this, we will have some data in NR to know how much time this endpoint takes on a real production db.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, I don't think the performance of this approach is inherently bad, but more that we might want to make this look more like an API for additional UX options in the future. No user will expect passing GET args to a .json file. However, if the file doesn't grow too large, always returning all of it too bad. But I do expect we'll feel constrained by the design in the future, as we want to make this endpoint smarter, so starting with an API instead of a JSON file seems like the proper design to me.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I'm fine shipping this as a test for now.

But I do think over time if this is the "one endpoint to get all the data you need in the page", there will be demands on this functionality that are resource intensive, vary by user, etc. That is where we'll start to get value from the API approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point about the .json name in the URL endpoint. We should definitely change that as first step.

I will think a little more about the expand argument. I don't have a strong opinion on that yet 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

When we get to discussing the API structure in the next phase, this would be a good conversation. This API could help us avoid APIv3 complexity.

Also on the difference between this and our API, would it make sense to use DRF APIv3 resources directly in the response here, or make this an APIv3 endpoint? This way weren't not making a 4th API version of sorts. The response for versions could be identical to the APIv3 response for versions as well.

We'll come back to the data security question around private projects/versions pretty quick here, but we need to already for this API endpoint and private projects.

"comment": (
"THIS RESPONSE IS IN ALPHA FOR TEST PURPOSES ONLY"
Expand Down Expand Up @@ -68,12 +77,28 @@ def get(self, request):
"enabled": True,
"query_selector": "[role=main]",
"versions": list(
project.versions.filter(active=True).values_list(
"slug", flat=True
)
project.versions.filter(active=True)
.only("slug")
.values_list("slug", flat=True)
),
},
"doc_diff": True,
"doc_diff": {
"enabled": True,
# "http://test-builds-local.devthedocs.org/en/latest/index.html"
"base_url": f"""{resolver.resolve(
project=project,
version_slug=project.get_default_version(),
language=project.language,
filename=unresolved_url.filename,
)}""",
humitos marked this conversation as resolved.
Show resolved Hide resolved
"root_selector": "[role=main]",
"inject_styles": True,
# NOTE: `base_host` and `base_page` are not required, since
# we are constructing the `base_url` in the backend instead
# of the frontend, as the doc-diff extension does.
"base_host": "",
"base_page": "",
},
"flyout": {
"translations": [],
"versions": [
Expand Down