Remove informdb PAT (#200)

* Remove informdb PAT

* Ensure we're building all components in tests

* Run system tests in CI

* Run system tests in CI

* Run system tests in CI

* Avoid test container clash

Also take remove volumes in docker compose

* Remove system tests docker container orphans

* Install core and cli separately

* Install packages in editable mode

* Use system test project name in test scripts

* Don't remove orphans in second call

Of course this removes the previously upped containers

* Set the system test name for fake star

* Add healthcheck for fake VNA

* Try quoting in post of dicom image

* Setup python for system tests in CI

* Remove interactive terminal for checking pixl anon

* Update expected rows in pixl anon

* Update tests system tests that links DICOM and parquet files

* Use the correct test resource directory

* Update star data to have parquet patients

* Fix docker shutdown in after system test

* Update test setup

* Deserialise message before callback

* Increase ORTHANC storage limit during tests

* Clarify notes aboout docker compose commands in system test

* Revert to using TAG for Satellite

---------

Co-authored-by: Milan Malfait <m.malfait@ucl.ac.uk>

.github/workflows/main.yml

@@ -20,6 +20,11 @@ on:
   pull_request:
   workflow_dispatch:
 
+# Only run actions on the most recent push to a branch
+concurrency:
+  group: "${{ github.workflow }}-${{ github.head_ref }}"
+  cancel-in-progress: true
+
 jobs:
 
   lint:
@@ -154,8 +159,6 @@ jobs:
 
       - name: Run tests
         working-directory: pixl_ehr/tests
-        env:
-          INFORMDB_PAT: ${{ secrets.INFORMDB_PAT }}
         run: |
           ./run-tests.sh
 
@@ -179,3 +182,20 @@ jobs:
         working-directory: pixl_pacs/tests
         run: |
           ./run-tests.sh
+
+  system-test:
+    runs-on: ubuntu-22.04
+    timeout-minutes: 15
+    steps:
+      - uses: actions/checkout@v3
+
+      - name: Init Python
+        uses: actions/setup-python@v4
+        with:
+          python-version: 3.10.6
+          cache: 'pip'
+
+      - name: Run tests
+        working-directory: test
+        run: |
+          ./run-system-test.sh

pixl_core/src/core/patient_queue/subscriber.py

@@ -69,13 +69,14 @@ async def run(self, callback: Callable[[Message], Awaitable[None]]) -> None:
                     await message.reject(requeue=True)
                     continue
 
+                pixl_message = deserialise(message.body)
                 try:
                     await asyncio.sleep(0.01)  # Avoid very fast callbacks
-                    await callback(deserialise(message.body))
+                    await callback(pixl_message)
                 except Exception:
                     logger.exception(
                         "Failed to process %s" "Not re-queuing message",
-                        message.body.decode(),
+                        pixl_message,
                     )
                 finally:
                     await message.ack()

pixl_ehr/tests/docker-compose.yml

@@ -97,7 +97,6 @@ services:
       dockerfile: ../../test/Dockerfile.fake_emap_star
       args:
         N_TABLE_ROWS: 2
-        INFORMDB_PAT: ${INFORMDB_PAT}
 
     healthcheck:
       test: ["CMD", "pg_isready", "-U", "postgres"]

pixl_ehr/tests/run-processing-tests.sh

@@ -32,10 +32,7 @@ THIS_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
 PACKAGE_DIR="${THIS_DIR%/*}"
 cd "$PACKAGE_DIR"/tests || exit
 
-if [ -z "${INFORMDB_PAT+x}" ]; then
-  echo "INFORMDB_PAT must be set as an environment variable" && exit 1
-fi
-
+docker compose down --volumes
 docker compose up -d --build
 docker exec pixl-test-ehr-api /bin/bash -c "pytest -m processing"
-docker compose down
+docker compose down --volumes

test/.env.test.sample → test/.env.test

@@ -3,8 +3,6 @@ DEBUG=True
 PIXL_DICOM_TRANSFER_TIMEOUT=120
 PIXL_QUERY_TIMEOUT=120
 
-INFORMDB_PAT=
-
 # PIXL PostgreSQL instance
 PIXL_DB_HOST=postgres
 PIXL_DB_PORT=5432
@@ -37,7 +35,7 @@ ORTHANC_RAW_USERNAME=orthanc_raw_username
 ORTHANC_RAW_PASSWORD=orthanc_raw_password
 ORTHANC_RAW_AE_TITLE=ORTHANCRAW
 ORTHANC_AUTOROUTE_RAW_TO_ANON=true
-ORTHANC_RAW_MAXIMUM_STORAGE_SIZE=1
+ORTHANC_RAW_MAXIMUM_STORAGE_SIZE=100
 
 # PIXL Orthanc anon instance
 ORTHANC_ANON_USERNAME=orthanc_anon_username

test/Dockerfile.fake_emap_star

@@ -14,15 +14,14 @@
 FROM postgres:15.0-bullseye
 
 # User definable arguments
-ARG INFORMDB_PAT
 ARG POSTGRES_USER=postgres
 ARG POSTGRES_PASSWORD=postgres
 ARG DATABASE_NAME=emap
 ARG N_TABLE_ROWS=2
 ARG INSERT_RATE=0.3
 ARG UPDATE_RATE=0.0
 ARG DELETE_RATE=0.0
-ARG INFORMDB_BRANCH_NAME=develop
+ARG EMAP_BRANCH_NAME=main
 ARG STAR_SCHEMA_NAME=star
 ARG FAKER_SEED=0
 ARG TIMEZONE="Europe/London"
@@ -57,14 +56,13 @@ RUN pip install --no-cache-dir . && \
 ENV POSTGRES_USER ${POSTGRES_USER}
 ENV POSTGRES_PASSWORD ${POSTGRES_PASSWORD}
 ENV DATABASE_NAME ${DATABASE_NAME}
-ENV INFORMDB_BRANCH_NAME ${INFORMDB_BRANCH_NAME}
+ENV EMAP_BRANCH_NAME ${EMAP_BRANCH_NAME}
 ENV STAR_SCHEMA_NAME ${STAR_SCHEMA_NAME}
 ENV FAKER_SEED ${FAKER_SEED}
 ENV TIMEZONE ${TIMEZONE}
 ENV INSERT_RATE ${INSERT_RATE}
 ENV UPDATE_RATE ${UPDATE_RATE}
 ENV DELETE_RATE ${DELETE_RATE}
-ENV INFORMDB_PAT ${INFORMDB_PAT}
 ENV LANG=en_GB.UTF-8
 ENV LC_ALL=en_GB.UTF-8
 

test/README.md

@@ -11,16 +11,7 @@ queue and the consumers started.
 **Then** a row in the "anon" EMAP data instance of the PIXL postgres instance exists
 and the DICOM study exists in the "anon" PIXL Orthanc instance.
 
-Spinning up the Docker containers requires the `$INFORMDB_PAT` environment variable to be set to a
-valid [GitHub Personal Access Token](https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens#creating-a-fine-grained-personal-access-token)
-with read access to the [UCLH-Foundry/Inform-DB](https://github.com/UCLH-Foundry/Inform-DB) repo.
-
-1. Create a [fine-grained Personal Access Token](https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens#creating-a-fine-grained-personal-access-token)
-   with **read** access to the *UCLH-Foundry/Inform-DB* repo
-2. Create `.env.test` with `cp .env.test.sample .env.test`
-3. Add your `githb_pat***` token to `.env.test` as `INFORMDB_PAT=***`
-
-Then, run the system test with:
+You can run the system test with:
 
 ```bash
 ./run-system-test.sh

test/docker-compose.yml

@@ -36,17 +36,21 @@ services:
       - "8043:8042"
     volumes:
       - ${PWD}/vna_config/:/run/secrets:ro
+    healthcheck:
+      test: [ "CMD", "curl", "-f", "-u" , "orthanc:orthanc", "http://vna-qr:8042/patients" ]
+      interval: 10s
+      timeout: 30s
+      retries: 5
     networks:
       pixl-net:
 
   star:
-    container_name: test-fake-star-db
+    container_name: system-test-fake-star-db
     build:
       context: ..
       dockerfile: test/Dockerfile.fake_emap_star
       args:
         N_TABLE_ROWS: 2
-        INFORMDB_PAT: ${INFORMDB_PAT}
     environment:
       POSTGRES_USER: ${EMAP_UDS_USER}
       POSTGRES_PASSWORD: ${EMAP_UDS_PASSWORD}

test/run-system-test.sh

@@ -17,22 +17,24 @@ BIN_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
 PACKAGE_DIR="${BIN_DIR%/*}"
 cd "${PACKAGE_DIR}/test"
 
-# Note: this doesn't work as a single command
-docker compose --env-file .env.test -p test up -d  --remove-orphans
+docker compose --env-file .env.test -p system-test down --volumes
+#
+# Note: cannot run as single docker compose command due to different build contexts
+docker compose --env-file .env.test -p system-test up -d --build --remove-orphans
+# Warning: Requires to be run from the project root
 cd .. && \
-  docker compose --env-file test/.env.test -p test up -d --build && \
+  docker compose --env-file test/.env.test -p system-test up -d --build && \
   cd "${PACKAGE_DIR}/test"
 
 ./scripts/insert_test_data.sh
 
-pip install "${PACKAGE_DIR}/pixl_core" "${PACKAGE_DIR}/cli"
-pixl populate "${PACKAGE_DIR}/test/data/resources/omop"
+pip install -e "${PACKAGE_DIR}/pixl_core" && pip install -e "${PACKAGE_DIR}/cli"
+pixl populate "${PACKAGE_DIR}/test/resources/omop"
 pixl start
 sleep 65  # need to wait until the DICOM image is "stable" = 60s
 ./scripts/check_entry_in_pixl_anon.sh
 ./scripts/check_entry_in_orthanc_anon.sh
 ./scripts/check_max_storage_in_orthanc_raw.sh
 
 cd "${PACKAGE_DIR}"
-docker compose -f docker-compose.yml -f ../docker-compose.yml -p test down
-docker volume rm test_postgres-data test_orthanc-raw-data test_orthanc-anon-data
+docker compose -f docker-compose.yml -f test/docker-compose.yml -p system-test down --volumes

test/scripts/check_entry_in_orthanc_anon.sh

@@ -16,4 +16,4 @@ set -euxo pipefail
 
 # This could be much improved by having more realistic test data some of
 # which actually was persisted
-docker logs test-orthanc-anon-1 2>&1 | grep "DICOM instance received"
+docker logs system-test-orthanc-anon-1 2>&1 | grep "DICOM instance received"

test/scripts/check_entry_in_pixl_anon.sh

@@ -15,6 +15,6 @@
 set -euxo pipefail
 
 _sql_command="select * from emap_data.ehr_anon"
-_result=$(docker exec -it test-postgres-1 /bin/bash -c \
+_result=$(docker exec system-test-postgres-1 /bin/bash -c \
   "PGPASSWORD=pixl_db_password psql -U pixl_db_username -d pixl -c \"$_sql_command\"")
-echo "$_result" | grep -q "1 row"
+echo "$_result" | grep -q "6 row"

test/scripts/check_max_storage_in_orthanc_raw.sh

@@ -17,5 +17,5 @@ set -euxo pipefail
 # This could be much improved by having more realistic test data some of
 # which actually was persisted
 source ./.env.test
-docker logs test-orthanc-raw-1 2>&1 | grep "At most ${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE}MB will be used for the storage area"
+docker logs system-test-orthanc-raw-1 2>&1 | grep "At most ${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE}MB will be used for the storage area"
 

test/scripts/insert_test_data.sh

@@ -17,11 +17,18 @@ set -euxo pipefail
 SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
 
 _sql_command="
-insert into star.mrn(mrn_id, mrn, research_opt_out) values (1234, 'patient_identifier', false);
+insert into star.mrn(mrn_id, mrn, research_opt_out) values (1234, '12345678', false);
+insert into star.mrn(mrn_id, mrn, research_opt_out) values (2345, '987654321', false);
+insert into star.mrn(mrn_id, mrn, research_opt_out) values (3456, '5020765', false);
 insert into star.core_demographic(mrn_id, sex) values (1234, 'F');
+insert into star.core_demographic(mrn_id, sex) values (2345, 'F');
+insert into star.core_demographic(mrn_id, sex) values (3456, 'F');
 "
-docker exec -it test-fake-star-db /bin/bash -c "psql -U postgres -d emap -c \"$_sql_command\"" || true
+docker exec -it system-test-fake-star-db /bin/bash -c "psql -U postgres -d emap -c \"$_sql_command\"" || true
 
-# Uses an accession number of "123456789"
-curl -X POST -u orthanc:orthanc http://localhost:8043/instances \
-  --data-binary @"$SCRIPT_DIR/../data/test.dcm"
+# Uses an accession number of "AA12345601" for MRN 987654321
+curl -X POST -u "orthanc:orthanc" "http://localhost:8043/instances" \
+  --data-binary @"$SCRIPT_DIR/../resources/Dicom1.dcm"
+# Uses an accession number of "AA12345605"  for MRN 987654321
+curl -X POST -u "orthanc:orthanc" "http://localhost:8043/instances" \
+  --data-binary @"$SCRIPT_DIR/../resources/Dicom2.dcm"