CI: MySQL version matrix (5.7-9.6) and named channels tests#85
Merged
renecannao merged 4 commits intomasterfrom Apr 9, 2026
Merged
CI: MySQL version matrix (5.7-9.6) and named channels tests#85renecannao merged 4 commits intomasterfrom
renecannao merged 4 commits intomasterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSplit the functional CI into a build job plus separate MySQL and PostgreSQL test jobs (MySQL uses a version matrix), parameterized Docker Compose, added MySQL-version-aware replication helpers, introduced a named-channels functional test, tightened curl timeouts, and adjusted MySQL replica binlog config keys. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions"
participant Build as "Build Job (compile)"
participant Artifact as "Artifact Storage"
participant Test as "Test Job (MySQL matrix)"
participant Docker as "Docker Compose"
participant Orch as "orchestrator (binary)"
participant MySQL as "MySQL instances"
GH->>Build: trigger build
Build->>Artifact: upload bin/orchestrator
GH->>Test: start matrix job (per MYSQL_IMAGE)
Test->>Artifact: download bin/orchestrator
Test->>Docker: start services (uses MYSQL_IMAGE)
Docker->>MySQL: boot mysql1/mysql2/mysql3
Test->>Orch: start orchestrator (downloaded binary)
Orch->>MySQL: discover/query replication state
Test->>MySQL: run setup, named-channel SQL (version-aware)
Orch->>Test: expose API (/api/v2/channels, /api/instance-channels/...)
Test->>GH: upload logs/artifacts and exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4 tasks
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the functional testing suite by introducing MySQL version awareness. Key changes include parameterizing MySQL image versions in docker-compose.yml, adding helper functions in lib.sh to abstract version-specific replication commands (e.g., CHANGE MASTER TO vs. CHANGE REPLICATION SOURCE TO), and updating existing replication setup and failover scripts (setup-replication.sh, test-failover.sh) to utilize these new helpers. A new test script (test-named-channels.sh) is also added to verify multi-source replication channel discovery, specifically for MySQL 8.0+. Feedback suggests refactoring setup-replication.sh to avoid duplicating version detection logic already present in lib.sh, improving the mysql_source_host function for better code reusability, and removing unreachable code in test-named-channels.sh.
Comment on lines
+9
to
+22
| # Detect MySQL version | ||
| MYSQL_FULL_VERSION=$($COMPOSE exec -T mysql1 mysql -uroot -ptestpass -Nse "SELECT VERSION()" 2>/dev/null | tr -d '[:space:]') | ||
| MYSQL_MAJOR=$(echo "$MYSQL_FULL_VERSION" | sed -E 's/^([0-9]+\.[0-9]+).*/\1/') | ||
| echo "Detected MySQL version: $MYSQL_FULL_VERSION (major: $MYSQL_MAJOR)" | ||
|
|
||
| if [ "$MYSQL_MAJOR" = "5.7" ]; then | ||
| CHANGE_SOURCE_CMD="CHANGE MASTER TO MASTER_HOST='mysql1', MASTER_PORT=3306, MASTER_USER='repl', MASTER_PASSWORD='repl_pass', MASTER_AUTO_POSITION=1" | ||
| START_REPLICA_CMD="START SLAVE" | ||
| SHOW_REPLICA_CMD="SHOW SLAVE STATUS\G" | ||
| else | ||
| CHANGE_SOURCE_CMD="CHANGE REPLICATION SOURCE TO SOURCE_HOST='mysql1', SOURCE_PORT=3306, SOURCE_USER='repl', SOURCE_PASSWORD='repl_pass', SOURCE_AUTO_POSITION=1, GET_SOURCE_PUBLIC_KEY=1" | ||
| START_REPLICA_CMD="START REPLICA" | ||
| SHOW_REPLICA_CMD="SHOW REPLICA STATUS\G" | ||
| fi |
There was a problem hiding this comment.
This script duplicates the logic for detecting the MySQL version and constructing version-specific SQL commands, which is already implemented in the new helper functions in tests/functional/lib.sh.
To improve maintainability and avoid code duplication, you should source lib.sh at the top of the script (e.g., after cd ...) and then replace this block with calls to the helper functions.
For example:
# At the top of the script
source tests/functional/lib.sh
# ...
# Replace the version detection block with this:
CHANGE_SOURCE_CMD=$(mysql_change_source_sql mysql1 3306 repl repl_pass)
START_REPLICA_CMD=$(mysql_start_replica_sql)
if mysql_is_57; then
SHOW_REPLICA_CMD="SHOW SLAVE STATUS\G"
else
SHOW_REPLICA_CMD="SHOW REPLICA STATUS\G"
fi
Comment on lines
178
to
187
| mysql_source_host() { | ||
| local CONTAINER="$1" | ||
| docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ | ||
| mysql -uroot -ptestpass -Nse "SHOW REPLICA STATUS\G" 2>/dev/null | grep "Source_Host" | awk '{print $2}' | ||
| if mysql_is_57; then | ||
| docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ | ||
| mysql -uroot -ptestpass -Nse "SHOW SLAVE STATUS\G" 2>/dev/null | grep "Master_Host" | awk '{print $2}' | ||
| else | ||
| docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ | ||
| mysql -uroot -ptestpass -Nse "SHOW REPLICA STATUS\G" 2>/dev/null | grep "Source_Host" | awk '{print $2}' | ||
| fi | ||
| } |
There was a problem hiding this comment.
The mysql_source_host function contains duplicated code for handling MySQL 5.7 and newer versions. You can refactor this to be more DRY by using variables for the version-specific commands and fields.
Suggested change
| mysql_source_host() { | |
| local CONTAINER="$1" | |
| docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ | |
| mysql -uroot -ptestpass -Nse "SHOW REPLICA STATUS\G" 2>/dev/null | grep "Source_Host" | awk '{print $2}' | |
| if mysql_is_57; then | |
| docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ | |
| mysql -uroot -ptestpass -Nse "SHOW SLAVE STATUS\G" 2>/dev/null | grep "Master_Host" | awk '{print $2}' | |
| else | |
| docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ | |
| mysql -uroot -ptestpass -Nse "SHOW REPLICA STATUS\G" 2>/dev/null | grep "Source_Host" | awk '{print $2}' | |
| fi | |
| } | |
| mysql_source_host() { | |
| local CONTAINER="$1" | |
| local show_cmd grep_field | |
| if mysql_is_57; then | |
| show_cmd="SHOW SLAVE STATUS\G" | |
| grep_field="Master_Host" | |
| else | |
| show_cmd="SHOW REPLICA STATUS\G" | |
| grep_field="Source_Host" | |
| fi | |
| docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ | |
| mysql -uroot -ptestpass -Nse "$show_cmd" 2>/dev/null | grep "$grep_field" | awk '{print $2}' | |
| } |
Comment on lines
+109
to
+113
| if mysql_is_57; then | ||
| RESET_CHANNEL_SQL="STOP SLAVE FOR CHANNEL 'extra'; RESET SLAVE ALL FOR CHANNEL 'extra';" | ||
| else | ||
| RESET_CHANNEL_SQL="STOP REPLICA FOR CHANNEL 'extra'; RESET REPLICA ALL FOR CHANNEL 'extra';" | ||
| fi |
There was a problem hiding this comment.
This if/else block for setting RESET_CHANNEL_SQL is unreachable because the script exits at the beginning if the MySQL version is 5.7. You can remove this conditional and just keep the else part.
Suggested change
| if mysql_is_57; then | |
| RESET_CHANNEL_SQL="STOP SLAVE FOR CHANNEL 'extra'; RESET SLAVE ALL FOR CHANNEL 'extra';" | |
| else | |
| RESET_CHANNEL_SQL="STOP REPLICA FOR CHANNEL 'extra'; RESET REPLICA ALL FOR CHANNEL 'extra';" | |
| fi | |
| RESET_CHANNEL_SQL="STOP REPLICA FOR CHANNEL 'extra'; RESET REPLICA ALL FOR CHANNEL 'extra';" |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/functional/setup-replication.sh (1)
9-22: Version detection logic is duplicated fromlib.sh.This script duplicates the MySQL version detection and command generation logic that exists in
tests/functional/lib.sh(seemysql_version(),mysql_change_source_sql(),mysql_start_replica_sql()). Consider sourcinglib.shto reuse those helpers and reduce maintenance burden.That said, keeping this script self-contained may be intentional if it runs in contexts where
lib.shisn't available.♻️ Optional refactor to reuse lib.sh helpers
#!/bin/bash # Set up replication after all MySQL containers are running set -euo pipefail +cd "$(dirname "$0")/../.." +source tests/functional/lib.sh COMPOSE="docker compose -f tests/functional/docker-compose.yml" echo "Setting up replication..." -# Detect MySQL version -MYSQL_FULL_VERSION=$($COMPOSE exec -T mysql1 mysql -uroot -ptestpass -Nse "SELECT VERSION()" 2>/dev/null | tr -d '[:space:]') -MYSQL_MAJOR=$(echo "$MYSQL_FULL_VERSION" | sed -E 's/^([0-9]+\.[0-9]+).*/\1/') -echo "Detected MySQL version: $MYSQL_FULL_VERSION (major: $MYSQL_MAJOR)" - -if [ "$MYSQL_MAJOR" = "5.7" ]; then - CHANGE_SOURCE_CMD="CHANGE MASTER TO MASTER_HOST='mysql1', MASTER_PORT=3306, MASTER_USER='repl', MASTER_PASSWORD='repl_pass', MASTER_AUTO_POSITION=1" - START_REPLICA_CMD="START SLAVE" - SHOW_REPLICA_CMD="SHOW SLAVE STATUS\G" -else - CHANGE_SOURCE_CMD="CHANGE REPLICATION SOURCE TO SOURCE_HOST='mysql1', SOURCE_PORT=3306, SOURCE_USER='repl', SOURCE_PASSWORD='repl_pass', SOURCE_AUTO_POSITION=1, GET_SOURCE_PUBLIC_KEY=1" - START_REPLICA_CMD="START REPLICA" - SHOW_REPLICA_CMD="SHOW REPLICA STATUS\G" -fi +VERSION=$(mysql_version) +echo "Detected MySQL version: $VERSION" + +CHANGE_SOURCE_CMD=$(mysql_change_source_sql mysql1 3306 repl repl_pass | sed 's/;$//') +START_REPLICA_CMD=$(mysql_start_replica_sql | sed 's/;$//') +SHOW_REPLICA_CMD=$(if mysql_is_57; then echo "SHOW SLAVE STATUS\G"; else echo "SHOW REPLICA STATUS\G"; fi)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/setup-replication.sh` around lines 9 - 22, The MySQL version detection and command construction in this script duplicates logic from tests/functional/lib.sh (see mysql_version(), mysql_change_source_sql(), mysql_start_replica_sql()); update the script to source lib.sh at the top and replace the local variables/commands (MYSQL_FULL_VERSION, MYSQL_MAJOR, CHANGE_SOURCE_CMD, START_REPLICA_CMD, SHOW_REPLICA_CMD) with calls to the shared helpers (mysql_version() to set version, mysql_change_source_sql(), mysql_start_replica_sql(), and the equivalent for show status) so the script reuses the centralized implementations and avoids duplication.tests/functional/test-named-channels.sh (2)
4-4: Add error handling forcdfailure.Per Shellcheck SC2164, if
cdfails the script would continue from an unexpected directory.♻️ Proposed fix
-cd "$(dirname "$0")/../.." +cd "$(dirname "$0")/../.." || exit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/test-named-channels.sh` at line 4, The cd command in test-named-channels.sh can fail and the script would continue in the wrong directory; update the line with cd "$(dirname "$0")/../.." to handle failure by checking its return and aborting on error (e.g., append an OR-fail clause to print an explanatory error and exit nonzero) so the script stops if the directory change fails.
33-39: Consider using channel-specificSTART REPLICA FOR CHANNEL.
mysql_start_replica_sql()returns a genericSTART REPLICA;which starts all channels. While this works (since the default channel should already be running), using a channel-specific start would be more precise:# More explicit: start only the 'extra' channel if mysql_is_57; then START_SQL="START SLAVE FOR CHANNEL 'extra';" else START_SQL="START REPLICA FOR CHANNEL 'extra';" fiThis is a minor readability/precision suggestion; the current code is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/test-named-channels.sh` around lines 33 - 39, The current test uses a generic START_SQL from mysql_start_replica_sql() which runs START REPLICA for all channels; change the test to start only the named channel created by mysql_change_source_channel_sql (the 'extra' channel) by constructing START_SQL to use a channel-specific command: use "START SLAVE FOR CHANNEL 'extra';" when mysql_is_57 is true and "START REPLICA FOR CHANNEL 'extra';" otherwise, then pass that START_SQL into the heredoc where CHANGE_SQL and START_SQL are executed so only the 'extra' channel is started.tests/functional/lib.sh (1)
105-118: Consider adding error handling for failed version detection.If the Docker exec or MySQL query fails silently (due to
2>/dev/null),MYSQL_MAJOR_VERSIONremains empty and all subsequentmysql_is_57checks default to 8.0+ syntax. This could cause hard-to-diagnose failures when running against MySQL 5.7 with connectivity issues.🔧 Optional: Add validation for detected version
mysql_version() { if [ -n "$MYSQL_MAJOR_VERSION" ]; then echo "$MYSQL_MAJOR_VERSION" return fi local FULL FULL=$(docker compose -f tests/functional/docker-compose.yml exec -T mysql1 \ mysql -uroot -ptestpass -Nse "SELECT VERSION()" 2>/dev/null | tr -d '[:space:]') MYSQL_MAJOR_VERSION=$(echo "$FULL" | sed -E 's/^([0-9]+\.[0-9]+).*/\1/') + if [ -z "$MYSQL_MAJOR_VERSION" ]; then + echo "ERROR: Failed to detect MySQL version" >&2 + return 1 + fi echo "$MYSQL_MAJOR_VERSION" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/lib.sh` around lines 105 - 118, The mysql_version function currently silences failures and can return an empty MYSQL_MAJOR_VERSION, causing downstream checks like mysql_is_57 to mis-detect version; update mysql_version to detect errors from the docker compose exec/mysql command (check the command exit status and whether FULL is non-empty), capture stderr instead of discarding it, and when the query fails set MYSQL_MAJOR_VERSION to an explicit empty/invalid sentinel and print a clear error message (or exit non-zero) via >&2 so callers know detection failed; reference the MYSQL_MAJOR_VERSION variable and the mysql_version function when making these changes so callers can handle a failure case instead of assuming 8.0+.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/functional/test-named-channels.sh`:
- Around line 12-15: The script checks VERSION and calls skip and summary when
MySQL 5.7 is detected but does not stop execution, so the test continues; modify
the conditional block that tests VERSION (uses the VERSION variable and calls
skip and summary) to exit immediately after summary (e.g., call exit with a
success code) so the test harness stops further execution when skipping named
channels tests on MySQL 5.7.
---
Nitpick comments:
In `@tests/functional/lib.sh`:
- Around line 105-118: The mysql_version function currently silences failures
and can return an empty MYSQL_MAJOR_VERSION, causing downstream checks like
mysql_is_57 to mis-detect version; update mysql_version to detect errors from
the docker compose exec/mysql command (check the command exit status and whether
FULL is non-empty), capture stderr instead of discarding it, and when the query
fails set MYSQL_MAJOR_VERSION to an explicit empty/invalid sentinel and print a
clear error message (or exit non-zero) via >&2 so callers know detection failed;
reference the MYSQL_MAJOR_VERSION variable and the mysql_version function when
making these changes so callers can handle a failure case instead of assuming
8.0+.
In `@tests/functional/setup-replication.sh`:
- Around line 9-22: The MySQL version detection and command construction in this
script duplicates logic from tests/functional/lib.sh (see mysql_version(),
mysql_change_source_sql(), mysql_start_replica_sql()); update the script to
source lib.sh at the top and replace the local variables/commands
(MYSQL_FULL_VERSION, MYSQL_MAJOR, CHANGE_SOURCE_CMD, START_REPLICA_CMD,
SHOW_REPLICA_CMD) with calls to the shared helpers (mysql_version() to set
version, mysql_change_source_sql(), mysql_start_replica_sql(), and the
equivalent for show status) so the script reuses the centralized implementations
and avoids duplication.
In `@tests/functional/test-named-channels.sh`:
- Line 4: The cd command in test-named-channels.sh can fail and the script would
continue in the wrong directory; update the line with cd "$(dirname "$0")/../.."
to handle failure by checking its return and aborting on error (e.g., append an
OR-fail clause to print an explanatory error and exit nonzero) so the script
stops if the directory change fails.
- Around line 33-39: The current test uses a generic START_SQL from
mysql_start_replica_sql() which runs START REPLICA for all channels; change the
test to start only the named channel created by mysql_change_source_channel_sql
(the 'extra' channel) by constructing START_SQL to use a channel-specific
command: use "START SLAVE FOR CHANNEL 'extra';" when mysql_is_57 is true and
"START REPLICA FOR CHANNEL 'extra';" otherwise, then pass that START_SQL into
the heredoc where CHANGE_SQL and START_SQL are executed so only the 'extra'
channel is started.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9b218fd-dd8f-4e89-9023-2e1e849ca877
📒 Files selected for processing (9)
.github/workflows/functional.ymltests/functional/docker-compose.ymltests/functional/lib.shtests/functional/mysql/master.cnftests/functional/mysql/replica.cnftests/functional/mysql/replica2.cnftests/functional/setup-replication.shtests/functional/test-failover.shtests/functional/test-named-channels.sh
Comment on lines
+12
to
+15
| if [ "$VERSION" = "5.7" ]; then | ||
| skip "Named channels tests require MySQL 8.0+ (detected $VERSION)" | ||
| summary | ||
| fi |
There was a problem hiding this comment.
Missing exit after skipping on MySQL 5.7 — test will continue executing.
When MySQL 5.7 is detected, the script calls skip and summary but doesn't exit. Control flow continues to line 17 and beyond, causing the test to run against MySQL 5.7 anyway (and likely fail).
🐛 Proposed fix
if [ "$VERSION" = "5.7" ]; then
skip "Named channels tests require MySQL 8.0+ (detected $VERSION)"
summary
+ exit 0
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ "$VERSION" = "5.7" ]; then | |
| skip "Named channels tests require MySQL 8.0+ (detected $VERSION)" | |
| summary | |
| fi | |
| if [ "$VERSION" = "5.7" ]; then | |
| skip "Named channels tests require MySQL 8.0+ (detected $VERSION)" | |
| summary | |
| exit 0 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/functional/test-named-channels.sh` around lines 12 - 15, The script
checks VERSION and calls skip and summary when MySQL 5.7 is detected but does
not stop execution, so the test continues; modify the conditional block that
tests VERSION (uses the VERSION variable and calls skip and summary) to exit
immediately after summary (e.g., call exit with a success code) so the test
harness stops further execution when skipping named channels tests on MySQL 5.7.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/functional/test-named-channels.sh (1)
12-15:⚠️ Potential issue | 🔴 CriticalStop execution after the MySQL 5.7 skip path.
On Line [15], control falls through after
skip/summary, so the test continues and runs 8.0+ SQL later. Add an explicit successful exit.🐛 Proposed fix
if [ "$VERSION" = "5.7" ]; then skip "Named channels tests require MySQL 8.0+ (detected $VERSION)" summary + exit 0 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/test-named-channels.sh` around lines 12 - 15, The test script's skip branch for MySQL 5.7 uses skip and summary but falls through to run subsequent 8.0+ SQL; update the conditional that checks VERSION (the block containing skip "Named channels tests require MySQL 8.0+ (detected $VERSION)" and summary) to terminate early by adding an explicit successful exit (e.g., exit 0) immediately after summary so control does not continue to run 8.0+ tests when VERSION is 5.7.
🧹 Nitpick comments (1)
tests/functional/test-named-channels.sh (1)
42-43: Prefer polling with timeout over fixed sleeps to reduce CI flakiness.Lines [42], [53], and [69] use fixed delays. In matrix CI, readiness/discovery times vary and this can cause intermittent failures.
♻️ Refactor sketch
-# Verify the extra channel is running -sleep 3 -CHANNEL_STATUS=$($COMPOSE exec -T mysql3 mysql -uroot -ptestpass -Nse \ - "SELECT SERVICE_STATE FROM performance_schema.replication_connection_status WHERE CHANNEL_NAME='extra'" 2>/dev/null | tr -d '[:space:]') +# Verify the extra channel is running (poll up to 15s) +CHANNEL_STATUS="" +for _ in $(seq 1 15); do + CHANNEL_STATUS=$($COMPOSE exec -T mysql3 mysql -uroot -ptestpass -Nse \ + "SELECT SERVICE_STATE FROM performance_schema.replication_connection_status WHERE CHANNEL_NAME='extra'" 2>/dev/null | tr -d '[:space:]') + [ "$CHANNEL_STATUS" = "ON" ] && break + sleep 1 +doneAlso applies to: 53-54, 68-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/test-named-channels.sh` around lines 42 - 43, Replace fixed sleep calls with a timed polling loop that checks readiness and fails after a timeout: create a helper (e.g., wait_for_mysql_or_timeout) that repeatedly runs the same command used to set CHANNEL_STATUS ($COMPOSE exec -T mysql3 mysql -uroot -ptestpass -Nse ...) or another lightweight readiness query, sleeping a short interval (e.g., 1s) between attempts and aborting after a configurable timeout; then call that helper in place of the three plain sleep invocations so tests wait until the mysql container is actually ready instead of waiting a fixed amount of time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/functional/test-named-channels.sh`:
- Line 4: The unguarded cd "$(dirname "$0")/../.." can fail and leave the script
in the wrong directory; update the invocation that sets the working dir (the cd
call) to check its exit status and abort on failure (e.g., guard the cd with an
inline failure handler that logs an error and exits) so subsequent operations
like sourcing tests/functional/lib.sh run from the intended location; locate the
cd invocation in tests/functional/test-named-channels.sh and replace it with a
guarded variant that exits non‑zero and prints a clear error if the cd fails.
---
Duplicate comments:
In `@tests/functional/test-named-channels.sh`:
- Around line 12-15: The test script's skip branch for MySQL 5.7 uses skip and
summary but falls through to run subsequent 8.0+ SQL; update the conditional
that checks VERSION (the block containing skip "Named channels tests require
MySQL 8.0+ (detected $VERSION)" and summary) to terminate early by adding an
explicit successful exit (e.g., exit 0) immediately after summary so control
does not continue to run 8.0+ tests when VERSION is 5.7.
---
Nitpick comments:
In `@tests/functional/test-named-channels.sh`:
- Around line 42-43: Replace fixed sleep calls with a timed polling loop that
checks readiness and fails after a timeout: create a helper (e.g.,
wait_for_mysql_or_timeout) that repeatedly runs the same command used to set
CHANNEL_STATUS ($COMPOSE exec -T mysql3 mysql -uroot -ptestpass -Nse ...) or
another lightweight readiness query, sleeping a short interval (e.g., 1s)
between attempts and aborting after a configurable timeout; then call that
helper in place of the three plain sleep invocations so tests wait until the
mysql container is actually ready instead of waiting a fixed amount of time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec8b71d0-9198-4190-b897-eb546eee5b41
📒 Files selected for processing (3)
docs/README.mddocs/toc.mdtests/functional/test-named-channels.sh
✅ Files skipped from review due to trivial changes (2)
- docs/toc.md
- docs/README.md
| #!/bin/bash | ||
| # Named channels functional tests — verify multi-source replication channel discovery | ||
| set -uo pipefail # no -e: we handle failures ourselves | ||
| cd "$(dirname "$0")/../.." |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "test-named-channels.sh" -type fRepository: ProxySQL/orchestrator
Length of output: 106
🏁 Script executed:
head -20 "tests/functional/test-named-channels.sh"Repository: ProxySQL/orchestrator
Length of output: 702
🏁 Script executed:
cd tests/functional && head -20 lib.shRepository: ProxySQL/orchestrator
Length of output: 353
Guard cd failure to prevent silent breakage of relative paths.
The cd command on line 4 is unguarded. If it fails, the script continues executing in the original directory, causing the source tests/functional/lib.sh on line 5 to fail or source the wrong file. Although the script uses set -uo pipefail (intentionally without -e), the file demonstrates explicit error handling elsewhere (e.g., wait_for_orchestrator || { ... exit 1; }), establishing a pattern of guarding critical operations.
🛠️ Proposed fix
-cd "$(dirname "$0")/../.."
+cd "$(dirname "$0")/../.." || exit 1
source tests/functional/lib.sh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cd "$(dirname "$0")/../.." | |
| cd "$(dirname "$0")/../.." || exit 1 | |
| source tests/functional/lib.sh |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 4-4: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/functional/test-named-channels.sh` at line 4, The unguarded cd
"$(dirname "$0")/../.." can fail and leave the script in the wrong directory;
update the invocation that sets the working dir (the cd call) to check its exit
status and abort on failure (e.g., guard the cd with an inline failure handler
that logs an error and exits) so subsequent operations like sourcing
tests/functional/lib.sh run from the intended location; locate the cd invocation
in tests/functional/test-named-channels.sh and replace it with a guarded variant
that exits non‑zero and prints a clear error if the cd fails.
3 tasks
766f9d7 to
e81056f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/functional.yml (1)
107-117:⚠️ Potential issue | 🟠 MajorPropagate
MYSQL_IMAGEinto the MySQL test steps.The compose file is parameterized by
MYSQL_IMAGE(lines 5, 26, 50 ofdocker-compose.ymlreference${MYSQL_IMAGE:-mysql:8.4}), and the test scripts invokedocker composedirectly vialib.sh. The four test steps (lines 107–117) do not exportMYSQL_IMAGE, so compose will use the default fallback value regardless of the matrix entry, breaking version-specific testing.Proposed fix
- name: Run smoke tests + env: + MYSQL_IMAGE: mysql:${{ matrix.mysql_version }} run: bash tests/functional/test-smoke.sh - name: Run regression tests + env: + MYSQL_IMAGE: mysql:${{ matrix.mysql_version }} run: bash tests/functional/test-regression.sh - name: Run failover tests + env: + MYSQL_IMAGE: mysql:${{ matrix.mysql_version }} run: bash tests/functional/test-failover.sh - name: Run named channels tests + env: + MYSQL_IMAGE: mysql:${{ matrix.mysql_version }} run: bash tests/functional/test-named-channels.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/functional.yml around lines 107 - 117, The four workflow steps named "Run smoke tests", "Run regression tests", "Run failover tests", and "Run named channels tests" do not pass the MYSQL_IMAGE matrix value into the shell that runs tests, so docker compose falls back to the default image; update each of those steps to propagate the matrix value by setting the MYSQL_IMAGE environment variable for the step (e.g., add an env: MYSQL_IMAGE: ${{ matrix.MYSQL_IMAGE }} for each step) or export MYSQL_IMAGE=${{ matrix.MYSQL_IMAGE }} in the run command before invoking the test scripts (lib.sh -> docker compose) so the compose file picks up the intended image.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/functional/lib.sh`:
- Around line 177-186: The helper mysql_source_host() currently assumes
single-source replication and can return concatenated hosts for multi-channel
replicas; update it to be channel-aware by adding an optional channel parameter
(e.g., channel="$2") and parse the output of SHOW SLAVE/REPLICA STATUS\G to map
Channel (or use absence of Channel for MySQL 5.7) to its corresponding
Master_Host/Source_Host; if a channel is provided, return only that channel's
host, and if no channel is provided but multiple channels are present, fail fast
with a clear error and non-zero exit code so callers don't receive concatenated
values (ensure changes touch mysql_source_host and its usage contract).
In `@tests/functional/test-named-channels.sh`:
- Around line 105-124: The test currently jumps from API assertions to cleanup
and never triggers a failover/recovery to verify named-channel durability;
insert a failover/recovery phase before the cleanup block that triggers a master
failover (using the same docker/compose pattern as other tests, e.g. via
$COMPOSE exec -T mysqlX or orchestrator command), wait for replication to
re-establish, then query performance_schema.replication_connection_status to
assert the "extra" channel still exists and is in a running state (reuse the
CHANNEL_AFTER lookup pattern and the pass/fail helpers to assert COUNT(*) > 0
and appropriate STATUS), only then proceed to the RESET_CHANNEL_SQL cleanup and
summary.
- Around line 26-39: The bootstrap mysql commands can fail silently because
stderr is discarded and the script doesn’t check exit status; update the block
that runs the seed and channel-setup to fail fast by removing the 2>/dev/null
redirection (or capture stdout/stderr) and check the exit code after the COMPOSE
exec calls, failing the test if non-zero; specifically, when you run the seed
write and the channel-change/start using CHANGE_SQL, START_SQL and the helper
functions mysql_change_source_channel_sql and mysql_start_replica_sql on
mysql2/mysql3, ensure you either enable errexit (set -e) for the script or
append a post-command check like "|| { echo 'mysql command failed: <context>';
exit 1; }" so any rejected write or channel change aborts immediately.
---
Outside diff comments:
In @.github/workflows/functional.yml:
- Around line 107-117: The four workflow steps named "Run smoke tests", "Run
regression tests", "Run failover tests", and "Run named channels tests" do not
pass the MYSQL_IMAGE matrix value into the shell that runs tests, so docker
compose falls back to the default image; update each of those steps to propagate
the matrix value by setting the MYSQL_IMAGE environment variable for the step
(e.g., add an env: MYSQL_IMAGE: ${{ matrix.MYSQL_IMAGE }} for each step) or
export MYSQL_IMAGE=${{ matrix.MYSQL_IMAGE }} in the run command before invoking
the test scripts (lib.sh -> docker compose) so the compose file picks up the
intended image.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d21aa68-89b4-413c-84fa-228ebc2d3afd
📒 Files selected for processing (13)
.github/workflows/functional.ymldocs/README.mddocs/toc.mdtests/functional/docker-compose.ymltests/functional/lib.shtests/functional/mysql/master.cnftests/functional/mysql/replica.cnftests/functional/mysql/replica2.cnftests/functional/setup-replication.shtests/functional/test-failover.shtests/functional/test-named-channels.shtests/functional/test-postgresql.shtests/functional/test-smoke.sh
✅ Files skipped from review due to trivial changes (6)
- docs/README.md
- docs/toc.md
- tests/functional/mysql/replica2.cnf
- tests/functional/docker-compose.yml
- tests/functional/mysql/replica.cnf
- tests/functional/setup-replication.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/functional/mysql/master.cnf
- tests/functional/test-failover.sh
Comment on lines
177
to
+186
| # Get MySQL replication source | ||
| mysql_source_host() { | ||
| local CONTAINER="$1" | ||
| docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ | ||
| mysql -uroot -ptestpass -Nse "SHOW REPLICA STATUS\G" 2>/dev/null | grep "Source_Host" | awk '{print $2}' | ||
| if mysql_is_57; then | ||
| docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ | ||
| mysql -uroot -ptestpass -Nse "SHOW SLAVE STATUS\G" 2>/dev/null | grep "Master_Host" | awk '{print $2}' | ||
| else | ||
| docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ | ||
| mysql -uroot -ptestpass -Nse "SHOW REPLICA STATUS\G" 2>/dev/null | grep "Source_Host" | awk '{print $2}' | ||
| fi |
There was a problem hiding this comment.
mysql_source_host() still assumes single-source replication.
On a multi-channel replica, SHOW SLAVE/REPLICA STATUS\G returns one block per channel. This helper will therefore emit every Master_Host/Source_Host it finds, so callers get a concatenated value as soon as mysql3 has both its default channel and extra. Please make this helper channel-aware, or fail fast when multiple channels are present.
🐛 Proposed fix
mysql_source_host() {
- local CONTAINER="$1"
+ local CONTAINER="$1" CHANNEL="${2:-}"
if mysql_is_57; then
+ local STATUS_SQL="SHOW SLAVE STATUS\\G"
+ [ -n "$CHANNEL" ] && STATUS_SQL="SHOW SLAVE STATUS FOR CHANNEL '${CHANNEL}'\\G"
docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \
- mysql -uroot -ptestpass -Nse "SHOW SLAVE STATUS\G" 2>/dev/null | grep "Master_Host" | awk '{print $2}'
+ mysql -uroot -ptestpass -Nse "$STATUS_SQL" 2>/dev/null |
+ awk -F': ' '$1 == "Master_Host" { print $2; exit }'
else
+ local STATUS_SQL="SHOW REPLICA STATUS\\G"
+ [ -n "$CHANNEL" ] && STATUS_SQL="SHOW REPLICA STATUS FOR CHANNEL '${CHANNEL}'\\G"
docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \
- mysql -uroot -ptestpass -Nse "SHOW REPLICA STATUS\G" 2>/dev/null | grep "Source_Host" | awk '{print $2}'
+ mysql -uroot -ptestpass -Nse "$STATUS_SQL" 2>/dev/null |
+ awk -F': ' '$1 == "Source_Host" { print $2; exit }'
fi
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/functional/lib.sh` around lines 177 - 186, The helper
mysql_source_host() currently assumes single-source replication and can return
concatenated hosts for multi-channel replicas; update it to be channel-aware by
adding an optional channel parameter (e.g., channel="$2") and parse the output
of SHOW SLAVE/REPLICA STATUS\G to map Channel (or use absence of Channel for
MySQL 5.7) to its corresponding Master_Host/Source_Host; if a channel is
provided, return only that channel's host, and if no channel is provided but
multiple channels are present, fail fast with a clear error and non-zero exit
code so callers don't receive concatenated values (ensure changes touch
mysql_source_host and its usage contract).
Comment on lines
+26
to
+39
| $COMPOSE exec -T mysql2 mysql -uroot -ptestpass -e " | ||
| CREATE DATABASE IF NOT EXISTS extra_db; | ||
| CREATE TABLE IF NOT EXISTS extra_db.test (id INT PRIMARY KEY AUTO_INCREMENT, val VARCHAR(100)); | ||
| INSERT INTO extra_db.test (val) VALUES ('channel-test'); | ||
| " 2>/dev/null | ||
|
|
||
| # Add a named channel 'extra' on mysql3 replicating from mysql2 | ||
| CHANGE_SQL=$(mysql_change_source_channel_sql mysql2 3306 repl repl_pass extra) | ||
| START_SQL=$(mysql_start_replica_sql) | ||
|
|
||
| $COMPOSE exec -T mysql3 mysql -uroot -ptestpass -e " | ||
| $CHANGE_SQL | ||
| $START_SQL | ||
| " 2>/dev/null |
There was a problem hiding this comment.
Fail fast if the channel setup SQL fails.
With -e intentionally disabled, both of these bootstrap commands can fail silently and the test will keep going with a half-configured topology. That makes the later failures hard to interpret, especially if mysql2 rejects the seed write or mysql3 rejects the channel change.
🐛 Proposed fix
-$COMPOSE exec -T mysql2 mysql -uroot -ptestpass -e "
+if ! $COMPOSE exec -T mysql2 mysql -uroot -ptestpass -e "
CREATE DATABASE IF NOT EXISTS extra_db;
CREATE TABLE IF NOT EXISTS extra_db.test (id INT PRIMARY KEY AUTO_INCREMENT, val VARCHAR(100));
INSERT INTO extra_db.test (val) VALUES ('channel-test');
-" 2>/dev/null
+"; then
+ fail "Seed extra_db on mysql2"
+ summary
+fi
@@
-$COMPOSE exec -T mysql3 mysql -uroot -ptestpass -e "
+if ! $COMPOSE exec -T mysql3 mysql -uroot -ptestpass -e "
$CHANGE_SQL
$START_SQL
-" 2>/dev/null
+"; then
+ fail "Configure named channel 'extra' on mysql3"
+ summary
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $COMPOSE exec -T mysql2 mysql -uroot -ptestpass -e " | |
| CREATE DATABASE IF NOT EXISTS extra_db; | |
| CREATE TABLE IF NOT EXISTS extra_db.test (id INT PRIMARY KEY AUTO_INCREMENT, val VARCHAR(100)); | |
| INSERT INTO extra_db.test (val) VALUES ('channel-test'); | |
| " 2>/dev/null | |
| # Add a named channel 'extra' on mysql3 replicating from mysql2 | |
| CHANGE_SQL=$(mysql_change_source_channel_sql mysql2 3306 repl repl_pass extra) | |
| START_SQL=$(mysql_start_replica_sql) | |
| $COMPOSE exec -T mysql3 mysql -uroot -ptestpass -e " | |
| $CHANGE_SQL | |
| $START_SQL | |
| " 2>/dev/null | |
| if ! $COMPOSE exec -T mysql2 mysql -uroot -ptestpass -e " | |
| CREATE DATABASE IF NOT EXISTS extra_db; | |
| CREATE TABLE IF NOT EXISTS extra_db.test (id INT PRIMARY KEY AUTO_INCREMENT, val VARCHAR(100)); | |
| INSERT INTO extra_db.test (val) VALUES ('channel-test'); | |
| "; then | |
| fail "Seed extra_db on mysql2" | |
| summary | |
| fi | |
| # Add a named channel 'extra' on mysql3 replicating from mysql2 | |
| CHANGE_SQL=$(mysql_change_source_channel_sql mysql2 3306 repl repl_pass extra) | |
| START_SQL=$(mysql_start_replica_sql) | |
| if ! $COMPOSE exec -T mysql3 mysql -uroot -ptestpass -e " | |
| $CHANGE_SQL | |
| $START_SQL | |
| "; then | |
| fail "Configure named channel 'extra' on mysql3" | |
| summary | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/functional/test-named-channels.sh` around lines 26 - 39, The bootstrap
mysql commands can fail silently because stderr is discarded and the script
doesn’t check exit status; update the block that runs the seed and channel-setup
to fail fast by removing the 2>/dev/null redirection (or capture stdout/stderr)
and check the exit code after the COMPOSE exec calls, failing the test if
non-zero; specifically, when you run the seed write and the channel-change/start
using CHANGE_SQL, START_SQL and the helper functions
mysql_change_source_channel_sql and mysql_start_replica_sql on mysql2/mysql3,
ensure you either enable errexit (set -e) for the script or append a
post-command check like "|| { echo 'mysql command failed: <context>'; exit 1; }"
so any rejected write or channel change aborts immediately.
Comment on lines
+105
to
+124
| # ---------------------------------------------------------------- | ||
| echo "" | ||
| echo "--- Cleanup: Remove extra channel from mysql3 ---" | ||
|
|
||
| # Script already exits on MySQL 5.7, so only 8.0+ syntax needed here | ||
| RESET_CHANNEL_SQL="STOP REPLICA FOR CHANNEL 'extra'; RESET REPLICA ALL FOR CHANNEL 'extra';" | ||
|
|
||
| $COMPOSE exec -T mysql3 mysql -uroot -ptestpass -e "$RESET_CHANNEL_SQL" 2>/dev/null | ||
|
|
||
| # Verify channel removed | ||
| CHANNEL_AFTER=$($COMPOSE exec -T mysql3 mysql -uroot -ptestpass -Nse \ | ||
| "SELECT COUNT(*) FROM performance_schema.replication_connection_status WHERE CHANNEL_NAME='extra'" 2>/dev/null | tr -d '[:space:]') | ||
|
|
||
| if [ "$CHANNEL_AFTER" = "0" ]; then | ||
| pass "Named channel 'extra' cleaned up successfully" | ||
| else | ||
| fail "Named channel 'extra' still present after cleanup" | ||
| fi | ||
|
|
||
| summary |
There was a problem hiding this comment.
This test never exercises the failover path.
After the API assertions, the script goes straight to cleanup, so CI still does not cover the #81 acceptance case that a recovery preserves the other replication channel(s). Please add a failover/recovery phase before cleanup and assert that extra is still present and running afterwards.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/functional/test-named-channels.sh` around lines 105 - 124, The test
currently jumps from API assertions to cleanup and never triggers a
failover/recovery to verify named-channel durability; insert a failover/recovery
phase before the cleanup block that triggers a master failover (using the same
docker/compose pattern as other tests, e.g. via $COMPOSE exec -T mysqlX or
orchestrator command), wait for replication to re-establish, then query
performance_schema.replication_connection_status to assert the "extra" channel
still exists and is in a running state (reuse the CHANNEL_AFTER lookup pattern
and the pass/fail helpers to assert COUNT(*) > 0 and appropriate STATUS), only
then proceed to the RESET_CHANNEL_SQL cleanup and summary.
MySQL version matrix (Issue #80): - Parameterize docker-compose.yml with MYSQL_IMAGE env var (default mysql:8.4) - Use log-slave-updates in cnf files (compatible with all MySQL versions 5.7-9.x) - Make setup-replication.sh version-aware (CHANGE MASTER vs CHANGE REPLICATION SOURCE) - Add version helper functions to lib.sh (mysql_version, mysql_is_57, mysql_change_source_sql, mysql_start/stop/reset_replica_sql, etc.) - Update test-failover.sh restore section to use version-aware helpers - Split CI workflow into build + functional-mysql (7-version matrix) + functional-postgresql jobs with unique artifact names per matrix entry Named channels functional tests (Issue #81): - Create test-named-channels.sh testing multi-source replication discovery - Sets up mysql3 with a named channel 'extra' replicating from mysql2 - Tests channel API endpoints (/api/v2/channels, /api/instance-channels) - Verifies data replication through named channel - Gracefully skips on MySQL 5.7 (requires 8.0+) - Cleans up channel after tests
- Add named-channels.md to docs/toc.md and README.md to fix doc validation check - Remove unreachable MySQL 5.7 branch in channel cleanup code (script already exits on 5.7)
All curl calls in test scripts and lib.sh lacked --max-time, causing tests to hang indefinitely if any API endpoint was slow to respond. Added --max-time 10 to every curl call across all test scripts.
e81056f to
fa42679
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/functional/test-named-channels.sh (2)
26-39:⚠️ Potential issue | 🟠 MajorChannel setup commands can fail silently.
Both the
mysql2seed andmysql3channel configuration suppress stderr and don't check exit status. If either fails, the test continues with a broken topology, making later failures hard to diagnose.🐛 Proposed fix
-$COMPOSE exec -T mysql2 mysql -uroot -ptestpass -e " +if ! $COMPOSE exec -T mysql2 mysql -uroot -ptestpass -e " CREATE DATABASE IF NOT EXISTS extra_db; CREATE TABLE IF NOT EXISTS extra_db.test (id INT PRIMARY KEY AUTO_INCREMENT, val VARCHAR(100)); INSERT INTO extra_db.test (val) VALUES ('channel-test'); -" 2>/dev/null +" 2>/dev/null; then + fail "Failed to seed extra_db on mysql2" + summary +fi # Add a named channel 'extra' on mysql3 replicating from mysql2 CHANGE_SQL=$(mysql_change_source_channel_sql mysql2 3306 repl repl_pass extra) START_SQL=$(mysql_start_replica_sql) -$COMPOSE exec -T mysql3 mysql -uroot -ptestpass -e " +if ! $COMPOSE exec -T mysql3 mysql -uroot -ptestpass -e " $CHANGE_SQL $START_SQL -" 2>/dev/null +" 2>/dev/null; then + fail "Failed to configure named channel 'extra' on mysql3" + summary +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/test-named-channels.sh` around lines 26 - 39, The commands that configure the extra DB and the named channel are silencing errors and not checked; remove the stderr redirection and add explicit exit-status checks after the COMPOSE exec calls that run the mysql statements (the blocks that use $COMPOSE exec -T mysql2/mysql3 ... with $CHANGE_SQL and $START_SQL), or wrap them so failures abort the test (e.g., test the return code ($?) or enable set -e at top of the script); ensure the outputs of mysql_change_source_channel_sql and mysql_start_replica_sql (and the CREATE/INSERT block) are executed and any non-zero exit causes the test to fail with a clear error message.
4-4:⚠️ Potential issue | 🟡 MinorGuard
cdto prevent silent failure.If the
cdfails, the script continues in the wrong directory, causingsource tests/functional/lib.shto fail or load the wrong file.🛠️ Proposed fix
-cd "$(dirname "$0")/../.." +cd "$(dirname "$0")/../.." || { echo "Failed to cd to repo root"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/test-named-channels.sh` at line 4, The initial cd command (cd "$(dirname "$0")/../..") must be guarded so the script fails fast if it cannot change to the intended repo root; modify the test-named-channels.sh startup so the cd is checked and the script exits with a clear error if cd fails (e.g., test the return status and call exit 1 with an error message) before attempting to source tests/functional/lib.sh, ensuring the script won't continue in the wrong directory.
🧹 Nitpick comments (1)
tests/functional/setup-replication.sh (1)
9-22: Consider using shared helpers fromlib.shinstead of duplicating version detection.The version detection and SQL command construction here duplicates what
lib.shalready provides viamysql_version(),mysql_is_57(),mysql_change_source_sql(), andmysql_start_replica_sql(). This creates a maintenance burden—changes to replication syntax must be made in two places.♻️ Suggested refactor using lib.sh helpers
COMPOSE="docker compose -f tests/functional/docker-compose.yml" +# Source shared helpers +source "$(dirname "$0")/lib.sh" + echo "Setting up replication..." -# Detect MySQL version -MYSQL_FULL_VERSION=$($COMPOSE exec -T mysql1 mysql -uroot -ptestpass -Nse "SELECT VERSION()" 2>/dev/null | tr -d '[:space:]') -MYSQL_MAJOR=$(echo "$MYSQL_FULL_VERSION" | sed -E 's/^([0-9]+\.[0-9]+).*/\1/') -echo "Detected MySQL version: $MYSQL_FULL_VERSION (major: $MYSQL_MAJOR)" - -if [ "$MYSQL_MAJOR" = "5.7" ]; then - CHANGE_SOURCE_CMD="CHANGE MASTER TO MASTER_HOST='mysql1', MASTER_PORT=3306, MASTER_USER='repl', MASTER_PASSWORD='repl_pass', MASTER_AUTO_POSITION=1" - START_REPLICA_CMD="START SLAVE" - SHOW_REPLICA_CMD="SHOW SLAVE STATUS\G" -else - CHANGE_SOURCE_CMD="CHANGE REPLICATION SOURCE TO SOURCE_HOST='mysql1', SOURCE_PORT=3306, SOURCE_USER='repl', SOURCE_PASSWORD='repl_pass', SOURCE_AUTO_POSITION=1, GET_SOURCE_PUBLIC_KEY=1" - START_REPLICA_CMD="START REPLICA" - SHOW_REPLICA_CMD="SHOW REPLICA STATUS\G" -fi +echo "Detected MySQL version: $(mysql_version)" +CHANGE_SOURCE_CMD=$(mysql_change_source_sql mysql1 3306 repl repl_pass) +START_REPLICA_CMD=$(mysql_start_replica_sql) +if mysql_is_57; then + SHOW_REPLICA_CMD="SHOW SLAVE STATUS\G" +else + SHOW_REPLICA_CMD="SHOW REPLICA STATUS\G" +fiNote: You may want to add a
mysql_show_replica_status_sql()helper tolib.shfor completeness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/setup-replication.sh` around lines 9 - 22, Replace the duplicated MySQL version detection and SQL construction in setup-replication.sh with the shared helpers from lib.sh: call mysql_version() or mysql_is_57() to branch and use mysql_change_source_sql(), mysql_start_replica_sql(), and add/use a mysql_show_replica_status_sql() helper in lib.sh for the SHOW ... STATUS command; update setup-replication.sh to source lib.sh, invoke these helpers to set CHANGE_SOURCE_CMD, START_REPLICA_CMD, and SHOW_REPLICA_CMD (or use their returned strings directly), and add the new mysql_show_replica_status_sql() implementation to lib.sh if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/functional/test-named-channels.sh`:
- Around line 26-39: The commands that configure the extra DB and the named
channel are silencing errors and not checked; remove the stderr redirection and
add explicit exit-status checks after the COMPOSE exec calls that run the mysql
statements (the blocks that use $COMPOSE exec -T mysql2/mysql3 ... with
$CHANGE_SQL and $START_SQL), or wrap them so failures abort the test (e.g., test
the return code ($?) or enable set -e at top of the script); ensure the outputs
of mysql_change_source_channel_sql and mysql_start_replica_sql (and the
CREATE/INSERT block) are executed and any non-zero exit causes the test to fail
with a clear error message.
- Line 4: The initial cd command (cd "$(dirname "$0")/../..") must be guarded so
the script fails fast if it cannot change to the intended repo root; modify the
test-named-channels.sh startup so the cd is checked and the script exits with a
clear error if cd fails (e.g., test the return status and call exit 1 with an
error message) before attempting to source tests/functional/lib.sh, ensuring the
script won't continue in the wrong directory.
---
Nitpick comments:
In `@tests/functional/setup-replication.sh`:
- Around line 9-22: Replace the duplicated MySQL version detection and SQL
construction in setup-replication.sh with the shared helpers from lib.sh: call
mysql_version() or mysql_is_57() to branch and use mysql_change_source_sql(),
mysql_start_replica_sql(), and add/use a mysql_show_replica_status_sql() helper
in lib.sh for the SHOW ... STATUS command; update setup-replication.sh to source
lib.sh, invoke these helpers to set CHANGE_SOURCE_CMD, START_REPLICA_CMD, and
SHOW_REPLICA_CMD (or use their returned strings directly), and add the new
mysql_show_replica_status_sql() implementation to lib.sh if missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d5eb54d-7793-4d64-b29c-b77ba290fcde
📒 Files selected for processing (13)
.github/workflows/functional.ymldocs/README.mddocs/toc.mdtests/functional/docker-compose.ymltests/functional/lib.shtests/functional/mysql/master.cnftests/functional/mysql/replica.cnftests/functional/mysql/replica2.cnftests/functional/setup-replication.shtests/functional/test-failover.shtests/functional/test-named-channels.shtests/functional/test-postgresql.shtests/functional/test-smoke.sh
✅ Files skipped from review due to trivial changes (5)
- docs/README.md
- tests/functional/mysql/replica2.cnf
- docs/toc.md
- tests/functional/docker-compose.yml
- tests/functional/test-postgresql.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/functional/mysql/master.cnf
- tests/functional/mysql/replica.cnf
- tests/functional/test-smoke.sh
- tests/functional/test-failover.sh
- Use /api/v2/instances/{host}/{port}/channels (actual endpoint)
instead of nonexistent /api/v2/channels and /api/instance-channels
- Fix fail() function: use ${2:-} to avoid 'unbound variable' error
with set -u when called with 1 argument
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
docker-compose.ymlwithMYSQL_IMAGEenv varlog-slave-updatesworks on all versions)setup-replication.shversion-aware (5.7 vs 8.0+ SQL syntax)lib.sh(mysql_version,mysql_is_57,mysql_change_source_sql, etc.)test-failover.shrestore section version-awaretest-named-channels.shfor multi-source replication channel testingbuild,functional-mysql(7-version matrix), andfunctional-postgresqljobsCloses #80, closes #81
Test plan
Summary by CodeRabbit
Tests
Documentation
Chores