Version sync: exclude external versions when deleting

[stsewd]

We weren't hitting this before bc we were excluding active versions.
But now, external versions can also be inactive.

Close #7738

[humitos]

I'm not sure to understand what's the issue we want to fix here. What was happening? What should happen? What are these scenarios?

Also, I think this should have a test so we don't hit this again and we are sure that we are doing what we expect here.

[stsewd]

I'm not sure to understand what's the issue we want to fix here. What was happening? What should happen? What are these scenarios?

The problem is explained in the linked issue, when a PR is closed and the branch is deleted we delete the external version, but we need to keep it. I'll add a test.

[ericholscher]

@stsewd hrm, this is tricky. It makes me think that we probably want to keep Version objects around for non-external versions too? I feel like we probably want to think more about this, and not delete any versions that have Builds, or that are active or built? Keeping external versions around but deleting normal versions in the same situation feels weird.

[stsewd]

It makes me think that we probably want to keep Version objects around for non-external versions too?

We want to keep the build log of normal versions, keeping the versions will lead to have weird slugs like v2_a, this doesn't happen in PRs as the identifier (PR number) is unique.

I feel like we probably want to think more about this, and not delete any versions that have Builds, or that are active or built?

This doesn't delete active versions, only inactive ones.

readthedocs.org/readthedocs/api/v2/utils.py

Lines 176 to 179 in 7ada88a

	to_delete_qs = (
	_get_deleted_versions_qs(project, version_data)
	.exclude(active=True)
	)

[ericholscher]

So external versions are not marked as active when they are built? Or is there some other bug that is causing this? I don't fully understand why this behavior is only necessary for external versions.

[stsewd]

So external versions are not marked as active when they are built? Or is there some other bug that is causing this? I don't fully understand why this behavior is only necessary for external versions.

External versions are marked as active and built as normal versions, but we want to delete external version after 90 days, not immediately, so we exclude external versions from this query set

[ericholscher]

Sure, but won't we still end up deleting normal versions that have Build's? The goal is to never delete those Build objects, and to remove the files from storage once they are deleted. So the proposed logic is this?:

• PR builds will have Version objects 90 days after the branch is deleted, then we will remove the Version, and also delete th files from storage. Build objects for these will last forever?
• Normal builds will delete Version objects immediately upon the branch being deleted, including removing all updated files, and Build objects?

[stsewd]

@ericholscher we want to keep build logs for both, but that's another PR #7679, this is to fix a bug in the first part of the implementation (delete external versions after 90 days)