Skip to content

Proxito: refactor doc serving #9726

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

Merged
merged 7 commits into from
Feb 1, 2023
Merged

Proxito: refactor doc serving #9726

merged 7 commits into from
Feb 1, 2023

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 10, 2022

This is so it's easier to share this code with the new implementation.

  • We were repeating the same steps before serving a file (getting the final URL from storage, joining paths, making it up for the index, etc). Now all those steps are inside the serving code.
  • Now we have a low level _serve_file method, to serve any file from storage, and 3 high level methods to serve files from docs, static files, or downloads.
  • Serving docs now moves around having a project, version, and file
  • Serving downloads moves around having a project, version, and type of download.
  • Serving static files moves around having just a file.

@stsewd stsewd force-pushed the refactor-file-serve branch 2 times, most recently from 9f5061e to a250642 Compare November 10, 2022 23:33
@stsewd stsewd force-pushed the refactor-file-serve branch from a250642 to d7f6155 Compare November 10, 2022 23:39
@stsewd stsewd marked this pull request as ready for review November 23, 2022 19:25
@stsewd stsewd requested a review from a team as a code owner November 23, 2022 19:25
@stsewd stsewd requested a review from humitos November 23, 2022 19:25
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

This is really good! 💯

I understand that there weren't logic changes here and most of it is moving code from one place to another. So, I didn't go too deep into reviewing the logic and everything. I trust on our test suite and in you 😄

I left some comments that will improve the reading of this code next time we have to deal with it and also ideas to apply some simplifications. Let me know what you think about them.

Comment on lines 117 to 119
Serve from the filesystem if using ``PYTHON_MEDIA``. We definitely
shouldn't do this in production, but I don't want to force a check for
``DEBUG``.
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 probably kill PYTHON_MEDIA now that we have all the set ups using Docker with NGINX 😄

@stsewd stsewd merged commit 093e1c2 into main Feb 1, 2023
@stsewd stsewd deleted the refactor-file-serve branch February 1, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants