diff --git a/docs/user/integrations.rst b/docs/user/integrations.rst index a53d5f32ce8..f04e86de1bc 100644 --- a/docs/user/integrations.rst +++ b/docs/user/integrations.rst @@ -153,10 +153,17 @@ token The integration token found on the project's **Integrations** dashboard page (:guilabel:`Admin` > :guilabel:`Integrations`). +default_branch + This is the default branch of the repository + (ie. the one checked out when cloning the repository without arguments) + + *Optional* + For example, the cURL command to build the ``dev`` branch, using the token ``1234``, would be:: - curl -X POST -d "branches=dev" -d "token=1234" https://readthedocs.org/api/v2/webhook/example-project/1/ + curl -X POST -d "branches=dev" -d "token=1234" -d "default_branch=main" + https://readthedocs.org/api/v2/webhook/example-project/1/ A command like the one above could be called from a cron job or from a hook inside Git_, Subversion_, Mercurial_, or Bazaar_. diff --git a/readthedocs/api/v2/views/integrations.py b/readthedocs/api/v2/views/integrations.py index 0b1dbeadc2d..f95ce369c43 100644 --- a/readthedocs/api/v2/views/integrations.py +++ b/readthedocs/api/v2/views/integrations.py @@ -18,6 +18,7 @@ from readthedocs.builds.constants import ( EXTERNAL_VERSION_STATE_CLOSED, EXTERNAL_VERSION_STATE_OPEN, + LATEST, ) from readthedocs.core.signals import webhook_bitbucket, webhook_github, webhook_gitlab from readthedocs.core.views.hooks import ( @@ -280,6 +281,30 @@ def get_closed_external_version_response(self, project): "versions": [version_closed] if version_closed else [], } + def update_default_branch(self, default_branch): + """ + Update the `Version.identifer` 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 `). + + Some VCS providers (GitHub and GitLab) send the `default_branch` via incoming webhooks. + We use that data to update our database and keep it in sync. + + This solves the problem about "changing the default branch in GitHub" + and also importing repositories with a different `default_branch` than `main` manually: + https://github.com/readthedocs/readthedocs.org/issues/9367 + + In case the user already selected a `default-branch` from the "Advanced settings", + it does not override it. + """ + if not self.project.default_branch: + ( + self.project.versions.filter(slug=LATEST).update( + identifier=default_branch + ) + ) + class GitHubWebhookView(WebhookMixin, APIView): @@ -417,6 +442,12 @@ def handle_webhook(self): event=event, ) + # 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) + if default_branch: + self.update_default_branch(default_branch) + if event == GITHUB_PING: return {"detail": "Webhook configured correctly"} @@ -467,14 +498,15 @@ def handle_webhook(self): # Trigger a build for all branches in the push if event == GITHUB_PUSH: try: - branches = [self._normalize_ref(self.data['ref'])] - return self.get_response_push(self.project, branches) + branch = self._normalize_ref(self.data["ref"]) + return self.get_response_push(self.project, [branch]) except KeyError: raise ParseError('Parameter "ref" is required') return None def _normalize_ref(self, ref): + """Remove `ref/(heads|tags)/` from the reference to match a Version on the db.""" pattern = re.compile(r'^refs/(heads|tags)/') return pattern.sub('', ref) @@ -568,6 +600,13 @@ def handle_webhook(self): data=self.request.data, event=event, ) + + # 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) + if default_branch: + self.update_default_branch(default_branch) + # Handle push events and trigger builds if event in (GITLAB_PUSH, GITLAB_TAG_PUSH): data = self.request.data @@ -583,8 +622,8 @@ def handle_webhook(self): return self.sync_versions_response(self.project) # Normal push to master try: - branches = [self._normalize_ref(data['ref'])] - return self.get_response_push(self.project, branches) + branch = self._normalize_ref(data["ref"]) + return self.get_response_push(self.project, [branch]) except KeyError: raise ParseError('Parameter "ref" is required') @@ -660,6 +699,11 @@ def handle_webhook(self): data=self.request.data, event=event, ) + + # 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. + if event == BITBUCKET_PUSH: try: data = self.request.data @@ -676,8 +720,11 @@ def handle_webhook(self): # we don't trigger the sync versions, because that # will be triggered with the normal push. if branches: - return self.get_response_push(self.project, branches) - log.debug('Triggered sync_versions.') + return self.get_response_push( + self.project, + branches, + ) + log.debug("Triggered sync_versions.") return self.sync_versions_response(self.project) except KeyError: raise ParseError('Invalid request') @@ -710,7 +757,8 @@ class APIWebhookView(WebhookMixin, APIView): Expects the following JSON:: { - "branches": ["master"] + "branches": ["master"], + "default_branch": "main" } """ @@ -753,8 +801,13 @@ def handle_webhook(self): 'branches', [self.project.get_default_branch()], ) + default_branch = self.request.data.get("default_branch", None) if isinstance(branches, str): branches = [branches] + + if default_branch and isinstance(default_branch, str): + self.update_default_branch(default_branch) + return self.get_response_push(self.project, branches) except TypeError: raise ParseError('Invalid request') diff --git a/readthedocs/builds/managers.py b/readthedocs/builds/managers.py index cf82b8b209b..3ecb62fae69 100644 --- a/readthedocs/builds/managers.py +++ b/readthedocs/builds/managers.py @@ -1,7 +1,6 @@ """Build and Version class model Managers.""" import structlog - from django.core.exceptions import ObjectDoesNotExist from django.db import models from polymorphic.managers import PolymorphicManager @@ -50,6 +49,12 @@ def create_stable(self, **kwargs): 'verbose_name': STABLE_VERBOSE_NAME, 'machine': True, 'active': True, + # TODO: double-check if we still require the `identifier: STABLE` field. + # At the time of creation, we don't really know what's the branch/tag identifier + # for the STABLE version. It makes sense to be `None`, probably. + # + # Note that we removed the `identifier: LATEST` from `create_latest` as a way to + # use the default branch. 'identifier': STABLE, 'type': TAG, } @@ -62,7 +67,6 @@ def create_latest(self, **kwargs): 'verbose_name': LATEST_VERBOSE_NAME, 'machine': True, 'active': True, - 'identifier': LATEST, 'type': BRANCH, } defaults.update(kwargs) diff --git a/readthedocs/builds/migrations/0046_identifier_null.py b/readthedocs/builds/migrations/0046_identifier_null.py new file mode 100644 index 00000000000..a2606b050a2 --- /dev/null +++ b/readthedocs/builds/migrations/0046_identifier_null.py @@ -0,0 +1,20 @@ +# Generated by Django 3.2.13 on 2022-07-11 08:53 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("builds", "0045_alter_build_status"), + ] + + operations = [ + migrations.AlterField( + model_name="version", + name="identifier", + field=models.CharField( + blank=True, max_length=255, null=True, verbose_name="Identifier" + ), + ), + ] diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 5dbed23cf5d..9652de04249 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -119,11 +119,15 @@ class Version(TimeStampedModel): ) # used by the vcs backend - #: The identifier is the ID for the revision this is version is for. This - #: might be the revision number (e.g. in SVN), or the commit hash (e.g. in - #: Git). If the this version is pointing to a branch, then ``identifier`` - #: will contain the branch name. - identifier = models.CharField(_('Identifier'), max_length=255) + #: The identifier is the ID for the revision this is version is for. + #: This might be the revision number (e.g. in SVN), + #: or the commit hash (e.g. in Git). + #: If the this version is pointing to a branch, + #: then ``identifier`` will contain the branch name. + #: `None`/`null` means it will use the VCS default branch. + identifier = models.CharField( + _("Identifier"), max_length=255, null=True, blank=True + ) #: This is the actual name that we got for the commit stored in #: ``identifier``. This might be the tag or branch name like ``"v1.0.4"``. @@ -363,6 +367,11 @@ def delete(self, *args, **kwargs): # pylint: disable=arguments-differ @property def identifier_friendly(self): """Return display friendly identifier.""" + if not self.identifier: + # Text shown to user when we don't know yet what's the ``identifier`` for this version. + # This usually happens when we haven't pulled the ``default_branch`` for LATEST. + return "Unknown yet" + if re.match(r'^[0-9a-f]{40}$', self.identifier, re.I): return self.identifier[:8] return self.identifier diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index d7ed214c0c8..49832a05339 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -52,7 +52,6 @@ def build_branches(project, branch_list): not_building = set() for branch in branch_list: versions = project.versions_from_branch_name(branch) - for version in versions: log.debug( 'Processing.', diff --git a/readthedocs/projects/migrations/0095_default_branch_helptext.py b/readthedocs/projects/migrations/0095_default_branch_helptext.py new file mode 100644 index 00000000000..4423cc29d33 --- /dev/null +++ b/readthedocs/projects/migrations/0095_default_branch_helptext.py @@ -0,0 +1,37 @@ +# Generated by Django 3.2.13 on 2022-07-11 09:06 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("projects", "0094_auto_20221221_1045"), + ] + + operations = [ + migrations.AlterField( + model_name="historicalproject", + name="default_branch", + field=models.CharField( + blank=True, + default=None, + help_text='What branch "latest" points to. Leave empty to use the default value for your VCS.', + max_length=255, + null=True, + verbose_name="Default branch", + ), + ), + migrations.AlterField( + model_name="project", + name="default_branch", + field=models.CharField( + blank=True, + default=None, + help_text='What branch "latest" points to. Leave empty to use the default value for your VCS.', + max_length=255, + null=True, + verbose_name="Default branch", + ), + ), + ] diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 68042b4aa7e..9c581f13f6a 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -175,8 +175,8 @@ class Project(models.Model): default=LATEST, help_text=_('The version of your project that / redirects to'), ) - # In default_branch, None means the backend should choose the - # appropriate branch. Eg 'master' for git + # In default_branch, ``None`` means the backend will use the default branch + # cloned for each backend. default_branch = models.CharField( _('Default branch'), max_length=255, @@ -185,8 +185,7 @@ class Project(models.Model): blank=True, help_text=_( 'What branch "latest" points to. Leave empty ' - 'to use the default value for your VCS (eg. ' - 'trunk or master).', + "to use the default value for your VCS.", ), ) requirements_file = models.CharField( @@ -480,21 +479,21 @@ def save(self, *args, **kwargs): # pylint: disable=arguments-differ if not self.slug: raise Exception(_('Model must have slug')) super().save(*args, **kwargs) - try: - latest = self.versions.filter(slug=LATEST).first() - default_branch = self.get_default_branch() - if latest and latest.identifier != default_branch: - latest.identifier = default_branch - latest.save() - except Exception: - log.exception('Failed to update latest identifier') try: - branch = self.get_default_branch() if not self.versions.filter(slug=LATEST).exists(): - self.versions.create_latest(identifier=branch) + self.versions.create_latest() except Exception: - log.exception('Error creating default branches') + 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) def delete(self, *args, **kwargs): # pylint: disable=arguments-differ from readthedocs.projects.tasks.utils import clean_project_resources diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 4a20cd48e2a..17379205894 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -928,6 +928,7 @@ def setUp(self): Project, build_queue=None, external_builds_enabled=True, + default_branch="master", ) self.version = get( Version, slug='master', verbose_name='master', @@ -2346,8 +2347,12 @@ def test_webhook_build_latest_and_master(self, trigger_build): self.project.slug, integration.pk, ), - {'token': integration.token, 'branches': default_branch.slug}, - format='json', + { + "token": integration.token, + "branches": default_branch.slug, + "default_branch": "master", + }, + format="json", ) self.assertEqual(resp.status_code, 200) self.assertTrue(resp.data['build_triggered']) @@ -2402,46 +2407,46 @@ def test_get_version_by_id(self): self.assertEqual(resp.status_code, 200) version_data = { - 'type': 'tag', - 'verbose_name': '0.8', - 'built': False, - 'id': 18, - 'active': True, - 'project': { - 'analytics_code': None, - 'analytics_disabled': False, - 'canonical_url': 'http://readthedocs.org/docs/pip/en/latest/', - 'cdn_enabled': False, - 'conf_py_file': '', - 'container_image': None, - 'container_mem_limit': None, - 'container_time_limit': None, - 'default_branch': None, - 'default_version': 'latest', - 'description': '', - 'documentation_type': 'sphinx', - 'environment_variables': {}, - 'enable_epub_build': True, - 'enable_pdf_build': True, - 'features': ['allow_deprecated_webhooks'], - 'has_valid_clone': False, - 'has_valid_webhook': False, - 'id': 6, - 'install_project': False, - 'language': 'en', - 'max_concurrent_builds': None, - 'name': 'Pip', - 'programming_language': 'words', - 'python_interpreter': 'python3', - 'repo': 'https://github.com/pypa/pip', - 'repo_type': 'git', - 'requirements_file': None, - 'show_advertising': True, - 'skip': False, - 'slug': 'pip', - 'use_system_packages': False, - 'users': [1], - 'urlconf': None, + "type": "tag", + "verbose_name": "0.8", + "built": False, + "id": 18, + "active": True, + "project": { + "analytics_code": None, + "analytics_disabled": False, + "canonical_url": "http://readthedocs.org/docs/pip/en/latest/", + "cdn_enabled": False, + "conf_py_file": "", + "container_image": None, + "container_mem_limit": None, + "container_time_limit": None, + "default_branch": None, + "default_version": "latest", + "description": "", + "documentation_type": "sphinx", + "environment_variables": {}, + "enable_epub_build": True, + "enable_pdf_build": True, + "features": ["allow_deprecated_webhooks"], + "has_valid_clone": False, + "has_valid_webhook": False, + "id": 6, + "install_project": False, + "language": "en", + "max_concurrent_builds": None, + "name": "Pip", + "programming_language": "words", + "python_interpreter": "python3", + "repo": "https://github.com/pypa/pip", + "repo_type": "git", + "requirements_file": None, + "show_advertising": True, + "skip": False, + "slug": "pip", + "use_system_packages": False, + "users": [1], + "urlconf": None, }, 'privacy_level': 'public', 'downloads': {}, diff --git a/readthedocs/rtd_tests/tests/test_backend.py b/readthedocs/rtd_tests/tests/test_backend.py index ac4a9fa0dfe..5d17600b392 100644 --- a/readthedocs/rtd_tests/tests/test_backend.py +++ b/readthedocs/rtd_tests/tests/test_backend.py @@ -200,8 +200,11 @@ def test_git_update_and_checkout(self): repo = self.project.vcs_repo() code, _, _ = repo.update() self.assertEqual(code, 0) - code, _, _ = repo.checkout() - self.assertEqual(code, 0) + + # Returns `None` because there is no `identifier`, + # so it uses the default branch + self.assertIsNone(repo.checkout()) + self.assertTrue(exists(repo.working_dir)) @patch('readthedocs.vcs_support.backends.git.Backend.fetch') diff --git a/readthedocs/rtd_tests/tests/test_project_forms.py b/readthedocs/rtd_tests/tests/test_project_forms.py index 75bedb43196..efc2c9b87bf 100644 --- a/readthedocs/rtd_tests/tests/test_project_forms.py +++ b/readthedocs/rtd_tests/tests/test_project_forms.py @@ -1,9 +1,7 @@ -from unittest import mock from django.contrib.auth.models import User from django.test import TestCase from django.test.utils import override_settings -from django.utils.translation import gettext_lazy as _ from django_dynamic_fixture import get from readthedocs.builds.constants import EXTERNAL, LATEST, STABLE @@ -25,11 +23,7 @@ UpdateProjectForm, WebHookForm, ) -from readthedocs.projects.models import ( - EnvironmentVariable, - Project, - WebHookEvent, -) +from readthedocs.projects.models import EnvironmentVariable, Project, WebHookEvent class TestProjectForms(TestCase): @@ -106,25 +100,6 @@ def test_empty_slug(self): self.assertFalse(form.is_valid()) self.assertIn('name', form.errors) - def test_changing_vcs_should_change_latest(self): - """When changing the project's VCS, latest should be changed too.""" - project = get(Project, repo_type=REPO_TYPE_HG, default_branch=None) - latest = project.versions.get(slug=LATEST) - self.assertEqual(latest.identifier, 'default') - - form = ProjectBasicsForm( - { - 'repo': 'http://github.com/test/test', - 'name': 'name', - 'repo_type': REPO_TYPE_GIT, - }, - instance=project, - ) - self.assertTrue(form.is_valid()) - form.save() - latest.refresh_from_db() - self.assertEqual(latest.identifier, 'master') - def test_changing_vcs_should_not_change_latest_is_not_none(self): """ When changing the project's VCS, diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 0cb7d6a4a45..16ca406a373 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -174,11 +174,7 @@ def fetch(self): code, stdout, stderr = self.run(*cmd) return code, stdout, stderr - def checkout_revision(self, revision=None): - if not revision: - branch = self.default_branch or self.fallback_branch - revision = 'origin/%s' % branch - + def checkout_revision(self, revision): try: code, out, err = self.run('git', 'checkout', '--force', revision) return [code, out, err] @@ -198,6 +194,24 @@ def clone(self): try: code, stdout, stderr = self.run(*cmd) + + # TODO: for those VCS providers that don't tell us the `default_branch` + # of the repository in the incoming webhook, + # we need to get it from the cloned repository itself. + # + # cmd = ['git', 'symbolic-ref', 'refs/remotes/origin/HEAD'] + # _, default_branch, _ = self.run(*cmd) + # default_branch = default_branch.replace('refs/remotes/origin/', '') + # + # The idea is to hit the APIv2 here to update the `latest` version with + # the `default_branch` we just got from the repository itself, + # after clonning it. + # However, we don't know the PK for the version we want to update. + # + # api_v2.version(pk).patch( + # {'default_branch': default_branch} + # ) + return code, stdout, stderr except RepositoryError: raise RepositoryError(RepositoryError.CLONE_ERROR()) @@ -322,12 +336,12 @@ def submodules(self): def checkout(self, identifier=None): """Checkout to identifier or latest.""" super().checkout() - # Find proper identifier + + # NOTE: if there is no identifier, we default to default branch cloned if not identifier: - identifier = self.default_branch or self.fallback_branch + return identifier = self.find_ref(identifier) - # Checkout the correct identifier for this branch. code, out, err = self.checkout_revision(identifier)