Build: show the command that's currently being executed (#12243)

- Allow updating build commands via the API (only creation was allowed)
- Start time, end time, and exit code can now be null, as the commands
are now created before they are finished.

Closes #4288

Author: stsewd

readthedocs/api/v2/serializers.py

@@ -197,6 +197,12 @@ class Meta:
         model = BuildCommandResult
         exclude = []
 
+    def update(self, instance, validated_data):
+        # Build isn't allowed to be updated after creation
+        # (e.g. to avoid moving commands to another build).
+        validated_data.pop("build", None)
+        return super().update(instance, validated_data)
+
 
 class BuildCommandReadOnlySerializer(BuildCommandSerializer):
     """

readthedocs/api/v2/views/model_views.py

@@ -388,7 +388,9 @@ def credentials_for_storage(self, request, **kwargs):
         return Response({"s3": asdict(credentials)})
 
 
-class BuildCommandViewSet(DisableListEndpoint, CreateModelMixin, UserSelectViewSet):
+class BuildCommandViewSet(
+    DisableListEndpoint, CreateModelMixin, UpdateModelMixin, UserSelectViewSet
+):
     parser_classes = [JSONParser, MultiPartParser]
     permission_classes = [HasBuildAPIKey | ReadOnlyPermission]
     renderer_classes = (JSONRenderer,)

readthedocs/builds/migrations/0063_alter_buildcommandresult.py

@@ -0,0 +1,31 @@
+# Generated by Django 5.2.1 on 2025-06-09 22:47
+
+from django.db import migrations
+from django.db import models
+from django_safemigrate import Safe
+
+
+class Migration(migrations.Migration):
+    safe = Safe.before_deploy()
+
+    dependencies = [
+        ("builds", "0062_rename_indexes"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="buildcommandresult",
+            name="start_time",
+            field=models.DateTimeField(blank=True, null=True, verbose_name="Start time"),
+        ),
+        migrations.AlterField(
+            model_name="buildcommandresult",
+            name="end_time",
+            field=models.DateTimeField(blank=True, null=True, verbose_name="End time"),
+        ),
+        migrations.AlterField(
+            model_name="buildcommandresult",
+            name="exit_code",
+            field=models.IntegerField(blank=True, null=True, verbose_name="Command exit code"),
+        ),
+    ]

readthedocs/builds/models.py

@@ -997,6 +997,15 @@ def failed(self):
         """
         return not self.successful
 
+    @property
+    def finished(self):
+        """
+        Check if the command has finished running.
+
+        This is determined by checking if the `end_time` is not None.
+        """
+        return self.end_time is not None
+
 
 class BuildCommandResult(BuildCommandResultMixin, models.Model):
     """Build command for a ``Build``."""
@@ -1011,10 +1020,10 @@ class BuildCommandResult(BuildCommandResultMixin, models.Model):
     command = models.TextField(_("Command"))
     description = models.TextField(_("Description"), blank=True)
     output = models.TextField(_("Command output"), blank=True)
-    exit_code = models.IntegerField(_("Command exit code"))
+    exit_code = models.IntegerField(_("Command exit code"), null=True, blank=True)
 
-    start_time = models.DateTimeField(_("Start time"))
-    end_time = models.DateTimeField(_("End time"))
+    start_time = models.DateTimeField(_("Start time"), null=True, blank=True)
+    end_time = models.DateTimeField(_("End time"), null=True, blank=True)
 
     class Meta:
         ordering = ["start_time"]

readthedocs/doc_builder/environments.py

@@ -75,6 +75,7 @@ def __init__(
         demux=False,
         **kwargs,
     ):
+        self.id = None
         self.command = command
         self.shell = shell
         self.cwd = cwd or settings.RTD_DOCKER_WORKDIR
@@ -239,11 +240,22 @@ def get_command(self):
         return self.command
 
     def save(self, api_client):
-        """Save this command and result via the API."""
+        """
+        Save this command and result via the API.
+
+        The command can be saved before or after it has been run,
+        if it's saved before it has been run, the exit_code,
+        start_time, and end_time will be None.
+
+        If the command is saved twice (before and after it has been run),
+        the second save will update the command instead of creating a new one.
+        The id of the command will be set the first time it is saved,
+        so it can be used to update the command later.
+        """
         # Force record this command as success to avoid Build reporting errors
         # on commands that are just for checking purposes and do not interferes
         # in the Build
-        if self.record_as_success:
+        if self.record_as_success and self.exit_code is not None:
             log.warning("Recording command exit_code as success")
             self.exit_code = 0
 
@@ -256,22 +268,39 @@ def save(self, api_client):
             "end_time": self.end_time,
         }
 
+        # If the command has an id, it means it has been saved before,
+        # so we update it instead of creating a new one.
         if self.build_env.project.has_feature(Feature.API_LARGE_DATA):
             # Don't use slumber directly here. Slumber tries to enforce a string,
             # which will break our multipart encoding here.
             encoder = MultipartEncoder({key: str(value) for key, value in data.items()})
-            resource = api_client.command
-            resp = resource._store["session"].post(
-                resource._store["base_url"] + "/",
-                data=encoder,
-                headers={
-                    "Content-Type": encoder.content_type,
-                },
-            )
-            log.debug("Post response via multipart form.", response=resp)
+            if self.id:
+                resource = api_client.command(self.id).patch
+                resp = resource._store["session"].patch(
+                    resource._store["url"],
+                    data=encoder,
+                    headers={
+                        "Content-Type": encoder.content_type,
+                    },
+                )
+            else:
+                resource = api_client.command.post
+                resp = resource._store["session"].post(
+                    resource._store["base_url"],
+                    data=encoder,
+                    headers={
+                        "Content-Type": encoder.content_type,
+                    },
+                )
+            log.debug("Response via multipart form.", response=resp)
         else:
-            resp = api_client.command.post(data)
-            log.debug("Post response via JSON encoded data.", response=resp)
+            if self.id:
+                resp = api_client.command(self.id).patch(data)
+            else:
+                resp = api_client.command.post(data)
+            log.debug("Response via JSON encoded data.", response=resp)
+
+        self.id = resp.get("id")
 
 
 class DockerBuildCommand(BuildCommand):
@@ -498,20 +527,25 @@ def run_command_class(
         kwargs["environment"] = environment
         kwargs["build_env"] = self
         build_cmd = cls(cmd, **kwargs)
-        build_cmd.run()
 
+        # Save the command that's running before it starts,
+        # then we will update the results after it has run.
         if record:
-            # TODO: I don't like how it's handled this entry point here since
-            # this class should know nothing about a BuildCommand (which are the
-            # only ones that can be saved/recorded)
             self.record_command(build_cmd)
-
             # We want append this command to the list of commands only if it has
             # to be recorded in the database (to keep consistency) and also, it
             # has to be added after ``self.record_command`` since its
             # ``exit_code`` can be altered because of ``record_as_success``
             self.commands.append(build_cmd)
 
+        build_cmd.run()
+
+        if record:
+            # TODO: I don't like how it's handled this entry point here since
+            # this class should know nothing about a BuildCommand (which are the
+            # only ones that can be saved/recorded)
+            self.record_command(build_cmd)
+
         if build_cmd.failed:
             if warn_only:
                 msg = "Command failed"

readthedocs/rtd_tests/tests/test_api.py

@@ -1178,14 +1178,43 @@ def test_build_commands_read_and_write_endpoints_for_build_api_token(self):
         resp = client.get(f"/api/v2/command/{command.pk}/")
         self.assertEqual(resp.status_code, 200)
 
+        # And updating them.
+        resp = client.patch(
+            f"/api/v2/command/{command.pk}/",
+            {
+                "command": "test2",
+                "exit_code": 1,
+                "output": "test2",
+                "end_time": None,
+                "start_time": None,
+            },
+        )
+        assert resp.status_code == 200
+        command.refresh_from_db()
+        assert command.command == "test2"
+        assert command.exit_code == 1
+        assert command.output == "test2"
+        assert command.start_time is None
+        assert command.end_time is None
+
+        # Isn't possible to update the build the command belongs to.
+        another_build = get(
+            Build, project=project_b, version=project_b.versions.first()
+        )
+        resp = client.patch(
+            f"/api/v2/command/{command.pk}/",
+            {
+                "build": another_build.pk,
+            },
+        )
+        assert resp.status_code == 200
+        command.refresh_from_db()
+        assert command.build == build
+
         # We don't allow deleting commands.
         resp = client.delete(f"/api/v2/command/{command.pk}/")
         self.assertEqual(resp.status_code, 405)
 
-        # Neither updating them.
-        resp = client.patch(f"/api/v2/command/{command.pk}/")
-        self.assertEqual(resp.status_code, 405)
-
         disallowed_builds = [
             get(Build, project=project_b, version=project_b.versions.first()),
             get(Build, project=project_c, version=project_c.versions.first()),
@@ -1214,7 +1243,7 @@ def test_build_commands_read_and_write_endpoints_for_build_api_token(self):
             self.assertEqual(resp.status_code, 405)
 
             resp = client.patch(f"/api/v2/command/{command.pk}/")
-            self.assertEqual(resp.status_code, 405)
+            self.assertEqual(resp.status_code, 404)
 
     def test_versions_read_only_endpoints_for_normal_user(self):
         user_normal = get(User, is_staff=False)

readthedocs/rtd_tests/tests/test_doc_building.py

@@ -38,6 +38,9 @@ def test_command_not_recorded(self):
 
     def test_record_command_as_success(self):
         api_client = mock.MagicMock()
+        api_client.command().patch.return_value = {
+            "id": 1,
+        }
         project = get(Project)
         build_env = LocalBuildEnvironment(
             project=project,
@@ -57,8 +60,19 @@ def test_record_command_as_success(self):
         self.assertEqual(len(build_env.commands), 1)
 
         command = build_env.commands[0]
-        self.assertEqual(command.exit_code, 0)
+        assert command.exit_code == 0
+        assert command.id == 1
         api_client.command.post.assert_called_once_with(
+            {
+                "build": mock.ANY,
+                "command": command.get_command(),
+                "output": "",
+                "exit_code": None,
+                "start_time": None,
+                "end_time": None,
+            }
+        )
+        api_client.command().patch.assert_called_once_with(
             {
                 "build": mock.ANY,
                 "command": command.get_command(),