Skip to content

Conversation

@duanemay
Copy link
Member

@duanemay duanemay commented Sep 5, 2025

Also adds testing for 'postgresql-18'. Refactor the test scripts.

@duanemay duanemay marked this pull request as ready for review October 3, 2025 19:53
would have helped to identify an issue
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the database matrix to support newer MySQL versions (8.4, 9) and PostgreSQL 18, while removing MySQL 5 support. The changes involve refactoring test scripts into modular library functions and updating the GitHub Actions workflows to use the new database versions.

Key changes:

  • Updates database matrix to include MySQL 8.4, 9 and PostgreSQL 18, removes MySQL 5
  • Refactors monolithic test scripts into modular library functions for database, LDAP, and utility helpers
  • Updates GitHub Actions workflows to use new database versions and refactored scripts

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
.github/workflows/*.yml Updates CI matrix to use new database versions and refactored script names
run-*.sh Refactors main test runner scripts to use new modular library functions
scripts/lib_*.sh New modular library functions for database, LDAP, and utility operations
scripts/*_tests.sh New unified test scripts replacing old hyphenated versions
scripts/unit-tests* Removes old unit test scripts
scripts/integration-tests* Removes old integration test scripts
scripts/start_db_helper.sh Removes old database helper in favor of new modular approach
Test files Minor updates including keystore size increase and test assertion addition
Comments suppressed due to low confidence (2)

scripts/start_docker_database.sh:1

  • The hardcoded '8.4' version in the stop command should be dynamic based on the actual database version being used. Consider using a variable like '${DOCKER_IMAGE}' or '${PROFILE_NAME}' instead.
#!/bin/bash

scripts/lib_db_helper.sh:1

  • There's an extra space after '${db_password}' in the CREATE USER statement that will cause the password to include a trailing space, potentially breaking authentication.
#!/usr/bin/env bash

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

strehle
strehle previously approved these changes Oct 8, 2025
Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, in general we can go with this but only the debug is something you can think about

@github-project-automation github-project-automation bot moved this from Inbox to Pending Merge | Prioritized in Foundational Infrastructure Working Group Oct 8, 2025
@strehle strehle requested a review from Copilot October 8, 2025 14:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

scripts/lib_db_helper.sh:1

  • There's an extra space after '${db_password}' in the CREATE USER statement, which will result in a malformed password.
#!/usr/bin/env bash

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

./gradlew -Dspring.profiles.active=${PROFILE_NAME} test
./gradlew -Dspring.profiles.active=${PROFILE_NAME} integrationTest
To stop:
docker stop \$(docker ps | grep 8.4 | tail -n 1 | awk '{print \$1}')
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded '8.4' in the stop command should be dynamic based on the actual database being used, similar to how the start command uses variables.

Suggested change
docker stop \$(docker ps | grep 8.4 | tail -n 1 | awk '{print \$1}')
docker stop \$(docker ps | grep ${DOCKER_IMAGE} | tail -n 1 | awk '{print \$1}')

Copilot uses AI. Check for mistakes.
strehle
strehle previously approved these changes Oct 8, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@duanemay duanemay merged commit cf0aea7 into develop Oct 8, 2025
33 checks passed
@duanemay duanemay deleted the mysql-tests branch October 8, 2025 20:20
@github-project-automation github-project-automation bot moved this from Pending Merge | Prioritized to Done in Foundational Infrastructure Working Group Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants