Migrate CI-basictests from jenkins-build-scripts to in-repo test/infra#5525
Migrate CI-basictests from jenkins-build-scripts to in-repo test/infra#5525renecannao wants to merge 22 commits intov3.0from
Conversation
Part of the CI migration from jenkins-build-scripts to in-repo test/infra/ infrastructure (#5522). Adds: - test/tap/groups/basictests/ group (benchmark, chuser, failover tests) - sysbench to docker-base Dockerfile for benchmark tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace hardcoded absolute paths with dynamically derived REPO_ROOT so these scripts work on any machine (CI runners, other developers). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Scripts are at test/tap/groups/<name>/pre-proxysql.bash, need 4 parent traversals to reach repo root, not 3. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code coverage should be opt-in, not default. Non-coverage builds fail with 'PROXYSQL GCOV DUMP' syntax error when WITHGCOV defaults to 1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The group's env.sh (setting TEST_PY_* flags) was only sourced on the host. The Docker test runner container didn't see these values, so env-isolated.bash defaults took over (all tests disabled). Now source group env.sh before env-isolated.bash inside the container so group-specific settings are preserved. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Update proxysql-tester.yml: benchmark script path uses $WORKSPACE - Remove all JENKINS_SCRIPTS_PATH references from proxysql-tester.py - Use TAP env vars in local-docker-benchmark.bash for host/port/password Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The in-repo infra uses hostgroup pairs like 1300/1301, but the legacy proxysql-tester.py benchmark validates against hostgroups 0 and 1. This setup hook copies the first hostgroup pair into 0/1 and remaps users and query rules accordingly. Benchmark and change_user tests now pass locally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The failover test had hardcoded 'infra-mysql57' hostnames and short aliases like 'mysql1' that don't resolve in the isolated Docker network. Now uses DEFAULT_MYSQL_INFRA env var to construct the correct DNS names (e.g. orc1.infra-mysql57, mysql1.infra-mysql57). All three basictests now pass locally: benchmark, change_user, failover. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSources group-specific TAP envs earlier, adds a basictests TAP group and setup hook, installs sysbench in test image, makes benchmark MySQL connection params env-driven, changes a coverage default, updates proxysql-tester to use WORKSPACE and alias backends, and replaces many hard-coded paths with a runtime-computed REPO_ROOT. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions / CI"
participant Runner as "run-tests-isolated (container)"
participant GroupEnv as "test/tap/groups/<TAP_GROUP>/env.sh"
participant EnvIso as "env-isolated.bash"
participant Infra as "test/infra/control (cluster_start/init)"
participant MySQL as "infra-mysql57 (Docker MySQL)"
participant Tester as "proxysql-tester.py"
GH->>Runner: start job / launch container
Runner->>GroupEnv: source group env (if present)
Runner->>EnvIso: source env defaults
Runner->>Infra: start ProxySQL cluster (cluster_start/init)
Infra->>MySQL: provision MySQL infra (docker-hoster, mysql containers)
Runner->>Tester: invoke proxysql-tester.py (uses DEFAULT_MYSQL_INFRA)
Tester->>MySQL: run benchmarks/failover tests
Tester->>Runner: return results / artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the CI-basictests workflow by internalizing its dependencies and configurations. By moving away from an external private repository and integrating test infrastructure directly into the main repository, the changes aim to enhance the maintainability, reliability, and transparency of the testing process. This migration standardizes test execution, simplifies environment setup, and ensures better compatibility across different test components. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the CI-basictests from the external jenkins-build-scripts repository to the in-repo test/infra infrastructure. The changes include adding a new basictests TAP group, removing hardcoded paths across numerous test scripts, and updating configurations to be more dynamic. The code is well-structured and achieves the goal of making the CI process more self-contained. I've identified a couple of minor areas for improvement regarding a hardcoded port and an opportunity to make a new script slightly more efficient. Overall, this is a solid refactoring effort.
| fo_stdout, fo_stderr = fop.communicate() | ||
| log.debug('Failover output is - {} / {}'.format(fo_stdout, fo_stderr)) | ||
| cf_cmd = '{} orchestrator-client -c topology -i mysql1'.format(orc_prefix) | ||
| cf_cmd = '{} orchestrator-client -c topology -i {}:3306'.format(orc_prefix, mysql1_alias) |
There was a problem hiding this comment.
The MySQL port 3306 is hardcoded here. It's better to use the TAP_MYSQLPORT environment variable to make the test more robust and configurable. This avoids potential issues if the backend MySQL port is changed in the test environment setup.
| cf_cmd = '{} orchestrator-client -c topology -i {}:3306'.format(orc_prefix, mysql1_alias) | |
| cf_cmd = '{} orchestrator-client -c topology -i {}:{}'.format(orc_prefix, mysql1_alias, os.environ.get('TAP_MYSQLPORT', '3306')) |
| ${MYSQL_CMD} -e "UPDATE mysql_users SET default_hostgroup = 0 WHERE default_hostgroup = ${WRITER_HG};" | ||
| ${MYSQL_CMD} -e "UPDATE mysql_query_rules SET destination_hostgroup = 0 WHERE destination_hostgroup = ${WRITER_HG};" | ||
| ${MYSQL_CMD} -e "UPDATE mysql_query_rules SET destination_hostgroup = 1 WHERE destination_hostgroup = ${READER_HG};" |
There was a problem hiding this comment.
For efficiency and readability, you can combine these three UPDATE statements into a single mysql command execution. This reduces the overhead of starting three separate docker exec processes.
| ${MYSQL_CMD} -e "UPDATE mysql_users SET default_hostgroup = 0 WHERE default_hostgroup = ${WRITER_HG};" | |
| ${MYSQL_CMD} -e "UPDATE mysql_query_rules SET destination_hostgroup = 0 WHERE destination_hostgroup = ${WRITER_HG};" | |
| ${MYSQL_CMD} -e "UPDATE mysql_query_rules SET destination_hostgroup = 1 WHERE destination_hostgroup = ${READER_HG};" | |
| ${MYSQL_CMD} -e " | |
| UPDATE mysql_users SET default_hostgroup = 0 WHERE default_hostgroup = ${WRITER_HG}; | |
| UPDATE mysql_query_rules SET destination_hostgroup = 0 WHERE destination_hostgroup = ${WRITER_HG}; | |
| UPDATE mysql_query_rules SET destination_hostgroup = 1 WHERE destination_hostgroup = ${READER_HG}; | |
| " |
| ${MYSQL_CMD} -e "LOAD MYSQL SERVERS TO RUNTIME;" | ||
| ${MYSQL_CMD} -e "LOAD MYSQL USERS TO RUNTIME;" | ||
| ${MYSQL_CMD} -e "LOAD MYSQL QUERY RULES TO RUNTIME;" |
There was a problem hiding this comment.
Similarly, these LOAD ... TO RUNTIME commands can be combined into a single mysql execution to improve performance and readability.
| ${MYSQL_CMD} -e "LOAD MYSQL SERVERS TO RUNTIME;" | |
| ${MYSQL_CMD} -e "LOAD MYSQL USERS TO RUNTIME;" | |
| ${MYSQL_CMD} -e "LOAD MYSQL QUERY RULES TO RUNTIME;" | |
| ${MYSQL_CMD} -e " | |
| LOAD MYSQL SERVERS TO RUNTIME; | |
| LOAD MYSQL USERS TO RUNTIME; | |
| LOAD MYSQL QUERY RULES TO RUNTIME; | |
| " |
There was a problem hiding this comment.
Pull request overview
Migrates the CI-basictests workflow away from the private proxysql/jenkins-build-scripts dependency and onto the in-repo test/infra/ runner + TAP group hooks, so benchmark/change_user/failover tests can run fully from this repository.
Changes:
- Added new TAP group
basictests(env + infra list + setup hook) to run the CI-basictests Python functional suite oninfra-mysql57. - Normalized TAP group
pre-proxysql.bashscripts by removing hardcoded developer paths and derivingREPO_ROOTdynamically. - Updated infra/test runner plumbing (Docker base image, isolated runner env sourcing, sysbench benchmark script, tester config/script) to support the migrated workflow.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/tap/groups/mysql93/pre-proxysql.bash | Replace hardcoded absolute paths with ${REPO_ROOT}-derived infra paths. |
| test/tap/groups/mysql93-gr/pre-proxysql.bash | Same ${REPO_ROOT} path normalization for mysql93-gr group. |
| test/tap/groups/mysql92/pre-proxysql.bash | Same ${REPO_ROOT} path normalization for mysql92 group. |
| test/tap/groups/mysql92-gr/pre-proxysql.bash | Same ${REPO_ROOT} path normalization for mysql92-gr group. |
| test/tap/groups/mysql91/pre-proxysql.bash | Same ${REPO_ROOT} path normalization for mysql91 group. |
| test/tap/groups/mysql91-gr/pre-proxysql.bash | Same ${REPO_ROOT} path normalization for mysql91-gr group. |
| test/tap/groups/mysql90/pre-proxysql.bash | Same ${REPO_ROOT} path normalization for mysql90 group. |
| test/tap/groups/mysql90-gr/pre-proxysql.bash | Same ${REPO_ROOT} path normalization for mysql90-gr group. |
| test/tap/groups/mysql84/pre-proxysql.bash | Same ${REPO_ROOT} path normalization; also updates cluster_start/init paths. |
| test/tap/groups/mysql84-gr/pre-proxysql.bash | Same ${REPO_ROOT} path normalization for mysql84-gr group. |
| test/tap/groups/legacy/pre-proxysql.bash | Switch legacy group cluster_start/init to ${REPO_ROOT} paths. |
| test/tap/groups/default/pre-proxysql.bash | Switch default group cluster_start/init to ${REPO_ROOT} paths with safer quoting. |
| test/tap/groups/basictests/setup-infras.bash | New group hook to alias first replication hostgroup pair to 0/1 for legacy basictests expectations. |
| test/tap/groups/basictests/infras.lst | Declares infra-mysql57 as the basictests backend infra. |
| test/tap/groups/basictests/env.sh | Defines basictests group environment and enables benchmark/chuser/failover flags. |
| test/scripts/etc/proxysql-tester.yml | Point benchmark script config to in-repo infra path under $WORKSPACE. |
| test/scripts/bin/proxysql-tester.py | Remove Jenkins path dependency and make failover orchestrator endpoints infra-aware. |
| test/infra/infra-mysql57/bin/local-docker-benchmark.bash | Use TAP_* env overrides for host/port/password instead of hardcoded defaults. |
| test/infra/docker-base/Dockerfile | Add sysbench to the CI base image to support benchmark tests. |
| test/infra/control/run-tests-isolated.bash | Source TAP group env.sh inside the runner container so group flags propagate. |
| test/infra/control/env-isolated.bash | Change WITHGCOV default to 0 (opt-in coverage). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| INSERT OR IGNORE INTO mysql_servers (hostgroup_id, hostname, port, max_connections, comment) | ||
| SELECT 0, hostname, port, max_connections, comment | ||
| FROM mysql_servers WHERE hostgroup_id = ${WRITER_HG}; | ||
| INSERT OR IGNORE INTO mysql_servers (hostgroup_id, hostname, port, max_connections, comment) | ||
| SELECT 1, hostname, port, max_connections, comment |
There was a problem hiding this comment.
The hostgroup aliasing copies mysql_servers rows into hostgroups 0/1 but only selects (hostname, port, max_connections, comment), dropping fields like max_replication_lag, weight, use_ssl, etc. If the source hostgroup rows set any non-default values (e.g. max_replication_lag is set in infra-mysql57), the aliased hostgroups won’t faithfully mirror the original behavior; consider copying all relevant columns from the source rows (or at least include max_replication_lag).
| INSERT OR IGNORE INTO mysql_servers (hostgroup_id, hostname, port, max_connections, comment) | |
| SELECT 0, hostname, port, max_connections, comment | |
| FROM mysql_servers WHERE hostgroup_id = ${WRITER_HG}; | |
| INSERT OR IGNORE INTO mysql_servers (hostgroup_id, hostname, port, max_connections, comment) | |
| SELECT 1, hostname, port, max_connections, comment | |
| INSERT OR IGNORE INTO mysql_servers (hostgroup_id, hostname, port, max_connections, max_replication_lag, comment) | |
| SELECT 0, hostname, port, max_connections, max_replication_lag, comment | |
| FROM mysql_servers WHERE hostgroup_id = ${WRITER_HG}; | |
| INSERT OR IGNORE INTO mysql_servers (hostgroup_id, hostname, port, max_connections, max_replication_lag, comment) | |
| SELECT 1, hostname, port, max_connections, max_replication_lag, comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/tap/groups/legacy/pre-proxysql.bash (1)
10-11: Quote the script paths for consistency with other TAP group scripts.The reference pattern in
test/tap/groups/default/pre-proxysql.bashuses quoted invocations ("${REPO_ROOT}/..."). Quoting handles edge cases with spaces in paths and maintains consistency across the codebase.Suggested fix
-${REPO_ROOT}/test/infra/control/cluster_start.bash -${REPO_ROOT}/test/infra/control/cluster_init.bash +"${REPO_ROOT}/test/infra/control/cluster_start.bash" +"${REPO_ROOT}/test/infra/control/cluster_init.bash"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/legacy/pre-proxysql.bash` around lines 10 - 11, Wrap the script invocations that reference ${REPO_ROOT} in quotes to match the pattern used elsewhere and guard against spaces in paths: update the two lines that call ${REPO_ROOT}/test/infra/control/cluster_start.bash and ${REPO_ROOT}/test/infra/control/cluster_init.bash so they use quoted expansions ("${REPO_ROOT}/test/infra/control/cluster_start.bash" and "${REPO_ROOT}/test/infra/control/cluster_init.bash"), preserving the same order and behavior.test/tap/groups/mysql84/pre-proxysql.bash (1)
16-16: Consider quoting command substitution to prevent word splitting.ShellCheck flags SC2046 here. While unlikely to cause issues in practice (paths rarely contain spaces), quoting the outer command substitution would be more robust:
🔧 Proposed fix
-INFRA=infra-$(basename $(dirname "$0") | sed 's/-g[0-9]//' | sed 's/_.*//') +INFRA=infra-$(basename "$(dirname "$0")" | sed 's/-g[0-9]//' | sed 's/_.*//')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/mysql84/pre-proxysql.bash` at line 16, The assignment to INFRA uses unquoted command substitution which can trigger word-splitting (SC2046); update the INFRA assignment so the entire RHS is wrapped in double quotes and inner command substitutions that use dirname and basename remain quoted (reference the INFRA variable assignment and the basename/dirname command substitutions) to ensure paths with spaces are preserved.test/tap/groups/basictests/setup-infras.bash (2)
25-26: Consider validating hostgroup IDs are numeric.
WRITER_HGandREADER_HGare parsed from database output and used directly in SQL statements. While the test environment is controlled, adding validation ensures robustness if unexpected data is returned.🛡️ Suggested validation
WRITER_HG=$(echo "${PAIR}" | awk '{print $1}') READER_HG=$(echo "${PAIR}" | awk '{print $2}') +# Validate hostgroup IDs are numeric +if ! [[ "${WRITER_HG}" =~ ^[0-9]+$ ]] || ! [[ "${READER_HG}" =~ ^[0-9]+$ ]]; then + echo ">>> ERROR: Invalid hostgroup IDs: writer=${WRITER_HG}, reader=${READER_HG}" + exit 1 +fi + if [[ "${WRITER_HG}" == "0" ]] && [[ "${READER_HG}" == "1" ]]; thenAlso applies to: 36-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/basictests/setup-infras.bash` around lines 25 - 26, WRITER_HG and READER_HG are taken from PAIR and used in SQL without checks; add a numeric validation step after the assignments (e.g., test each variable against a numeric regex like '^[0-9]+$' or use grep -E) and exit with an error if validation fails so invalid hostgroup IDs cannot be used; apply the same validation for the other parsing block referenced (lines 36-50) to ensure both writer and reader hostgroup IDs are numeric before they are used in SQL.
20-31: Use[[instead of[for bash conditional tests.Per static analysis and bash best practices,
[[is safer and more feature-rich than[for conditional tests in bash scripts.♻️ Suggested fix
-if [ -z "${PAIR}" ]; then +if [[ -z "${PAIR}" ]]; then echo ">>> No replication hostgroups found. Skipping hostgroup aliasing." exit 0 fi WRITER_HG=$(echo "${PAIR}" | awk '{print $1}') READER_HG=$(echo "${PAIR}" | awk '{print $2}') -if [ "${WRITER_HG}" = "0" ] && [ "${READER_HG}" = "1" ]; then +if [[ "${WRITER_HG}" == "0" ]] && [[ "${READER_HG}" == "1" ]]; then echo ">>> Hostgroups already 0/1. Skipping aliasing." exit 0 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/basictests/setup-infras.bash` around lines 20 - 31, Replace POSIX [ tests with bash [[ conditional expressions and ensure proper quoting: change testing of PAIR to use [[ -z "${PAIR}" ]] and replace the WRITER_HG/READER_HG comparisons to use [[ "${WRITER_HG}" == "0" && "${READER_HG}" == "1" ]] (or separate [[ ... ]] with &&) instead of [ ... ] and [ ... ]. Update the two conditional blocks that reference PAIR, WRITER_HG, and READER_HG to use [[ ... ]] so bash-specific features and safer string comparisons are used.test/tap/groups/basictests/env.sh (1)
1-3: Consider adding a shebang or shellcheck directive.Since this file is designed to be sourced (not executed directly), adding either a shebang (
#!/bin/bash) or a shellcheck directive (# shellcheck shell=bash) at the top would:
- Silence the SC2148 linter warning
- Document the expected shell for anyone reading or maintaining this file
📝 Suggested fix
+#!/bin/bash +# shellcheck shell=bash # Basictests Group Environment # Runs Python-based functional tests: sysbench benchmark, change-user, failover. # No ProxySQL cluster needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/basictests/env.sh` around lines 1 - 3, Add a top-of-file shell directive to silence SC2148 and document the intended shell for this sourced script: update env.sh to include either a shebang (#!/bin/bash) or a shellcheck directive (# shellcheck shell=bash) as the first non-empty line so linters and readers know the expected shell for the Basictests Group Environment script.test/scripts/bin/proxysql-tester.py (1)
1139-1147: Note: shell=True usage with environment-derived inputs.Static analysis flags these
subprocess.Popencalls withshell=Trueas a security concern (S602). In this testing context with controlled infrastructure, the risk is low sincemysql_infracomes from a trusted environment variable with a safe default.For production code this would warrant parameterized execution, but for test infrastructure this is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/scripts/bin/proxysql-tester.py` around lines 1139 - 1147, The subprocess.Popen calls constructing fo_cmd and cf_cmd use shell=True with environment-derived values (orc_prefix, mysql1_alias), which static analysis flags (S602); fix by invoking subprocess without a shell using argument lists (replace fo_cmd and cf_cmd strings with lists and call subprocess.Popen([...], shell=False)) or, if you must keep shell=True in this test-only script, explicitly validate/sanitize orc_prefix and mysql1_alias before use and add a clear comment justifying the trusted test-only source and suppress the linter (e.g., disable S602) for these lines; update the code paths around fo_cmd/fop and cf_cmd/cfp accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/groups/basictests/setup-infras.bash`:
- Around line 43-55: After inserting into mysql_replication_hostgroups (the
INSERT OR IGNORE statement that creates the 0/1 alias), add a runtime load for
replication hostgroups by invoking "LOAD MYSQL REPLICATION HOSTGROUPS TO
RUNTIME;" so ProxySQL picks up the new mysql_replication_hostgroups entry before
loading users and query rules; ensure the LOAD MYSQL REPLICATION HOSTGROUPS TO
RUNTIME call appears immediately after the INSERT and before the other LOAD
MYSQL ... TO RUNTIME calls.
---
Nitpick comments:
In `@test/scripts/bin/proxysql-tester.py`:
- Around line 1139-1147: The subprocess.Popen calls constructing fo_cmd and
cf_cmd use shell=True with environment-derived values (orc_prefix,
mysql1_alias), which static analysis flags (S602); fix by invoking subprocess
without a shell using argument lists (replace fo_cmd and cf_cmd strings with
lists and call subprocess.Popen([...], shell=False)) or, if you must keep
shell=True in this test-only script, explicitly validate/sanitize orc_prefix and
mysql1_alias before use and add a clear comment justifying the trusted test-only
source and suppress the linter (e.g., disable S602) for these lines; update the
code paths around fo_cmd/fop and cf_cmd/cfp accordingly.
In `@test/tap/groups/basictests/env.sh`:
- Around line 1-3: Add a top-of-file shell directive to silence SC2148 and
document the intended shell for this sourced script: update env.sh to include
either a shebang (#!/bin/bash) or a shellcheck directive (# shellcheck
shell=bash) as the first non-empty line so linters and readers know the expected
shell for the Basictests Group Environment script.
In `@test/tap/groups/basictests/setup-infras.bash`:
- Around line 25-26: WRITER_HG and READER_HG are taken from PAIR and used in SQL
without checks; add a numeric validation step after the assignments (e.g., test
each variable against a numeric regex like '^[0-9]+$' or use grep -E) and exit
with an error if validation fails so invalid hostgroup IDs cannot be used; apply
the same validation for the other parsing block referenced (lines 36-50) to
ensure both writer and reader hostgroup IDs are numeric before they are used in
SQL.
- Around line 20-31: Replace POSIX [ tests with bash [[ conditional expressions
and ensure proper quoting: change testing of PAIR to use [[ -z "${PAIR}" ]] and
replace the WRITER_HG/READER_HG comparisons to use [[ "${WRITER_HG}" == "0" &&
"${READER_HG}" == "1" ]] (or separate [[ ... ]] with &&) instead of [ ... ] and
[ ... ]. Update the two conditional blocks that reference PAIR, WRITER_HG, and
READER_HG to use [[ ... ]] so bash-specific features and safer string
comparisons are used.
In `@test/tap/groups/legacy/pre-proxysql.bash`:
- Around line 10-11: Wrap the script invocations that reference ${REPO_ROOT} in
quotes to match the pattern used elsewhere and guard against spaces in paths:
update the two lines that call
${REPO_ROOT}/test/infra/control/cluster_start.bash and
${REPO_ROOT}/test/infra/control/cluster_init.bash so they use quoted expansions
("${REPO_ROOT}/test/infra/control/cluster_start.bash" and
"${REPO_ROOT}/test/infra/control/cluster_init.bash"), preserving the same order
and behavior.
In `@test/tap/groups/mysql84/pre-proxysql.bash`:
- Line 16: The assignment to INFRA uses unquoted command substitution which can
trigger word-splitting (SC2046); update the INFRA assignment so the entire RHS
is wrapped in double quotes and inner command substitutions that use dirname and
basename remain quoted (reference the INFRA variable assignment and the
basename/dirname command substitutions) to ensure paths with spaces are
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9600043e-9676-4a5c-95f1-356d07b16e2d
📒 Files selected for processing (21)
test/infra/control/env-isolated.bashtest/infra/control/run-tests-isolated.bashtest/infra/docker-base/Dockerfiletest/infra/infra-mysql57/bin/local-docker-benchmark.bashtest/scripts/bin/proxysql-tester.pytest/scripts/etc/proxysql-tester.ymltest/tap/groups/basictests/env.shtest/tap/groups/basictests/infras.lsttest/tap/groups/basictests/setup-infras.bashtest/tap/groups/default/pre-proxysql.bashtest/tap/groups/legacy/pre-proxysql.bashtest/tap/groups/mysql84-gr/pre-proxysql.bashtest/tap/groups/mysql84/pre-proxysql.bashtest/tap/groups/mysql90-gr/pre-proxysql.bashtest/tap/groups/mysql90/pre-proxysql.bashtest/tap/groups/mysql91-gr/pre-proxysql.bashtest/tap/groups/mysql91/pre-proxysql.bashtest/tap/groups/mysql92-gr/pre-proxysql.bashtest/tap/groups/mysql92/pre-proxysql.bashtest/tap/groups/mysql93-gr/pre-proxysql.bashtest/tap/groups/mysql93/pre-proxysql.bash
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/tap/groups/legacy/pre-proxysql.bashtest/tap/groups/default/pre-proxysql.bashtest/tap/groups/mysql84/pre-proxysql.bashtest/infra/infra-mysql57/bin/local-docker-benchmark.bashtest/tap/groups/mysql90/pre-proxysql.bashtest/tap/groups/mysql92-gr/pre-proxysql.bashtest/tap/groups/mysql90-gr/pre-proxysql.bashtest/tap/groups/mysql91-gr/pre-proxysql.bashtest/tap/groups/mysql92/pre-proxysql.bashtest/infra/control/run-tests-isolated.bashtest/tap/groups/mysql91/pre-proxysql.bashtest/tap/groups/mysql93-gr/pre-proxysql.bashtest/tap/groups/basictests/env.shtest/scripts/bin/proxysql-tester.py
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
test/tap/groups/legacy/pre-proxysql.bashtest/infra/control/run-tests-isolated.bashtest/tap/groups/basictests/setup-infras.bashtest/tap/groups/basictests/env.shtest/scripts/bin/proxysql-tester.py
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Feature tiers are controlled by build flags: PROXYSQL31=1 for v3.1.x, PROXYSQLGENAI=1 for v4.0.x; PROXYSQLGENAI=1 implies PROXYSQL31=1
Applied to files:
test/infra/control/env-isolated.bash
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Build with NOJEMALLOC=1 WITHASAN=1 for AddressSanitizer testing; NOJEMALLOC=1 is required with WITHASAN=1
Applied to files:
test/infra/control/env-isolated.bash
🪛 GitHub Check: SonarCloud Code Analysis
test/tap/groups/basictests/setup-infras.bash
[failure] 28-28: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 28-28: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 20-20: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
🪛 Ruff (0.15.6)
test/scripts/bin/proxysql-tester.py
[error] 1140-1140: subprocess call with shell=True identified, security issue
(S602)
[error] 1147-1147: subprocess call with shell=True identified, security issue
(S602)
🪛 Shellcheck (0.11.0)
test/tap/groups/mysql84/pre-proxysql.bash
[warning] 16-16: Quote this to prevent word splitting.
(SC2046)
[warning] 46-46: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql90/pre-proxysql.bash
[warning] 42-42: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql93/pre-proxysql.bash
[warning] 42-42: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql92-gr/pre-proxysql.bash
[warning] 42-42: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql90-gr/pre-proxysql.bash
[warning] 42-42: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql91-gr/pre-proxysql.bash
[warning] 42-42: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql92/pre-proxysql.bash
[warning] 42-42: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql91/pre-proxysql.bash
[warning] 42-42: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql84-gr/pre-proxysql.bash
[warning] 42-42: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql93-gr/pre-proxysql.bash
[warning] 42-42: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/basictests/env.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🔇 Additional comments (21)
test/tap/groups/default/pre-proxysql.bash (1)
11-14: LGTM!The dynamic path computation is correctly implemented. Using
${BASH_SOURCE[0]}with proper directory traversal (4 levels up fromtest/tap/groups/default/) accurately resolves to the repository root. Variable expansions are properly quoted, following bash best practices.test/tap/groups/mysql91/pre-proxysql.bash (1)
4-4: LGTM — REPO_ROOT computation and path updates are correct.The dynamic computation of
REPO_ROOTusing${BASH_SOURCE[0]}and the four-level traversal (/../../../..) correctly resolves to the repository root fromtest/tap/groups/mysql91/. The updated paths for infra control scripts are now portable across environments.The ShellCheck SC1090 warning about non-constant source is expected here since
${INFRA}is computed at runtime; this is intentional design for multi-infra support.Also applies to: 15-15, 42-42, 45-46
test/tap/groups/mysql84/pre-proxysql.bash (1)
4-4: LGTM — REPO_ROOT and path updates are correct.The
REPO_ROOTcomputation and all path substitutions (including the cluster control scripts on lines 11-12) are correctly implemented.Also applies to: 11-12, 19-19, 46-46, 49-50
test/tap/groups/mysql93-gr/pre-proxysql.bash (1)
4-4: LGTM — Consistent with the REPO_ROOT refactoring pattern.The path changes correctly use
${REPO_ROOT}and align with the same refactoring applied to the otherpre-proxysql.bashscripts.Also applies to: 15-15, 42-42, 45-46
test/tap/groups/mysql93/pre-proxysql.bash (1)
4-4: LGTM — Follows the established REPO_ROOT pattern.Changes are consistent with the other
pre-proxysql.bashscripts in this PR.Also applies to: 15-15, 42-42, 45-46
test/tap/groups/mysql90-gr/pre-proxysql.bash (1)
4-4: LGTM — Correctly implements REPO_ROOT-based paths.The refactoring is consistent with the other scripts.
Also applies to: 15-15, 42-42, 45-46
test/tap/groups/mysql90/pre-proxysql.bash (1)
4-4: LGTM — Consistent REPO_ROOT refactoring.Also applies to: 15-15, 42-42, 45-46
test/tap/groups/mysql91-gr/pre-proxysql.bash (1)
4-4: LGTM — Matches the REPO_ROOT pattern across all pre-proxysql scripts.Also applies to: 15-15, 42-42, 45-46
test/infra/infra-mysql57/bin/local-docker-benchmark.bash (1)
12-14: LGTM — Environment-driven connection parameters improve flexibility.The change from hardcoded values to
${TAP_ROOTHOST:-127.0.0.1},${TAP_ROOTPORT:-6033}, and${TAP_ROOTPASSWORD:-root}allows the benchmark script to work in both local and isolated/containerized test environments. The fallback values match the defaults intest/infra/common/env.sh, ensuring backward compatibility when the environment variables are not explicitly set.test/infra/docker-base/Dockerfile (1)
27-27:sysbenchaddition is aligned with benchmark test requirements.This change cleanly enables benchmark tooling in the shared test-runner image.
test/tap/groups/mysql92/pre-proxysql.bash (1)
4-4: Path de-hardcoding is solid and improves portability.Using
REPO_ROOTconsistently for destroy/env/init hooks removes host-specific coupling and matches the migration goal.Also applies to: 15-15, 42-42, 45-46
test/tap/groups/mysql92-gr/pre-proxysql.bash (1)
4-4: Repo-relative infra paths look correct here as well.This keeps the group setup script environment-agnostic and aligned with in-repo infra controls.
Also applies to: 15-15, 42-42, 45-46
test/tap/groups/mysql84-gr/pre-proxysql.bash (1)
4-4: Good portability improvement viaREPO_ROOTpath migration.The updated destroy/env/init hook paths are consistent and fit the CI migration intent.
Also applies to: 15-15, 42-42, 45-46
test/tap/groups/basictests/infras.lst (1)
1-1: Infra list entry is correct for the new basictests group.
infra-mysql57aligns with the migrated CI target environment.test/infra/control/env-isolated.bash (1)
76-76:WITHGCOVopt-in default is implemented safely.Using
${WITHGCOV:-0}keeps explicit external/group overrides intact while making coverage default-off.test/infra/control/run-tests-isolated.bash (1)
322-333: Environment sourcing order fix is correct.Loading group
env.shbefore isolated defaults is the right precedence model forTEST_PY_*and infra-specific overrides.test/scripts/etc/proxysql-tester.yml (1)
3-3: Benchmark script path migration looks good.Switching to the
$WORKSPACEin-repo benchmark script removes the legacy external dependency as intended.test/scripts/bin/proxysql-tester.py (3)
809-809: LGTM!Good defensive change using
os.environ.get("WORKSPACE", ".")instead of direct dictionary access, which preventsKeyErrorifWORKSPACEis not set.
1132-1154: LGTM - failover DNS construction migrated correctly.The changes properly derive orchestrator API hosts and MySQL alias from
DEFAULT_MYSQL_INFRAenvironment variable, supporting both DNS mode and localhost mode appropriately.One minor observation: the verification on line 1154 loosened from
b"mysql2:3306"tob"mysql2". This is acceptable since the intent is to verify that either mysql2 or mysql3 took over as master, and a substring match suffices for this test.
1557-1557: LGTM!Good change to use
os.environ.get("WORKSPACE", ".")instead of the removedJENKINS_SCRIPTS_PATH, aligning with the PR's goal of removing jenkins-build-scripts dependencies.test/tap/groups/basictests/env.sh (1)
5-21: LGTM - well-structured group environment configuration.The environment variables are correctly set for the basictests group:
DEFAULT_MYSQL_INFRAaligns withinfras.lstSKIP_CLUSTER_START=1properly gates cluster initialization (verified via context snippets incluster_start.bashandcluster_init.bash)- Test selection flags enable only the relevant Python tests (benchmark, change_user, failover)
| INSERT OR IGNORE INTO mysql_replication_hostgroups (writer_hostgroup, reader_hostgroup, comment) | ||
| VALUES (0, 1, 'basictests alias'); | ||
| " | ||
|
|
||
| # Remap users, query rules, and replication hostgroups to 0/1 | ||
| ${MYSQL_CMD} -e "UPDATE mysql_users SET default_hostgroup = 0 WHERE default_hostgroup = ${WRITER_HG};" | ||
| ${MYSQL_CMD} -e "UPDATE mysql_query_rules SET destination_hostgroup = 0 WHERE destination_hostgroup = ${WRITER_HG};" | ||
| ${MYSQL_CMD} -e "UPDATE mysql_query_rules SET destination_hostgroup = 1 WHERE destination_hostgroup = ${READER_HG};" | ||
|
|
||
| # Load all changes to runtime | ||
| ${MYSQL_CMD} -e "LOAD MYSQL SERVERS TO RUNTIME;" | ||
| ${MYSQL_CMD} -e "LOAD MYSQL USERS TO RUNTIME;" | ||
| ${MYSQL_CMD} -e "LOAD MYSQL QUERY RULES TO RUNTIME;" |
There was a problem hiding this comment.
Missing LOAD MYSQL REPLICATION HOSTGROUPS TO RUNTIME after inserting the alias.
Line 44 inserts a new entry into mysql_replication_hostgroups but the corresponding LOAD command is missing. Without this, ProxySQL's runtime won't recognize the new replication hostgroup pair (0/1), which could affect monitoring and read/write split behavior for the aliased hostgroups.
🔧 Suggested fix
# Load all changes to runtime
${MYSQL_CMD} -e "LOAD MYSQL SERVERS TO RUNTIME;"
+${MYSQL_CMD} -e "LOAD MYSQL REPLICATION HOSTGROUPS TO RUNTIME;"
${MYSQL_CMD} -e "LOAD MYSQL USERS TO RUNTIME;"
${MYSQL_CMD} -e "LOAD MYSQL QUERY RULES TO RUNTIME;"📝 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.
| INSERT OR IGNORE INTO mysql_replication_hostgroups (writer_hostgroup, reader_hostgroup, comment) | |
| VALUES (0, 1, 'basictests alias'); | |
| " | |
| # Remap users, query rules, and replication hostgroups to 0/1 | |
| ${MYSQL_CMD} -e "UPDATE mysql_users SET default_hostgroup = 0 WHERE default_hostgroup = ${WRITER_HG};" | |
| ${MYSQL_CMD} -e "UPDATE mysql_query_rules SET destination_hostgroup = 0 WHERE destination_hostgroup = ${WRITER_HG};" | |
| ${MYSQL_CMD} -e "UPDATE mysql_query_rules SET destination_hostgroup = 1 WHERE destination_hostgroup = ${READER_HG};" | |
| # Load all changes to runtime | |
| ${MYSQL_CMD} -e "LOAD MYSQL SERVERS TO RUNTIME;" | |
| ${MYSQL_CMD} -e "LOAD MYSQL USERS TO RUNTIME;" | |
| ${MYSQL_CMD} -e "LOAD MYSQL QUERY RULES TO RUNTIME;" | |
| INSERT OR IGNORE INTO mysql_replication_hostgroups (writer_hostgroup, reader_hostgroup, comment) | |
| VALUES (0, 1, 'basictests alias'); | |
| " | |
| # Remap users, query rules, and replication hostgroups to 0/1 | |
| ${MYSQL_CMD} -e "UPDATE mysql_users SET default_hostgroup = 0 WHERE default_hostgroup = ${WRITER_HG};" | |
| ${MYSQL_CMD} -e "UPDATE mysql_query_rules SET destination_hostgroup = 0 WHERE destination_hostgroup = ${WRITER_HG};" | |
| ${MYSQL_CMD} -e "UPDATE mysql_query_rules SET destination_hostgroup = 1 WHERE destination_hostgroup = ${READER_HG};" | |
| # Load all changes to runtime | |
| ${MYSQL_CMD} -e "LOAD MYSQL SERVERS TO RUNTIME;" | |
| ${MYSQL_CMD} -e "LOAD MYSQL REPLICATION HOSTGROUPS TO RUNTIME;" | |
| ${MYSQL_CMD} -e "LOAD MYSQL USERS TO RUNTIME;" | |
| ${MYSQL_CMD} -e "LOAD MYSQL QUERY RULES TO RUNTIME;" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/groups/basictests/setup-infras.bash` around lines 43 - 55, After
inserting into mysql_replication_hostgroups (the INSERT OR IGNORE statement that
creates the 0/1 alias), add a runtime load for replication hostgroups by
invoking "LOAD MYSQL REPLICATION HOSTGROUPS TO RUNTIME;" so ProxySQL picks up
the new mysql_replication_hostgroups entry before loading users and query rules;
ensure the LOAD MYSQL REPLICATION HOSTGROUPS TO RUNTIME call appears immediately
after the INSERT and before the other LOAD MYSQL ... TO RUNTIME calls.
Two new CI workflows for the legacy-g2 TAP test group: - CI-legacy-g2: runs against ubuntu22-tap build (standard) - CI-legacy-g2-genai: runs against ubuntu24-tap-genai-gcov build (PROXYSQLGENAI=1, WITHGCOV=1, with coverage report upload) Both use SKIP_CLUSTER_START=1 and TAP_USE_NOISE=1. Reusable workflows are on the GH-Actions branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/CI-legacy-g2.yml:
- Line 16: Replace the mutable branch ref in the reusable workflow usage string
"uses: sysown/proxysql/.github/workflows/ci-legacy-g2.yml@GH-Actions" with a
full-length commit SHA from the referenced repository; locate the "uses:
sysown/proxysql/.github/workflows/ci-legacy-g2.yml@GH-Actions" occurrence, fetch
the exact commit SHA you want to pin (40 hex characters), and update the ref to
"uses: sysown/proxysql/.github/workflows/ci-legacy-g2.yml@<commit-sha>" so the
workflow is pinned to that immutable commit.
- Line 19: The workflow currently forwards the entire GitHub context via
"trigger: ${{ toJson(github) }}"; replace that with an allowlisted minimal
payload (e.g., a serialized object containing only github.repository,
github.ref, github.sha, github.event_name, github.workflow, and github.actor)
instead of the full toJson(github), and ensure you explicitly exclude any
sensitive fields (notably github.token or any secrets) by building the object
inline or as discrete inputs; update the use sites that consume "trigger" to
read the new minimal keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7379b93-5fc7-4f9d-86d9-607688d48f35
📒 Files selected for processing (2)
.github/workflows/CI-legacy-g2-genai.yml.github/workflows/CI-legacy-g2.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/CI-legacy-g2-genai.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
- GitHub Check: claude-review
|
|
||
| jobs: | ||
| run: | ||
| uses: sysown/proxysql/.github/workflows/ci-legacy-g2.yml@GH-Actions |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For GitHub Actions reusable workflows, what are the security/reproducibility recommendations for using branch refs vs commit SHAs in jobs.<job_id>.uses?
💡 Result:
For GitHub Actions reusable workflows, in jobs.<job_id>.uses, use commit SHAs instead of branch refs for optimal security and reproducibility. Security: Pinning to a full-length commit SHA provides immutability, preventing supply chain attacks where a bad actor pushes malicious code to a branch or moves a tag. Branches and tags can change, allowing potential backdoors without SHA collision. Official GitHub docs explicitly recommend SHA pinning for both actions and reusable workflows, stating it is the safest option. GitHub offers policies to enforce SHA pinning. Reproducibility: SHAs ensure the exact same workflow version is used every time, guaranteeing consistent behavior across runs. Branch refs fetch the latest commit on that branch, which may change between runs (e.g., new pushes), leading to non-deterministic outcomes. When re-running jobs with non-SHA refs, it may pull updated versions. Tags are better than branches but still mutable if repository access is compromised. Alternatives: Tags from verified creators are acceptable if SHA pinning is inconvenient, but less secure. For same-repo local references (./.github/workflows/file.yml), it uses the caller's commit, offering reproducibility without explicit SHA. Use Dependabot for updates. Example: jobs: my-job: uses: owner/repo/.github/workflows/reusable.yml@abc123def4567890abcdef1234567890abcdef12 # full SHA
Citations:
- 1: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions
- 2: https://docs.github.com/en/actions/reference/security/secure-use
- 3: https://docs.github.com/en/actions/concepts/workflows-and-actions/reusable-workflows
- 4: https://docs.github.com/en/actions/using-workflows/reusing-workflows
- 5: https://docs.github.com/en/actions/security-for-github-actions/security-guides/using-githubs-security-features-to-secure-your-use-of-github-actions
Pin the reusable workflow to a commit SHA.
Line 16 uses @GH-Actions (mutable branch ref). This allows the caller to execute different workflow code over time without any change in this repository, creating reproducibility and supply chain risks. GitHub's official security guidance recommends pinning to a full-length commit SHA as the safest option. Branch refs can drift and enable potential backdoors if the branch is moved or updated.
Suggested change
- uses: sysown/proxysql/.github/workflows/ci-legacy-g2.yml@GH-Actions
+ uses: sysown/proxysql/.github/workflows/ci-legacy-g2.yml@<commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/CI-legacy-g2.yml at line 16, Replace the mutable branch
ref in the reusable workflow usage string "uses:
sysown/proxysql/.github/workflows/ci-legacy-g2.yml@GH-Actions" with a
full-length commit SHA from the referenced repository; locate the "uses:
sysown/proxysql/.github/workflows/ci-legacy-g2.yml@GH-Actions" occurrence, fetch
the exact commit SHA you want to pin (40 hex characters), and update the ref to
"uses: sysown/proxysql/.github/workflows/ci-legacy-g2.yml@<commit-sha>" so the
workflow is pinned to that immutable commit.
| uses: sysown/proxysql/.github/workflows/ci-legacy-g2.yml@GH-Actions | ||
| secrets: inherit | ||
| with: | ||
| trigger: ${{ toJson(github) }} |
There was a problem hiding this comment.
Avoid passing the full github context as workflow input.
Line 19 forwards toJson(github), which is broader than needed and can carry sensitive/context-heavy data into downstream logs and processing. Pass an allowlisted, minimal payload instead.
What fields are included in the GitHub Actions `github` context, and are there cautions about exposing it (for example `github.token`) when serializing or logging?
Suggested change
with:
- trigger: ${{ toJson(github) }}
+ trigger: ${{ toJson(fromJSON(format(
+ '{{"event_name":"{0}","workflow":"{1}","ref_name":"{2}","sha":"{3}","run_id":"{4}"}}',
+ github.event_name,
+ github.workflow,
+ github.ref_name,
+ github.sha,
+ github.run_id
+ ))) }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/CI-legacy-g2.yml at line 19, The workflow currently
forwards the entire GitHub context via "trigger: ${{ toJson(github) }}"; replace
that with an allowlisted minimal payload (e.g., a serialized object containing
only github.repository, github.ref, github.sha, github.event_name,
github.workflow, and github.actor) instead of the full toJson(github), and
ensure you explicitly exclude any sensitive fields (notably github.token or any
secrets) by building the object inline or as discrete inputs; update the use
sites that consume "trigger" to read the new minimal keys.
Comprehensive documentation covering: - Two-tier architecture (caller + reusable workflows) - Execution flow and trigger cascade - Build matrix (debian12-dbg, ubuntu22-tap, ubuntu24-tap-genai-gcov) - Cache strategy and key naming - All 25 workflows with status, purpose, and dependencies - Third-party integration testing (10 workflows) - Test infrastructure pipeline (ensure-infras → run-tests → cleanup) - TAP group system and hooks - Guide for adding new test workflows Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/GH-Actions/README.md`:
- Around line 14-39: Three fenced code blocks in doc/GH-Actions/README.md are
missing language identifiers (MD040); add a language tag (e.g., text or yaml) to
the blocks containing the "Push/PR" ASCII diagram, the block starting with
"MATRIX_3P_AIOMYSQL_infradb_mysql" and the block containing "ensure-infras.bash"
so each opening ``` becomes ```text (or another appropriate language) to satisfy
markdownlint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5df0574-225b-4b0a-96f6-5318d2ac9672
📒 Files selected for processing (1)
doc/GH-Actions/README.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
🪛 LanguageTool
doc/GH-Actions/README.md
[uncategorized] ~65-~65: The official name of this software platform is spelled with a capital “H”.
Context: ...manual dispatch | | Paths ignored | .github/**, **.md | | Purpose | Entry po...
(GITHUB)
[grammar] ~158-~158: Ensure spelling is correct
Context: ... of legacy TAP tests using the standard ubuntu22 build. Noise injection is enabled to he...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~206-~206: Ensure spelling is correct
Context: ...` from jenkins-build-scripts. | ### CI-shuntest | | | |---|---| | Status | Broken (#...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~343-~343: The official name of this software platform is spelled with a capital “H”.
Context: ...d>: 1. **Reusable workflow** — Create .github/workflows/ci-.ymlon theGH-Act...
(GITHUB)
[uncategorized] ~345-~345: The official name of this software platform is spelled with a capital “H”.
Context: ...src). 2. **Caller workflow** — Create .github/workflows/CI-.yml` on the main br...
(GITHUB)
[uncategorized] ~371-~371: The official name of this software platform is spelled with a capital “H”.
Context: ... Where | |---|---| | Caller workflows | .github/workflows/CI-*.yml (main branch) | | R...
(GITHUB)
[uncategorized] ~372-~372: The official name of this software platform is spelled with a capital “H”.
Context: ... (main branch) | | Reusable workflows | .github/workflows/ci-*.yml (GH-Actions branc...
(GITHUB)
🪛 markdownlint-cli2 (0.21.0)
doc/GH-Actions/README.md
[warning] 14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 271-271: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 296-296: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| ``` | ||
| Push/PR | ||
| └─► CI-trigger (on: push/pull_request) | ||
| │ | ||
| ├─[in_progress]─► CI-builds (3 matrix builds, caches artifacts) | ||
| │ ├── debian12-dbg | ||
| │ ├── ubuntu22-tap | ||
| │ └── ubuntu24-tap-genai-gcov | ||
| │ | ||
| ├─[in_progress]─► CI-legacy-g2 (ubuntu22-tap cache) | ||
| ├─[in_progress]─► CI-legacy-g2-genai (ubuntu24-tap-genai-gcov cache) | ||
| │ | ||
| ├─[completed]──► CI-selftests (ubuntu22-tap cache) | ||
| ├─[completed]──► CI-basictests (ubuntu22-tap cache) | ||
| ├─[completed]──► CI-maketest (ubuntu22-tap cache) | ||
| ├─[completed]──► CI-taptests-groups (ubuntu22-tap cache) [BROKEN: #5521] | ||
| ├─[completed]──► CI-taptests (ubuntu22-tap cache) [BROKEN: #5521] | ||
| ├─[completed]──► CI-taptests-ssl (ubuntu22-tap cache) [BROKEN: #5521] | ||
| ├─[completed]──► CI-taptests-asan (ubuntu22-tap cache) [BROKEN: #5521] | ||
| ├─[completed]──► CI-repltests (ubuntu22-tap cache) [BROKEN: #5521] | ||
| ├─[completed]──► CI-shuntest (ubuntu22-tap cache) [BROKEN: #5521] | ||
| ├─[completed]──► CI-codeql | ||
| ├─[completed]──► CI-package-build | ||
| │ | ||
| └─[completed]──► CI-3p-* (10 third-party integration workflows) | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy markdownlint.
At Line 14, Line 271, and Line 296, fenced blocks are missing language tags (MD040). Please annotate them (for example text or yaml) so lint passes consistently.
📝 Suggested patch
-```
+```text
Push/PR
└─► CI-trigger (on: push/pull_request)
@@
-```
+```
-```
+```text
MATRIX_3P_AIOMYSQL_infradb_mysql # e.g. ["mysql57", "mysql84", "mysql91"]
MATRIX_3P_AIOMYSQL_connector_mysql # e.g. ["0.1.1", "0.2.0"]
MATRIX_3P_AIOMYSQL_infradb_mariadb # e.g. ["mariadb105", "mariadb115"]
MATRIX_3P_AIOMYSQL_connector_mariadb # e.g. ["0.1.1", "0.2.0"]
-```
+```
-```
+```text
ensure-infras.bash # Start ProxySQL + backends based on TAP group's infras.lst
├── start-proxysql-isolated.bash # ProxySQL in Docker container
@@
destroy-infras.bash # Cleanup backends
-```
+```Also applies to: 271-276, 296-310
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/GH-Actions/README.md` around lines 14 - 39, Three fenced code blocks in
doc/GH-Actions/README.md are missing language identifiers (MD040); add a
language tag (e.g., text or yaml) to the blocks containing the "Push/PR" ASCII
diagram, the block starting with "MATRIX_3P_AIOMYSQL_infradb_mysql" and the
block containing "ensure-infras.bash" so each opening ``` becomes ```text (or
another appropriate language) to satisfy markdownlint.
- Remove references to non-existent infra-default/docker-compose-destroy.bash
- Fix paths from test/infra/control/${INFRA}/ to test/infra/${INFRA}/
(infra directories live under test/infra/, not test/infra/control/)
Affects: mysql84, mysql84-gr, mysql90-mysql93, mysql90-gr-mysql93-gr
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unit tests in groups.json may reference binaries that haven't been compiled (they require a separate build step). Previously, not-found binaries were counted as test failures. Now they are skipped with a message, so the group passes if all compiled tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This reverts commit 75d3246.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/tap/groups/mysql91-gr/pre-proxysql.bash (1)
50-50:⚠️ Potential issue | 🔴 CriticalFix the sed pattern to properly strip test group suffix
-grwhen computing INFRA directory.The sed pattern
's/-g[0-9]//'only removes-gfollowed by a single digit, not the-grsuffix. Formysql91-gr, it computesINFRA=infra-mysql91-gr(non-existent), causing the script to fail at line 40 when sourcing the missing.envfile. This preventsWHGfrom being defined via the sourced environment.The pattern must also handle
-gr:INFRA=infra-$(basename $(dirname "$0") | sed 's/-g[0-9r]*$//' | sed 's/_.*//')Alternatively, create the missing infrastructure directories (
infra-mysql90,infra-mysql91,infra-mysql92,infra-mysql93) or configure these test groups to reuse existing infrastructure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/mysql91-gr/pre-proxysql.bash` at line 50, The INFRA computation currently uses a sed pattern that only removes "-g" plus a single digit and therefore leaves "-gr" in names like "mysql91-gr"; update the sed invocation that builds INFRA (the line setting INFRA using basename $(dirname "$0")) to strip either "-gr" or "-g<digits>" at the end and then remove any trailing underscore parts so INFRA becomes infra-<group> (e.g., infra-mysql91); in short, modify the sed expression that currently appears as 's/-g[0-9]//' to an anchored pattern that removes "-gr" or "-g" followed by one or more digits and then a separate sed to drop "_..." suffixes so the sourced .env path resolves and WHG gets defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/tap/groups/mysql91-gr/pre-proxysql.bash`:
- Line 50: The INFRA computation currently uses a sed pattern that only removes
"-g" plus a single digit and therefore leaves "-gr" in names like "mysql91-gr";
update the sed invocation that builds INFRA (the line setting INFRA using
basename $(dirname "$0")) to strip either "-gr" or "-g<digits>" at the end and
then remove any trailing underscore parts so INFRA becomes infra-<group> (e.g.,
infra-mysql91); in short, modify the sed expression that currently appears as
's/-g[0-9]//' to an anchored pattern that removes "-gr" or "-g" followed by one
or more digits and then a separate sed to drop "_..." suffixes so the sourced
.env path resolves and WHG gets defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 779a42af-aba2-43af-996e-142832ced7ed
📒 Files selected for processing (11)
test/infra/control/run-tests-isolated.bashtest/tap/groups/mysql84-gr/pre-proxysql.bashtest/tap/groups/mysql84/pre-proxysql.bashtest/tap/groups/mysql90-gr/pre-proxysql.bashtest/tap/groups/mysql90/pre-proxysql.bashtest/tap/groups/mysql91-gr/pre-proxysql.bashtest/tap/groups/mysql91/pre-proxysql.bashtest/tap/groups/mysql92-gr/pre-proxysql.bashtest/tap/groups/mysql92/pre-proxysql.bashtest/tap/groups/mysql93-gr/pre-proxysql.bashtest/tap/groups/mysql93/pre-proxysql.bash
🚧 Files skipped from review as they are similar to previous changes (1)
- test/infra/control/run-tests-isolated.bash
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/tap/groups/mysql84-gr/pre-proxysql.bashtest/tap/groups/mysql93-gr/pre-proxysql.bashtest/tap/groups/mysql90-gr/pre-proxysql.bashtest/tap/groups/mysql84/pre-proxysql.bashtest/tap/groups/mysql90/pre-proxysql.bashtest/tap/groups/mysql91-gr/pre-proxysql.bashtest/tap/groups/mysql91/pre-proxysql.bashtest/tap/groups/mysql92-gr/pre-proxysql.bashtest/tap/groups/mysql93/pre-proxysql.bash
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
test/tap/groups/mysql90-gr/pre-proxysql.bashtest/tap/groups/mysql90/pre-proxysql.bashtest/tap/groups/mysql92-gr/pre-proxysql.bash
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/groups/mysql84/pre-proxysql.bashtest/tap/groups/mysql92-gr/pre-proxysql.bash
🪛 Shellcheck (0.11.0)
test/tap/groups/mysql84-gr/pre-proxysql.bash
[warning] 40-40: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql93-gr/pre-proxysql.bash
[warning] 40-40: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql90-gr/pre-proxysql.bash
[warning] 40-40: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql84/pre-proxysql.bash
[warning] 44-44: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql90/pre-proxysql.bash
[warning] 40-40: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql91-gr/pre-proxysql.bash
[warning] 40-40: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql91/pre-proxysql.bash
[warning] 40-40: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql92-gr/pre-proxysql.bash
[warning] 40-40: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql92/pre-proxysql.bash
[warning] 40-40: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/tap/groups/mysql93/pre-proxysql.bash
[warning] 40-40: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
🔇 Additional comments (13)
test/tap/groups/mysql91/pre-proxysql.bash (1)
4-4: LGTM!The dynamic
REPO_ROOTcalculation correctly navigates 4 levels up fromtest/tap/groups/mysql91/to the repository root. The path updates for sourcing.envand invokingdocker-compose-init.bashare correct.The ShellCheck SC1090 warning on line 40 is expected since the source path is intentionally dynamic (depends on
$INFRA). This is acceptable; optionally, you could add# shellcheck source=/dev/nullabove the source line to suppress it.Also applies to: 40-40, 44-44
test/tap/groups/mysql93/pre-proxysql.bash (1)
4-4: LGTM!Changes are consistent with the pattern applied across other TAP group scripts. The
REPO_ROOTcalculation and path updates are correct.Also applies to: 40-40, 44-44
test/tap/groups/mysql90/pre-proxysql.bash (1)
4-4: LGTM!The
REPO_ROOTderivation and path updates follow the same correct pattern as the other MySQL group scripts.Also applies to: 40-40, 44-44
test/tap/groups/mysql92/pre-proxysql.bash (1)
4-4: LGTM!Consistent with the other MySQL group scripts. Path calculations are correct.
Also applies to: 40-40, 44-44
test/tap/groups/mysql84/pre-proxysql.bash (1)
4-4: LGTM!The
REPO_ROOTcalculation is correct, and the path updates properly reference:
test/infra/control/cluster_start.bashandcluster_init.bash(lines 11-12)test/infra/${INFRA}/.env(line 44)test/infra/${INFRA}/docker-compose-init.bash(line 48)The additional cluster start/init calls are appropriate for the mysql84 base infra group.
Also applies to: 11-12, 44-44, 48-48
test/tap/groups/mysql92-gr/pre-proxysql.bash (1)
4-4: LGTM!The
REPO_ROOTpath traversal correctly handles the-grsuffix in the directory name. Changes are consistent with the other TAP group scripts.Also applies to: 40-40, 44-44
test/tap/groups/mysql84-gr/pre-proxysql.bash (1)
4-4: LGTM!Path updates are correct and consistent. Note that unlike
mysql84/pre-proxysql.bash, the GR variant does not include cluster start/init calls, which is appropriate since Group Replication handles clustering differently.Also applies to: 40-40, 44-44
test/tap/groups/mysql93-gr/pre-proxysql.bash (1)
4-4: LGTM!Consistent with the other TAP group scripts. The
REPO_ROOTderivation and path updates are correct.Also applies to: 40-40, 44-44
test/tap/groups/mysql90-gr/pre-proxysql.bash (3)
50-50: Same WHG variable dependency as mysql91-gr.This script has the same reliance on
${WHG}being set in the execution environment. See the verification comment on the mysql91-gr script.
4-4: LGTM - Consistent REPO_ROOT derivation.Same correct pattern as other group scripts, navigating four directory levels up to reach the repository root.
40-44: LGTM - Paths correctly updated to use REPO_ROOT.The changes are consistent with the other
pre-proxysql.bashscripts in this PR. The ShellCheck SC1090 warning is expected for dynamic source paths.test/tap/groups/mysql91-gr/pre-proxysql.bash (2)
4-4: LGTM - Dynamic REPO_ROOT derivation.The path computation correctly navigates four directory levels up from
test/tap/groups/mysql91-gr/to reach the repository root. This approach properly replaces the previously hardcoded paths.
40-44: LGTM - Paths now use dynamic REPO_ROOT.The updated paths correctly reference the infra environment and init scripts via
${REPO_ROOT}. The ShellCheck SC1090 warning about non-constant source is expected here since${INFRA}is intentionally computed at runtime based on the test group name.
Unit tests failed to link because feature flags (PROXYSQLCLICKHOUSE, PROXYSQLFFTO, etc.) were only set from environment variables that may not be present during standalone `make` in test/tap/tests/unit/. Now auto-detects which features libproxysql.a was built with by checking for characteristic symbols (ClickHouse_Server, MySQLFFTO, GenAI_Thread, ProxySQL_TSDB). Also fixes ClickHouse→LZ4 link order. All 18 unit test binaries now compile and link successfully. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unit test binaries live in test/tap/tests/unit/ but TAP_WORKDIRS only included test/tap/tests/ and tests_with_deps/. Without this, proxysql-tester.py cannot find unit test binaries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Without this, `make build_tap_test_debug` calls `make` (default target) in test/tap/tests/unit/ instead of `make debug`. The debug target adds -DDEBUG which is needed to define __thread stubs like mysql_thread___session_debug that are guarded by #ifdef DEBUG. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…conflicts Previously the hook copied servers from 1300/1301 into 0/1, leaving both pairs active. The replication hostgroup monitor managed both pairs simultaneously, causing race conditions that removed servers from 0/1. Now moves servers in-place (UPDATE) and deletes the original 1300/1301 replication entry, so only one pair (0/1) exists for the monitor. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Additional debug_filters entries for PgSQL, Query_Processor, HostGroups_Manager, and other verbose modules.
Stop redirecting stderr to /dev/null and silently continuing on failure. If the config dump queries fail, we need to see the actual error and the test run should fail visibly, not silently proceed with a broken configuration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This test does 122-second CPU measurement waits, making it too slow for the legacy-g2 group which needs to complete in a reasonable time.
proxysql-tester.py only checked WITHGCOV to decide whether to run PROXYSQL GCOV DUMP. Since WITHGCOV now defaults to 0, passing COVERAGE=1 to run-multi-group.bash had no effect — the tester skipped coverage collection. Now checks both WITHGCOV and COVERAGE_MODE (which is passed into the container by run-tests-isolated.bash). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ensure-infras.bash: Move pre-proxysql hook (cluster setup) to run BEFORE backend infras start. Backends need ProxySQL and cluster to be fully ready because their docker-proxy-post.bash configures ProxySQL. Previous order was: ProxySQL → infras → cluster. New order: ProxySQL → cluster → infras (one by one) → setup hook. run-tests-isolated.bash: Add a readiness loop that waits up to 30s for ProxySQL admin port to be reachable from inside the test-runner container before running anything. Docker DNS resolution on newly created containers can take several seconds under load, causing "no route to host" (errno 113) errors on the first connection attempt. On failure, dumps DNS resolution diagnostics instead of silently proceeding. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Summary
Migrates the CI-basictests workflow from depending on the private
proxysql/jenkins-build-scriptsrepository to using the in-repotest/infra/infrastructure. Closes #5522.test/tap/groups/basictests/— runs benchmark (sysbench), change_user, and failover tests viaproxysql-tester.pyproxysql-tester.pyandproxysql-tester.ymlpre-proxysql.bashscripts (12 files) — now use dynamically derivedREPO_ROOTdocker-baseDockerfileDEFAULT_MYSQL_INFRAinstead of hardcoded valuesAll three basictests pass locally (benchmark, change_user, failover).
The companion workflow change (
ci-basictests.yml) was pushed directly to theGH-Actionsbranch.Test plan
ensure-infras.bash+run-tests-isolated.bash— all 3 tests passCI-basictestsworkflow on this branch after mergeSummary by CodeRabbit
New Features
Refactor
Tests
Documentation
Chores