Skip to content

Old Pull Request previews are infinite redirecting #9109

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.

Sign up for GitHub

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

Jump to bottom
Closed
ericholscher opened this issue Apr 13, 2022 · 2 comments · Fixed by #9128
Closed

Old Pull Request previews are infinite redirecting #9109

ericholscher opened this issue Apr 13, 2022 · 2 comments · Fixed by #9128
Assignees
@humitos
Labels
Accepted Accepted issue on our roadmap Bug A bug

Comments

@ericholscher
Copy link
Member

ericholscher commented Apr 13, 2022 •
edited by humitos
Loading

This is an outcome of #9048, but we need to figure out how to solve this issue. We could special-case external versions to avoid this logic, or the PR build domain, but I think we probably need to think a bit more about this issue.

We do note that this should continue to work in the docs:

The built documentation is kept for 90 days after the pull request has been closed or merged.
https://docs.readthedocs.io/en/latest/pull-requests.html#limitations

Example:
@ericholscher ericholscher moved this to Planned in 📍Roadmap Apr 13, 2022
@ericholscher ericholscher added the Bug A bug label Apr 13, 2022
@humitos humitos added the Accepted Accepted issue on our roadmap label Apr 14, 2022
@humitos
Copy link
Member

humitos commented Apr 14, 2022

We are currently marking external versions as active=False when the PR got closed/merged at

readthedocs.org/readthedocs/core/views/hooks.py

Line 186 in ccdad23

external_version.active = False

Then, we have a Celery task that collects external versions that are active=False and modified_lte=90 days ago at

readthedocs.org/readthedocs/builds/tasks.py

Line 231 in ccdad23

def delete_inactive_external_versions(limit=200, days=30 * 3):

So we could:

  1. change this logic adding a new field Version.state=['open', 'closed', 'merged'] to track the state and know when we have to delete these versions from the Celery task
  2. exclude external versions that are active=False from the proxito logic
  3. exclude the PR domain from the proxito logic
  4. another idea?

None of them convince me 100%, but I think the clearest one is 1) since does not add special cases to the main logic: "We do not serve inactive versions". However, it's the one that requires more changes in the code 😞 . What do you think? Do you have some opinions here?

@ericholscher
Copy link
Member Author

None of them convince me 100%, but I think the clearest one is 1) since does not add special cases to the main logic: "We do not serve inactive versions". However, it's the one that requires more changes in the code 😞 . What do you think? Do you have some opinions here?

It seems like having the state of the PR in the Version model would be useful for a few UI elements, so that seems like a reasonable approach. I agree having fewer special cases and weird exceptions is good, and making it explicit what we're doing is better. 👍

humitos added a commit that referenced this issue Apr 21, 2022
@humitos
External versions: save state (open / closed)
340e773 
When an external Version is created, we mark it as `.state="open"` and when it's
closed/merged we mark is as `.state="closed"`.

This will be used by El Proxito to decide whether or not this version has to be
served. After some time it got closed/merged, a Celery task will delete these
versions. Note that we cannot use `.active=False` for this because we want to
keep serving closed/merged PRs during some extra time after they are
closed/merged and we are not serving inactive versions.

Related #9109
@humitos humitos mentioned this issue Apr 21, 2022
Merged
humitos added a commit that referenced this issue Apr 21, 2022
@humitos
External versions: save state (open / closed)
1de3bd8 
When an external Version is created, we mark it as `.state="open"` and when it's
closed/merged we mark is as `.state="closed"`.

Note that we cannot use `.active=False` for this as we were doing, because we
want to keep serving closed/merged PRs during some extra time after they are
closed/merged and El Proxito is not serving inactive versions.

With this approach, external versions will be always `.active=True` but with
different `.state=`. That state is used by the periodic Celery task to know if
the version has to be deleted or not.

Closes #9109
humitos added a commit that referenced this issue Apr 21, 2022
@humitos
External versions: save state (open / closed)
588cece 
When an external Version is created, we mark it as `.state="open"` and when it's
closed/merged we mark is as `.state="closed"`.

Note that we cannot use `.active=False` for this as we were doing, because we
want to keep serving closed/merged PRs during some extra time after they are
closed/merged and El Proxito is not serving inactive versions.

With this approach, external versions will be always `.active=True` but with
different `.state=`. That state is used by the periodic Celery task to know if
the version has to be deleted or not.

Closes #9109
@humitos humitos moved this from Planned to Needs review in 📍Roadmap Apr 21, 2022
humitos added a commit that referenced this issue Apr 21, 2022
@humitos
External versions: save state (open / closed)
a7f10ff 
When an external Version is created, we mark it as `.state="open"` and when it's
closed/merged we mark is as `.state="closed"`.

Note that we cannot use `.active=False` for this as we were doing, because we
want to keep serving closed/merged PRs during some extra time after they are
closed/merged and El Proxito is not serving inactive versions.

With this approach, external versions will be always `.active=True` but with
different `.state=`. That state is used by the periodic Celery task to know if
the version has to be deleted or not.

Closes #9109
humitos added a commit that referenced this issue Apr 21, 2022
@humitos
External versions: save state (open / closed)
5c65f16 
When an external Version is created, we mark it as `.state="open"` and when it's
closed/merged we mark is as `.state="closed"`.

Note that we cannot use `.active=False` for this as we were doing, because we
want to keep serving closed/merged PRs during some extra time after they are
closed/merged and El Proxito is not serving inactive versions.

With this approach, external versions will be always `.active=True` but with
different `.state=`. That state is used by the periodic Celery task to know if
the version has to be deleted or not.

Closes #9109
humitos added a commit that referenced this issue Apr 21, 2022
@humitos
External versions: save state (open / closed)
8bff524 
When an external Version is created, we mark it as `.state="open"` and when it's
closed/merged we mark is as `.state="closed"`.

Note that we cannot use `.active=False` for this as we were doing, because we
want to keep serving closed/merged PRs during some extra time after they are
closed/merged and El Proxito is not serving inactive versions.

With this approach, external versions will be always `.active=True` but with
different `.state=`. That state is used by the periodic Celery task to know if
the version has to be deleted or not.

Closes #9109
Repository owner moved this from Needs review to Done in 📍Roadmap Apr 25, 2022
@humitos humitos mentioned this issue Jun 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@humitos humitos

Labels
Accepted Accepted issue on our roadmap Bug A bug
Projects
📍Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

External versions: save state (open / closed) readthedocs/readthedocs.org
2 participants
@ericholscher @humitos