From 52f402b3cf3b42f7b5864cbaaf342f3eeba973dc Mon Sep 17 00:00:00 2001
From: Jonathan Green <jonathan@razzard.com>
Date: Thu, 18 Apr 2024 20:57:38 -0300
Subject: [PATCH 1/5] Fix CI issue where we couldn't start old container

---
 docker/ci/test_migrations.sh  | 214 +++++++++++++++++++++-------------
 docker/ci/test_migrations.yml |  18 +++
 2 files changed, 149 insertions(+), 83 deletions(-)
 create mode 100644 docker/ci/test_migrations.yml

diff --git a/docker/ci/test_migrations.sh b/docker/ci/test_migrations.sh
index e6523c04d6..321f4b67dc 100755
--- a/docker/ci/test_migrations.sh
+++ b/docker/ci/test_migrations.sh
@@ -3,139 +3,187 @@
 # This script makes sure that our database migrations bring the database up to date
 # so that the resulting database is the same as if we had initialized a new instance.
 #
-# This is done by (1) checking out an older version of our codebase at the commit on
-# which the first migration was added and then (2) initializing a new instance. Then
-# we check out the current version of our codebase and run our migrations.
+# This is done by:
+# (1) Finding the id of the first DB migration.
+# (2) Initializing the database with an old version of the app. This old version is
+#     the version of the app that was current when the first migration was created.
+#     This version is started in a separate container called `webapp-old`. This
+#     container is defined in the `test_migrations.yml` file.
+# (3) Then the current version of the app is started in a container called `webapp`.
+# (4) We run the migrations in the `webapp` container to bring the database up to date.
+#     and then check that the database schema matches the model.
+# (5) We then run the downgrade migrations in the `webapp` container to bring the database
+#     back to the state it was in when the first migration was created.
+# (6) We then check that the database schema matches the model in the `webapp-old` container.
+# (7) Finally, we repeat step (4) to make sure that the up migrations stay in sync.
 #
-# After the migrations are complete we use `alembic check` [1] to make sure that the
-# database model matches the migrated database. If the model matches, then the database
-# database is in sync and the migrations are up to date. If the database doesn't match
-# then a new migration is required. We then repeat this process with our down
-# migrations to make sure that the down migrations stay in sync as well.
+# After the migrations are complete in step (4) and (6) we use `alembic check` [1] to
+# make sure that the database model matches the migrated database. If the model matches,
+# then the database is in sync and the migrations are up to date. If the database doesn't
+# match then there is a problem with the migrations and the script will fail.
 #
 # Note: This test cannot be added to the normal migration test suite since it requires
-# manipulating the git history and checking out older versions of the codebase. All of
-# the commands in this script are run inside a docker-compose environment.
+# us to have access to an older version of our code base. To facilitate this we use the
+# `test_migrations.yml` file to define a container that runs an older version of the app.
+# And run all the commands in this script in a docker-compose environment.
 #
 # [1] https://alembic.sqlalchemy.org/en/latest/autogenerate.html#running-alembic-check-to-test-for-new-upgrade-operations
 
+# Text colors
+RESET='\033[0m'       # Text Reset
+GREEN='\033[1;32m'       # Green
+
+# Keeps track of whether we are in a group or not
+IN_GROUP=0
+
+gh_command() {
+  local COMMAND=$1
+  local MESSAGE=${2:-""}
+  echo "::${COMMAND}::${MESSAGE}"
+}
+
+gh_group() {
+  local MESSAGE=$1
+  gh_command group "$MESSAGE"
+  IN_GROUP=1
+#  set -x
+}
+
+gh_endgroup() {
+  if [[ $IN_GROUP -eq 1 ]]; then
+#    set +x
+    gh_command endgroup
+    IN_GROUP=0
+  fi
+}
+
+gh_error() {
+  gh_endgroup
+  local MESSAGE=$1
+  gh_command error "$MESSAGE"
+}
+
+success() {
+  gh_endgroup
+  local MESSAGE=$1
+  echo -e "${GREEN}Success:${RESET} ${MESSAGE}"
+}
 
 compose_cmd() {
-  docker --log-level ERROR compose --progress quiet "$@"
+  echo "++ docker compose $*" >&2
+  docker compose -f docker-compose.yml -f docker/ci/test_migrations.yml --progress quiet "$@"
 }
 
 run_in_container()
 {
-  compose_cmd run --build --rm --no-deps webapp /bin/bash -c "source env/bin/activate && $*"
+  local CONTAINER_NAME=$1
+  shift 1
+  echo "+ $*" >&2
+  compose_cmd run --build --rm --no-deps "${CONTAINER_NAME}" /bin/bash -c "source env/bin/activate && $*"
 }
 
 cleanup() {
   compose_cmd down
-  git checkout -q "${current_branch}"
+}
+
+error_and_cleanup() {
+  local MESSAGE=$1
+  local EXIT_CODE=$2
+  cleanup
+  gh_error "$MESSAGE"
+  exit "$EXIT_CODE"
 }
 
 run_migrations() {
-  ALEMBIC_CMD=$1
-  run_in_container "alembic ${ALEMBIC_CMD}"
+  local CONTAINER_NAME=$1
+  local ALEMBIC_CMD=$2
+  run_in_container "${CONTAINER_NAME}" "alembic ${ALEMBIC_CMD}"
   exit_code=$?
   if [[ $exit_code -ne 0 ]]; then
-    echo "ERROR: Running database migrations failed."
-    cleanup
-    exit $exit_code
+    error_and_cleanup "Running database migrations failed." $exit_code
   fi
-  echo ""
 }
 
 check_db() {
-  DETAILED_ERROR=$1
-  run_in_container "alembic check"
-  exit_code=$?
-  if [[ $exit_code -eq 0 ]]; then
-    echo "SUCCESS: Database is in sync."
-    echo ""
-  else
-    echo "ERROR: Database is out of sync! ${DETAILED_ERROR}"
-    cleanup
-    exit $exit_code
+  local CONTAINER_NAME=$1
+  local DETAILED_ERROR=$2
+  run_in_container "${CONTAINER_NAME}" "alembic check"
+  local exit_code=$?
+  if [[ $exit_code -ne 0 ]]; then
+    error_and_cleanup "Database is out of sync! ${DETAILED_ERROR}" $exit_code
   fi
+  success "Database is in sync."
 }
 
-if ! git diff --quiet; then
-  echo "ERROR: You have uncommitted changes. These changes will be lost if you run this script."
-  echo "  Please commit or stash your changes and try again."
-  exit 1
-fi
-
-# Find the currently checked out branch
-current_branch=$(git symbolic-ref -q --short HEAD)
-current_branch_exit_code=$?
-
-# If we are not on a branch, then we are in a detached HEAD state, so
-# we use the commit hash instead. This happens in CI when being run
-# against a PR instead of a branch.
-# See: https://stackoverflow.com/questions/69935511/how-do-i-save-the-current-head-so-i-can-check-it-back-out-in-the-same-way-later
-if [[ $current_branch_exit_code -ne 0 ]]; then
-  current_branch=$(git rev-parse HEAD)
-  echo "WARNING: You are in a detached HEAD state. This is normal when running in CI."
-  echo "  The current commit hash will be used instead of a branch name."
-fi
-
-echo "Current branch: ${current_branch}"
-
-# Find the first migration file
-first_migration_id=$(run_in_container alembic history -r'base:base+1' -v | head -n 1 | cut -d ' ' -f2)
+# Find all the info we need about the first migration in the git history
+# including its id, the file it was added in, and the commit it was added in.
+gh_group "Finding first migration"
+first_migration_id=$(run_in_container "webapp" alembic history -r'base:base+1' -v | head -n 1 | cut -d ' ' -f2)
 if [[ -z $first_migration_id ]]; then
-  echo "ERROR: Could not find first migration id."
-  exit 1
+  error_and_cleanup "Could not find first migration id." 1
 fi
 
 first_migration_file=$(find alembic/versions -name "*${first_migration_id}*.py")
 if [[ -z $first_migration_file ]]; then
-  echo "ERROR: Could not find first migration file."
-  exit 1
+  error_and_cleanup "Could not find first migration file." 1
 fi
 
-echo "First migration file: ${first_migration_file}"
-echo ""
-
 # Find the git commit where the first migration file was added
 first_migration_commit=$(git log --follow --format=%H --reverse "${first_migration_file}" | head -n 1)
 if [[ -z $first_migration_commit ]]; then
-  echo "ERROR: Could not find first migration commit hash."
-  exit 1
+  error_and_cleanup "Could not find first migration commit hash." 1
 fi
+first_migration_container="ghcr.io/thepalaceproject/circ-webapp:sha-${first_migration_commit:0:7}"
+echo "First migration info:"
+echo "  id: ${first_migration_id}"
+echo "  file: ${first_migration_file}"
+echo "  commit: ${first_migration_commit}"
+echo "  container: ${first_migration_container}"
+
+container_image=$(sed -n 's/^ *image: "\(.*\)"/\1/p' docker/ci/test_migrations.yml)
+if [[ -z $container_image ]]; then
+  error_and_cleanup "Could not find container image in test_migrations.yml" 1
+fi
+
+if [[ "$container_image" != "$first_migration_container" ]]; then
+  error_and_cleanup "Incorrect container image in test_migrations.yml. Please update." 1
+fi
+gh_endgroup
 
-echo "Starting containers"
+gh_group "Starting service containers"
 compose_cmd down
-compose_cmd up -d pg
+compose_cmd up -d pg os
+gh_endgroup
 
-echo "Initializing database at commit ${first_migration_commit}"
-git checkout -q "${first_migration_commit}"
-run_in_container "./bin/util/initialize_instance"
+gh_group "Initializing database"
+run_in_container "webapp-old" "./bin/util/initialize_instance"
 initialize_exit_code=$?
 if [[ $initialize_exit_code -ne 0 ]]; then
-  echo "ERROR: Failed to initialize instance."
-  cleanup
-  exit $initialize_exit_code
+  error_and_cleanup "Failed to initialize instance." $initialize_exit_code
 fi
-echo ""
+gh_endgroup
 
 # Migrate up to the current commit and check if the database is in sync
-echo "Testing upgrade migrations on branch ${current_branch}"
-git checkout -q "${current_branch}"
-run_migrations "upgrade head"
-check_db "A new migration is required or a up migration is broken."
+gh_group "Testing upgrade migrations"
+run_migrations "webapp" "upgrade head"
+check_db "webapp" "A new migration is required or a up migration is broken."
+gh_endgroup
 
 # Migrate down to the first migration and check if the database is in sync
-echo "Testing downgrade migrations"
-run_migrations "downgrade ${first_migration_id}"
-git checkout -q "${first_migration_commit}"
-check_db "A down migration is broken."
+gh_group "Testing downgrade migrations"
+run_migrations "webapp" "downgrade ${first_migration_id}"
+check_db "webapp-old" "A down migration is broken."
+gh_endgroup
 
 # Migrate back up once more to make sure that the database is still in sync
-echo "Testing upgrade migrations a second time"
-git checkout -q "${current_branch}"
-run_migrations "upgrade head"
-check_db "A up migration is likely broken."
+gh_group "Testing upgrade migrations a second time"
+run_migrations "webapp" "upgrade head"
+check_db "webapp" "A up migration is likely broken."
+gh_endgroup
+
+echo ""
+success "All migrations are up to date 🎉"
 
+gh_group "Shutting down service containers"
 cleanup
+gh_endgroup
diff --git a/docker/ci/test_migrations.yml b/docker/ci/test_migrations.yml
new file mode 100644
index 0000000000..d4f1b8d7b4
--- /dev/null
+++ b/docker/ci/test_migrations.yml
@@ -0,0 +1,18 @@
+version: "3.9"
+
+services:
+  webapp-old:
+    image: "ghcr.io/thepalaceproject/circ-webapp:sha-54fa9ef"
+
+    environment:
+      SIMPLIFIED_PRODUCTION_DATABASE: "postgresql://palace:test@pg:5432/circ"
+      PALACE_SEARCH_URL: "http://os:9200"
+
+    depends_on:
+      pg:
+        condition: service_healthy
+      os:
+        condition: service_healthy
+
+    ports:
+      - "6500:80"

From ac03f43b0f6b8b5517e3f68f1a36dabbabcc4b22 Mon Sep 17 00:00:00 2001
From: Jonathan Green <jonathan@razzard.com>
Date: Fri, 19 Apr 2024 12:54:19 -0300
Subject: [PATCH 2/5] Add some better comments

---
 docker/ci/test_migrations.sh  | 22 +++++++++++++++++-----
 docker/ci/test_migrations.yml |  6 +++---
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/docker/ci/test_migrations.sh b/docker/ci/test_migrations.sh
index 321f4b67dc..c1c39c4312 100755
--- a/docker/ci/test_migrations.sh
+++ b/docker/ci/test_migrations.sh
@@ -36,44 +36,54 @@ GREEN='\033[1;32m'       # Green
 # Keeps track of whether we are in a group or not
 IN_GROUP=0
 
+# Functions to interact with GitHub Actions
+# https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions
 gh_command() {
   local COMMAND=$1
   local MESSAGE=${2:-""}
   echo "::${COMMAND}::${MESSAGE}"
 }
 
+# Create a group of log lines
+# https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#grouping-log-lines
 gh_group() {
   local MESSAGE=$1
   gh_command group "$MESSAGE"
   IN_GROUP=1
-#  set -x
 }
 
+# End a group of log lines
 gh_endgroup() {
   if [[ $IN_GROUP -eq 1 ]]; then
-#    set +x
     gh_command endgroup
     IN_GROUP=0
   fi
 }
 
+# Log an error message
+# Note: if this is called in a group, the group will be closed before the error message is logged.
 gh_error() {
   gh_endgroup
   local MESSAGE=$1
   gh_command error "$MESSAGE"
 }
 
+# Log a success message
+# Note: if this is called in a group, the group will be closed before the success message is logged.
 success() {
   gh_endgroup
   local MESSAGE=$1
   echo -e "${GREEN}Success:${RESET} ${MESSAGE}"
 }
 
+# Run a docker-compose command
 compose_cmd() {
   echo "++ docker compose $*" >&2
   docker compose -f docker-compose.yml -f docker/ci/test_migrations.yml --progress quiet "$@"
 }
 
+# Run a command in a particular container using docker-compose
+# The command is run in a bash shell with the palace virtualenv activated
 run_in_container()
 {
   local CONTAINER_NAME=$1
@@ -82,10 +92,12 @@ run_in_container()
   compose_cmd run --build --rm --no-deps "${CONTAINER_NAME}" /bin/bash -c "source env/bin/activate && $*"
 }
 
+# Cleanup any running containers
 cleanup() {
   compose_cmd down
 }
 
+# Cleanup any running containers and exit with an error message
 error_and_cleanup() {
   local MESSAGE=$1
   local EXIT_CODE=$2
@@ -94,6 +106,7 @@ error_and_cleanup() {
   exit "$EXIT_CODE"
 }
 
+# Run an alembic migration command in a container
 run_migrations() {
   local CONTAINER_NAME=$1
   local ALEMBIC_CMD=$2
@@ -104,6 +117,7 @@ run_migrations() {
   fi
 }
 
+# Check if the database is in sync with the model
 check_db() {
   local CONTAINER_NAME=$1
   local DETAILED_ERROR=$2
@@ -115,8 +129,7 @@ check_db() {
   success "Database is in sync."
 }
 
-# Find all the info we need about the first migration in the git history
-# including its id, the file it was added in, and the commit it was added in.
+# Find all the info we need about the first migration in the git history.
 gh_group "Finding first migration"
 first_migration_id=$(run_in_container "webapp" alembic history -r'base:base+1' -v | head -n 1 | cut -d ' ' -f2)
 if [[ -z $first_migration_id ]]; then
@@ -128,7 +141,6 @@ if [[ -z $first_migration_file ]]; then
   error_and_cleanup "Could not find first migration file." 1
 fi
 
-# Find the git commit where the first migration file was added
 first_migration_commit=$(git log --follow --format=%H --reverse "${first_migration_file}" | head -n 1)
 if [[ -z $first_migration_commit ]]; then
   error_and_cleanup "Could not find first migration commit hash." 1
diff --git a/docker/ci/test_migrations.yml b/docker/ci/test_migrations.yml
index d4f1b8d7b4..ee82dee2f2 100644
--- a/docker/ci/test_migrations.yml
+++ b/docker/ci/test_migrations.yml
@@ -1,5 +1,8 @@
 version: "3.9"
 
+# This docker-compose file is used to run the old webapp for testing purposes
+# see test_migrations.sh for more information.
+
 services:
   webapp-old:
     image: "ghcr.io/thepalaceproject/circ-webapp:sha-54fa9ef"
@@ -13,6 +16,3 @@ services:
         condition: service_healthy
       os:
         condition: service_healthy
-
-    ports:
-      - "6500:80"

From 923e8c9f2d880f6fbc488eee103cd737fac558dd Mon Sep 17 00:00:00 2001
From: Jonathan Green <jonathan@razzard.com>
Date: Fri, 19 Apr 2024 13:09:12 -0300
Subject: [PATCH 3/5] Make echoing to stderr more reliable

---
 docker/ci/test_migrations.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/docker/ci/test_migrations.sh b/docker/ci/test_migrations.sh
index c1c39c4312..4dc1a41b89 100755
--- a/docker/ci/test_migrations.sh
+++ b/docker/ci/test_migrations.sh
@@ -76,9 +76,13 @@ success() {
   echo -e "${GREEN}Success:${RESET} ${MESSAGE}"
 }
 
+stderr_echo() {
+  echo "$@" 1>&2
+}
+
 # Run a docker-compose command
 compose_cmd() {
-  echo "++ docker compose $*" >&2
+  stderr_echo "++ docker compose $*"
   docker compose -f docker-compose.yml -f docker/ci/test_migrations.yml --progress quiet "$@"
 }
 
@@ -88,7 +92,7 @@ run_in_container()
 {
   local CONTAINER_NAME=$1
   shift 1
-  echo "+ $*" >&2
+  stderr_echo "+ $*"
   compose_cmd run --build --rm --no-deps "${CONTAINER_NAME}" /bin/bash -c "source env/bin/activate && $*"
 }
 

From 9c5d5265df1306e51eca9212ab594bafe834a2fe Mon Sep 17 00:00:00 2001
From: Jonathan Green <jonathan@razzard.com>
Date: Fri, 19 Apr 2024 13:34:05 -0300
Subject: [PATCH 4/5] Don't use stderr since it gets out of sync

---
 docker/ci/test_migrations.sh | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/docker/ci/test_migrations.sh b/docker/ci/test_migrations.sh
index 4dc1a41b89..856868b6f3 100755
--- a/docker/ci/test_migrations.sh
+++ b/docker/ci/test_migrations.sh
@@ -36,6 +36,9 @@ GREEN='\033[1;32m'       # Green
 # Keeps track of whether we are in a group or not
 IN_GROUP=0
 
+# Allow a command to run without echoing its output
+DEBUG_ECHO_ENABLED=1
+
 # Functions to interact with GitHub Actions
 # https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions
 gh_command() {
@@ -76,13 +79,15 @@ success() {
   echo -e "${GREEN}Success:${RESET} ${MESSAGE}"
 }
 
-stderr_echo() {
-  echo "$@" 1>&2
+debug_echo() {
+  if [[ $DEBUG_ECHO_ENABLED -eq 1 ]]; then
+    echo "$@"
+  fi
 }
 
 # Run a docker-compose command
 compose_cmd() {
-  stderr_echo "++ docker compose $*"
+  debug_echo "++ docker compose $*"
   docker compose -f docker-compose.yml -f docker/ci/test_migrations.yml --progress quiet "$@"
 }
 
@@ -92,7 +97,7 @@ run_in_container()
 {
   local CONTAINER_NAME=$1
   shift 1
-  stderr_echo "+ $*"
+  debug_echo "+ $*"
   compose_cmd run --build --rm --no-deps "${CONTAINER_NAME}" /bin/bash -c "source env/bin/activate && $*"
 }
 
@@ -135,7 +140,11 @@ check_db() {
 
 # Find all the info we need about the first migration in the git history.
 gh_group "Finding first migration"
+run_in_container "webapp" alembic history -r'base:base+1' -v
+# Debug echo is disabled since we are capturing the output of the command
+DEBUG_ECHO_ENABLED=0
 first_migration_id=$(run_in_container "webapp" alembic history -r'base:base+1' -v | head -n 1 | cut -d ' ' -f2)
+DEBUG_ECHO_ENABLED=1
 if [[ -z $first_migration_id ]]; then
   error_and_cleanup "Could not find first migration id." 1
 fi

From db1347aa947919a4b8cd24e30b3a12356c606304 Mon Sep 17 00:00:00 2001
From: Jonathan Green <jonathan@razzard.com>
Date: Fri, 19 Apr 2024 14:40:53 -0300
Subject: [PATCH 5/5] Make sure shell escaping is correct & fix an.

---
 docker/ci/test_migrations.sh | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/docker/ci/test_migrations.sh b/docker/ci/test_migrations.sh
index 856868b6f3..408231fb4a 100755
--- a/docker/ci/test_migrations.sh
+++ b/docker/ci/test_migrations.sh
@@ -81,14 +81,16 @@ success() {
 
 debug_echo() {
   if [[ $DEBUG_ECHO_ENABLED -eq 1 ]]; then
-    echo "$@"
+    printf "%q " "$@"
+    printf "\n"
   fi
 }
 
 # Run a docker-compose command
 compose_cmd() {
-  debug_echo "++ docker compose $*"
-  docker compose -f docker-compose.yml -f docker/ci/test_migrations.yml --progress quiet "$@"
+  args=(docker compose -f docker-compose.yml -f docker/ci/test_migrations.yml --progress quiet)
+  debug_echo "++" "${args[@]}" "$@"
+  "${args[@]}" "$@"
 }
 
 # Run a command in a particular container using docker-compose
@@ -97,7 +99,7 @@ run_in_container()
 {
   local CONTAINER_NAME=$1
   shift 1
-  debug_echo "+ $*"
+  debug_echo "+" "$@"
   compose_cmd run --build --rm --no-deps "${CONTAINER_NAME}" /bin/bash -c "source env/bin/activate && $*"
 }
 
@@ -118,8 +120,8 @@ error_and_cleanup() {
 # Run an alembic migration command in a container
 run_migrations() {
   local CONTAINER_NAME=$1
-  local ALEMBIC_CMD=$2
-  run_in_container "${CONTAINER_NAME}" "alembic ${ALEMBIC_CMD}"
+  shift 1
+  run_in_container "${CONTAINER_NAME}" "alembic" "$@"
   exit_code=$?
   if [[ $exit_code -ne 0 ]]; then
     error_and_cleanup "Running database migrations failed." $exit_code
@@ -130,7 +132,7 @@ run_migrations() {
 check_db() {
   local CONTAINER_NAME=$1
   local DETAILED_ERROR=$2
-  run_in_container "${CONTAINER_NAME}" "alembic check"
+  run_in_container "${CONTAINER_NAME}" alembic check
   local exit_code=$?
   if [[ $exit_code -ne 0 ]]; then
     error_and_cleanup "Database is out of sync! ${DETAILED_ERROR}" $exit_code
@@ -190,20 +192,20 @@ gh_endgroup
 
 # Migrate up to the current commit and check if the database is in sync
 gh_group "Testing upgrade migrations"
-run_migrations "webapp" "upgrade head"
-check_db "webapp" "A new migration is required or a up migration is broken."
+run_migrations "webapp" upgrade head
+check_db "webapp" "A new migration is required or an up migration is broken."
 gh_endgroup
 
 # Migrate down to the first migration and check if the database is in sync
 gh_group "Testing downgrade migrations"
-run_migrations "webapp" "downgrade ${first_migration_id}"
+run_migrations "webapp" downgrade "${first_migration_id}"
 check_db "webapp-old" "A down migration is broken."
 gh_endgroup
 
 # Migrate back up once more to make sure that the database is still in sync
 gh_group "Testing upgrade migrations a second time"
-run_migrations "webapp" "upgrade head"
-check_db "webapp" "A up migration is likely broken."
+run_migrations "webapp" upgrade head
+check_db "webapp" "An up migration is likely broken."
 gh_endgroup
 
 echo ""