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

Use different setting for footer api url #6131

Merged
merged 9 commits into from Nov 4, 2019

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Sep 3, 2019

Currently we use 'api_host' in our js to call the footer api.
I'm making a separate setting just for the footer api.
This is to allow us to proxy the footer api from the same domain that
is serving the docs (this is useful for .com).

We can't proxy the whole api from the serve app because there are some
places where we call reverse, and on the serve app we don't have
access to all urls (like project dashboard).

We need to publish the sphinx-extension with this change first.

Are we ok with this? Other option is to pass the whole endpoint (including /api/v2/footer_html/)

Currently we use 'api_host' in our js to call the footer api.
I'm making a separate setting just for the footer api.
This is to allow us to proxy the footer api from the same domain that
is serving the docs (this is useful for .com).

We can't proxy the whole api from the serve app because there are some
places where we call `reverse`, and on the serve app we don't have
access to all urls (like project dashboard).

We need to publish the sphinx-extension with this change first.
stsewd added a commit to stsewd/readthedocs-sphinx-ext that referenced this issue Sep 3, 2019
@stsewd stsewd requested review from ericholscher, humitos and and removed request for humitos Sep 3, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Sep 5, 2019

I don't like a one-off setting like this. I'd much prefer a pattern that is more generic that we can move to over time. I think something like proxied_api_host or similar, so that we can reuse it later if we start proxying more API's on the users domains.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 5, 2019

@ericholscher done. PR for the sphinx-ext readthedocs/readthedocs-sphinx-ext#77

Note: the ext should be released first, otherwise it wouldn't set the proxied_api_host attr on the js object.

@humitos
Copy link
Member

@humitos humitos commented Sep 5, 2019

If I understand correctly, the idea here is to hit the endpoint of the footer_api from the serve instance instead of the main instance (so we can apply authentication) --is that correct? In case it is, don't we need to use a different path as well that includes the DOC_PATH_PREFIX?

I'm thinking that proxied_api_host will be docs.company.org and the full URL for the endpoint should be https://docs.company.org/_/api/v2/footer_html/.

Unlikely, but possible, https://docs.company.org/api/v2/footer_html/ could be a valid page.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 5, 2019

@humitos yes is to hit the api from the serve instance. For .org it would point to the public api on readthedocs.org/api/. For .com it would be an absolute url without domain and using the DOC_PATH_PREFIX. So, for .com it would be /_/ -> foo.com/_/api/v2/...

humitos
humitos approved these changes Sep 9, 2019
Copy link
Member

@humitos humitos left a comment

The changes look good.

It would be good to have some explanation in code comments about this because it's hard to differentiate api_host and proxied_api_host just reading the code.

readthedocs/settings/base.py Show resolved Hide resolved
@stale
Copy link

@stale stale bot commented Oct 24, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale label Oct 24, 2019
@stsewd stsewd added Needed: design decision and removed Status: stale labels Oct 24, 2019
Copy link
Member

@ericholscher ericholscher left a comment

This looks 💯

readthedocs/settings/base.py Outdated Show resolved Hide resolved
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Nov 1, 2019

We need to wait to merge this until the backend is working, correct?

Edit: I guess not, since it defaults to RTD.org, so it should keep working.

stsewd and others added 2 commits Nov 4, 2019
Co-Authored-By: Eric Holscher <25510+ericholscher@users.noreply.github.com>
@stsewd stsewd merged commit f897447 into readthedocs:master Nov 4, 2019
2 checks passed
@stsewd stsewd deleted the use-setting-for-footer-api branch Nov 4, 2019
stsewd added a commit to stsewd/readthedocs-sphinx-ext that referenced this issue Nov 5, 2019
@stsewd
Copy link
Member Author

@stsewd stsewd commented Nov 5, 2019

@ericholscher this one can be reviewed/merged now readthedocs/readthedocs-sphinx-ext#77

stsewd added a commit that referenced this issue Mar 10, 2020
This reverts:
- #6355
- #6131

And force the use of the proxied api to all docs.
This wasn't done before because .com wasn't ready.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: design decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants