From a0001fa4a02ba989ffc1e3b8e9dcf5d328bdd32c Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 17 Oct 2024 11:23:47 +0100 Subject: [PATCH 1/2] Use the correct python version when running jobs In local dev environments, we run with `just`, which explicitly uses the virtual env bin to run all commands, without the user needing to manually activate it. However, dispatcher jobs are run in a subprocess and just run with `python ...` and so don't automatically use the virtualenv python. In the Dockerfile (i.e. for production), we prepend the virtualenv bin path to the PATH environment variable, which ensures that the virtualenv python will be the first python encountered and used. We now check for an ABSOLUTE_BIN env variable (set in the justfile, so this will only apply for local envs, not prod) and ensure that it is first in the PATH when running a job. Note that we could set PATH itself in the justfile so that it is passed through in os.environ to the job subprocess, but if we do, it'll set the user's global PATH, which is not necessarily what they want. --- bennettbot/dispatcher.py | 8 ++++++-- justfile | 2 ++ tests/job_configs.py | 3 +++ tests/test_dispatcher.py | 19 +++++++++++++++++++ 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/bennettbot/dispatcher.py b/bennettbot/dispatcher.py index b2ee94c..5917d9b 100644 --- a/bennettbot/dispatcher.py +++ b/bennettbot/dispatcher.py @@ -89,7 +89,11 @@ def run_command(self): stdout_path=self.stdout_path, stderr_path=self.stdout_path, ) - + env = {**os.environ, "PYTHONPATH": settings.APPLICATION_ROOT} + bin_path = os.getenv("ABSOLUTE_BIN") + if bin_path and not env["PATH"].startswith(bin_path): # pragma: no cover + # If we have an ABSOLUTE_BIN env variable, ensure that it's set first in the path + env["PATH"] = f"{bin_path}:{env['PATH']}" with open(self.stdout_path, "w") as stdout, open( self.stderr_path, "w" ) as stderr: @@ -99,7 +103,7 @@ def run_command(self): cwd=self.cwd, stdout=stdout, stderr=stderr, - env={**os.environ, "PYTHONPATH": settings.APPLICATION_ROOT}, + env=env, shell=True, ) rc = rv.returncode diff --git a/justfile b/justfile index af8498d..fc9edf9 100644 --- a/justfile +++ b/justfile @@ -7,6 +7,8 @@ export PIP := BIN + if os_family() == "unix" { "/python -m pip" } else { "/pytho export DEFAULT_PYTHON := if os_family() == "unix" { "python3.12" } else { "python" } +export ABSOLUTE_BIN := `pwd` + "/" + BIN + set dotenv-load := true diff --git a/tests/job_configs.py b/tests/job_configs.py index fbbc4ed..2f47c75 100644 --- a/tests/job_configs.py +++ b/tests/job_configs.py @@ -5,6 +5,9 @@ raw_config = { "test": { "jobs": { + "python_version": { + "run_args_template": 'python -c "import platform; print(platform.python_version())"', + }, "good_job": { "run_args_template": "cat poem", }, diff --git a/tests/test_dispatcher.py b/tests/test_dispatcher.py index 9bbde0c..a67330e 100644 --- a/tests/test_dispatcher.py +++ b/tests/test_dispatcher.py @@ -1,6 +1,8 @@ import json import os +import platform import shutil +from pathlib import Path from unittest.mock import Mock, patch import httpretty @@ -658,3 +660,20 @@ def test_message_checker_matched_messages(keyword, support_channel, reaction): "timestamp": ["100.3"], }, ] + + +def test_python_version(): + # check that we are using the same python version in jobs as we are + # in this test + # this would previously fail in local test runs if the user's system + # python version was different from the venv python and they were + # running `just test` without manually activating the venv first + log_dir = build_log_dir("test_python_version") + + scheduler.schedule_job("test_python_version", {}, "channel", TS, 0) + job = scheduler.reserve_job() + + do_job(slack_web_client(), job) + + version_in_job = (Path(log_dir) / "stdout").read_text().strip() + assert version_in_job == platform.python_version() From 7d438a5225e8776cc9722dcba3bb1c46c662c4c7 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 17 Oct 2024 13:04:21 +0100 Subject: [PATCH 2/2] Add comment to prod.sh entrypoint The equivalent comment is already in the Dockerfile, but this is included now with the entrypoint script for the benefit of future us coming across it again. --- docker/entrypoints/prod.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docker/entrypoints/prod.sh b/docker/entrypoints/prod.sh index 21c8994..a517d50 100755 --- a/docker/entrypoints/prod.sh +++ b/docker/entrypoints/prod.sh @@ -1,5 +1,11 @@ #!/bin/bash +# Note: this is set as the default CMD in the Dockerfile +# In production, dokku extracts the Procfile and uses the commands defined +# there as the entrypoints, so this script is ignored in production. It is +# used only if we're running the production service with docker compose +# https://dokku.com/docs/processes/process-management/#procfile + set -euo pipefail just run bot