-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Hosting: manual integrations via build contract #10127
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
Changes from 29 commits
5b28071
e18b40f
7754d5f
2925ed9
3385649
1e391b8
6afca0b
7ce98a4
3ca96ec
f4f1268
a596512
33fdb2b
4ced5c3
ea2af4c
79b7393
17effaf
2b9cdbf
0116f41
d5130cc
761e3b6
bd9f70e
842a228
2ad30cc
89662fa
f83eee6
d14115a
067bb4c
17c1af3
53366aa
0f89186
48de597
4b05e77
e90af75
364de9c
c1cf8cb
3930915
2d7b6fe
ab43635
85ab2e7
3439e24
6154c7f
0e8f245
a6c5977
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,22 @@ | ||
# Generated by Django 3.2.18 on 2023-03-13 15:15 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("builds", "0047_build_default_triggered"), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name="version", | ||
name="build_data", | ||
field=models.JSONField( | ||
default=None, | ||
null=True, | ||
verbose_name="Data generated at build time by the doctool (`readthedocs-build.yaml`).", | ||
), | ||
), | ||
] |
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 want to ship this with the app, or version it directly on S3, like we do with the ad client? I think keeping it deployable outside of the application seems right to me. https://github.com/readthedocs/ethical-ad-client/tags https://media.ethicalads.io/media/client/v1.4.0/ethicalads.min.js 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 haven't thought too much about this and I don't have experience deploying the script with a different process than the normal one. I'm not sure about the pros/cons here. I put this file here because we need it for development as well. We could just put this file in the local MinIO S3, tho, as well. Note this file is generated by running 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. We need to also think about what version are we going to serve by default. The latest one? Would people be able to pin to a particular version? Are we going to support multiple versions at the same time? How do we deploy new features to those that are pinned to an older version? Do we care? Too many questions 🤷🏼 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 think we always deploy the latest, but if the goal is for other people to integrate the library, then it can be versioned based on their needs. The versioning is primarily valuable for strict SRI protection, like PyPI does, to validate the hash of the library hasn't changed for security reasons. I think we should definitely deploy the client outside of our application. We don't need to decide on a proper deployment pattern yet though, but I think we should keep it out of the application from the start. For now, we can just manually upload it to S3, and use that everywhere? |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,6 +2,7 @@ | |||||||||
import tarfile | ||||||||||
|
||||||||||
import structlog | ||||||||||
import yaml | ||||||||||
from django.conf import settings | ||||||||||
from django.utils.translation import gettext_lazy as _ | ||||||||||
|
||||||||||
|
@@ -187,6 +188,7 @@ def build(self): | |||||||||
self.build_epub() | ||||||||||
|
||||||||||
self.run_build_job("post_build") | ||||||||||
self.store_readthedocs_build_yaml() | ||||||||||
|
||||||||||
after_build.send( | ||||||||||
sender=self.data.version, | ||||||||||
|
@@ -392,6 +394,7 @@ def run_build_commands(self): | |||||||||
# Update the `Version.documentation_type` to match the doctype defined | ||||||||||
# by the config file. When using `build.commands` it will be `GENERIC` | ||||||||||
self.data.version.documentation_type = self.data.config.doctype | ||||||||||
self.store_readthedocs_build_yaml() | ||||||||||
|
||||||||||
def install_build_tools(self): | ||||||||||
""" | ||||||||||
|
@@ -625,3 +628,33 @@ def get_build_env_vars(self): | |||||||||
def is_type_sphinx(self): | ||||||||||
"""Is documentation type Sphinx.""" | ||||||||||
return "sphinx" in self.data.config.doctype | ||||||||||
|
||||||||||
def store_readthedocs_build_yaml(self): | ||||||||||
# load YAML from user | ||||||||||
yaml_path = os.path.join( | ||||||||||
self.data.project.artifact_path( | ||||||||||
version=self.data.version.slug, type_="html" | ||||||||||
), | ||||||||||
"readthedocs-build.yaml", | ||||||||||
) | ||||||||||
|
||||||||||
try: | ||||||||||
with open(yaml_path, "r") as f: | ||||||||||
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. We should use |
||||||||||
data = yaml.safe_load(f) | ||||||||||
except Exception: | ||||||||||
# NOTE: skip this work for now until we decide whether or not this | ||||||||||
humitos marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
# YAML file is required. | ||||||||||
# | ||||||||||
# NOTE: decide whether or not we want this | ||||||||||
# file to be mandatory and raise an exception here. | ||||||||||
return | ||||||||||
|
||||||||||
log.info("readthedocs-build.yaml loaded.", path=yaml_path) | ||||||||||
|
||||||||||
# TODO: validate the YAML generated by the user | ||||||||||
# self._validate_readthedocs_build_yaml(data) | ||||||||||
|
||||||||||
# Copy the YAML data into `Version.build_data`. | ||||||||||
# It will be saved when the API is hit. | ||||||||||
# This data will be used by the `/_/readthedocs-config.json` API endpoint. | ||||||||||
self.data.version.build_data = data | ||||||||||
Comment on lines
+661
to
+664
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 want to actually parse the data into memory, or just store the file contents directly as a string? I think we just want to store a string to start? I'd like to avoid as much YAML parsing as possible... I also wonder if we should make this file JSON, instead of YAML? If the goal is for it to be aligned with the JSON data returned via the API, I think that makes more sense. But if it's closer to our 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 think we will need to parse it so we can validate it at some point, anyways. This also allows us to use a JSON field in the database that we can query in the future, looking for answers.
I decide to use YAML here on purpose. It's a lot easier to write than JSON, a lot less nit picking (e.g. requires no trailing comma in the last element of a list), supports comments, works better with more data types, and others. The structure is going to be just a dictionary, YAML is going to be just the representation/serialization of it In particular, being able to put comments in readthedocs.org/readthedocs/proxito/views/hosting.py Lines 36 to 39 in 48de597
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. Yea.. let's use YAML for anything human-writable, and JSON for machine-writable 👍 |
Uh oh!
There was an error while loading. Please reload this page.