From a72d45bf281942ea97d86bdbcdbcaf15b3d2b46b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 24 Dec 2024 13:32:08 -0500 Subject: [PATCH 1/3] Build: use actual git identifier for READTHEDOCS_GIT_IDENTIFIER Closes https://github.com/readthedocs/readthedocs.org/issues/11662 --- readthedocs/api/v2/serializers.py | 1 + readthedocs/builds/models.py | 53 ++++++++----------- readthedocs/doc_builder/director.py | 2 +- .../projects/tests/test_build_tasks.py | 2 +- readthedocs/rtd_tests/tests/test_api.py | 1 + .../rtd_tests/tests/test_project_forms.py | 23 -------- readthedocs/rtd_tests/tests/test_version.py | 12 ++--- .../tests/test_version_commit_name.py | 38 ++++++++++--- 8 files changed, 61 insertions(+), 71 deletions(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index 0dd9ceed76f..45e6a03c67e 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -187,6 +187,7 @@ class Meta(VersionSerializer.Meta): "build_data", "canonical_url", "machine", + "git_identifier", ] diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 25e576c630a..b577a290365 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -31,7 +31,6 @@ PREDEFINED_MATCH_ARGS, PREDEFINED_MATCH_ARGS_VALUES, STABLE, - TAG, VERSION_TYPES, ) from readthedocs.builds.managers import ( @@ -330,50 +329,35 @@ def config(self): return None @property - def commit_name(self): + def git_identifier(self): """ - Return the branch name, the tag name or the revision identifier. + Return the branch or tag name of the version. The result could be used as ref in a git repo, e.g. for linking to GitHub, Bitbucket or GitLab. + + - If the version is latest (machine created), we resolve to the default branch of the project. + - If the version is stable (machine created), we resolve to the branch that the stable version points to. + - If the version is external, we return the identifier, since we don't have access to the name of the branch. """ - # LATEST is special as it is usually a branch but does not contain the - # name in verbose_name. + # Latest is special as it doesn't contain the actual name in verbose_name. if self.slug == LATEST and self.machine: return self.project.get_default_branch() + # Stable is special as it doesn't contain the actual name in verbose_name. 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 - # name from the commit identifier, but it's hacky. - # TODO: Refactor ``Version`` to store more actual info about - # the underlying commits. - if self.identifier.startswith("origin/"): - return self.identifier[len("origin/") :] - return self.identifier - - 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 - # include the "origin/..." part in the case of a branch. A tag - # would contain the hash in identifier, which is not as pretty as - # the actual tag name. - return self.verbose_name + original_stable = self.project.get_original_stable_version() + # NOTE: we no longer save branch names with the "origin/" prefix, + # but we remove it for old versions. + if original_stable: + return original_stable.verbose_name.removeprefix("origin/") + return self.identifier.removeprefix("origin/") if self.type == EXTERNAL: - # If this version is a EXTERNAL version, the identifier will - # contain the actual commit hash. which we can use to - # generate url for a given file name return self.identifier - # If we came that far it's not a special version - # nor a branch, tag or EXTERNAL version. - # Therefore just return the identifier to make a safe guess. - log.debug( - "TODO: Raise an exception here. Testing what cases it happens", - ) - return self.identifier + # For all other cases, verbose_name contains the actual name of the branch/tag. + return self.verbose_name def get_absolute_url(self): """ @@ -561,6 +545,10 @@ class APIVersion(Version): """ project = None + # This is a property in the original model, in order to + # be able to assign it a value in the constructor, we need to re-declare it + # as an attribute here. + git_identifier = None class Meta: proxy = True @@ -568,6 +556,7 @@ class Meta: def __init__(self, *args, **kwargs): self.project = APIProject(**kwargs.pop("project", {})) self.canonical_url = kwargs.pop("canonical_url", None) + self.git_identifier = kwargs.pop("git_identifier", None) # These fields only exist on the API return, not on the model, so we'll # remove them to avoid throwing exceptions due to unexpected fields for key in ["resource_uri", "absolute_url", "downloads"]: diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 51b61fe6cf7..0c2c5265dcb 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -696,7 +696,7 @@ def get_rtd_env_vars(self): # TODO: we don't have access to the database from the builder. # We need to find a way to expose HTML_URL here as well. # "READTHEDOCS_GIT_HTML_URL": self.data.project.remote_repository.html_url, - "READTHEDOCS_GIT_IDENTIFIER": self.data.version.identifier, + "READTHEDOCS_GIT_IDENTIFIER": self.data.version.git_identifier, "READTHEDOCS_GIT_COMMIT_HASH": self.data.build["commit"], "READTHEDOCS_PRODUCTION_DOMAIN": settings.PRODUCTION_DOMAIN, } diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index 1dad403d2c2..b9380048fc6 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -372,7 +372,7 @@ def test_get_env_vars(self, load_yaml_config, build_environment, config, externa self.project.checkout_path(self.version.slug), "_readthedocs/" ), "READTHEDOCS_GIT_CLONE_URL": self.project.repo, - "READTHEDOCS_GIT_IDENTIFIER": self.version.identifier, + "READTHEDOCS_GIT_IDENTIFIER": self.version.git_identifier, "READTHEDOCS_GIT_COMMIT_HASH": self.build.commit, "READTHEDOCS_PRODUCTION_DOMAIN": settings.PRODUCTION_DOMAIN, } diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 725d57c3896..e9c0339b37b 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -3292,6 +3292,7 @@ def test_get_version_by_id(self): "privacy_level": "public", "downloads": {}, "identifier": "2404a34eba4ee9c48cc8bc4055b99a48354f4950", + "git_identifier": "0.8", "slug": "0.8", "has_epub": False, "has_htmlzip": False, diff --git a/readthedocs/rtd_tests/tests/test_project_forms.py b/readthedocs/rtd_tests/tests/test_project_forms.py index 0e738fea171..145b87a0262 100644 --- a/readthedocs/rtd_tests/tests/test_project_forms.py +++ b/readthedocs/rtd_tests/tests/test_project_forms.py @@ -478,29 +478,6 @@ def test_list_user_created_latest_and_stable_versions_in_default_branch_choices( ], ) - def test_commit_name_not_in_default_branch_choices(self): - form = UpdateProjectForm(instance=self.project) - # This version is created by the user - latest = self.project.versions.filter(slug=LATEST) - # This version is created by the user - stable = self.project.versions.filter(slug=STABLE) - - # `commit_name` can not be used as the value for the choices - self.assertNotIn( - latest.first().commit_name, - [ - identifier - for identifier, _ in form.fields["default_branch"].widget.choices - ], - ) - self.assertNotIn( - stable.first().commit_name, - [ - identifier - for identifier, _ in form.fields["default_branch"].widget.choices - ], - ) - def test_external_version_not_in_default_branch_choices(self): external_version = get( Version, diff --git a/readthedocs/rtd_tests/tests/test_version.py b/readthedocs/rtd_tests/tests/test_version.py index 1970066eaa8..99330658cd5 100644 --- a/readthedocs/rtd_tests/tests/test_version.py +++ b/readthedocs/rtd_tests/tests/test_version.py @@ -94,15 +94,15 @@ def test_vcs_url_for_manual_stable_version(self): stable.save() assert "https://github.com/pypa/pip/tree/stable/" == stable.vcs_url - def test_commit_name_for_stable_version(self): - self.assertEqual(self.branch_version.commit_name, "stable") + def test_git_identifier_for_stable_version(self): + self.assertEqual(self.branch_version.git_identifier, "stable") - def test_commit_name_for_latest_version(self): - self.assertEqual(self.tag_version.commit_name, "master") + def test_git_identifier_for_latest_version(self): + self.assertEqual(self.tag_version.git_identifier, "master") - def test_commit_name_for_external_version(self): + def test_git_identifier_for_external_version(self): self.assertEqual( - self.external_version.commit_name, self.external_version.identifier + self.external_version.git_identifier, self.external_version.identifier ) @override_settings( diff --git a/readthedocs/rtd_tests/tests/test_version_commit_name.py b/readthedocs/rtd_tests/tests/test_version_commit_name.py index a4dfd4debdc..0fa5004ce5e 100644 --- a/readthedocs/rtd_tests/tests/test_version_commit_name.py +++ b/readthedocs/rtd_tests/tests/test_version_commit_name.py @@ -33,7 +33,7 @@ def test_branch_name(self): verbose_name="release-2.5.x", type=BRANCH, ) - self.assertEqual(version.commit_name, "release-2.5.x") + self.assertEqual(version.git_identifier, "release-2.5.x") def test_tag_name(self): version = new( @@ -43,7 +43,7 @@ def test_tag_name(self): verbose_name="release-2.5.0", type=TAG, ) - self.assertEqual(version.commit_name, "release-2.5.0") + self.assertEqual(version.git_identifier, "release-2.5.0") def test_branch_with_name_stable(self): version = new( @@ -53,7 +53,7 @@ def test_branch_with_name_stable(self): verbose_name="stable", type=BRANCH, ) - self.assertEqual(version.commit_name, "stable") + self.assertEqual(version.git_identifier, "stable") def test_stable_version_tag(self): version = new( @@ -65,10 +65,32 @@ def test_stable_version_tag(self): machine=True, ) self.assertEqual( - version.commit_name, + version.git_identifier, "3d92b728b7d7b842259ac2020c2fa389f13aff0d", ) + def test_stable_version_tag_uses_original_stable(self): + project = get(Project) + version = get( + Version, + project=project, + identifier="3d92b728b7d7b842259ac2020c2fa389f13aff0d", + slug=STABLE, + verbose_name=STABLE, + type=TAG, + machine=True, + ) + get( + Version, + project=project, + identifier="3d92b728b7d7b842259ac2020c2fa389f13aff0d", + slug="v2.0", + verbose_name="v2.0", + type=TAG, + machine=True, + ) + self.assertEqual(version.git_identifier, "v2.0") + def test_manual_stable_version(self): project = get(Project) version = get( @@ -80,7 +102,7 @@ def test_manual_stable_version(self): type=BRANCH, machine=False, ) - self.assertEqual(version.commit_name, "stable") + self.assertEqual(version.git_identifier, "stable") def test_git_latest_branch(self): git_project = get(Project, repo_type=REPO_TYPE_GIT) @@ -93,14 +115,14 @@ def test_git_latest_branch(self): type=BRANCH, machine=True, ) - self.assertEqual(version.commit_name, "master") + self.assertEqual(version.git_identifier, "master") def test_manual_latest_version(self): project = get(Project) version = project.versions.get(slug=LATEST) version.machine = False version.save() - self.assertEqual(version.commit_name, "latest") + self.assertEqual(version.git_identifier, "latest") def test_external_version(self): identifier = "ec26de721c3235aad62de7213c562f8c821" @@ -111,4 +133,4 @@ def test_external_version(self): verbose_name="11", type=EXTERNAL, ) - self.assertEqual(version.commit_name, identifier) + self.assertEqual(version.git_identifier, identifier) From 9bfc4c4b842de0a5fac97c1561fd9ce12b085020 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 24 Dec 2024 13:43:04 -0500 Subject: [PATCH 2/3] Update --- readthedocs/builds/models.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index b577a290365..daf3198e3fa 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -354,6 +354,9 @@ def git_identifier(self): return self.identifier.removeprefix("origin/") if self.type == EXTERNAL: + # If this version is a EXTERNAL version, the identifier will + # contain the actual commit hash. which we can use to + # generate url for a given file name return self.identifier # For all other cases, verbose_name contains the actual name of the branch/tag. From 2d02cd1d9513b97ab0b86cc66742ad2d15695726 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 24 Dec 2024 13:44:16 -0500 Subject: [PATCH 3/3] Test --- readthedocs/rtd_tests/tests/test_version_commit_name.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_version_commit_name.py b/readthedocs/rtd_tests/tests/test_version_commit_name.py index 0fa5004ce5e..07c32b07bde 100644 --- a/readthedocs/rtd_tests/tests/test_version_commit_name.py +++ b/readthedocs/rtd_tests/tests/test_version_commit_name.py @@ -71,7 +71,7 @@ def test_stable_version_tag(self): def test_stable_version_tag_uses_original_stable(self): project = get(Project) - version = get( + stable = get( Version, project=project, identifier="3d92b728b7d7b842259ac2020c2fa389f13aff0d", @@ -80,7 +80,7 @@ def test_stable_version_tag_uses_original_stable(self): type=TAG, machine=True, ) - get( + original_stable = get( Version, project=project, identifier="3d92b728b7d7b842259ac2020c2fa389f13aff0d", @@ -89,7 +89,8 @@ def test_stable_version_tag_uses_original_stable(self): type=TAG, machine=True, ) - self.assertEqual(version.git_identifier, "v2.0") + self.assertEqual(stable.git_identifier, "v2.0") + self.assertEqual(original_stable.git_identifier, "v2.0") def test_manual_stable_version(self): project = get(Project)