CR

switch SET_NULL back to DO_NOTHING

We want to keep the repo_id value as-is, even if the associated repo has
disappeared, so as to keep related object grouped together, and to
prevent uniqueness constraint violations.

src/lando/main/admin.py

@@ -9,12 +9,6 @@
     RevisionLandingJob,
     Worker,
 )
-from lando.pushlog.models import (
-    Commit,
-    File,
-    Push,
-    Tag,
-)
 
 admin.site.site_title = gettext_lazy("Lando Admin")
 admin.site.site_header = gettext_lazy("Lando Administration")
@@ -49,61 +43,3 @@ class LandingJobAdmin(admin.ModelAdmin):
 admin.site.register(Repo, admin.ModelAdmin)
 admin.site.register(Worker, admin.ModelAdmin)
 admin.site.register(ConfigurationVariable, admin.ModelAdmin)
-
-
-class PushLogAdmin(admin.ModelAdmin):
-    """A base ModelAdmin class for PushLog-related admin parameters."""
-
-    def has_add_permission(self, request, obj=None):
-        """Fordib addition of any pushlog object from the admin interface."""
-        return False
-
-    def has_delete_permission(self, request, obj=None):
-        """Fordib deletion of any pushlog object from the admin interface."""
-        return False
-
-
-class PushAdmin(PushLogAdmin):
-    readonly_fields = (
-        "push_id",
-        "repo",
-        "repo_url",
-        "branch",
-        "datetime",
-        "user",
-        "commits",
-    )
-
-
-class CommitAdmin(PushLogAdmin):
-    readonly_fields = (
-        "repo",
-        "hash",
-        "parents",
-        "author",
-        "datetime",
-        "desc",
-        "_files",
-        "_parents",
-    )
-
-
-class FileAdmin(PushLogAdmin):
-    readonly_fields = (
-        "repo",
-        "name",
-    )
-
-
-class TagAdmin(PushLogAdmin):
-    readonly_fields = (
-        "repo",
-        "name",
-        "commit",
-    )
-
-
-admin.site.register(Push, PushAdmin)
-admin.site.register(Commit, CommitAdmin)
-admin.site.register(File, FileAdmin)
-admin.site.register(Tag, TagAdmin)

src/lando/main/scm/abstract_scm.py

@@ -4,7 +4,7 @@
 from pathlib import Path
 from typing import Any, ContextManager, Optional
 
-from .commit import Commit
+from lando.main.scm.commit import Commit
 
 logger = logging.getLogger(__name__)
 

src/lando/main/scm/hg.py

@@ -37,6 +37,8 @@
 
 REQUEST_USER_ENV_VAR = "AUTOLAND_REQUEST_USER"
 
+NULL_PARENT_HASH = 40 * "0"
+
 
 class HgException(SCMException):
     """
@@ -345,7 +347,7 @@ def describe_commit(self, revision_id: str = ".") -> Commit:
         metadata["parents"] = metadata["parents"].split()
         # {parents} in Mercurial is empty if the commit has a single parent.
         # We re-add it manually, but only if it is a non-null parent.
-        if not metadata["parents"] and not metadata["parent"] == 40 * "0":
+        if not metadata["parents"] and not metadata["parent"] == NULL_PARENT_HASH:
             metadata["parents"] = metadata["parent"]
         del metadata["parent"]
 

src/lando/pushlog/admin.py

@@ -0,0 +1,66 @@
+from django.contrib import admin
+
+from lando.pushlog.models import (
+    Commit,
+    File,
+    Push,
+    Tag,
+)
+
+
+class PushLogAdmin(admin.ModelAdmin):
+    """A base ModelAdmin class for PushLog-related admin parameters."""
+
+    def has_add_permission(self, request, obj=None):
+        """Fordib addition of any pushlog object from the admin interface."""
+        return False
+
+    def has_delete_permission(self, request, obj=None):
+        """Fordib deletion of any pushlog object from the admin interface."""
+        return False
+
+
+class PushAdmin(PushLogAdmin):
+    readonly_fields = (
+        "push_id",
+        "repo",
+        "repo_url",
+        "branch",
+        "datetime",
+        "user",
+        "commits",
+    )
+
+
+class CommitAdmin(PushLogAdmin):
+    readonly_fields = (
+        "repo",
+        "hash",
+        "parents",
+        "author",
+        "datetime",
+        "desc",
+        "_files",
+        "_parents",
+    )
+
+
+class FileAdmin(PushLogAdmin):
+    readonly_fields = (
+        "repo",
+        "name",
+    )
+
+
+class TagAdmin(PushLogAdmin):
+    readonly_fields = (
+        "repo",
+        "name",
+        "commit",
+    )
+
+
+admin.site.register(Push, PushAdmin)
+admin.site.register(Commit, CommitAdmin)
+admin.site.register(File, FileAdmin)
+admin.site.register(Tag, TagAdmin)

src/lando/pushlog/management/commands/view_pushlog.py

@@ -31,7 +31,7 @@ def handle(self, *args, **options):
             pushes = pushes[:limit]
 
         for push in pushes:
-            print(push)
+            self.stdout.write(push)
             if with_commits:
                 for commit in push.commits.order_by("-datetime"):
-                    print(f"  {commit}")
+                    self.stdout.write(f"  {commit}")

src/lando/pushlog/models/commit.py

@@ -18,12 +18,15 @@
 
 
 class File(models.Model):
+    """A file in a repository.
+
+    Files get associated to the Commits that modify them.
+    """
+
     name = models.CharField(max_length=MAX_PATH_LENGTH)
 
     repo = models.ForeignKey(
         Repo,
-        # We don't want to delete the PushLog, even if we were to delete the repo
-        # object.
         on_delete=models.DO_NOTHING,
     )
 
@@ -38,6 +41,12 @@ def __str__(self):
 
 
 class Commit(models.Model):
+    """An SCM commit.
+
+    They hold all the commit metadata, as well as DAG relationship to other commits, and
+    lists of modified files. They get aggregated in Pushes.
+    """
+
     hash = models.CharField(
         max_length=COMMIT_ID_HEX_LENGTH,
         db_index=True,
@@ -51,8 +60,6 @@ class Commit(models.Model):
 
     repo = models.ForeignKey(
         Repo,
-        # We don't want to delete the PushLog, even if we were to delete the repo
-        # object.
         on_delete=models.DO_NOTHING,
     )
 
@@ -216,14 +223,14 @@ def add_files(self, files: list[str]):
 
 
 class Tag(models.Model):
+    """A human-readable tag pointing to a Commit."""
+
     # Tag names are limited by how long a filename the filesystem support.
     name = models.CharField(max_length=MAX_FILENAME_LENGTH)
     commit = models.ForeignKey(Commit, on_delete=models.CASCADE)
 
     repo = models.ForeignKey(
         Repo,
-        # We don't want to delete the PushLog, even if we were to delete the repo
-        # object.
         on_delete=models.DO_NOTHING,
     )
 

src/lando/pushlog/models/push.py

@@ -10,12 +10,12 @@
 
 
 class Push(models.Model):
+    """A Push object records the list of Commits pushed at once."""
+
     push_id = models.PositiveIntegerField()
 
     repo = models.ForeignKey(
         Repo,
-        # We don't want to delete the PushLog, even if we were to delete the repo
-        # object.
         on_delete=models.DO_NOTHING,
     )
 

src/lando/pushlog/pushlog.py

@@ -46,21 +46,21 @@ class PushLog:
     push: Push
     user: str
 
-    confirmed: bool = False
+    is_confirmed: bool = False
 
     commits: list
 
     def __init__(
         self,
         repo: Repo,
         user: str,
-        commits: list = [],
+        commits: list = None,
     ):
         self.repo = repo
         self.user = user
 
         if not commits:
-            # We cannot us the default value of the argument as a mutable type, and will
+            # We cannot use the default value of the argument as a mutable type, and will
             # get reused on every initialisation.
             commits = []
         self.commits = commits
@@ -86,7 +86,14 @@ def remove_tip_commit(self) -> Commit:
         return tip_commit
 
     def confirm(self, value: bool = True):
-        self.confirmed = value
+        """Mark the push as confirmed and ready to record.
+
+        While the PushLogForRepo ContextManager is used to capture unhandled exceptions
+        and make sure the Push is otherwise recorded, we also need a way for the code to
+        signal that a Push is read, rather than other (handled) failure
+        cases that should not lead to the record being written.
+        """
+        self.is_confirmed = value
 
     @transaction.atomic
     def record_push(self) -> Optional[Push]:
@@ -96,7 +103,7 @@ def record_push(self) -> Optional[Push]:
 
         Don't use directly if using a PushLogForRepo ContextManager.
         """
-        if not self.confirmed:
+        if not self.is_confirmed:
             logger.warning(
                 f"Push for {self.repo.url} wasn't confirmed; aborting ...\n{self}",
                 extra={"pushlog": self},

src/lando/pushlog/tests/conftest.py

@@ -10,7 +10,7 @@
 @pytest.fixture
 def make_repo():
     def repo_factory(seqno: int) -> Repo:
-        "Create a non-descript repository with a sequence number in the test DB."
+        """Create a non-descript repository with a sequence number in the test DB."""
         # Model.objects.create() creates _and saves_ the object
         return Repo.objects.create(
             name=f"repo-{seqno}",
@@ -24,10 +24,8 @@ def repo_factory(seqno: int) -> Repo:
 
 @pytest.fixture
 def make_hash():
-    """Create a 40-character string, of which the first 8 bytes represent the seqno
-    in decimal representation."""
-
     def hash_factory(seqno: int):
+        """Create a hash-like hex string, including the seqno in decimal representation."""
         return str(seqno).zfill(8) + "f" + 31 * "0"
 
     return hash_factory
@@ -36,7 +34,7 @@ def hash_factory(seqno: int):
 @pytest.fixture
 def make_commit(make_hash):
     def commit_factory(repo: Repo, seqno: int, message=None) -> Commit:
-        "Create a non-descript commit with a sequence number in the test DB."
+        """Create a non-descript commit with a sequence number in the test DB."""
         if not message:
             message = f"Commit {seqno}"
 
@@ -54,7 +52,7 @@ def commit_factory(repo: Repo, seqno: int, message=None) -> Commit:
 @pytest.fixture
 def make_file():
     def file_factory(repo: Repo, seqno: int) -> File:
-        "Create a non-descript file with a sequence number in the test DB."
+        """Create a non-descript file with a sequence number in the test DB."""
         return File.objects.create(
             repo=repo,
             name=f"file-{seqno}",
@@ -66,7 +64,7 @@ def file_factory(repo: Repo, seqno: int) -> File:
 @pytest.fixture
 def make_tag():
     def tag_factory(repo: Repo, seqno: int, commit: Commit) -> Tag:
-        "Create a non-descript tag with a sequence number in the test DB."
+        """Create a non-descript tag with a sequence number in the test DB."""
         return Tag.objects.create(
             repo=repo,
             name=f"tag-{seqno}",
@@ -79,7 +77,7 @@ def tag_factory(repo: Repo, seqno: int, commit: Commit) -> Tag:
 @pytest.fixture
 def make_push():
     def push_factory(repo: Repo, commits: list[Commit]):
-        "Create a non-descript push containing the associated commits in the test DB."
+        """Create a non-descript push containing the associated commits in the test DB."""
         push = Push.objects.create(repo=repo, user="Push-User")
         for c in commits:
             push.commits.add(c)