From 8ea7576b98663eed702e4b36db693c7752229336 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 27 Nov 2023 17:25:33 -0500 Subject: [PATCH 01/13] Build: fix mismatch between webhook and version sync for latest Closes https://github.com/readthedocs/readthedocs.org/issues/10915 --- readthedocs/api/v2/views/integrations.py | 5 +++-- readthedocs/builds/tasks.py | 8 ++++++-- readthedocs/vcs_support/backends/git.py | 6 +++--- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/readthedocs/api/v2/views/integrations.py b/readthedocs/api/v2/views/integrations.py index 011bec245d2..7a94a2146a8 100644 --- a/readthedocs/api/v2/views/integrations.py +++ b/readthedocs/api/v2/views/integrations.py @@ -17,7 +17,7 @@ from rest_framework.status import HTTP_400_BAD_REQUEST from rest_framework.views import APIView -from readthedocs.builds.constants import LATEST +from readthedocs.builds.constants import BRANCH, LATEST from readthedocs.core.signals import webhook_bitbucket, webhook_github, webhook_gitlab from readthedocs.core.views.hooks import ( build_branches, @@ -316,7 +316,8 @@ def update_default_branch(self, default_branch): # Always check for the machine attribute, since latest can be user created. # RTD doesn't manage those. self.project.versions.filter(slug=LATEST, machine=True).update( - identifier=default_branch + identifier=default_branch, + type=BRANCH, ) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index 008086738e7..2b9f116618f 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -359,9 +359,13 @@ def sync_versions_task(project_pk, tags_data, branches_data, **kwargs): versions=added_versions, ) - # Sync latest and stable to match the correct type and identifier. - project.update_latest_version() + # Sync latest to match the correct type and identifier + # if the project has a default branch set, + # otherwise the latest version was already updated in the previous step. + if project.default_branch: + project.update_latest_version() # TODO: move this to an automation rule + # Sync stable to match the correct type and identifier. promoted_version = project.update_stable_version() new_stable = project.get_stable_version() if promoted_version and new_stable and new_stable.active: diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 2cd16baa50b..c1167a32def 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -173,7 +173,7 @@ def clone(self): # of the default branch. Once this case has been made redundant, we can have # --no-checkout for all clones. # --depth 1: Shallow clone, fetch as little data as possible. - cmd = ["git", "clone", "--depth", "1", self.repo_url, "."] + cmd = ["git", "clone", "--depth", "1", "--", self.repo_url, "."] try: # TODO: Explain or remove the return value @@ -204,7 +204,7 @@ def fetch(self): if remote_reference: # TODO: We are still fetching the latest 50 commits. # A PR might have another commit added after the build has started... - cmd.append(remote_reference) + cmd.extend(["--", remote_reference]) # Log a warning, except for machine versions since it's a known bug that # we haven't stored a remote refspec in Version for those "stable" versions. @@ -310,7 +310,7 @@ def lsremote(self, include_tags=True, include_branches=True): if include_branches: extra_args.append("--heads") - cmd = ["git", "ls-remote", *extra_args, self.repo_url] + cmd = ["git", "ls-remote", *extra_args, "--", self.repo_url] self.check_working_dir() _, stdout, _ = self.run(*cmd, demux=True, record=False) From ec6a4c9075f823942eafbdaab1d491996256ce79 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 28 Nov 2023 18:26:15 -0500 Subject: [PATCH 02/13] Finally? --- readthedocs/api/v2/views/integrations.py | 4 ++ readthedocs/builds/models.py | 22 ++++------- readthedocs/builds/tasks.py | 8 +--- readthedocs/core/views/hooks.py | 12 +++--- readthedocs/doc_builder/director.py | 7 +++- readthedocs/projects/models.py | 42 ++++++++++---------- readthedocs/projects/tasks/builds.py | 49 ++++++++++++++++++------ readthedocs/vcs_support/backends/bzr.py | 1 - readthedocs/vcs_support/backends/git.py | 19 +++++++++ readthedocs/vcs_support/base.py | 6 ++- 10 files changed, 107 insertions(+), 63 deletions(-) diff --git a/readthedocs/api/v2/views/integrations.py b/readthedocs/api/v2/views/integrations.py index 7a94a2146a8..9b9f5c0ca3d 100644 --- a/readthedocs/api/v2/views/integrations.py +++ b/readthedocs/api/v2/views/integrations.py @@ -447,6 +447,7 @@ def handle_webhook(self): event=event, ) + # TODO: do we need this? # Always update `latest` branch to point to the default branch in the repository # even if the event is not gonna be handled. This helps us to keep our db in sync. default_branch = self.data.get("repository", {}).get("default_branch", None) @@ -611,6 +612,7 @@ def handle_webhook(self): integration = self.get_integration() + # TODO: do we need this? # Always update `latest` branch to point to the default branch in the repository # even if the event is not gonna be handled. This helps us to keep our db in sync. default_branch = self.data.get("project", {}).get("default_branch", None) @@ -713,6 +715,7 @@ def handle_webhook(self): event=event, ) + # TODO: do we need this? # NOTE: we can't call `self.update_default_branch` here because # BitBucket does not tell us what is the `default_branch` for a # repository in these incoming webhooks. @@ -836,6 +839,7 @@ def handle_webhook(self): if isinstance(branches, str): branches = [branches] + # TODO: do we need this? if default_branch and isinstance(default_branch, str): self.update_default_branch(default_branch) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index e782f52599f..f62283df305 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -30,7 +30,6 @@ EXTERNAL_VERSION_STATES, INTERNAL, LATEST, - NON_REPOSITORY_VERSIONS, PREDEFINED_MATCH_ARGS, PREDEFINED_MATCH_ARGS_VALUES, STABLE, @@ -292,10 +291,10 @@ def ref(self): def vcs_url(self): version_name = self.verbose_name if not self.is_external: - if self.slug == STABLE: + if self.slug == STABLE and self.machine: version_name = self.ref - elif self.slug == LATEST: - version_name = self.project.get_default_branch() + elif self.slug == LATEST and self.machine: + version_name = self.identifier else: version_name = self.slug if 'bitbucket' in self.project.repo: @@ -339,12 +338,12 @@ def commit_name(self): The result could be used as ref in a git repo, e.g. for linking to GitHub, Bitbucket or GitLab. """ - # LATEST is special as it is usually a branch but does not contain the - # name in verbose_name. - if self.slug == LATEST: - return self.project.get_default_branch() + # LATEST is special as it doesn't contain the branch/tag name in + # verbose_name, but in identifier. + if self.slug == LATEST and self.machine: + return self.identifier - if self.slug == STABLE: + if self.slug == STABLE and self.machine: if self.type == BRANCH: # Special case, as we do not store the original branch name # that the stable version works on. We can only interpolate the @@ -355,11 +354,6 @@ def commit_name(self): return self.identifier[len('origin/'):] return self.identifier - # By now we must have handled all special versions. - if self.slug in NON_REPOSITORY_VERSIONS: - # pylint: disable=broad-exception-raised - raise Exception('All special versions must be handled by now.') - if self.type in (BRANCH, TAG): # If this version is a branch or a tag, the verbose_name will # contain the actual name. We cannot use identifier as this might diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index 2b9f116618f..008086738e7 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -359,13 +359,9 @@ def sync_versions_task(project_pk, tags_data, branches_data, **kwargs): versions=added_versions, ) - # Sync latest to match the correct type and identifier - # if the project has a default branch set, - # otherwise the latest version was already updated in the previous step. - if project.default_branch: - project.update_latest_version() + # Sync latest and stable to match the correct type and identifier. + project.update_latest_version() # TODO: move this to an automation rule - # Sync stable to match the correct type and identifier. promoted_version = project.update_stable_version() new_stable = project.get_stable_version() if promoted_version and new_stable and new_stable.active: diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index 267ac60f553..b1f7e9dd5c6 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -7,6 +7,7 @@ EXTERNAL, EXTERNAL_VERSION_STATE_CLOSED, EXTERNAL_VERSION_STATE_OPEN, + LATEST, ) from readthedocs.core.utils import trigger_build from readthedocs.projects.models import Feature, Project @@ -88,14 +89,11 @@ def trigger_sync_versions(project): return None try: - version_identifier = project.get_default_branch() - version = ( - project.versions.filter( - identifier=version_identifier, - ).first() - ) + version = project.versions.filter(slug=LATEST).first() if not version: - log.info('Unable to sync from version.', version_identifier=version_identifier) + log.info( + "Unable to sync from version latest version.", project_slug=project.slug + ) return None if project.has_feature(Feature.SKIP_SYNC_VERSIONS): diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index fcbb4c47c2e..02d8335b540 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -226,8 +226,11 @@ def checkout(self): self.vcs_repository.update() identifier = self.data.build_commit or self.data.version.identifier - log.info("Checking out.", identifier=identifier) - self.vcs_repository.checkout(identifier) + if not self.data.project.default_branch or not identifier: + log.info("Skipping checkout, using default branch.") + else: + log.info("Checking out.", identifier=identifier) + self.vcs_repository.checkout(identifier) # The director is responsible for understanding which config file to use for a build. # In order to reproduce a build 1:1, we may use readthedocs_yaml_path defined by the build diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 6a01a9aa57e..2952b3e4248 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -645,21 +645,7 @@ def save(self, *args, **kwargs): _("Model must have slug") ) super().save(*args, **kwargs) - - try: - if not self.versions.filter(slug=LATEST).exists(): - self.versions.create_latest() - except Exception: - log.exception("Error creating default branches") - - # Update `Version.identifier` for `latest` with the default branch the user has selected, - # even if it's `None` (meaning to match the `default_branch` of the repository) - # NOTE: this code is required to be *after* ``create_latest()``. - # It has to be updated after creating LATEST originally. - log.debug( - "Updating default branch.", slug=LATEST, identifier=self.default_branch - ) - self.versions.filter(slug=LATEST).update(identifier=self.default_branch) + self.update_latest_version() def delete(self, *args, **kwargs): from readthedocs.projects.tasks.utils import clean_project_resources @@ -1190,15 +1176,19 @@ def update_latest_version(self): A machine created LATEST version is an alias for the default branch/tag, so we need to update it to match the type and identifier of the default branch/tag. + + If the project doesn't have an explicit default branch set, + we don't update the latest version, since it will be updated after a successful build. """ latest = self.get_latest_version() if not latest: latest = self.versions.create_latest() - if not latest.machine: + + if not self.default_branch or not latest.machine: return # default_branch can be a tag or a branch name! - default_version_name = self.get_default_branch() + default_version_name = self.default_branch original_latest = self.get_original_latest_version() latest.verbose_name = LATEST_VERBOSE_NAME latest.type = original_latest.type if original_latest else BRANCH @@ -1282,14 +1272,24 @@ def get_default_version(self): return LATEST def get_default_branch(self): - """Get the version representing 'latest'.""" + """ + Get the branch/tag name of the version representing 'latest'. + + If the project has a default branch explicitly set, we use that, + otherwise we try to get it from the latest version, + since that should be in sync with the default branch of the repository. + """ if self.default_branch: return self.default_branch - if self.remote_repository and self.remote_repository.default_branch: - return self.remote_repository.default_branch + fallback_branch = self.vcs_class.fallback_branch + latest_version = self.versions.filter(slug=LATEST).first() + if latest_version: + if latest_version.machine: + return latest_version.identifer or fallback_branch + return latest_version.verbose_name or fallback_branch - return self.vcs_class().fallback_branch + return fallback_branch def add_subproject(self, child, alias=None): subproject, _ = ProjectRelationship.objects.get_or_create( diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 19a0e508a85..3e14a623336 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -22,6 +22,7 @@ from readthedocs.builds.constants import ( ARTIFACT_TYPES, ARTIFACT_TYPES_WITHOUT_MULTIPLE_FILES_SUPPORT, + BRANCH, BUILD_FINAL_STATES, BUILD_STATE_BUILDING, BUILD_STATE_CLONING, @@ -32,6 +33,7 @@ BUILD_STATUS_FAILURE, BUILD_STATUS_SUCCESS, EXTERNAL, + LATEST, UNDELETABLE_ARTIFACT_TYPES, ) from readthedocs.builds.models import APIVersion, Build @@ -119,6 +121,10 @@ class TaskData: config: BuildConfigV2 = None project: APIProject = None version: APIVersion = None + # Default branch for the project. + # This is used to update the latest version in case the project doesn't + # have an explicit default branch set. + default_branch: str = None # Dictionary returned from the API. build: dict = field(default_factory=dict) @@ -634,17 +640,26 @@ def on_success(self, retval, task_id, args, kwargs): # TODO: remove this condition and *always* update the DB Version instance if "html" in valid_artifacts: try: - self.data.api_client.version(self.data.version.pk).patch( - { - "built": True, - "documentation_type": self.data.version.documentation_type, - "has_pdf": "pdf" in valid_artifacts, - "has_epub": "epub" in valid_artifacts, - "has_htmlzip": "htmlzip" in valid_artifacts, - "build_data": self.data.version.build_data, - "addons": self.data.version.addons, - } - ) + payload = { + "built": True, + "documentation_type": self.data.version.documentation_type, + "has_pdf": "pdf" in valid_artifacts, + "has_epub": "epub" in valid_artifacts, + "has_htmlzip": "htmlzip" in valid_artifacts, + "build_data": self.data.version.build_data, + "addons": self.data.version.addons, + } + # Update the latest version to point to the current default branch, + # if the project doesn't have a default branch set. + if ( + not self.data.project.default_branch + and self.data.version.slug == LATEST + and self.data.version.machine + and self.data.default_branch + ): + payload["identifier"] = self.data.default_branch + payload["type"] = BRANCH + self.data.api_client.version(self.data.version.pk).patch(payload) except HttpClientError: # NOTE: I think we should fail the build if we cannot update # the version at this point. Otherwise, we will have inconsistent data @@ -777,6 +792,18 @@ def execute(self): with self.data.build_director.vcs_environment: self.data.build_director.setup_vcs() + # Save the default branch of the repository if the project doesn't + # have an explicit default branch set. The latest version is + # updated if the build succeeds. + if ( + not self.data.project.default_branch + and self.data.version.slug == LATEST + and self.data.version.machine + ): + self.data.default_branch = ( + self.data.build_director.vcs_repository.get_default_branch() + ) + # Sync tags/branches from VCS repository into Read the Docs' # `Version` objects in the database. This method runs commands # (e.g. "hg tags") inside the VCS environment, so it requires to be diff --git a/readthedocs/vcs_support/backends/bzr.py b/readthedocs/vcs_support/backends/bzr.py index 7e60e5d3fe3..517655834a3 100644 --- a/readthedocs/vcs_support/backends/bzr.py +++ b/readthedocs/vcs_support/backends/bzr.py @@ -13,7 +13,6 @@ class Backend(BaseVCS): """Bazaar VCS backend.""" supports_tags = True - fallback_branch = "" def clone(self): self.make_clean_working_dir() diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index c1167a32def..3ac3856b3ac 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -295,6 +295,25 @@ def checkout_revision(self, revision): RepositoryError.FAILED_TO_CHECKOUT.format(revision), ) from exc + def get_default_branch(self): + """ + Return the default branch of the repository. + + The default branch is the branch that is checked out when cloning the + repository. This is usually master or main, it can be configured + in the repository settings. + + The ``git symbolic-ref`` command will produce an output like: + + .. code-block:: text + + origin/main + """ + cmd = ["git", "symbolic-ref", "--short", "refs/remotes/origin/HEAD"] + _, stdout, _ = self.run(*cmd, demux=True, record=False) + default_branch = stdout.strip().removeprefix("origin/") + return default_branch or self.fallback_branch + def lsremote(self, include_tags=True, include_branches=True): """ Use ``git ls-remote`` to list branches and tags without cloning the repository. diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py index 3304fefef24..e89f26db41e 100644 --- a/readthedocs/vcs_support/base.py +++ b/readthedocs/vcs_support/base.py @@ -48,6 +48,8 @@ class BaseVCS: # Whether this VCS supports listing remotes (branches, tags) without cloning supports_lsremote = False + fallback_branch = "" + # ========================================================================= # General methods # ========================================================================= @@ -63,7 +65,6 @@ def __init__( version_type=None, **kwargs ): - self.default_branch = project.default_branch self.project = project self.name = project.name self.repo_url = project.clean_repo @@ -163,3 +164,6 @@ def update_submodules(self, config): :type config: readthedocs.config.BuildConfigBase """ raise NotImplementedError + + def get_default_branch(self): + return self.fallback_branch From 9c2a7fa1e027de035e30a618d8326f293641a148 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 28 Nov 2023 19:05:48 -0500 Subject: [PATCH 03/13] updates --- readthedocs/api/v2/views/integrations.py | 2 +- readthedocs/projects/models.py | 4 ++-- readthedocs/vcs_support/backends/git.py | 20 ++++++++++++-------- tox.ini | 4 ---- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/readthedocs/api/v2/views/integrations.py b/readthedocs/api/v2/views/integrations.py index 9b9f5c0ca3d..2f6596b1153 100644 --- a/readthedocs/api/v2/views/integrations.py +++ b/readthedocs/api/v2/views/integrations.py @@ -294,7 +294,7 @@ def get_closed_external_version_response(self, project): def update_default_branch(self, default_branch): """ - Update the `Version.identifer` for `latest` with the VCS's `default_branch`. + Update the `Version.identifier` for `latest` with the VCS's `default_branch`. The VCS's `default_branch` is the branch cloned when there is no specific branch specified (e.g. `git clone `). diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 90b930f55ee..79da72d3459 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1300,11 +1300,11 @@ def get_default_branch(self): if self.default_branch: return self.default_branch - fallback_branch = self.vcs_class.fallback_branch + fallback_branch = self.vcs_class().fallback_branch latest_version = self.versions.filter(slug=LATEST).first() if latest_version: if latest_version.machine: - return latest_version.identifer or fallback_branch + return latest_version.identifier or fallback_branch return latest_version.verbose_name or fallback_branch return fallback_branch diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 3ac3856b3ac..4b89de6c0b6 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -116,8 +116,8 @@ def get_remote_fetch_refspec(self): # Git backend. # If version_identifier is empty, then the fetch operation cannot know what to fetch # and will fetch everything, in order to build what might be defined elsewhere - # as the "default branch". This can be the case for an initial build started BEFORE - # a webhook or sync versions task has concluded what the default branch is. + # as the "default branch". This can be the case for old projects that don't have + # their latest branch in sync with the default branch. if self.version_type == BRANCH and self.version_identifier: # Here we point directly to the remote branch name and update our local remote # refspec to point here. @@ -199,12 +199,16 @@ def fetch(self): "--depth", str(self.repo_depth), ] - remote_reference = self.get_remote_fetch_refspec() - - if remote_reference: - # TODO: We are still fetching the latest 50 commits. - # A PR might have another commit added after the build has started... - cmd.extend(["--", remote_reference]) + is_rtd_latest = ( + self.verbose_name == LATEST_VERBOSE_NAME and self.version_machine + ) + ommit_remote_reference = is_rtd_latest and not self.project.default_branch + if not ommit_remote_reference: + remote_reference = self.get_remote_fetch_refspec() + if remote_reference: + # TODO: We are still fetching the latest 50 commits. + # A PR might have another commit added after the build has started... + cmd.extend(["--", remote_reference]) # Log a warning, except for machine versions since it's a known bug that # we haven't stored a remote refspec in Version for those "stable" versions. diff --git a/tox.ini b/tox.ini index 8043aa0dc70..685e8210b37 100644 --- a/tox.ini +++ b/tox.ini @@ -19,10 +19,6 @@ basepython = # https://pytest-cov.readthedocs.io/en/latest/debuggers.html # https://github.com/microsoft/vscode-python/issues/693 commands = - sh -c '\ - export DJANGO_SETTINGS_MODULE=readthedocs.settings.test; \ - pytest {env:PYTEST_COVERAGE} --pyargs readthedocs --suppress-no-test-exit-code -m "not proxito and not embed_api" {posargs:{env:TOX_POSARGS:-m "not search and not proxito and not embed_api"}}' - sh -c '\ export DJANGO_SETTINGS_MODULE=readthedocs.settings.proxito.test; \ pytest {env:PYTEST_COVERAGE} --pyargs readthedocs -m proxito --suppress-no-test-exit-code {posargs}' From 40975ca89ed3c013a34870c832884daf998df11b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 28 Nov 2023 19:08:16 -0500 Subject: [PATCH 04/13] Revert this --- tox.ini | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tox.ini b/tox.ini index 685e8210b37..8043aa0dc70 100644 --- a/tox.ini +++ b/tox.ini @@ -19,6 +19,10 @@ basepython = # https://pytest-cov.readthedocs.io/en/latest/debuggers.html # https://github.com/microsoft/vscode-python/issues/693 commands = + sh -c '\ + export DJANGO_SETTINGS_MODULE=readthedocs.settings.test; \ + pytest {env:PYTEST_COVERAGE} --pyargs readthedocs --suppress-no-test-exit-code -m "not proxito and not embed_api" {posargs:{env:TOX_POSARGS:-m "not search and not proxito and not embed_api"}}' + sh -c '\ export DJANGO_SETTINGS_MODULE=readthedocs.settings.proxito.test; \ pytest {env:PYTEST_COVERAGE} --pyargs readthedocs -m proxito --suppress-no-test-exit-code {posargs}' From f533f093788cb28bff25f0575aabed60f6695c52 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 29 Nov 2023 15:28:43 -0500 Subject: [PATCH 05/13] Again... --- readthedocs/builds/models.py | 4 +- readthedocs/doc_builder/director.py | 6 +- readthedocs/projects/models.py | 10 +- readthedocs/projects/tasks/builds.py | 13 +- .../projects/tests/test_build_tasks.py | 286 +++++++++++++++++- .../rtd_tests/tests/test_sync_versions.py | 5 +- readthedocs/rtd_tests/tests/test_version.py | 14 +- .../tests/test_version_commit_name.py | 7 +- 8 files changed, 320 insertions(+), 25 deletions(-) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index f62283df305..e141eaf8903 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -294,7 +294,7 @@ def vcs_url(self): if self.slug == STABLE and self.machine: version_name = self.ref elif self.slug == LATEST and self.machine: - version_name = self.identifier + version_name = self.project.get_default_branch() else: version_name = self.slug if 'bitbucket' in self.project.repo: @@ -341,7 +341,7 @@ def commit_name(self): # LATEST is special as it doesn't contain the branch/tag name in # verbose_name, but in identifier. if self.slug == LATEST and self.machine: - return self.identifier + return self.project.get_default_branch() if self.slug == STABLE and self.machine: if self.type == BRANCH: diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 02d8335b540..9b751fad9d5 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -17,7 +17,7 @@ from django.utils import timezone from django.utils.translation import gettext_lazy as _ -from readthedocs.builds.constants import EXTERNAL +from readthedocs.builds.constants import EXTERNAL, LATEST from readthedocs.core.utils.filesystem import safe_open from readthedocs.doc_builder.config import load_yaml_config from readthedocs.doc_builder.exceptions import BuildUserError @@ -226,7 +226,9 @@ def checkout(self): self.vcs_repository.update() identifier = self.data.build_commit or self.data.version.identifier - if not self.data.project.default_branch or not identifier: + is_rtd_latest = self.data.version.slug == LATEST and self.data.version.machine + skip_checkout = not identifier or (is_rtd_latest and not self.data.project.default_branch) + if skip_checkout: log.info("Skipping checkout, using default branch.") else: log.info("Checking out.", identifier=identifier) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 79da72d3459..1b5e1b600df 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1200,13 +1200,14 @@ def update_latest_version(self): """ latest = self.get_latest_version() if not latest: - latest = self.versions.create_latest() + default_branch = self.get_default_branch() + latest = self.versions.create_latest(identifier=default_branch) - if not self.default_branch or not latest.machine: + if not latest.machine: return # default_branch can be a tag or a branch name! - default_version_name = self.default_branch + default_version_name = self.get_default_branch() original_latest = self.get_original_latest_version() latest.verbose_name = LATEST_VERBOSE_NAME latest.type = original_latest.type if original_latest else BRANCH @@ -1301,6 +1302,9 @@ def get_default_branch(self): return self.default_branch fallback_branch = self.vcs_class().fallback_branch + if self.remote_repository and self.remote_repository.default_branch: + fallback_branch = self.remote_repository.default_branch + latest_version = self.versions.filter(slug=LATEST).first() if latest_version: if latest_version.machine: diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 3e14a623336..e39c4902ec2 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -651,10 +651,10 @@ def on_success(self, retval, task_id, args, kwargs): } # Update the latest version to point to the current default branch, # if the project doesn't have a default branch set. + is_rtd_latest = self.data.version.slug == LATEST and self.data.version.machine if ( - not self.data.project.default_branch - and self.data.version.slug == LATEST - and self.data.version.machine + is_rtd_latest + and not self.data.project.default_branch and self.data.default_branch ): payload["identifier"] = self.data.default_branch @@ -795,11 +795,8 @@ def execute(self): # Save the default branch of the repository if the project doesn't # have an explicit default branch set. The latest version is # updated if the build succeeds. - if ( - not self.data.project.default_branch - and self.data.version.slug == LATEST - and self.data.version.machine - ): + is_rtd_latest = self.data.version.slug == LATEST and self.data.version.machine + if is_rtd_latest and not self.data.project.default_branch: self.data.default_branch = ( self.data.build_director.vcs_repository.get_default_branch() ) diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index de60cd4d5f3..91953c6c078 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -312,6 +312,8 @@ def test_build_updates_documentation_type(self, load_yaml_config): "has_pdf": True, "has_epub": True, "has_htmlzip": False, + "identifier": mock.ANY, + "type": "branch", } @pytest.mark.parametrize( @@ -585,6 +587,8 @@ def test_successful_build( "has_pdf": True, "has_epub": True, "has_htmlzip": True, + "identifier": mock.ANY, + "type": "branch", } # Set project has valid clone assert self.requests_mock.request_history[8]._request.method == "PATCH" @@ -691,7 +695,7 @@ def test_failed_build( assert revoke_key_request.path == "/api/v2/revoke/" @mock.patch("readthedocs.doc_builder.director.load_yaml_config") - def test_build_commands_executed( + def test_build_commands_executed_latest_version( self, load_yaml_config, ): @@ -719,7 +723,284 @@ def test_build_commands_executed( self.mocker.mocks["git.Backend.run"].assert_has_calls( [ - mock.call("git", "clone", "--depth", "1", mock.ANY, "."), + mock.call("git", "clone", "--depth", "1", "--", mock.ANY, "."), + mock.call( + "git", + "fetch", + "origin", + "--force", + "--prune", + "--prune-tags", + "--depth", + "50", + ), + mock.call('git', 'symbolic-ref', '--short', 'refs/remotes/origin/HEAD', demux=True, record=False), + mock.call( + "git", + "ls-remote", + "--tags", + "--heads", + "--", + mock.ANY, + demux=True, + record=False, + ), + ] + ) + + python_version = settings.RTD_DOCKER_BUILD_SETTINGS["tools"]["python"]["3"] + self.mocker.mocks["environment.run"].assert_has_calls( + [ + mock.call( + "cat", + "readthedocs.yml", + cwd="/tmp/readthedocs-tests/git-repository", + ), + mock.call("asdf", "install", "python", python_version), + mock.call("asdf", "global", "python", python_version), + mock.call("asdf", "reshim", "python", record=False), + mock.call( + "python", + "-mpip", + "install", + "-U", + "virtualenv", + "setuptools", + ), + mock.call( + "python", + "-mvirtualenv", + "$READTHEDOCS_VIRTUALENV_PATH", + bin_path=None, + cwd=None, + ), + mock.call( + mock.ANY, + "-m", + "pip", + "install", + "--upgrade", + "--no-cache-dir", + "pip", + "setuptools", + bin_path=mock.ANY, + cwd=mock.ANY, + ), + mock.call( + mock.ANY, + "-m", + "pip", + "install", + "--upgrade", + "--no-cache-dir", + "sphinx", + "readthedocs-sphinx-ext", + bin_path=mock.ANY, + cwd=mock.ANY, + ), + # FIXME: shouldn't this one be present here? It's not now because + # we are mocking `append_conf` which is the one that triggers this + # command. + # + # mock.call( + # 'cat', + # 'docs/conf.py', + # cwd=mock.ANY, + # ), + mock.call( + mock.ANY, + "-m", + "sphinx", + "-T", + "-E", + "-b", + "html", + "-d", + "_build/doctrees", + "-D", + "language=en", + ".", + "$READTHEDOCS_OUTPUT/html", + cwd=mock.ANY, + bin_path=mock.ANY, + ), + mock.call( + mock.ANY, + "-m", + "sphinx", + "-T", + "-E", + "-b", + "readthedocssinglehtmllocalmedia", + "-d", + "_build/doctrees", + "-D", + "language=en", + ".", + "$READTHEDOCS_OUTPUT/htmlzip", + cwd=mock.ANY, + bin_path=mock.ANY, + ), + mock.call( + "mktemp", + "--directory", + record=False, + ), + mock.call( + "mv", + mock.ANY, + mock.ANY, + cwd=mock.ANY, + record=False, + ), + mock.call( + "mkdir", + "--parents", + mock.ANY, + cwd=mock.ANY, + record=False, + ), + mock.call( + "zip", + "--recurse-paths", + "--symlinks", + mock.ANY, + mock.ANY, + cwd=mock.ANY, + record=False, + ), + mock.call( + mock.ANY, + "-m", + "sphinx", + "-T", + "-E", + "-b", + "latex", + "-d", + "_build/doctrees", + "-D", + "language=en", + ".", + "$READTHEDOCS_OUTPUT/pdf", + cwd=mock.ANY, + bin_path=mock.ANY, + ), + mock.call("cat", "latexmkrc", cwd=mock.ANY), + # NOTE: pdf `mv` commands and others are not here because the + # PDF resulting file is not found in the process (`_post_build`) + mock.call( + mock.ANY, + "-m", + "sphinx", + "-T", + "-E", + "-b", + "epub", + "-d", + "_build/doctrees", + "-D", + "language=en", + ".", + "$READTHEDOCS_OUTPUT/epub", + cwd=mock.ANY, + bin_path=mock.ANY, + ), + mock.call( + "mv", + mock.ANY, + "/tmp/project-latest.epub", + cwd=mock.ANY, + record=False, + ), + mock.call( + "rm", + "--recursive", + "$READTHEDOCS_OUTPUT/epub", + cwd=mock.ANY, + record=False, + ), + mock.call( + "mkdir", + "--parents", + "$READTHEDOCS_OUTPUT/epub", + cwd=mock.ANY, + record=False, + ), + mock.call( + "mv", + "/tmp/project-latest.epub", + mock.ANY, + cwd=mock.ANY, + record=False, + ), + mock.call( + "test", + "-x", + "_build/html", + record=False, + cwd=mock.ANY, + ), + # FIXME: I think we are hitting this issue here: + # https://github.com/pytest-dev/pytest-mock/issues/234 + mock.call("lsb_release", "--description", record=False, demux=True), + mock.call("python", "--version", record=False, demux=True), + mock.call( + "dpkg-query", + "--showformat", + "${package} ${version}\\n", + "--show", + record=False, + demux=True, + ), + mock.call( + "python", + "-m", + "pip", + "list", + "--pre", + "--local", + "--format", + "json", + record=False, + demux=True, + ), + ] + ) + + @mock.patch("readthedocs.doc_builder.director.load_yaml_config") + def test_build_commands_executed_non_machine_version( + self, + load_yaml_config, + ): + load_yaml_config.return_value = get_build_config( + { + "version": 2, + "formats": "all", + "sphinx": { + "configuration": "docs/conf.py", + }, + }, + validate=True, + ) + + self.version.machine = False + self.version.save() + + # Create the artifact paths, so it's detected by the builder + os.makedirs(self.project.artifact_path(version=self.version.slug, type_="html")) + os.makedirs(self.project.artifact_path(version=self.version.slug, type_="json")) + os.makedirs( + self.project.artifact_path(version=self.version.slug, type_="htmlzip") + ) + os.makedirs(self.project.artifact_path(version=self.version.slug, type_="epub")) + os.makedirs(self.project.artifact_path(version=self.version.slug, type_="pdf")) + + self._trigger_update_docs_task() + + self.mocker.mocks["git.Backend.run"].assert_has_calls( + [ + mock.call("git", "clone", "--depth", "1", "--", mock.ANY, "."), mock.call( "git", "fetch", @@ -746,6 +1027,7 @@ def test_build_commands_executed( "ls-remote", "--tags", "--heads", + "--", mock.ANY, demux=True, record=False, diff --git a/readthedocs/rtd_tests/tests/test_sync_versions.py b/readthedocs/rtd_tests/tests/test_sync_versions.py index a7934726fac..c1b6c37cdb5 100644 --- a/readthedocs/rtd_tests/tests/test_sync_versions.py +++ b/readthedocs/rtd_tests/tests/test_sync_versions.py @@ -302,8 +302,11 @@ def test_update_latest_version_type(self): ) latest_version = self.pip.versions.get(slug=LATEST) + # Since the project doesn't have a default branch, the latest version + # will be synced after the next build. + self.assertEqual(self.pip.default_branch, None) self.assertEqual(latest_version.type, BRANCH) - self.assertEqual(latest_version.identifier, "master") + self.assertEqual(latest_version.identifier, "abc123") self.assertEqual(latest_version.verbose_name, "latest") self.assertEqual(latest_version.machine, True) diff --git a/readthedocs/rtd_tests/tests/test_version.py b/readthedocs/rtd_tests/tests/test_version.py index 8b3f1dab096..464a48619e9 100644 --- a/readthedocs/rtd_tests/tests/test_version.py +++ b/readthedocs/rtd_tests/tests/test_version.py @@ -2,7 +2,7 @@ from django.test.utils import override_settings from django_dynamic_fixture import get -from readthedocs.builds.constants import BRANCH, EXTERNAL, LATEST, TAG +from readthedocs.builds.constants import BRANCH, EXTERNAL, LATEST, LATEST_VERBOSE_NAME, STABLE, STABLE_VERBOSE_NAME, TAG from readthedocs.builds.models import Version from readthedocs.projects.models import Project @@ -26,20 +26,22 @@ def setUp(self): self.branch_version = get( Version, identifier="origin/stable", - verbose_name="stable", - slug="stable", + verbose_name=STABLE_VERBOSE_NAME, + slug=STABLE, project=self.pip, active=True, type=BRANCH, + machine=True, ) self.tag_version = get( Version, - identifier="origin/master", - verbose_name="latest", - slug="latest", + identifier="master", + verbose_name=LATEST_VERBOSE_NAME, + slug=LATEST, project=self.pip, active=True, type=TAG, + machine=True, ) self.subproject = get(Project, slug="subproject", language="en") diff --git a/readthedocs/rtd_tests/tests/test_version_commit_name.py b/readthedocs/rtd_tests/tests/test_version_commit_name.py index 167c92b9bd1..161afc296a8 100644 --- a/readthedocs/rtd_tests/tests/test_version_commit_name.py +++ b/readthedocs/rtd_tests/tests/test_version_commit_name.py @@ -21,6 +21,7 @@ def test_branch_name_made_friendly_when_sha(self): slug=STABLE, verbose_name=STABLE, type=TAG, + machine=True, ) # we shorten commit hashes to keep things readable self.assertEqual(version.identifier_friendly, "3d92b728") @@ -52,6 +53,7 @@ def test_branch_with_name_stable(self): slug=STABLE, verbose_name="stable", type=BRANCH, + machine=True, ) self.assertEqual(version.commit_name, "stable") @@ -62,6 +64,7 @@ def test_stable_version_tag(self): slug=STABLE, verbose_name=STABLE, type=TAG, + machine=True, ) self.assertEqual( version.commit_name, @@ -77,6 +80,7 @@ def test_hg_latest_branch(self): verbose_name=LATEST, type=BRANCH, project=hg_project, + machine=True, ) self.assertEqual(version.commit_name, "default") @@ -85,10 +89,11 @@ def test_git_latest_branch(self): version = new( Version, project=git_project, - identifier="origin/master", + identifier="master", slug=LATEST, verbose_name=LATEST, type=BRANCH, + machine=True, ) self.assertEqual(version.commit_name, "master") From b080406c461250c3f9b46c17c7753a53e5e6671a Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 29 Nov 2023 16:21:44 -0500 Subject: [PATCH 06/13] Again --- readthedocs/builds/models.py | 4 ++-- readthedocs/projects/models.py | 7 ++----- readthedocs/projects/tasks/builds.py | 9 +++++---- readthedocs/projects/tests/test_build_tasks.py | 2 ++ readthedocs/rtd_tests/tests/test_sync_versions.py | 4 +--- 5 files changed, 12 insertions(+), 14 deletions(-) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index e141eaf8903..3f76b1dbd91 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -338,8 +338,8 @@ def commit_name(self): The result could be used as ref in a git repo, e.g. for linking to GitHub, Bitbucket or GitLab. """ - # LATEST is special as it doesn't contain the branch/tag name in - # verbose_name, but in identifier. + # LATEST is special as it is usually a branch but does not contain the + # name in verbose_name. if self.slug == LATEST and self.machine: return self.project.get_default_branch() diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 1b5e1b600df..e771015e466 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1194,9 +1194,6 @@ def update_latest_version(self): A machine created LATEST version is an alias for the default branch/tag, so we need to update it to match the type and identifier of the default branch/tag. - - If the project doesn't have an explicit default branch set, - we don't update the latest version, since it will be updated after a successful build. """ latest = self.get_latest_version() if not latest: @@ -1301,10 +1298,10 @@ def get_default_branch(self): if self.default_branch: return self.default_branch - fallback_branch = self.vcs_class().fallback_branch if self.remote_repository and self.remote_repository.default_branch: - fallback_branch = self.remote_repository.default_branch + return self.remote_repository.default_branch + fallback_branch = self.vcs_class().fallback_branch latest_version = self.versions.filter(slug=LATEST).first() if latest_version: if latest_version.machine: diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index e39c4902ec2..53f826e3f81 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -649,7 +649,7 @@ def on_success(self, retval, task_id, args, kwargs): "build_data": self.data.version.build_data, "addons": self.data.version.addons, } - # Update the latest version to point to the current default branch, + # Update the latest version to point to the current default branch # if the project doesn't have a default branch set. is_rtd_latest = self.data.version.slug == LATEST and self.data.version.machine if ( @@ -792,9 +792,10 @@ def execute(self): with self.data.build_director.vcs_environment: self.data.build_director.setup_vcs() - # Save the default branch of the repository if the project doesn't - # have an explicit default branch set. The latest version is - # updated if the build succeeds. + # Get the default branch of the repository if the project doesn't + # have an explicit default branch set and we are building latest. + # The identifier from latest will be updated with this value + # if the build succeeds. is_rtd_latest = self.data.version.slug == LATEST and self.data.version.machine if is_rtd_latest and not self.data.project.default_branch: self.data.default_branch = ( diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index 91953c6c078..27791fab3ab 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -1010,6 +1010,8 @@ def test_build_commands_executed_non_machine_version( "--prune-tags", "--depth", "50", + "--", + "refs/heads/master:refs/remotes/origin/master", ), mock.call( "git", diff --git a/readthedocs/rtd_tests/tests/test_sync_versions.py b/readthedocs/rtd_tests/tests/test_sync_versions.py index c1b6c37cdb5..ee0ac311905 100644 --- a/readthedocs/rtd_tests/tests/test_sync_versions.py +++ b/readthedocs/rtd_tests/tests/test_sync_versions.py @@ -302,11 +302,9 @@ def test_update_latest_version_type(self): ) latest_version = self.pip.versions.get(slug=LATEST) - # Since the project doesn't have a default branch, the latest version - # will be synced after the next build. self.assertEqual(self.pip.default_branch, None) self.assertEqual(latest_version.type, BRANCH) - self.assertEqual(latest_version.identifier, "abc123") + self.assertEqual(latest_version.identifier, "master") self.assertEqual(latest_version.verbose_name, "latest") self.assertEqual(latest_version.machine, True) From 377c7158d7643f409e24cdf0692e8c16a77a0938 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 30 Nov 2023 12:42:09 -0500 Subject: [PATCH 07/13] Rollback some changes --- readthedocs/core/views/hooks.py | 12 +++++++----- readthedocs/projects/models.py | 10 +++++++--- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index b1f7e9dd5c6..267ac60f553 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -7,7 +7,6 @@ EXTERNAL, EXTERNAL_VERSION_STATE_CLOSED, EXTERNAL_VERSION_STATE_OPEN, - LATEST, ) from readthedocs.core.utils import trigger_build from readthedocs.projects.models import Feature, Project @@ -89,11 +88,14 @@ def trigger_sync_versions(project): return None try: - version = project.versions.filter(slug=LATEST).first() + version_identifier = project.get_default_branch() + version = ( + project.versions.filter( + identifier=version_identifier, + ).first() + ) if not version: - log.info( - "Unable to sync from version latest version.", project_slug=project.slug - ) + log.info('Unable to sync from version.', version_identifier=version_identifier) return None if project.has_feature(Feature.SKIP_SYNC_VERSIONS): diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index e771015e466..cab3c09f0c9 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -650,7 +650,12 @@ def save(self, *args, **kwargs): _("Model must have slug") ) super().save(*args, **kwargs) - self.update_latest_version() + + try: + if not self.versions.filter(slug=LATEST).exists(): + self.versions.create_latest(identifier=self.get_default_branch()) + except Exception: + log.exception("Error creating default branches") def delete(self, *args, **kwargs): from readthedocs.projects.tasks.utils import clean_project_resources @@ -1197,8 +1202,7 @@ def update_latest_version(self): """ latest = self.get_latest_version() if not latest: - default_branch = self.get_default_branch() - latest = self.versions.create_latest(identifier=default_branch) + latest = self.versions.create_latest(identifier=self.get_default_branch()) if not latest.machine: return From 601def43ac41990364e5b65e302cdfc2698e03fe Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 30 Nov 2023 14:50:12 -0500 Subject: [PATCH 08/13] Fix tests --- .../rtd_tests/tests/test_sync_versions.py | 67 +++++++++++++++++-- 1 file changed, 61 insertions(+), 6 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_sync_versions.py b/readthedocs/rtd_tests/tests/test_sync_versions.py index ee0ac311905..edebf8749d6 100644 --- a/readthedocs/rtd_tests/tests/test_sync_versions.py +++ b/readthedocs/rtd_tests/tests/test_sync_versions.py @@ -302,9 +302,11 @@ def test_update_latest_version_type(self): ) latest_version = self.pip.versions.get(slug=LATEST) - self.assertEqual(self.pip.default_branch, None) + self.assertIsNone(self.pip.default_branch) + # Since the project doesn't have an explicit default branch, the latest + # version will be updated after the next build. self.assertEqual(latest_version.type, BRANCH) - self.assertEqual(latest_version.identifier, "master") + self.assertEqual(latest_version.identifier, "abc123") self.assertEqual(latest_version.verbose_name, "latest") self.assertEqual(latest_version.machine, True) @@ -719,13 +721,38 @@ def test_machine_attr_when_user_define_latest_tag_and_delete_it(self): tags_data=[], ) - # The latest isn't stuck with the previous commit + # Latest isn't stuck with the previous commit, as it's managed by RTD now. + # Since the project doesn't have an explicit default branch, the latest + # version will be updated after the next build. version_latest = self.pip.versions.get(slug="latest") + self.assertIsNone(self.pip.default_branch) + self.assertTrue(version_latest.machine) self.assertEqual( - "master", version_latest.identifier, + "1abc2def3", + ) + + # Test with an explicit default branch (tag). + self.pip.default_branch = "default-tag" + self.pip.save() + + tags_data = [ + { + "identifier": "1abc2def3", + "verbose_name": "default-tag", + } + ] + + sync_versions_task( + self.pip.pk, + branches_data=branches_data, + tags_data=tags_data, ) + + version_latest = self.pip.versions.get(slug="latest") self.assertTrue(version_latest.machine) + self.assertEqual(version_latest.identifier, "default-tag") + self.assertEqual(version_latest.type, TAG) def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): """The user creates a branch named ``latest`` on an existing repo, when @@ -758,6 +785,7 @@ def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): "origin/latest", version_latest.identifier, ) + self.assertFalse(version_latest.machine) # Deleting the branch should return the RTD's latest branches_data = [ @@ -773,13 +801,40 @@ def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): tags_data=[], ) - # The latest isn't stuck with the previous branch + # Latest isn't stuck with the previous branch, as it's managed by RTD now. + # Since the project doesn't have an explicit default branch, the latest + # version will be updated after the next build. version_latest = self.pip.versions.get(slug="latest") + self.assertIsNone(self.pip.default_branch) + self.assertTrue(version_latest.machine) self.assertEqual( - "master", version_latest.identifier, + "origin/latest", ) + + # Test with an explicit default branch. + branches_data = [ + { + "identifier": "origin/master", + "verbose_name": "master", + }, + { + "identifier": "origin/default-branch", + "verbose_name": "default-branch", + }, + ] + self.pip.default_branch = "default-branch" + self.pip.save() + sync_versions_task( + self.pip.pk, + branches_data=branches_data, + tags_data=[], + ) + + version_latest = self.pip.versions.get(slug="latest") self.assertTrue(version_latest.machine) + self.assertEqual(version_latest.identifier, "default-branch") + self.assertEqual(version_latest.type, BRANCH) def test_deletes_version_with_same_identifier(self): branches_data = [ From 24edfde8ddf4186a54320b24d0c2b332628c585e Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 30 Nov 2023 14:52:40 -0500 Subject: [PATCH 09/13] Format --- readthedocs/doc_builder/director.py | 4 +++- readthedocs/projects/tasks/builds.py | 8 ++++++-- readthedocs/projects/tests/test_build_tasks.py | 9 ++++++++- readthedocs/rtd_tests/tests/test_version.py | 10 +++++++++- 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 9b751fad9d5..a289b3bc4eb 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -227,7 +227,9 @@ def checkout(self): identifier = self.data.build_commit or self.data.version.identifier is_rtd_latest = self.data.version.slug == LATEST and self.data.version.machine - skip_checkout = not identifier or (is_rtd_latest and not self.data.project.default_branch) + skip_checkout = not identifier or ( + is_rtd_latest and not self.data.project.default_branch + ) if skip_checkout: log.info("Skipping checkout, using default branch.") else: diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 53f826e3f81..326345913da 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -651,7 +651,9 @@ def on_success(self, retval, task_id, args, kwargs): } # Update the latest version to point to the current default branch # if the project doesn't have a default branch set. - is_rtd_latest = self.data.version.slug == LATEST and self.data.version.machine + is_rtd_latest = ( + self.data.version.slug == LATEST and self.data.version.machine + ) if ( is_rtd_latest and not self.data.project.default_branch @@ -796,7 +798,9 @@ def execute(self): # have an explicit default branch set and we are building latest. # The identifier from latest will be updated with this value # if the build succeeds. - is_rtd_latest = self.data.version.slug == LATEST and self.data.version.machine + is_rtd_latest = ( + self.data.version.slug == LATEST and self.data.version.machine + ) if is_rtd_latest and not self.data.project.default_branch: self.data.default_branch = ( self.data.build_director.vcs_repository.get_default_branch() diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index 27791fab3ab..dd1da05b238 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -734,7 +734,14 @@ def test_build_commands_executed_latest_version( "--depth", "50", ), - mock.call('git', 'symbolic-ref', '--short', 'refs/remotes/origin/HEAD', demux=True, record=False), + mock.call( + "git", + "symbolic-ref", + "--short", + "refs/remotes/origin/HEAD", + demux=True, + record=False, + ), mock.call( "git", "ls-remote", diff --git a/readthedocs/rtd_tests/tests/test_version.py b/readthedocs/rtd_tests/tests/test_version.py index 464a48619e9..82801bc6241 100644 --- a/readthedocs/rtd_tests/tests/test_version.py +++ b/readthedocs/rtd_tests/tests/test_version.py @@ -2,7 +2,15 @@ from django.test.utils import override_settings from django_dynamic_fixture import get -from readthedocs.builds.constants import BRANCH, EXTERNAL, LATEST, LATEST_VERBOSE_NAME, STABLE, STABLE_VERBOSE_NAME, TAG +from readthedocs.builds.constants import ( + BRANCH, + EXTERNAL, + LATEST, + LATEST_VERBOSE_NAME, + STABLE, + STABLE_VERBOSE_NAME, + TAG, +) from readthedocs.builds.models import Version from readthedocs.projects.models import Project From c358398be65494b417ccfb6102885857688b5895 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 30 Nov 2023 15:21:22 -0500 Subject: [PATCH 10/13] Remove todos --- readthedocs/api/v2/views/integrations.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/readthedocs/api/v2/views/integrations.py b/readthedocs/api/v2/views/integrations.py index 2f6596b1153..ba5049b4871 100644 --- a/readthedocs/api/v2/views/integrations.py +++ b/readthedocs/api/v2/views/integrations.py @@ -447,7 +447,6 @@ def handle_webhook(self): event=event, ) - # TODO: do we need this? # Always update `latest` branch to point to the default branch in the repository # even if the event is not gonna be handled. This helps us to keep our db in sync. default_branch = self.data.get("repository", {}).get("default_branch", None) @@ -612,7 +611,6 @@ def handle_webhook(self): integration = self.get_integration() - # TODO: do we need this? # Always update `latest` branch to point to the default branch in the repository # even if the event is not gonna be handled. This helps us to keep our db in sync. default_branch = self.data.get("project", {}).get("default_branch", None) @@ -715,7 +713,6 @@ def handle_webhook(self): event=event, ) - # TODO: do we need this? # NOTE: we can't call `self.update_default_branch` here because # BitBucket does not tell us what is the `default_branch` for a # repository in these incoming webhooks. @@ -839,7 +836,6 @@ def handle_webhook(self): if isinstance(branches, str): branches = [branches] - # TODO: do we need this? if default_branch and isinstance(default_branch, str): self.update_default_branch(default_branch) From 92266856c0b52599774406d7f8b633020dd09c8c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 30 Nov 2023 15:37:49 -0500 Subject: [PATCH 11/13] Back to buggy version --- readthedocs/projects/models.py | 9 +-------- .../rtd_tests/tests/test_sync_versions.py | 16 +++++----------- 2 files changed, 6 insertions(+), 19 deletions(-) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index cab3c09f0c9..f40c239e9bb 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1305,14 +1305,7 @@ def get_default_branch(self): if self.remote_repository and self.remote_repository.default_branch: return self.remote_repository.default_branch - fallback_branch = self.vcs_class().fallback_branch - latest_version = self.versions.filter(slug=LATEST).first() - if latest_version: - if latest_version.machine: - return latest_version.identifier or fallback_branch - return latest_version.verbose_name or fallback_branch - - return fallback_branch + return self.vcs_class().fallback_branch def add_subproject(self, child, alias=None): subproject, _ = ProjectRelationship.objects.get_or_create( diff --git a/readthedocs/rtd_tests/tests/test_sync_versions.py b/readthedocs/rtd_tests/tests/test_sync_versions.py index edebf8749d6..3739ae7b80e 100644 --- a/readthedocs/rtd_tests/tests/test_sync_versions.py +++ b/readthedocs/rtd_tests/tests/test_sync_versions.py @@ -303,10 +303,8 @@ def test_update_latest_version_type(self): latest_version = self.pip.versions.get(slug=LATEST) self.assertIsNone(self.pip.default_branch) - # Since the project doesn't have an explicit default branch, the latest - # version will be updated after the next build. self.assertEqual(latest_version.type, BRANCH) - self.assertEqual(latest_version.identifier, "abc123") + self.assertEqual(latest_version.identifier, "master") self.assertEqual(latest_version.verbose_name, "latest") self.assertEqual(latest_version.machine, True) @@ -721,15 +719,13 @@ def test_machine_attr_when_user_define_latest_tag_and_delete_it(self): tags_data=[], ) - # Latest isn't stuck with the previous commit, as it's managed by RTD now. - # Since the project doesn't have an explicit default branch, the latest - # version will be updated after the next build. + # The latest isn't stuck with the previous commit version_latest = self.pip.versions.get(slug="latest") self.assertIsNone(self.pip.default_branch) self.assertTrue(version_latest.machine) self.assertEqual( + "master", version_latest.identifier, - "1abc2def3", ) # Test with an explicit default branch (tag). @@ -801,15 +797,13 @@ def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): tags_data=[], ) - # Latest isn't stuck with the previous branch, as it's managed by RTD now. - # Since the project doesn't have an explicit default branch, the latest - # version will be updated after the next build. + # The latest isn't stuck with the previous branch version_latest = self.pip.versions.get(slug="latest") self.assertIsNone(self.pip.default_branch) self.assertTrue(version_latest.machine) self.assertEqual( + "master", version_latest.identifier, - "origin/latest", ) # Test with an explicit default branch. From 12128d01ba585eb9972926f1a40f906c05c4e4f9 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 30 Nov 2023 16:04:14 -0500 Subject: [PATCH 12/13] Update docstring --- readthedocs/projects/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index f40c239e9bb..85a9e871279 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1296,8 +1296,8 @@ def get_default_branch(self): Get the branch/tag name of the version representing 'latest'. If the project has a default branch explicitly set, we use that, - otherwise we try to get it from the latest version, - since that should be in sync with the default branch of the repository. + otherwise we try to get it from the remote repository, + or fallback to the default branch of the VCS backend. """ if self.default_branch: return self.default_branch From 7bdec0731e8d394d87b7a9c19ec7ad7b42db599c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 30 Nov 2023 16:07:55 -0500 Subject: [PATCH 13/13] Typo --- readthedocs/vcs_support/backends/git.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 4b89de6c0b6..253224b77a9 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -202,8 +202,8 @@ def fetch(self): is_rtd_latest = ( self.verbose_name == LATEST_VERBOSE_NAME and self.version_machine ) - ommit_remote_reference = is_rtd_latest and not self.project.default_branch - if not ommit_remote_reference: + omit_remote_reference = is_rtd_latest and not self.project.default_branch + if not omit_remote_reference: remote_reference = self.get_remote_fetch_refspec() if remote_reference: # TODO: We are still fetching the latest 50 commits.