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

Proxito: always check 404/index.hmtml #9983

Merged
merged 8 commits into from
Feb 7, 2023
Merged

Proxito: always check 404/index.hmtml #9983

merged 8 commits into from
Feb 7, 2023

Conversation

humitos
Copy link
Member

@humitos humitos commented Feb 3, 2023

With the introduction of build.commands we cannot use version.documentation_type anymore since those versions will be generic and we can't skip checking for this file location.

Note this commit may add and extra call to S3 API for all the 404 pages where our regular Maze will be shown. However, it removes 2 databsae calls from all the 404 requests.

We could only add this extra check on S3 for
version.documentation_type='generic', but that would make the code a little more complex and we won't be removing these 2 db queries.

Reference: readthedocs/sphinx-notfound-page#215 (comment)

With the introduction of `build.commands` we cannot use
`version.documentation_type` anymore since those versions will be `generic` and
we can't skip checking for this file location.

Note this commit may add and extra call to S3 API for all the 404 pages where
our regular Maze will be shown. However, it removes 2 databsae calls from all
the 404 requests.

We could only add this extra check on S3 for
`version.documentation_type='generic'`, but that would make the code a little
more complex and we won't be removing these 2 db queries.

Reference: readthedocs/sphinx-notfound-page#215 (comment)
@humitos humitos requested a review from a team as a code owner February 3, 2023 12:11
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.

Note this commit may add and extra call to S3 API for all the 404 pages where our regular Maze will be shown. However, it removes 2 databsae calls from all the 404 requests.

Can you explain this a bit more? I don't see the 2 removals.

Overall this is the right approach. Let's just serve what is on the filesystem at either reasonable path, and not try to be magical here.

Do we need to update the docs to note that secondary location?

@humitos
Copy link
Member Author

humitos commented Feb 7, 2023

Can you explain this a bit more? I don't see the 2 removals.

We are removing the db queries to know the documentation_type of the 2 versions:

Do we need to update the docs to note that secondary location?

Yes. Good point.

@humitos humitos requested a review from a team as a code owner February 7, 2023 08:38
@ericholscher
Copy link
Member

Oh, database calls, not S3 calls :) That makes more sense.

@ericholscher
Copy link
Member

Also @benjaoming, note the docs update here. Another point for getting our branch merged into main so we can stop diverging.

@benjaoming
Copy link
Contributor

oh there's been quite a lot of those already, I merge main into diataxis/main frequently. Will do that now 👍

@ericholscher
Copy link
Member

ericholscher commented Feb 7, 2023

Two steps ahead! :)

@benjaoming
Copy link
Contributor

Wow, no conflicts. Very elegant @humitos 😍

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.

None yet

3 participants