diff --git a/backend/code_review_backend/issues/admin.py b/backend/code_review_backend/issues/admin.py index a29efb634..775098467 100644 --- a/backend/code_review_backend/issues/admin.py +++ b/backend/code_review_backend/issues/admin.py @@ -14,19 +14,30 @@ class RepositoryAdmin(admin.ModelAdmin): class DiffInline(admin.TabularInline): # Read only inline model = Diff - readonly_fields = ("id", "repository", "mercurial_hash", "phid", "review_task_id") + readonly_fields = ( + "id", + "repository", + "mercurial_hash", + "provider_id", + "review_task_id", + ) class RevisionAdmin(admin.ModelAdmin): list_display = ( "id", - "phabricator_id", + "provider", + "provider_id", "title", "bugzilla_id", "base_repository", "head_repository", ) - list_filter = ("base_repository", "head_repository") + list_filter = ( + "base_repository", + "head_repository", + "provider", + ) inlines = (DiffInline,) diff --git a/backend/code_review_backend/issues/api.py b/backend/code_review_backend/issues/api.py index e9fc5966f..381503832 100644 --- a/backend/code_review_backend/issues/api.py +++ b/backend/code_review_backend/issues/api.py @@ -77,10 +77,12 @@ def create(self, request, *args, **kwargs): # When a revision already exists with that phabricator ID we return its data without creating a new one # This value is used by the bot to identify a revision and publish new Phabricator diffs. # The phabricator ID can be null (on mozilla-central) so we must always try to create a revision for that case - phabricator_id = request.data["phabricator_id"] - if phabricator_id is not None: + provider = request.data["provider"] + provider_id = request.data["provider_id"] + if provider_id is not None: if revision := Revision.objects.filter( - phabricator_id=phabricator_id + provider=provider, + provider_id=provider_id, ).first(): serializer = RevisionSerializer( instance=revision, context={"request": request} @@ -166,7 +168,7 @@ def get_queryset(self): if query is not None: search_query = ( Q(id__icontains=query) - | Q(revision__phabricator_id__icontains=query) + | Q(revision__provider_id__icontains=query) | Q(revision__bugzilla_id__icontains=query) | Q(revision__title__icontains=query) ) diff --git a/backend/code_review_backend/issues/management/commands/load_issues.py b/backend/code_review_backend/issues/management/commands/load_issues.py index 76e2f7eaa..15ad4c8b2 100644 --- a/backend/code_review_backend/issues/management/commands/load_issues.py +++ b/backend/code_review_backend/issues/management/commands/load_issues.py @@ -213,9 +213,9 @@ def build_revision_and_diff(self, data, task_id): return None, None revision, _ = head_repository.head_revisions.get_or_create( - phabricator_id=data["id"], + provider="phabricator", + provider_id=data["id"], defaults={ - "phabricator_phid": data["phid"], "title": data["title"], "bugzilla_id": int(data["bugzilla_id"]) if data["bugzilla_id"] @@ -227,7 +227,7 @@ def build_revision_and_diff(self, data, task_id): id=data["diff_id"], defaults={ "repository": head_repository, - "phid": data["diff_phid"], + "provider_id": data["diff_phid"], "review_task_id": task_id, "mercurial_hash": data["mercurial_revision"], }, diff --git a/backend/code_review_backend/issues/migrations/0016_alter_revision_options_and_more.py b/backend/code_review_backend/issues/migrations/0016_alter_revision_options_and_more.py new file mode 100644 index 000000000..ba0230a1e --- /dev/null +++ b/backend/code_review_backend/issues/migrations/0016_alter_revision_options_and_more.py @@ -0,0 +1,50 @@ +# Generated by Django 5.1.6 on 2026-01-21 16:01 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("issues", "0015_remove_repository_phid_alter_repository_id"), + ] + + operations = [ + migrations.AlterModelOptions( + name="revision", + options={"ordering": ("provider", "provider_id", "id")}, + ), + migrations.RemoveConstraint( + model_name="revision", + name="revision_unique_phab_id", + ), + migrations.RenameField( + model_name="revision", + old_name="phabricator_id", + new_name="provider_id", + ), + migrations.AddField( + model_name="revision", + name="provider", + field=models.CharField( + choices=[("phabricator", "Phabricator"), ("github", "Github")], + default="phabricator", + max_length=20, + ), + ), + migrations.AddConstraint( + model_name="revision", + constraint=models.UniqueConstraint( + condition=models.Q(("provider_id__isnull", False)), + fields=("provider_id",), + name="revision_unique_phab_id", + ), + ), + migrations.RemoveConstraint( + model_name="revision", + name="revision_unique_phab_phabid", + ), + migrations.RemoveField( + model_name="revision", + name="phabricator_phid", + ), + ] diff --git a/backend/code_review_backend/issues/migrations/0017_rename_phid_diff_provider_id.py b/backend/code_review_backend/issues/migrations/0017_rename_phid_diff_provider_id.py new file mode 100644 index 000000000..7c0bbbce9 --- /dev/null +++ b/backend/code_review_backend/issues/migrations/0017_rename_phid_diff_provider_id.py @@ -0,0 +1,17 @@ +# Generated by Django 5.1.6 on 2026-01-27 16:13 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("issues", "0016_alter_revision_options_and_more"), + ] + + operations = [ + migrations.RenameField( + model_name="diff", + old_name="phid", + new_name="provider_id", + ), + ] diff --git a/backend/code_review_backend/issues/models.py b/backend/code_review_backend/issues/models.py index da457656b..d11ea12da 100644 --- a/backend/code_review_backend/issues/models.py +++ b/backend/code_review_backend/issues/models.py @@ -13,6 +13,13 @@ LEVEL_ERROR = "error" ISSUE_LEVELS = ((LEVEL_WARNING, "Warning"), (LEVEL_ERROR, "Error")) +PROVIDER_PHABRICATOR = "phabricator" +PROVIDER_GITHUB = "github" +PROVIDERS = ( + (PROVIDER_PHABRICATOR, "Phabricator"), + (PROVIDER_GITHUB, "Github"), +) + class Repository(models.Model): id = models.AutoField(primary_key=True) @@ -33,11 +40,12 @@ def __str__(self): class Revision(models.Model): id = models.BigAutoField(primary_key=True) + # Phabricator references will be left empty when ingesting a decision task (e.g. from MC or autoland) - phabricator_id = models.PositiveIntegerField(unique=True, null=True, blank=True) - phabricator_phid = models.CharField( - max_length=40, unique=True, null=True, blank=True + provider = models.CharField( + max_length=20, choices=PROVIDERS, default=PROVIDER_PHABRICATOR ) + provider_id = models.PositiveIntegerField(unique=True, null=True, blank=True) created = models.DateTimeField(auto_now_add=True) updated = models.DateTimeField(auto_now=True) @@ -72,43 +80,44 @@ class Revision(models.Model): bugzilla_id = models.PositiveIntegerField(null=True) class Meta: - ordering = ("phabricator_id", "id") + ordering = ("provider", "provider_id", "id") indexes = (models.Index(fields=["head_repository", "head_changeset"]),) constraints = [ models.UniqueConstraint( - fields=["phabricator_id"], + fields=["provider_id"], name="revision_unique_phab_id", - condition=Q(phabricator_id__isnull=False), - ), - models.UniqueConstraint( - fields=["phabricator_phid"], - name="revision_unique_phab_phabid", - condition=Q(phabricator_phid__isnull=False), + condition=Q(provider_id__isnull=False), ), ] def __str__(self): - if self.phabricator_id is not None: - return f"D{self.phabricator_id} - {self.title}" + if self.provider == PROVIDER_PHABRICATOR and self.provider_id is not None: + return f"Phabricator D{self.provider_id} - {self.title}" return f"#{self.id} - {self.title}" @property - def phabricator_url(self): - if self.phabricator_id is None: + def url(self): + if self.provider_id is None: return - parser = urllib.parse.urlparse(settings.PHABRICATOR_HOST) - return f"{parser.scheme}://{parser.netloc}/D{self.phabricator_id}" + + if self.provider == PROVIDER_PHABRICATOR: + parser = urllib.parse.urlparse(settings.PHABRICATOR_HOST) + return f"{parser.scheme}://{parser.netloc}/D{self.provider_id}" + elif self.provider == PROVIDER_GITHUB: + return f"{self.base_repository.url}/issues/{self.provider_id}" + else: + raise NotImplementedError class Diff(models.Model): - """Reference of a specific code patch (diff) in Phabricator. + """Reference of a specific code patch (diff) in Phabricator or Github. A revision can be linked to multiple successive diffs, or none in case of a repository push. """ # Phabricator's attributes id = models.PositiveIntegerField(primary_key=True) - phid = models.CharField(max_length=40, unique=True) + provider_id = models.CharField(max_length=40, unique=True) created = models.DateTimeField(auto_now_add=True) updated = models.DateTimeField(auto_now=True) diff --git a/backend/code_review_backend/issues/serializers.py b/backend/code_review_backend/issues/serializers.py index 888fd52ff..601214de8 100644 --- a/backend/code_review_backend/issues/serializers.py +++ b/backend/code_review_backend/issues/serializers.py @@ -74,8 +74,8 @@ class RevisionSerializer(serializers.ModelSerializer): issues_bulk_url = serializers.HyperlinkedIdentityField( view_name="revision-issues-bulk", lookup_url_kwarg="revision_id" ) - phabricator_url = serializers.URLField(read_only=True) - phabricator_id = serializers.IntegerField( + url = serializers.URLField(read_only=True) + provider_id = serializers.IntegerField( required=False, allow_null=True, min_value=1, @@ -90,13 +90,13 @@ class Meta: "head_repository", "base_changeset", "head_changeset", - "phabricator_id", - "phabricator_phid", + "provider", + "provider_id", "title", "bugzilla_id", "diffs_url", "issues_bulk_url", - "phabricator_url", + "url", ) @@ -107,21 +107,21 @@ class RevisionLightSerializer(serializers.ModelSerializer): base_repository = RepositoryGetOrCreateField() head_repository = RepositoryGetOrCreateField() - phabricator_url = serializers.URLField(read_only=True) + url = serializers.URLField(read_only=True) class Meta: model = Revision fields = ( "id", - "phabricator_id", + "provider", + "provider_id", "base_repository", "head_repository", "base_changeset", "head_changeset", - "phabricator_id", "title", "bugzilla_id", - "phabricator_url", + "url", ) @@ -142,7 +142,7 @@ class Meta: model = Diff fields = ( "id", - "phid", + "provider_id", "review_task_id", "repository", "mercurial_hash", @@ -187,7 +187,7 @@ class Meta: fields = ( "id", "revision", - "phid", + "provider_id", "review_task_id", "repository", "mercurial_hash", diff --git a/backend/code_review_backend/issues/tests/commands/test_cleanup_issues.py b/backend/code_review_backend/issues/tests/commands/test_cleanup_issues.py index b6ba5fe40..135d92362 100644 --- a/backend/code_review_backend/issues/tests/commands/test_cleanup_issues.py +++ b/backend/code_review_backend/issues/tests/commands/test_cleanup_issues.py @@ -56,8 +56,8 @@ def setUp(self): [ Revision( id=i, - phabricator_id=i, - phabricator_phid=i, + provider="phabricator", + provider_id=i, title=f"Revision {i}", base_repository=repo, head_repository=repo, diff --git a/backend/code_review_backend/issues/tests/test_api.py b/backend/code_review_backend/issues/tests/test_api.py index 09d3e8ff4..b73ea6538 100644 --- a/backend/code_review_backend/issues/tests/test_api.py +++ b/backend/code_review_backend/issues/tests/test_api.py @@ -31,15 +31,15 @@ def setUp(self): ) # Create revision and diff self.revision = self.repo_try.head_revisions.create( - phabricator_id=456, - phabricator_phid="PHID-REV-XXX", + provider="phabricator", + provider_id=456, title="Bug XXX - Yet Another bug", bugzilla_id=78901, base_repository=self.repo, ) self.diff = self.revision.diffs.create( id=1234, - phid="PHID-DIFF-xxx", + provider_id="PHID-DIFF-xxx", review_task_id="deadbeef123", mercurial_hash="coffee12345", repository=self.repo_try, @@ -51,8 +51,8 @@ def test_create_revision(self): """ self.revision.delete() data = { - "phabricator_id": 123, - "phabricator_phid": "PHID-REV-xxx", + "provider": "phabricator", + "provider_id": 123, "title": "Bug XXX - Some bug", "bugzilla_id": 123456, "base_repository": "http://repo.test/myrepo", @@ -78,9 +78,9 @@ def test_create_revision(self): "bugzilla_id": 123456, "diffs_url": f"http://testserver/v1/revision/{revision_id}/diffs/", "issues_bulk_url": f"http://testserver/v1/revision/{revision_id}/issues/", - "phabricator_url": "https://phabricator.services.mozilla.com/D123", - "phabricator_id": 123, - "phabricator_phid": "PHID-REV-xxx", + "url": "https://phabricator.services.mozilla.com/D123", + "provider": "phabricator", + "provider_id": 123, "base_repository": "http://repo.test/myrepo", "head_repository": "http://repo.test/myrepo", "base_changeset": "123456789ABCDEF", @@ -88,7 +88,7 @@ def test_create_revision(self): "title": "Bug XXX - Some bug", } self.assertEqual(Revision.objects.count(), 1) - revision = Revision.objects.get(phabricator_id=123) + revision = Revision.objects.get(provider_id=123) self.assertEqual(revision.title, "Bug XXX - Some bug") self.assertEqual(revision.bugzilla_id, 123456) self.assertDictEqual(response.json(), expected_response) @@ -103,8 +103,8 @@ def test_create_revision(self): def test_create_revision_wrong_new_repo(self): self.revision.delete() data = { - "phabricator_id": 123, - "phabricator_phid": "PHID-REV-xxx", + "provider": "phabricator", + "provider_id": 123, "title": "Bug XXX - Some bug", "bugzilla_id": 123456, "base_repository": "http://notamozrepo.test/myrepo", @@ -128,8 +128,8 @@ def test_create_revision_wrong_new_repo(self): def test_create_revision_creates_new_repo(self): self.revision.delete() data = { - "phabricator_id": 123, - "phabricator_phid": "PHID-REV-xxx", + "provider": "phabricator", + "provider_id": 123, "title": "Bug XXX - Some bug", "bugzilla_id": 123456, "base_repository": "http://repo.test/myrepo", @@ -153,7 +153,7 @@ def test_create_diff(self): self.diff.delete() data = { "id": 1234, - "phid": "PHID-DIFF-xxx", + "provider_id": "PHID-DIFF-xxx", "review_task_id": "deadbeef123", "mercurial_hash": "coffee12345", "repository": "http://repo.test/try", @@ -354,9 +354,9 @@ def test_create_issue_bulk_with_diff(self): ) self.assertEqual(response.status_code, status.HTTP_201_CREATED) - self.assertEqual(Issue.objects.filter(revisions__phabricator_id=456).count(), 1) + self.assertEqual(Issue.objects.filter(revisions__provider_id=456).count(), 1) - issue = Issue.objects.filter(revisions__phabricator_id=456).get() + issue = Issue.objects.filter(revisions__provider_id=456).get() self.assertDictEqual( response.json(), { @@ -665,7 +665,7 @@ def test_create_issue_already_referenced(self): self.client.force_authenticate(user=self.user) another_diff = self.revision.diffs.create( id=56748, - phid="PHID-DIFF-yyy", + provider_id="PHID-DIFF-yyy", review_task_id="deadbeef456", mercurial_hash="coffee67890", repository=self.repo_try, diff --git a/backend/code_review_backend/issues/tests/test_compare.py b/backend/code_review_backend/issues/tests/test_compare.py index 78c05ef0b..6c163633c 100644 --- a/backend/code_review_backend/issues/tests/test_compare.py +++ b/backend/code_review_backend/issues/tests/test_compare.py @@ -27,8 +27,8 @@ def setUp(self): # Create a simple stack with 2 diffs self.revision = self.repo_try.head_revisions.create( - phabricator_id=1, - phabricator_phid="PHID-DREV-1", + provider="phabricator", + provider_id=1, title="Revision XYZ", bugzilla_id=1234567, base_repository=self.repo, @@ -36,7 +36,7 @@ def setUp(self): for i in range(2): self.revision.diffs.create( id=i + 1, - phid=f"PHID-DIFF-{i+1}", + provider_id=f"PHID-DIFF-{i+1}", review_task_id=f"task-{i}", mercurial_hash=hashlib.sha1(f"hg {i}".encode()).hexdigest(), repository=self.repo_try, diff --git a/backend/code_review_backend/issues/tests/test_diff.py b/backend/code_review_backend/issues/tests/test_diff.py index 6359ec73b..b43b8a4e4 100644 --- a/backend/code_review_backend/issues/tests/test_diff.py +++ b/backend/code_review_backend/issues/tests/test_diff.py @@ -30,8 +30,8 @@ def setUpTestData(cls): for i in range(2): cls.repo_try.head_revisions.create( id=i + 1, - phabricator_id=i + 1, - phabricator_phid=f"PHID-DREV-{i+1}", + provider="phabricator", + provider_id=i + 1, title=f"Revision {i+1}", bugzilla_id=10000 + i, base_repository=cls.repo, @@ -39,7 +39,7 @@ def setUpTestData(cls): for i in range(3): Diff.objects.create( id=i + 1, - phid=f"PHID-DIFF-{i+1}", + provider_id=f"PHID-DIFF-{i+1}", revision_id=(i % 2) + 1, review_task_id=f"task-{i}", mercurial_hash=hashlib.sha1(f"hg {i}".encode()).hexdigest(), @@ -58,6 +58,7 @@ def test_list_diffs(self): """ response = self.client.get("/v1/diff/") self.assertEqual(response.status_code, status.HTTP_200_OK) + self.maxDiff = None self.assertDictEqual( response.json(), { @@ -73,20 +74,20 @@ def test_list_diffs(self): "head_repository": "http://repo.test/try", "base_changeset": None, "head_changeset": None, - "phabricator_id": 1, - "phabricator_phid": "PHID-DREV-1", + "provider": "phabricator", + "provider_id": 1, "title": "Revision 1", "bugzilla_id": 10000, "diffs_url": "http://testserver/v1/revision/1/diffs/", "issues_bulk_url": "http://testserver/v1/revision/1/issues/", - "phabricator_url": "https://phabricator.services.mozilla.com/D1", + "url": "https://phabricator.services.mozilla.com/D1", }, "repository": { "id": 2, "slug": "myrepo-try", "url": "http://repo.test/try", }, - "phid": "PHID-DIFF-3", + "provider_id": "PHID-DIFF-3", "review_task_id": "task-2", "mercurial_hash": "30b501affc4d3b9c670fc297ab903b406afd5f04", "issues_url": "http://testserver/v1/diff/3/issues/", @@ -104,20 +105,20 @@ def test_list_diffs(self): "head_repository": "http://repo.test/try", "base_changeset": None, "head_changeset": None, - "phabricator_id": 2, - "phabricator_phid": "PHID-DREV-2", + "provider": "phabricator", + "provider_id": 2, "title": "Revision 2", "bugzilla_id": 10001, "diffs_url": "http://testserver/v1/revision/2/diffs/", "issues_bulk_url": "http://testserver/v1/revision/2/issues/", - "phabricator_url": "https://phabricator.services.mozilla.com/D2", + "url": "https://phabricator.services.mozilla.com/D2", }, "repository": { "id": 2, "slug": "myrepo-try", "url": "http://repo.test/try", }, - "phid": "PHID-DIFF-2", + "provider_id": "PHID-DIFF-2", "review_task_id": "task-1", "mercurial_hash": "32d2a594cfef74fcb524028d1521d0d4bd98bd35", "issues_url": "http://testserver/v1/diff/2/issues/", @@ -135,20 +136,20 @@ def test_list_diffs(self): "head_repository": "http://repo.test/try", "base_changeset": None, "head_changeset": None, - "phabricator_id": 1, - "phabricator_phid": "PHID-DREV-1", + "provider": "phabricator", + "provider_id": 1, "title": "Revision 1", "bugzilla_id": 10000, "diffs_url": "http://testserver/v1/revision/1/diffs/", "issues_bulk_url": "http://testserver/v1/revision/1/issues/", - "phabricator_url": "https://phabricator.services.mozilla.com/D1", + "url": "https://phabricator.services.mozilla.com/D1", }, "repository": { "id": 2, "slug": "myrepo-try", "url": "http://repo.test/try", }, - "phid": "PHID-DIFF-1", + "provider_id": "PHID-DIFF-1", "review_task_id": "task-0", "mercurial_hash": "a2ac78b7d12d6e55b9b15c1c2048a16c58c6c803", "issues_url": "http://testserver/v1/diff/1/issues/", diff --git a/backend/code_review_backend/issues/tests/test_issue.py b/backend/code_review_backend/issues/tests/test_issue.py index 070ca4679..a4c5c2311 100644 --- a/backend/code_review_backend/issues/tests/test_issue.py +++ b/backend/code_review_backend/issues/tests/test_issue.py @@ -24,16 +24,16 @@ def setUp(self): mock_now.return_value = datetime.fromisoformat("2010-01-01:10") self.revision = self.repo.base_revisions.create( id=4000, - phabricator_id=1111, - phabricator_phid="PH-1111", + provider="phabricator", + provider_id=1111, head_changeset="1" * 40, head_repository=self.repo, ) mock_now.return_value = datetime.fromisoformat("2000-01-01:10") self.old_revision = self.repo.base_revisions.create( id=4001, - phabricator_id=2222, - phabricator_phid="PH-2222", + provider="phabricator", + provider_id=2222, head_changeset="2" * 40, head_repository=self.repo, ) diff --git a/backend/code_review_backend/issues/tests/test_revision.py b/backend/code_review_backend/issues/tests/test_revision.py index fb1f93668..1fb6f4265 100644 --- a/backend/code_review_backend/issues/tests/test_revision.py +++ b/backend/code_review_backend/issues/tests/test_revision.py @@ -18,21 +18,19 @@ def setUp(self): def test_phabricator_url(self): rev = Revision.objects.create( - phabricator_id=12, - phabricator_phid="PHID-REV-12345", + provider="phabricator", + provider_id=12, base_repository=self.repo, head_repository=self.repo, ) # Default settings - self.assertEqual( - rev.phabricator_url, "https://phabricator.services.mozilla.com/D12" - ) + self.assertEqual(rev.url, "https://phabricator.services.mozilla.com/D12") # Override host with /api settings.PHABRICATOR_HOST = "http://phab.test/api" - self.assertEqual(rev.phabricator_url, "http://phab.test/D12") + self.assertEqual(rev.url, "http://phab.test/D12") # Override host with complex url settings.PHABRICATOR_HOST = "http://anotherphab.test/api123/?custom" - self.assertEqual(rev.phabricator_url, "http://anotherphab.test/D12") + self.assertEqual(rev.url, "http://anotherphab.test/D12") diff --git a/backend/code_review_backend/issues/tests/test_stats.py b/backend/code_review_backend/issues/tests/test_stats.py index 1746b64ca..9d7321574 100644 --- a/backend/code_review_backend/issues/tests/test_stats.py +++ b/backend/code_review_backend/issues/tests/test_stats.py @@ -29,8 +29,8 @@ def setUp(self): # Create a revision revision = self.repo_try.head_revisions.create( - phabricator_id=10, - phabricator_phid="PHID-DREV-arev", + provider="phabricator", + provider_id=10, title="Revision A", bugzilla_id=None, base_repository=self.repo, @@ -40,7 +40,7 @@ def setUp(self): for i in range(10): revision.diffs.create( id=i + 1, - phid=f"PHID-DIFF-{i+1}", + provider_id=f"PHID-DIFF-{i+1}", review_task_id=f"task-{i}", mercurial_hash=hashlib.sha1(f"hg {i}".encode()).hexdigest(), repository=self.repo_try, @@ -251,12 +251,13 @@ def check_issue(issue): # Revision rev = diff["revision"] self.assertTrue(rev["id"] > 0) - self.assertTrue(rev["phabricator_id"] > 0) + self.assertEqual(rev["provider"], "phabricator") + self.assertTrue(rev["provider_id"] > 0) self.assertEqual(rev["base_repository"], "http://repo.test/myrepo") self.assertEqual(rev["head_repository"], "http://repo.test/myrepo-try") self.assertEqual( - rev["phabricator_url"], - f"http://anotherphab.test/D{rev['phabricator_id']}", + rev["url"], + f"http://anotherphab.test/D{rev['provider_id']}", ) return True diff --git a/bot/code_review_bot/backend.py b/bot/code_review_bot/backend.py index 17019b5b3..5d4af5fab 100644 --- a/bot/code_review_bot/backend.py +++ b/bot/code_review_bot/backend.py @@ -9,6 +9,7 @@ from code_review_bot import taskcluster from code_review_bot.config import GetAppUserAgent, settings +from code_review_bot.revisions.github import GithubRevision from code_review_bot.tasks.lint import MozLintIssue logger = structlog.get_logger(__name__) @@ -46,6 +47,10 @@ def publish_revision(self, revision): logger.warn("Skipping revision publication on backend") return + if isinstance(revision, GithubRevision): + logger.warn("Skipping revision publication for github") + return {} + # Check the repositories are urls for url in (revision.base_repository, revision.head_repository): assert isinstance(url, str), "Repository must be a string" @@ -61,8 +66,7 @@ def publish_revision(self, revision): # Create revision on backend if it does not exists data = { - "phabricator_id": revision.phabricator_id, - "phabricator_phid": revision.phabricator_phid, + "provider_id": revision.phabricator_id, "title": revision.title, "bugzilla_id": revision.bugzilla_id, "base_repository": revision.base_repository, @@ -94,7 +98,7 @@ def publish_revision(self, revision): # Create diff attached to revision on backend data = { "id": revision.diff_id, - "phid": revision.diff_phid, + "provider_id": revision.diff_phid, "review_task_id": settings.taskcluster.task_id, "mercurial_hash": revision.head_changeset, "repository": revision.head_repository, @@ -115,6 +119,10 @@ def publish_issues(self, issues, revision): logger.warn("Skipping issues publication on backend") return + if isinstance(revision, GithubRevision): + logger.warn("Skipping issues publication for github") + return {} + published = 0 assert ( revision.issues_url is not None diff --git a/bot/code_review_bot/report/lando.py b/bot/code_review_bot/report/lando.py index 21ed73771..a6758bec6 100644 --- a/bot/code_review_bot/report/lando.py +++ b/bot/code_review_bot/report/lando.py @@ -6,7 +6,7 @@ from code_review_bot import Level from code_review_bot.report.base import Reporter -from code_review_bot.revisions import PhabricatorRevision +from code_review_bot.revisions import GithubRevision logger = structlog.get_logger(__name__) @@ -30,10 +30,9 @@ def publish(self, issues, revision, task_failures, links, reviewers): """ Send an email to administrators """ - if not isinstance(revision, PhabricatorRevision): - raise NotImplementedError( - "Only Phabricator revisions are supported for now" - ) + if isinstance(revision, GithubRevision): + logger.warning("No Lando publication for Github yet") + return assert ( revision.phabricator_id and revision.phabricator_phid and revision.diff diff --git a/bot/code_review_bot/revisions/__init__.py b/bot/code_review_bot/revisions/__init__.py index 10d4224fa..f98bd00a7 100644 --- a/bot/code_review_bot/revisions/__init__.py +++ b/bot/code_review_bot/revisions/__init__.py @@ -3,6 +3,7 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. from code_review_bot.revisions.base import ImprovementPatch, Revision +from code_review_bot.revisions.github import GithubRevision from code_review_bot.revisions.phabricator import PhabricatorRevision -__all__ = [ImprovementPatch, Revision, PhabricatorRevision] +__all__ = [ImprovementPatch, Revision, PhabricatorRevision, GithubRevision] diff --git a/bot/code_review_bot/revisions/base.py b/bot/code_review_bot/revisions/base.py index 1cd319401..962a50db6 100644 --- a/bot/code_review_bot/revisions/base.py +++ b/bot/code_review_bot/revisions/base.py @@ -240,14 +240,17 @@ def as_dict(self): @staticmethod def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorAPI): """ - Load identifiers from Phabricator, using the remote task description + Load identifiers from Phabricator or Github, using the remote task description """ + from code_review_bot.revisions.github import GithubRevision from code_review_bot.revisions.phabricator import PhabricatorRevision # Load build target phid from the task env code_review = try_task["extra"]["code-review"] - # TODO: support github revision here too - return PhabricatorRevision.from_try_task( - code_review, decision_task, phabricator - ) + if "github" in code_review: + return GithubRevision(**code_review["github"]) + else: + return PhabricatorRevision.from_try_task( + code_review, decision_task, phabricator + ) diff --git a/bot/code_review_bot/revisions/github.py b/bot/code_review_bot/revisions/github.py new file mode 100644 index 000000000..54123a482 --- /dev/null +++ b/bot/code_review_bot/revisions/github.py @@ -0,0 +1,52 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import requests +import structlog + +from code_review_bot.revisions import Revision + +logger = structlog.get_logger(__name__) + + +class GithubRevision(Revision): + """ + A revision from a github pull-request + """ + + def __init__(self, repo_url, branch, pull_number, pull_head_sha): + super().__init__() + + self.repo_url = repo_url + self.branch = branch + self.pull_number = pull_number + self.pull_head_sha = pull_head_sha + + # Load the patch from Github + self.patch = self.load_patch() + + def __str__(self): + return f"Github pull request {self.repo_url} #{self.pull_number} ({self.pull_head_sha[:8]})" + + def __repr__(self): + return f"GithubRevision repo_url={self.repo_url} branch={self.branch} pull_number={self.pull_number} sha={self.pull_head_sha}" + + def load_patch(self): + """ + Load the patch content for the current pull request HEAD + """ + # TODO: use specific sha + url = f"{self.repo_url}/pull/{self.pull_number}.diff" + logger.info("Loading github patch", url=url) + resp = requests.get(url, allow_redirects=True) + resp.raise_for_status() + return resp.content.decode() + + def as_dict(self): + return { + "repo_url": self.repo_url, + "branch": self.branch, + "pull_number": self.pull_number, + "pull_head_sha": self.pull_head_sha, + } diff --git a/bot/code_review_bot/revisions/phabricator.py b/bot/code_review_bot/revisions/phabricator.py index 8970e19ae..e50632ee8 100644 --- a/bot/code_review_bot/revisions/phabricator.py +++ b/bot/code_review_bot/revisions/phabricator.py @@ -130,12 +130,13 @@ def __str__(self): return f"Phabricator #{self.diff_id} - {self.diff_phid}" @staticmethod - def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorAPI): + def from_try_task( + code_review: dict, decision_task: dict, phabricator: PhabricatorAPI + ): """ Load identifiers from Phabricator, using the remote task description """ # Load build target phid from the task env - code_review = try_task["extra"]["code-review"] build_target_phid = code_review.get("phabricator-diff") or code_review.get( "phabricator-build-target" ) diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 260a638e3..d107c4aed 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -20,7 +20,7 @@ from code_review_bot.config import settings from code_review_bot.mercurial import MercurialWorker, Repository, robust_checkout from code_review_bot.report.debug import DebugReporter -from code_review_bot.revisions import PhabricatorRevision, Revision +from code_review_bot.revisions import GithubRevision, PhabricatorRevision, Revision from code_review_bot.sources.phabricator import ( PhabricatorActions, PhabricatorBuildState, @@ -673,11 +673,14 @@ def update_status(self, revision, state): """ Update build status on HarborMaster """ - if not isinstance(revision, PhabricatorRevision): - raise NotImplementedError( - "Only Phabricator revisions are supported for now" - ) assert isinstance(state, BuildState) + + if isinstance(revision, GithubRevision): + logger.warning( + "No github status update yet", revision=revision, status=state + ) + return + if not revision.build_target_phid: logger.info( "No build target found, skipping HarborMaster update", state=state.value @@ -701,6 +704,7 @@ def publish_link(self, revision: Revision, slug: str, name: str, url: str): raise NotImplementedError( "Only Phabricator revisions are supported for now" ) + if not revision.build_target_phid: logger.info( "No build target found, skipping HarborMaster link creation", diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 9d75614dc..7d49d2a42 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -332,10 +332,10 @@ def mock_revision(mock_phabricator, mock_try_task, mock_decision_task, mock_conf """ Mock a mercurial revision """ - from code_review_bot.revisions import PhabricatorRevision + from code_review_bot.revisions import Revision with mock_phabricator as api: - return PhabricatorRevision.from_try_task(mock_try_task, mock_decision_task, api) + return Revision.from_try_task(mock_try_task, mock_decision_task, api) @pytest.fixture diff --git a/bot/tests/test_backend.py b/bot/tests/test_backend.py index e6cc07bba..368978df0 100644 --- a/bot/tests/test_backend.py +++ b/bot/tests/test_backend.py @@ -37,8 +37,7 @@ def test_publication(mock_clang_tidy_issues, mock_revision, mock_backend, mock_h assert revisions[1] == { "bugzilla_id": 1234567, "id": 1, - "phabricator_id": 51, - "phabricator_phid": "PHID-DREV-zzzzz", + "provider_id": 51, "title": "Static Analysis tests", "diffs_url": "http://code-review-backend.test/v1/revision/1/diffs/", "issues_bulk_url": "http://code-review-backend.test/v1/revision/1/issues/", @@ -55,7 +54,7 @@ def test_publication(mock_clang_tidy_issues, mock_revision, mock_backend, mock_h "id": 42, "issues_url": "http://code-review-backend.test/v1/diff/42/issues/", "mercurial_hash": "deadbeef1234", - "phid": "PHID-DIFF-test", + "provider_id": "PHID-DIFF-test", "review_task_id": "local instance", "repository": "http://hgmo/test-try", } @@ -132,8 +131,7 @@ def test_missing_bugzilla_id(mock_revision, mock_backend, mock_hgmo): assert revisions[1] == { "id": 1, "bugzilla_id": None, - "phabricator_id": 51, - "phabricator_phid": "PHID-DREV-zzzzz", + "provider_id": 51, "title": "Static Analysis tests", "diffs_url": "http://code-review-backend.test/v1/revision/1/diffs/", "issues_bulk_url": "http://code-review-backend.test/v1/revision/1/issues/", diff --git a/bot/tests/test_index.py b/bot/tests/test_index.py index 768bf06d8..fa5e6c032 100644 --- a/bot/tests/test_index.py +++ b/bot/tests/test_index.py @@ -5,7 +5,7 @@ from unittest import mock from code_review_bot.config import TaskCluster -from code_review_bot.revisions import PhabricatorRevision +from code_review_bot.revisions import PhabricatorRevision, Revision class MockPhabricatorRevision(PhabricatorRevision): @@ -199,9 +199,8 @@ def test_index_from_try( """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) mock_workflow.index_service = mock.Mock() mock_config.taskcluster = TaskCluster("/tmp/dummy", "12345deadbeef", 0, False) diff --git a/bot/tests/test_reporter_phabricator.py b/bot/tests/test_reporter_phabricator.py index 9fa67ccea..cc9e88af6 100644 --- a/bot/tests/test_reporter_phabricator.py +++ b/bot/tests/test_reporter_phabricator.py @@ -13,7 +13,7 @@ from code_review_bot import Level from code_review_bot.report.phabricator import PhabricatorReporter -from code_review_bot.revisions import ImprovementPatch, PhabricatorRevision +from code_review_bot.revisions import ImprovementPatch, PhabricatorRevision, Revision from code_review_bot.tasks.clang_format import ClangFormatIssue, ClangFormatTask from code_review_bot.tasks.clang_tidy import ClangTidyIssue, ClangTidyTask from code_review_bot.tasks.clang_tidy_external import ( @@ -270,9 +270,8 @@ def test_phabricator_clang_tidy( """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "another_test.cpp": [41, 42, 43] @@ -308,9 +307,8 @@ def test_phabricator_clang_format( """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "test.cpp": [41, 42, 43], @@ -352,9 +350,8 @@ def test_phabricator_mozlint( """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "python/test.py": [41, 42, 43], @@ -443,9 +440,8 @@ def test_phabricator_coverage( Test Phabricator reporter publication on a mock coverage issue """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -511,9 +507,8 @@ def raise_404(*args, **kwargs): raise HTTPError(response=resp_mock) with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -547,9 +542,8 @@ def test_phabricator_clang_tidy_and_coverage( """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -673,9 +667,8 @@ def test_phabricator_analyzers( api.comment = unittest.mock.Mock(return_value=True) # Always use the same setup, only varies the analyzers - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = {"test.cpp": [0, 41, 42, 43], "dom/test.cpp": [42]} revision.id = 52 reporter = PhabricatorReporter( @@ -759,9 +752,8 @@ def test_phabricator_clang_tidy_build_error( from code_review_bot import Level with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "test.cpp": [41, 42, 43] @@ -819,9 +811,8 @@ def test_full_file( """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "xx.cpp": [123, 124, 125] @@ -883,9 +874,8 @@ def test_task_failures(mock_phabricator, phab, mock_try_task, mock_decision_task """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.id = 52 reporter = PhabricatorReporter({"analyzers": ["clang-tidy"]}, api=api) @@ -910,9 +900,8 @@ def test_extra_errors( """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = {"path/to/file.py": [1, 2, 3]} revision.files = ["path/to/file.py"] revision.id = 52 @@ -1003,9 +992,8 @@ def test_phabricator_notices(mock_phabricator, phab, mock_try_task, mock_decisio """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "test.rst": [41, 42, 43], @@ -1053,9 +1041,8 @@ def test_phabricator_tgdiff(mock_phabricator, phab, mock_try_task, mock_decision """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "test.rst": [41, 42, 43], @@ -1091,9 +1078,8 @@ def test_phabricator_external_tidy( """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "another_test.cpp": [41, 42, 43] @@ -1144,9 +1130,8 @@ def test_phabricator_newer_diff( """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -1224,9 +1209,8 @@ def test_phabricator_former_diff_comparison( """ with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -1374,9 +1358,8 @@ def test_phabricator_before_after_comment( mock_taskcluster_config.secrets = {"BEFORE_AFTER_RATIO": 1} with mock_phabricator as api: - revision = PhabricatorRevision.from_try_task( - mock_try_task, mock_decision_task, api - ) + revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff "test.txt": [0], diff --git a/frontend/src/Diffs.vue b/frontend/src/Diffs.vue index 5aa43c888..1a7fc769e 100644 --- a/frontend/src/Diffs.vue +++ b/frontend/src/Diffs.vue @@ -156,7 +156,7 @@ export default { name: 'revision', params: { revisionId: diff.revision.id }, }" - >D{{ diff.revision.phabricator_id }}Revision {{ diff.revision.provider_id }} @ base: {{ diff.revision.base_repository | short_repo }} - head: {{ diff.revision.head_repository | short_repo }} @@ -217,10 +217,18 @@ export default {
On {{ revision.head_repository }} -
- View on Phabricator
+ View on Github
-
- D{{ group.revision.phabricator_id }}D{{ group.revision.provider_id }}
+ PR n°{{ group.revision.provider_id }}
({{ group.msg
}} +{{ group.remaining }}