Skip to content

Proxito: do not serve non-existent versions #9048

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 4 commits into from
Apr 4, 2022

Conversation

humitos
Copy link
Member

@humitos humitos commented Mar 29, 2022

Avoid serving files that are still in the storage (S3) for some reason but its
associated version was deleted or de-activated.

Closes https://github.com/readthedocs/readthedocs-ops/issues/1139

@humitos humitos requested a review from a team as a code owner March 29, 2022 14:20
# Skip serving versions that don't exist or are not active. This is to
# avoid serving files that we have in the storage, but its associated
# version does not exist anymore or it was de-activated
if not version or not version.active:
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious how this will impact our 404's. It's definitely the right approach, but something to watch on deployment.

Avoid serving files that are still in the storage (S3) for some reason but its
associated version was deleted or de-activated.

Closes readthedocs/readthedocs-ops#1139
@humitos humitos force-pushed the humitos/dont-serve-inactive-non-existent-versions branch from 305b6d3 to 7d238cf Compare March 30, 2022 14:39
@humitos humitos enabled auto-merge March 30, 2022 14:40
@humitos humitos self-assigned this Mar 30, 2022
@humitos
Copy link
Member Author

humitos commented Mar 31, 2022

@ericholscher please take another look at this. There were some tests failing and when I review them it seemed that I had a mistake in the logic that returns 404s. I've updated that logic to handle all the cases:

  • the version does exist and is marked as inactive
  • the version does not exist but a version slug was detected in the URL

This logic does not interfere with the case:

  • the version does not exist and there is no version slug detected in the URL --which is the case when the domain is accessed at / directly and it's a valid case that needs to be redirected to the default version.

I'm not sure if I was able to explain this here or even in the comment. Feel free to make a suggestion if you find a better way to explain this 😄

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.

I think this looks good, but we definitely want to watch this closely. I feel like there's a non-zero chance of us accidently breaking single version projects or similar.

humitos and others added 2 commits April 1, 2022 12:04
@humitos humitos merged commit 678870f into main Apr 4, 2022
@humitos humitos deleted the humitos/dont-serve-inactive-non-existent-versions branch April 4, 2022 08:58
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