Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Hosting: manual integrations via build contract #10127
Changes from 34 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
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The 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
npm run build
from another repository.There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The 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.
There was a problem hiding this comment.
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
hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
.readthedocs.yaml
config, then YAML makes sense 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.yaml
is important for ourselves and our users as well. That was one of the reasons why I picked YAML for this file as well. Otherwise, you end up with things like this in JSON: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 comment
The 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 👍