Make healthcheck a required parameter

In process_emails_from_sqs, require that a path to the healthcheck file
is set. For local development, place this at tmp/healthcheck.json, in a
new temporary directory for local development files.

.dockerignore

@@ -14,6 +14,7 @@ static/scss/libs
 staticfiles
 coverage/
 junit.xml
+tmp/
 
 # frontend/.gitignore
 frontend/public/mockServiceWorker.js

emails/management/commands/check_health.py

@@ -13,6 +13,7 @@ https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-read
 
 from argparse import FileType
 from datetime import datetime, timezone
+import io
 import json
 import logging
 
@@ -33,7 +34,7 @@ class Command(BaseCommand):
     def add_arguments(self, parser):
         """Add command-line arguments (called by BaseCommand)"""
         parser.add_argument(
-            "healthcheck_path",
+            "--healthcheck_path",
             type=FileType("r", encoding="utf8"),
             default=settings.PROCESS_EMAIL_HEALTHCHECK_PATH,
             help=self.help_message_for_setting(
@@ -52,7 +53,11 @@ class Command(BaseCommand):
 
     def handle(self, healthcheck_path, max_age, verbosity, *args, **kwargs):
         """Handle call from command line (called by BaseCommand)"""
-        context = self.check_healthcheck(healthcheck_path, max_age)
+        if isinstance(healthcheck_path, io.TextIOWrapper):
+            healthcheck_file = healthcheck_path
+        else:
+            healthcheck_file = open(healthcheck_path, mode="r", encoding="utf8")
+        context = self.check_healthcheck(healthcheck_file, max_age)
         if context["success"]:
             if verbosity > 1:
                 logger.info("Healthcheck passed", extra=context)

emails/management/commands/process_emails_from_sqs.py

@@ -89,9 +89,8 @@ class Command(BaseCommand):
         )
 
         healthcheck_path = healthcheck_path or settings.PROCESS_EMAIL_HEALTHCHECK_PATH
-        if healthcheck_path is None:
-            self.healthcheck_file = None
-        elif isinstance(healthcheck_path, io.TextIOWrapper):
+        assert healthcheck_path
+        if isinstance(healthcheck_path, io.TextIOWrapper):
             self.healthcheck_file = healthcheck_path
         else:
             self.healthcheck_file = open(healthcheck_path, mode="w", encoding="utf8")
@@ -123,9 +122,7 @@ class Command(BaseCommand):
         assert self.wait_seconds > 0
         assert self.visibility_seconds > 0
         assert self.max_seconds is None or self.max_seconds > 0.0
-        assert self.healthcheck_file is None or isinstance(
-            self.healthcheck_file, io.TextIOWrapper
-        )
+        assert isinstance(self.healthcheck_file, io.TextIOWrapper)
         assert 0 <= self.verbosity <= 3
 
     def help_message_for_setting(self, text, setting_name):
@@ -201,14 +198,13 @@ class Command(BaseCommand):
     def handle(self, *args, **kwargs):
         """Handle call from command line (called by BaseCommand)"""
         self.init_vars(*args, **kwargs)
-        healthcheck_path = self.healthcheck_file.name if self.healthcheck_file else None
         logger.info(
             "Starting process_emails_from_sqs",
             extra={
                 "batch_size": self.batch_size,
                 "wait_seconds": self.wait_seconds,
                 "visibility_seconds": self.visibility_seconds,
-                "healthcheck_path": healthcheck_path,
+                "healthcheck_path": self.healthcheck_file.name,
                 "delete_failed_messages": self.delete_failed_messages,
                 "max_seconds": self.max_seconds,
                 "aws_region": self.aws_region,
@@ -224,6 +220,7 @@ class Command(BaseCommand):
 
         process_data = self.process_queue()
         logger.info("Exiting process_emails_from_sqs", extra=process_data)
+        self.healthcheck_file.close()
 
     def create_client(self):
         """Create the SQS client."""
@@ -504,8 +501,6 @@ class Command(BaseCommand):
 
     def write_healthcheck(self):
         """Update the healthcheck file with operations data, if path is set."""
-        if not self.healthcheck_file:
-            return
         data = {
             "timestamp": datetime.now(tz=timezone.utc).isoformat(),
             "cycles": self.cycles,
@@ -530,9 +525,3 @@ class Command(BaseCommand):
             return f"{value} {singular}"
         else:
             return f"{value} {plural or (singular + 's')}"
-
-    def close_healthcheck(self):
-        """Close the healthcheck file if open."""
-        if self.healthcheck_file:
-            self.healthcheck_file.close()
-            self.healthcheck_file = None

emails/tests/mgmt_check_health_tests.py

@@ -10,7 +10,15 @@ from django.core.management import call_command, CommandError
 from emails.management.commands.check_health import Command
 
 
-def write_healthcheck(folder, age=0):
+@pytest.fixture(autouse=True)
+def test_settings(settings, tmp_path):
+    """Override settings for tests"""
+    settings.PROCESS_EMAIL_HEALTHCHECK_PATH = tmp_path / "healthcheck.json"
+    settings.PROCESS_EMAIL_MAX_AGE = 120
+    return settings
+
+
+def write_healthcheck(path, age=0):
     """
     Write a valid healthcheck file.
 
@@ -20,45 +28,47 @@ def write_healthcheck(folder, age=0):
 
     Returns the path to the healthcheck file
     """
-    path = folder / "healthcheck.json"
     timestamp = (datetime.now(tz=timezone.utc) - timedelta(seconds=age)).isoformat()
     data = {"timestamp": timestamp, "testing": True}
     with path.open("w", encoding="utf8") as f:
         json.dump(data, f)
-    return path
 
 
-def test_check_health_passed_no_logs(tmp_path, caplog):
+def test_check_health_passed_no_logs(test_settings, caplog):
     """check health succeeds when the timestamp is recent."""
-    path = write_healthcheck(tmp_path)
-    call_command("check_health", str(path))
+    path = test_settings.PROCESS_EMAIL_HEALTHCHECK_PATH
+    write_healthcheck(path)
+    call_command("check_health")
     assert caplog.record_tuples == []
 
 
-def test_check_health_passed_logs(tmp_path, caplog):
+def test_check_health_passed_logs(test_settings, caplog):
     """check health success and logs at verbosity 2."""
-    path = write_healthcheck(tmp_path)
-    call_command("check_health", str(path), f"--verbosity=2")
+    path = test_settings.PROCESS_EMAIL_HEALTHCHECK_PATH
+    write_healthcheck(path)
+    call_command("check_health", "--verbosity=2")
     assert caplog.record_tuples == [
         ("eventsinfo.check_health", logging.INFO, "Healthcheck passed")
     ]
 
 
-def test_check_health_too_old(tmp_path, caplog):
+def test_check_health_too_old(test_settings, caplog):
     """check health fails when the timestamp is too old."""
-    path = write_healthcheck(tmp_path, 130)
+    path = test_settings.PROCESS_EMAIL_HEALTHCHECK_PATH
+    write_healthcheck(path, 130)
     with pytest.raises(CommandError) as excinfo:
-        call_command("check_health", str(path), "--max-age=120")
+        call_command("check_health")
     assert str(excinfo.value) == "Healthcheck failed: Timestamp is too old"
     assert caplog.record_tuples == [
         ("eventsinfo.check_health", logging.ERROR, "Healthcheck failed")
     ]
 
 
-def test_check_health_failed_no_logs(tmp_path, caplog):
+def test_check_health_failed_no_logs(test_settings, caplog):
     """check health failure do not log at verbosity=0."""
-    path = write_healthcheck(tmp_path, 130)
+    path = test_settings.PROCESS_EMAIL_HEALTHCHECK_PATH
+    write_healthcheck(path, 130)
     with pytest.raises(CommandError) as excinfo:
-        call_command("check_health", str(path), "--max-age=120", "--verbosity=0")
+        call_command("check_health", "--verbosity=0")
     assert str(excinfo.value) == "Healthcheck failed: Timestamp is too old"
     assert caplog.record_tuples == []

emails/tests/mgmt_process_emails_from_sqs_tests.py

@@ -60,7 +60,7 @@ def mock_sns_inbound_logic():
 
 
 @pytest.fixture(autouse=True)
-def test_settings(settings):
+def test_settings(settings, tmp_path):
     settings.AWS_SNS_TOPIC = {TEST_SNS_MESSAGE["TopicArn"]}
     settings.AWS_REGION = "us-east-1"
 
@@ -70,7 +70,7 @@ def test_settings(settings):
     )
     settings.PROCESS_EMAIL_BATCH_SIZE = 10
     settings.PROCESS_EMAIL_DELETE_FAILED_MESSAGES = False
-    settings.PROCESS_EMAIL_HEALTHCHECK_PATH = None
+    settings.PROCESS_EMAIL_HEALTHCHECK_PATH = tmp_path / "healthcheck.json"
     settings.PROCESS_EMAIL_MAX_SECONDS = None
     settings.PROCESS_EMAIL_VERBOSITY = 2
     settings.PROCESS_EMAIL_VISIBILITY_SECONDS = 120
@@ -268,13 +268,9 @@ def test_process_queue_verify_sns_header_fails(test_settings):
     assert res["failed_messages"] == 1
 
 
-def test_process_queue_write_healthcheck(tmp_path):
-    """write_healthcheck writes the timestamp to the specified path."""
-    healthcheck_path = tmp_path / "healthcheck.json"
-    with open(healthcheck_path, "w", encoding="utf8") as hc_file:
-        Command(
-            queue=fake_queue(), healthcheck_file=hc_file, max_seconds=3
-        ).process_queue()
+def test_process_queue_write_healthcheck(test_settings):
+    healthcheck_path = test_settings.PROCESS_EMAIL_HEALTHCHECK_PATH
+    Command(queue=fake_queue(), max_seconds=3).process_queue()
     content = json.loads(healthcheck_path.read_bytes())
     ts = datetime.fromisoformat(content["timestamp"])
     duration = (datetime.now(tz=timezone.utc) - ts).total_seconds()
@@ -335,14 +331,12 @@ def test_command_setup_from_settings():
     assert command.verbosity == 2
     assert command.visibility_seconds == 1200
     assert command.wait_seconds == 10
-    command.close_healthcheck()
 
 
-def test_write_healthcheck(tmp_path):
+def test_write_healthcheck(test_settings):
     """write_healthcheck writes the timestamp to the specified path."""
-    healthcheck_path = tmp_path / "healthcheck.json"
-    with open(healthcheck_path, "w", encoding="utf8") as hc_file:
-        Command(queue=fake_queue(), healthcheck_file=hc_file).write_healthcheck()
+    healthcheck_path = test_settings.PROCESS_EMAIL_HEALTHCHECK_PATH
+    Command(queue=fake_queue()).write_healthcheck()
     content = json.loads(healthcheck_path.read_bytes())
     assert content == {
         "timestamp": content["timestamp"],
@@ -359,22 +353,11 @@ def test_write_healthcheck(tmp_path):
     assert 0.0 < duration < 0.5
 
 
-def test_write_healthcheck_twice(tmp_path):
+def test_write_healthcheck_twice(test_settings):
     """write_healthcheck overwrites the file each time."""
-    healthcheck_path = tmp_path / "healthcheck.json"
-    with open(healthcheck_path, "w", encoding="utf8") as hc_file:
-        cmd = Command(queue=fake_queue(), healthcheck_file=hc_file)
-        cmd.write_healthcheck()
-        cmd.write_healthcheck()
+    healthcheck_path = test_settings.PROCESS_EMAIL_HEALTHCHECK_PATH
+    cmd = Command(queue=fake_queue())
+    cmd.write_healthcheck()
+    cmd.write_healthcheck()
     content = json.loads(healthcheck_path.read_bytes())
     assert content['queue_count_not_visible'] == 3
-
-
-def test_close_healthcheck(tmp_path):
-    """close_healthcheck() can be used to manually close the healthcheck file."""
-    healthcheck_path = tmp_path / "healthcheck.json"
-    command = Command(healthcheck_file=healthcheck_path)
-    assert command.healthcheck_file.name == str(healthcheck_path)
-    command.close_healthcheck()
-    assert command.healthcheck_file is None
-    command.close_healthcheck()  # Closing twice is OK

privaterelay/settings.py

@@ -39,7 +39,7 @@ except ImportError:
 
 # Build paths inside the project like this: os.path.join(BASE_DIR, ...)
 BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
-
+TMP_DIR = os.path.join(BASE_DIR, "tmp")
 
 # Quick-start development settings - unsuitable for production
 # See https://docs.djangoproject.com/en/2.2/howto/deployment/checklist/
@@ -718,7 +718,9 @@ PROCESS_EMAIL_BATCH_SIZE = config(
 PROCESS_EMAIL_DELETE_FAILED_MESSAGES = config(
     "PROCESS_EMAIL_DELETE_FAILED_MESSAGES", False, cast=bool
 )
-PROCESS_EMAIL_HEALTHCHECK_PATH = config("PROCESS_EMAIL_HEALTHCHECK_PATH", "") or None
+PROCESS_EMAIL_HEALTHCHECK_PATH = config(
+    "PROCESS_EMAIL_HEALTHCHECK_PATH", os.path.join(TMP_DIR, "healthcheck.json")
+)
 PROCESS_EMAIL_MAX_SECONDS = config("PROCESS_EMAIL_MAX_SECONDS", 0, cast=int) or None
 PROCESS_EMAIL_VERBOSITY = config(
     "PROCESS_EMAIL_VERBOSITY", 1, cast=Choices(range(0, 4), cast=int)

tmp/.gitignore

@@ -0,0 +1,3 @@
+*
+!README.md
+!.gitignore

tmp/README.md

@@ -0,0 +1,3 @@
+# Temporary folder
+
+This folder contains temporary files created in local development environments.