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

Port additional features to proxito #6286

Merged
merged 24 commits into from
Nov 18, 2019
Merged

Port additional features to proxito #6286

merged 24 commits into from
Nov 18, 2019

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 14, 2019

This moves over almost all of the doc serving logic from Django into Proxito. Most importantly:

  • sitemap handling
  • robots.txt handling
  • 404.html handling for users

It also fixes some of the bugs that we saw during the initial deployment:

  • Improperly serving /en/latest/foo where it should serve foo/index.html
  • Not having support for README.html as an index file

This required additional nginx configuration changes, which I will put in another PR in ops. It would be cool to be able to test them against this, but not quite there yet.

Closes #6331
Closes #6332

@ericholscher ericholscher changed the title Clean up proxito logging Clean up proxito code Oct 15, 2019
@ericholscher ericholscher added the PR: hotfix Pull request applied as hotfix to release label Oct 17, 2019
@ericholscher ericholscher removed the PR: hotfix Pull request applied as hotfix to release label Oct 17, 2019
@ericholscher ericholscher changed the title Clean up proxito code Port additional features to proxito Oct 17, 2019
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.

Look good to me! I left some comments about style or to improve tests readability.

I can do some QA once we have the PR for ops with the NGINX required changes.

readthedocs/proxito/tests/test_full.py Show resolved Hide resolved
readthedocs/proxito/tests/test_full.py Outdated Show resolved Hide resolved
Comment on lines +220 to +222
self.assertEqual(
response.status_code, 404
)
Copy link
Member

Choose a reason for hiding this comment

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

We may want to assert that we are receiving the content of the custom page here as well and not the default maze.

readthedocs/proxito/tests/test_full.py Outdated Show resolved Hide resolved
readthedocs/proxito/views.py Outdated Show resolved Hide resolved
readthedocs/proxito/views.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member

humitos commented Nov 7, 2019

I can do some QA once we have the PR for ops with the NGINX required changes.

robots.txt, sitemap.xml and custom 404 (on latest, on specific version and not on latest) work properly with the NGINX fallback.

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.

Everything looks good to me!

@ericholscher ericholscher merged commit c9e9070 into master Nov 18, 2019
@ericholscher ericholscher deleted the proxito-cleanup branch November 18, 2019 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle serving README.html as default index file with Proxito Handle dirhtml builds with El Proxito
2 participants