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

Proxy footer api on docs' domains #6630

Merged
merged 20 commits into from
Mar 9, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Feb 5, 2020

Still need to add tests.

Also, since we are serving everything from proxito, I'm adding the proxied urls only there. This also allows us to override it from corporate.

@stsewd stsewd added the Needed: tests Tests are required label Feb 5, 2020
Copy link
Member

@ericholscher ericholscher 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 a good start, but lets go ahead and start proxying everything we need for doc pages, and then we can adapt it later if needed.

from .views.proxied import ProxiedFooterHTML

api_footer_urls = [
url(r'footer_html/', ProxiedFooterHTML.as_view(), name='footer_html'),
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 add search in here while we are updating it. Probably also the sustainability API for completeness.


# DRF has BasicAuthentication and SessionAuthentication as default classes.
# We don't support neither in the community site.
authentication_classes = []
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we don't want them to be present on the footer? Seems like it doesn't hurt anything to have them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not strong reason on that I guess, I can add it back (or remove the line actually so it takes the defaults)

readthedocs/proxito/urls.py Outdated Show resolved Hide resolved
stsewd added a commit to stsewd/readthedocs.org that referenced this pull request Feb 6, 2020
In proxito/serve app we don't have access to the urlconf from the main
app. So we need to hardcode the urls, since this template is used in
both apps (main and proxito).

This is needed for readthedocs#6630
stsewd added a commit to stsewd/readthedocs.org that referenced this pull request Feb 6, 2020
This is needed to reuse these tests for the proxied footer html

- All calls to get are replaced for .render() so it can be overridden
(`HTTP_HOST=self.host`).
- The url isn't hardcoded
- For some reason if the class is inherited, the fixtures don't work.
  So I just used django_dynamic_fixture.

Ref readthedocs#6630
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks good once tests pass 👍 Feels pretty complex, so I'm excited to hopefully have this somewhat simplified in the future.

@stsewd stsewd changed the title Proxi footer api on docs' domains Proxy footer api on docs' domains Feb 12, 2020
@ericholscher
Copy link
Member

@stsewd can we fix the tests here, or is #6644 needed?

# the Corporate site we don't define this URL if ``-ext`` module is not
# installed
cls.url = reverse('doc_search')
self.url = reverse('doc_search')
Copy link
Member Author

Choose a reason for hiding this comment

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

Tests were failing bc the tests for proxied api were using doc_search from readthedocs.urls for both classes, instead of evaluating it for each class.

@stsewd stsewd removed the Needed: tests Tests are required label Feb 27, 2020
@ericholscher
Copy link
Member

@stsewd this PR doesn't change the URL the client code uses, are we planning to deploy that after we've confirmed the proxying works?

@ericholscher ericholscher merged commit 23f9d19 into readthedocs:master Mar 9, 2020
@stsewd stsewd deleted the proxi-api-on-docs-domain branch March 10, 2020 00:20
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.

2 participants