Fix remaining templates for MySQL 8.4+ replication syntax#23
Fix remaining templates for MySQL 8.4+ replication syntax#23renecannao merged 2 commits intomasterfrom
Conversation
|
Warning Rate limit exceeded
⌛ 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 (4)
📝 WalkthroughWalkthroughThe pull request updates system dependencies in CI, reconfigures Go linter settings, and introduces version-dependent replication command selection across template and Go code. Error handling is added for command pipe creation, and conditional test skipping is implemented when MySQL binaries are unavailable. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 improves the sandbox environment's compatibility with modern MySQL versions, particularly 8.4 and newer, by addressing deprecated replication SQL syntax. It integrates dynamic version checks and conditional logic across various templates and the core sandbox creation process. This ensures that the correct and up-to-date replication commands and parameters are utilized based on the specific MySQL server version, thereby maintaining functional replication setups in sandboxes across diverse MySQL releases. Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request updates several templates to handle deprecated MySQL replication syntax, which is a valuable improvement. The approach of using version detection is solid. I've identified a critical bug in one of the shell scripts related to incorrect variable expansion, which would cause runtime failures. I've also suggested some refactoring in another template to improve maintainability by removing magic numbers and reducing code duplication. Please see the detailed comments for suggestions.
| echo "Connected to $master_path" >> $active_replication | ||
| echo "#!{{.ShellPath}}" > $remove_replication | ||
| echo "$SBDIR/use -v -e 'stop slave; reset slave'" >> $remove_replication | ||
| echo "$SBDIR/use -v -e '$stop_reset_cmd'" >> $remove_replication |
There was a problem hiding this comment.
There is a bug in this line. The variable $stop_reset_cmd is inside single quotes, which prevents it from being expanded by the shell. As a result, the literal string '$stop_reset_cmd' is written to the remove_replication script instead of its value. This will cause an error when the script is executed. To fix this, you should use double quotes around the variable's value to allow for expansion, while ensuring the SQL commands are passed as a single argument to -e.
echo "$SBDIR/use -v -e \"$stop_reset_cmd\"" >> $remove_replication
| $sandbox_dir/m -e 'SHOW MASTER STATUS' | ||
| sortable_version=$(echo $version | awk -F. '{printf "%03d%03d%03d", $1, $2, $3}') | ||
| show_master_cmd="SHOW MASTER STATUS" | ||
| if [[ "v$sortable_version" > "v008002000" ]]; then show_master_cmd="SHOW BINARY LOG STATUS"; fi |
There was a problem hiding this comment.
The version string "v008002000" is a magic number. It's better to define it as a variable with a descriptive name (e.g., minimum_version_binary_log) to improve readability and maintainability. You can define the variable before this line and use it in the comparison.
if [[ "v$sortable_version" > "v$minimum_version_binary_log" ]]; then show_master_cmd="SHOW BINARY LOG STATUS"; fi
| if [[ "v$sortable_version" > "v008000022" ]]; then | ||
| $sandbox_dir/use_all_slaves "STOP REPLICA; SET GLOBAL replica_parallel_workers=3; START REPLICA;show processlist " | ||
| else | ||
| $sandbox_dir/use_all_slaves "STOP SLAVE; SET GLOBAL slave_parallel_workers=3; START SLAVE;show processlist " | ||
| fi |
There was a problem hiding this comment.
This if/else block contains duplicated code. It can be refactored to be more DRY (Don't Repeat Yourself) by using variables for the parts that change. This improves readability and makes future changes easier. Additionally, the version string "v008000022" is a magic number and should be defined as a variable.
minimum_version_replica="008000022"
if [[ "v$sortable_version" > "v$minimum_version_replica" ]]; then
stop_cmd="STOP REPLICA"
start_cmd="START REPLICA"
parallel_workers_param="replica_parallel_workers"
else
stop_cmd="STOP SLAVE"
start_cmd="START SLAVE"
parallel_workers_param="slave_parallel_workers"
fi
$sandbox_dir/use_all_slaves "$stop_cmd; SET GLOBAL $parallel_workers_param=3; $start_cmd;show processlist "
There was a problem hiding this comment.
Code Review
This pull request updates several templates to support newer MySQL replication syntax, which is a valuable improvement. The changes correctly identify and handle version-specific commands. My review includes a few suggestions to enhance maintainability by removing magic numbers from shell scripts and eliminating duplicated logic in the Go code.
| $sandbox_dir/m -e 'SHOW MASTER STATUS' | ||
| sortable_version=$(echo $version | awk -F. '{printf "%03d%03d%03d", $1, $2, $3}') | ||
| show_master_cmd="SHOW MASTER STATUS" | ||
| if [[ "v$sortable_version" > "v008002000" ]]; then show_master_cmd="SHOW BINARY LOG STATUS"; fi |
There was a problem hiding this comment.
For better readability and maintainability, consider extracting the magic version string v008002000 into a variable with a descriptive name, such as minimum_version_binary_log. This approach is used in sandbox/templates/single/replicate_from.gotxt and makes the code easier to understand and update.
|
|
||
| header "Running a multiple query in all slaves" | ||
| (set -x | ||
| if [[ "v$sortable_version" > "v008000022" ]]; then |
There was a problem hiding this comment.
Similar to the previous comment, it would be better to extract the magic version string v008000022 into a variable (e.g., minimum_version_replica). This will improve code clarity and consistency with other scripts in the project.
sandbox/sandbox.go
Outdated
| "ChangeMasterTo": "CHANGE MASTER TO", | ||
| "MasterHostParam": "master_host", | ||
| "MasterPortParam": "master_port", | ||
| "MasterUserParam": "master_user", | ||
| "MasterPasswordParam": "master_password", | ||
| } | ||
|
|
||
| // Use version-appropriate replication syntax for connection info | ||
| useChangeSource, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumChangeReplicationSourceVersion) | ||
| if useChangeSource { | ||
| data["ChangeMasterTo"] = "CHANGE REPLICATION SOURCE TO" | ||
| data["MasterHostParam"] = "source_host" | ||
| data["MasterPortParam"] = "source_port" | ||
| data["MasterUserParam"] = "source_user" | ||
| data["MasterPasswordParam"] = "source_password" | ||
| } |
There was a problem hiding this comment.
The logic to determine version-appropriate replication syntax is duplicated here. The replicationCommands function in sandbox/replication.go already provides this functionality. To improve maintainability and reduce code duplication, you should refactor this to use replicationCommands.
You can call replicationCommands(sandboxDef.Version) and then populate the data map with the relevant values from the returned map. For example:
// This would replace lines 713-728
replCmds := replicationCommands(sandboxDef.Version)
data["ChangeMasterTo"] = replCmds["ChangeMasterTo"]
data["MasterHostParam"] = replCmds["MasterHostParam"]
data["MasterPortParam"] = replCmds["MasterPortParam"]
data["MasterUserParam"] = replCmds["MasterUserParam"]
data["MasterPasswordParam"] = replCmds["MasterPasswordParam"]There was a problem hiding this comment.
Pull request overview
Updates remaining sandbox/cookbook templates to use MySQL 8.4+/9.x replication syntax by selecting the correct SQL keywords/parameter names based on server version.
Changes:
- Add version-aware replication SQL in the single-sandbox
replicate_fromworkflow (SHOW/CHANGE/START/STOP variants). - Parameterize the
connection.sqltemplate so it can emit eitherCHANGE MASTER TOorCHANGE REPLICATION SOURCE TO. - Extend single-sandbox template data in Go to provide the new template variables.
- Update cookbook replication operations to use version-appropriate SHOW/STOP/START commands.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| sandbox/templates/single/replicate_from.gotxt | Adds runtime version detection for several replication commands/params. |
| sandbox/templates/single/connection_info_sql.gotxt | Switches to templated keyword/param names for replication connection SQL. |
| sandbox/sandbox.go | Populates new template variables for connection SQL generation. |
| cookbook/templates/replication-operations.gotxt | Updates cookbook commands to use version-aware replication syntax. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log_file_param="master_log_file" | ||
| log_pos_param="master_log_pos" | ||
| public_key_param="GET_MASTER_PUBLIC_KEY" | ||
| if [[ "v$slave_sortable_version" > "v$minimum_version_change_source" ]] |
There was a problem hiding this comment.
The switch to CHANGE REPLICATION SOURCE TO is gated by [[ ... > ... ]], so minimum_version_change_source="008000023" will only trigger at 8.0.24+. If the intent is 8.0.23+, include equality in the comparison or use arithmetic >= (e.g. with 10#...).
| if [[ "v$slave_sortable_version" > "v$minimum_version_change_source" ]] | |
| if (( 10#$slave_sortable_version >= 10#$minimum_version_change_source )) |
| {{.ChangeMasterTo}} {{.MasterHostParam}}="{{.SbHost}}", | ||
| {{.MasterPortParam}}={{.Port}}, | ||
| {{.MasterUserParam}}="{{.RplUser}}", | ||
| {{.MasterPasswordParam}}="{{.RplPassword}}" |
There was a problem hiding this comment.
connection.sql is consumed by replicate_from on the slave side, but this template now renders the statement keyword (CHANGE MASTER TO vs CHANGE REPLICATION SOURCE TO) and parameter names (master_* vs source_*) at sandbox generation time (i.e., based on the master sandbox’s version). For cross-version replication (slave newer than master), this can generate syntax that is invalid on the slave. Consider keeping this file version-neutral (values only) and letting replicate_from construct the correct statement for the slave version.
| (set -x | ||
| $sandbox_dir/m -e 'SHOW MASTER STATUS' | ||
| sortable_version=$(echo $version | awk -F. '{printf "%03d%03d%03d", $1, $2, $3}') | ||
| show_master_cmd="SHOW MASTER STATUS" | ||
| if [[ "v$sortable_version" > "v008002000" ]]; then show_master_cmd="SHOW BINARY LOG STATUS"; fi | ||
| $sandbox_dir/m -e "$show_master_cmd" |
There was a problem hiding this comment.
sortable_version is computed inside the first ( ... ) subshell, so it won’t be available in the later subshell at lines 61-65. That makes the if [[ "v$sortable_version" ... ]] check effectively compare an empty value and will pick the wrong branch on newer MySQL versions. Move the sortable_version=... computation outside the subshells (once, near the top) so it can be reused reliably.
|
|
||
| (set -x | ||
| $sandbox_dir/m -e 'SHOW MASTER STATUS' | ||
| sortable_version=$(echo $version | awk -F. '{printf "%03d%03d%03d", $1, $2, $3}') |
There was a problem hiding this comment.
The sortable_version construction assumes $version starts with numeric components. If the cookbook is invoked with versions that include a flavor prefix (e.g. ps8.0.36, ma10.6.1), awk will treat $1 as 0 and the version gating will be wrong. Consider stripping any non-numeric prefix before building sortable_version.
| sortable_version=$(echo $version | awk -F. '{printf "%03d%03d%03d", $1, $2, $3}') | |
| sortable_version=$(echo "$version" | sed 's/^[^0-9]*//' | awk -F. '{printf "%03d%03d%03d", $1, $2, $3}') |
| # MySQL 8.2+ renamed SHOW MASTER STATUS to SHOW BINARY LOG STATUS | ||
| show_master_status_cmd="show master status" | ||
| minimum_version_binary_log="008002000" | ||
| if [[ "v$master_sortable_version" > "v$minimum_version_binary_log" ]] |
There was a problem hiding this comment.
This version gate uses a strict string > comparison, which excludes the boundary version itself. With minimum_version_binary_log="008002000", MySQL 8.2.0 would still run SHOW MASTER STATUS even though the intent is “8.2+”. Consider using an arithmetic >= comparison (e.g. ((10#$master_sortable_version >= 10#$minimum_version_binary_log))) or explicitly including equality in the [[ ... ]] condition.
| if [[ "v$master_sortable_version" > "v$minimum_version_binary_log" ]] | |
| if ((10#$master_sortable_version >= 10#$minimum_version_binary_log)) |
| show_replica_cmd="SHOW SLAVE STATUS" | ||
| stop_reset_cmd="stop slave; reset slave" | ||
| minimum_version_replica="008000022" | ||
| if [[ "v$slave_sortable_version" > "v$minimum_version_replica" ]] |
There was a problem hiding this comment.
Similarly, this [[ ... > ... ]] gate means minimum_version_replica="008000022" won’t enable ... REPLICA commands on 8.0.22 (only 8.0.23+). If the rename applies starting with 8.0.22, include equality or switch to an arithmetic >= comparison.
| if [[ "v$slave_sortable_version" > "v$minimum_version_replica" ]] | |
| if [[ "v$slave_sortable_version" > "v$minimum_version_replica" ]] || [[ "v$slave_sortable_version" == "v$minimum_version_replica" ]] |
| if [[ "v$sortable_version" > "v008000022" ]]; then | ||
| $sandbox_dir/use_all_slaves "STOP REPLICA; SET GLOBAL replica_parallel_workers=3; START REPLICA;show processlist " | ||
| else | ||
| $sandbox_dir/use_all_slaves "STOP SLAVE; SET GLOBAL slave_parallel_workers=3; START SLAVE;show processlist " | ||
| fi |
There was a problem hiding this comment.
The version gates use a strict string > comparison, so boundary versions won’t switch (e.g. v008000022 will still take the SLAVE branch). If the intent is “8.0.22+”, include equality in the condition or use an arithmetic >= check (e.g. ((10#$sortable_version >= 10#008000022))).
sandbox/sandbox.go
Outdated
| } | ||
|
|
||
| // Use version-appropriate replication syntax for connection info | ||
| useChangeSource, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumChangeReplicationSourceVersion) |
There was a problem hiding this comment.
common.GreaterOrEqualVersion is marked DEPRECATED (see common/checks.go). Since verList is already computed earlier in this function, consider using common.GreaterOrEqualVersionList(verList, globals.MinimumChangeReplicationSourceVersion) (and handling the error) to avoid adding new uses of the deprecated helper.
| useChangeSource, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumChangeReplicationSourceVersion) | |
| useChangeSource, _ := common.GreaterOrEqualVersionList(verList, globals.MinimumChangeReplicationSourceVersion) |
| change_master_cmd="CHANGE MASTER TO" | ||
| auto_position_param="master_auto_position" | ||
| log_file_param="master_log_file" | ||
| log_pos_param="master_log_pos" | ||
| public_key_param="GET_MASTER_PUBLIC_KEY" | ||
| if [[ "v$slave_sortable_version" > "v$minimum_version_change_source" ]] | ||
| then | ||
| change_master_cmd="CHANGE REPLICATION SOURCE TO" |
There was a problem hiding this comment.
change_master_cmd is set here but never used later in the script. Given the surrounding refactor, this looks like incomplete logic and makes it easy to think the CHANGE ... keyword is being version-switched when it isn’t. Either remove the unused variable or use it when constructing the CHANGE ... statement.
| change_master_cmd="CHANGE MASTER TO" | |
| auto_position_param="master_auto_position" | |
| log_file_param="master_log_file" | |
| log_pos_param="master_log_pos" | |
| public_key_param="GET_MASTER_PUBLIC_KEY" | |
| if [[ "v$slave_sortable_version" > "v$minimum_version_change_source" ]] | |
| then | |
| change_master_cmd="CHANGE REPLICATION SOURCE TO" | |
| auto_position_param="master_auto_position" | |
| log_file_param="master_log_file" | |
| log_pos_param="master_log_pos" | |
| public_key_param="GET_MASTER_PUBLIC_KEY" | |
| if [[ "v$slave_sortable_version" > "v$minimum_version_change_source" ]] | |
| then |
| connection_string=$(cat $master_connection ; echo -n ", $auto_position_param=1") | ||
| else | ||
| connection_string=$(cat $master_connection ; echo -n ", $log_file_param=\"$binlog_file\", $log_pos_param=$binlog_pos" ) | ||
| if [ -f clone_replication.sql ] |
There was a problem hiding this comment.
connection_string is built by concatenating the contents of $master_connection (from the master sandbox) with additional options, but the resulting statement is executed on the slave. If master and slave versions differ, $master_connection may contain deprecated/removed syntax/parameter names for the slave (e.g., slave 8.4+). Consider building the full statement in this script based on slave_sortable_version rather than reusing connection.sql from the master.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mkwiki/make_docs.go (1)
42-50:⚠️ Potential issue | 🟠 MajorAdd
cmd.Wait()after reading stdout to release process resources and detect errors.The
StdoutPipe()contract requires callingWait()after reading completes. Without it, the child process becomes a zombie and its resources leak until the parent exits. Additionally, the error fromio.ReadAll()is silently ignored, and the manualstdout.Close()is redundant sinceWait()closes the pipe automatically.Suggested fix
stdout, err := cmd.StdoutPipe() if err != nil { common.Exitf(1, "# ERROR: %s", err) } if err = cmd.Start(); err != nil { common.Exitf(1, "# ERROR: %s", err) } -slurp, _ := io.ReadAll(stdout) -_ = stdout.Close() -return fmt.Sprintf("%s", slurp) +slurp, err := io.ReadAll(stdout) +if err != nil { + common.Exitf(1, "# ERROR: %s", err) +} +if err = cmd.Wait(); err != nil { + common.Exitf(1, "# ERROR: %s", err) +} +return string(slurp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mkwiki/make_docs.go` around lines 42 - 50, The code reads from the Stdout pipe but never calls cmd.Wait() and ignores the io.ReadAll error; update the sequence after cmd.StdoutPipe()/cmd.Start() in make_docs.go so that you read slurp, check and handle the error returned by io.ReadAll(stdout), then call cmd.Wait() to reap the child and capture any process error, and remove the explicit stdout.Close() because Wait() will close the pipe; refer to the stdout variable, io.ReadAll call, and cmd.Wait()/cmd.Start() when making the change.mkreadme/make_readme.go (1)
42-50:⚠️ Potential issue | 🟠 MajorAdd
cmd.Wait()call after reading stdout to handle exit status and release resources.
Start()initiates the command but does not return its exit status. After reading fromStdoutPipe()withio.ReadAll(), you must callWait()to obtain the exit status, release resources, and properly close the pipes. The current code ignores the command's exit status entirely, and the explicitstdout.Close()is redundant sinceWait()closes pipes automatically.Suggested fix
stdout, err := cmd.StdoutPipe() if err != nil { common.Exitf(1, "# ERROR: %s", err) } if err = cmd.Start(); err != nil { common.Exitf(1, "# ERROR: %s", err) } -slurp, _ := io.ReadAll(stdout) -_ = stdout.Close() -return fmt.Sprintf("%s", slurp) +slurp, err := io.ReadAll(stdout) +if err != nil { + common.Exitf(1, "# ERROR: %s", err) +} +if err = cmd.Wait(); err != nil { + common.Exitf(1, "# ERROR: %s", err) +} +return string(slurp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mkreadme/make_readme.go` around lines 42 - 50, After reading from the command's StdoutPipe (variables: cmd, stdout, slurp), call cmd.Wait() and handle its error to obtain the process exit status and release resources; remove the explicit stdout.Close() (Wait() closes pipes automatically) and if cmd.Wait() returns an error, propagate or Exitf with that error (similar to existing error handling for cmd.Start()) so the command's failure isn't ignored.
🧹 Nitpick comments (1)
.github/workflows/all_tests.yml (1)
42-46: Pin Linux runner version to ensure stable package resolution.The workflow installs Ubuntu 24.04-specific packages like
libaio1t64(line 46) but uses floatingubuntu-latestlabels across jobs (lines 8, 34, 52). When the runner image rolls forward, CI will break again. Pin toubuntu-24.04to make dependency resolution deterministic.Suggested changes
lint: name: Lint - runs-on: ubuntu-latest + runs-on: ubuntu-24.04 steps: test: strategy: matrix: go-version: ['1.22', '1.23'] - os: [ubuntu-latest, macos-latest] + os: [ubuntu-24.04, macos-latest] build: name: Build - runs-on: ubuntu-latest + runs-on: ubuntu-24.04🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/all_tests.yml around lines 42 - 46, The workflow currently uses floating runner labels like "ubuntu-latest" (seen across jobs that include the "Install libraries (Linux)" step and other jobs referencing runner images), causing package mismatches for Ubuntu 24.04-only packages; update all runs-on entries that use "ubuntu-latest" to the specific "ubuntu-24.04" runner to pin the environment and ensure deterministic apt package resolution (search for runs-on: ubuntu-latest in the workflow and replace with runs-on: ubuntu-24.04, including the job containing the "Install libraries (Linux)" step).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 16-26: The current golangci-lint config uses nonstandard
gosec.excludes and a repo-wide errcheck exclusion; replace the gosec
suppressions and the errcheck "lock.Unlock" exclusion by moving them under
issues.exclude-rules with explicit linters: [gosec] and linters: [errcheck]
respectively, and add path fields to scope each rule (e.g., restrict G115|G112
to test or specific directories and restrict "lock.Unlock" to the known
file/path that needs it) so the suppressions are properly recognized and not
applied repo-wide.
In `@cookbook/templates/replication-operations.gotxt`:
- Around line 67-74: The code switches to START/STOP REPLICA for MySQL >= 8.0.22
but unconditionally tries to SET GLOBAL replica_parallel_workers (which exists
only from 8.0.26), causing failures on 8.0.22–8.0.25; update the logic around
sandbox_dir/use_all_slaves so that the SET GLOBAL replica_parallel_workers=3 is
executed only when sortable_version >= the 8.0.26 threshold (introduce a new
minimum_version_replica_parallel constant or variable) and otherwise skip that
SET or use the older slave_parallel_workers name when appropriate; keep the
START/STOP REPLICA vs START/STOP SLAVE switch unchanged, but gate the
replica_parallel_workers assignment behind the new version check (referencing
sortable_version, minimum_version_replica, minimum_version_replica_parallel,
START REPLICA/STOP REPLICA, replica_parallel_workers, slave_parallel_workers,
and sandbox_dir/use_all_slaves).
In `@sandbox/templates/single/replicate_from.gotxt`:
- Around line 221-243: The replication setup concatenates the source's
pre-rendered master_connection (and clone_replication.sql) directly into
connection_string, which can embed source-version-specific CHANGE MASTER /
MASTER_* syntax that newer replicas (>=8.0.23/8.4) cannot run; fix by parsing
master_connection (and clone_replication.sql if present) to extract the
canonical credentials/host/port/user/password/ssl params and then rebuild the
connection_string using the replica's chosen parameter names (use
auto_position_param, log_file_param, log_pos_param, public_key_param) and GTID
logic in the current template (respecting using_gtid) so the final statement is
rendered with the replica's syntax rather than copying the source's
pre-generated SQL.
In `@test/go-unit-tests.sh`:
- Around line 34-40: The skip logic currently treats SANDBOX_BINARY as unset and
skips sandbox/ts/ts_static prematurely; before that if-block, initialize
SANDBOX_BINARY to a sensible default (e.g.,
SANDBOX_BINARY="${SANDBOX_BINARY:-$HOME/opt/mysql}") so the subsequent checks
against SANDBOX_BINARY and the assignment to SKIP_SANDBOX_TESTS use the default
location when the variable wasn't exported; update the block that sets
SKIP_SANDBOX_TESTS and echo to operate on the (possibly defaulted)
SANDBOX_BINARY variable.
---
Outside diff comments:
In `@mkreadme/make_readme.go`:
- Around line 42-50: After reading from the command's StdoutPipe (variables:
cmd, stdout, slurp), call cmd.Wait() and handle its error to obtain the process
exit status and release resources; remove the explicit stdout.Close() (Wait()
closes pipes automatically) and if cmd.Wait() returns an error, propagate or
Exitf with that error (similar to existing error handling for cmd.Start()) so
the command's failure isn't ignored.
In `@mkwiki/make_docs.go`:
- Around line 42-50: The code reads from the Stdout pipe but never calls
cmd.Wait() and ignores the io.ReadAll error; update the sequence after
cmd.StdoutPipe()/cmd.Start() in make_docs.go so that you read slurp, check and
handle the error returned by io.ReadAll(stdout), then call cmd.Wait() to reap
the child and capture any process error, and remove the explicit stdout.Close()
because Wait() will close the pipe; refer to the stdout variable, io.ReadAll
call, and cmd.Wait()/cmd.Start() when making the change.
---
Nitpick comments:
In @.github/workflows/all_tests.yml:
- Around line 42-46: The workflow currently uses floating runner labels like
"ubuntu-latest" (seen across jobs that include the "Install libraries (Linux)"
step and other jobs referencing runner images), causing package mismatches for
Ubuntu 24.04-only packages; update all runs-on entries that use "ubuntu-latest"
to the specific "ubuntu-24.04" runner to pin the environment and ensure
deterministic apt package resolution (search for runs-on: ubuntu-latest in the
workflow and replace with runs-on: ubuntu-24.04, including the job containing
the "Install libraries (Linux)" step).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec8b029d-1a01-4947-96b5-ce3f5448a6f0
📒 Files selected for processing (9)
.github/workflows/all_tests.yml.golangci.ymlcookbook/templates/replication-operations.gotxtmkreadme/make_readme.gomkwiki/make_docs.gosandbox/sandbox.gosandbox/templates/single/connection_info_sql.gotxtsandbox/templates/single/replicate_from.gotxttest/go-unit-tests.sh
| gosec: | ||
| excludes: | ||
| - G115 # integer overflow int64->uint64 on file sizes — safe | ||
| - G112 # Slowloris in test code only | ||
|
|
||
| issues: | ||
| exclude-use-default: true | ||
| max-issues-per-linter: 50 | ||
| max-same-issues: 10 | ||
| exclude-rules: | ||
| - linters: | ||
| - errcheck | ||
| text: "lock.Unlock" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name ".golangci.yml" -o -name "golangci.yml" | head -20Repository: ProxySQL/dbdeployer
Length of output: 78
🏁 Script executed:
cat -n .golangci.ymlRepository: ProxySQL/dbdeployer
Length of output: 686
🌐 Web query:
golangci-lint gosec.excludes path-scoped exclusion rules configuration
💡 Result:
In golangci-lint, the gosec linter has no dedicated per-rule excludes configuration in its linters.settings.gosec section (official docs confirm "No settings available"). Global exclusions apply instead. For global rule exclusions (not path-scoped): Use issues.exclude-rules (v1 syntax) or linters.exclusions.rules (v2 syntax) with linters: [gosec] and text matching the rule ID regex like "G404". Example (v2): linters: exclusions: rules: - linters: [gosec] text: "G404" For path-scoped exclusion rules: Use linters.exclusions.rules (or issues.exclude-rules) with path glob/regex and linters: [gosec]. This excludes gosec issues matching the path. Example (exclude gosec from all test files): linters: exclusions: rules: - path: _test.go linters: - gosec Example (exclude specific gosec rule from specific path): linters: exclusions: rules: - path: internal/hmac/ text: "weak cryptographic primitive" linters: - gosec Inline: Use //nolint:gosec or //gosec:ignore comments. Presets like common-false-positives exclude common gosec false positives (G103, G204, G304). Legacy linters-settings.gosec.excludes examples exist but are outdated/non-standard per current docs (2026). Use global exclusions for reliability.
Citations:
- 1: https://golangci-lint.run/docs/linters/configuration/
- 2: https://golangci-lint.run/docs/configuration/file
- 3: https://golangci-lint.run/docs/linters/false-positives
- 4: Support ignoring test files and other directories for
goseclinter golangci/golangci-lint#5064 - 5: gosec ignores excludes section in golangci.yml golangci/golangci-lint#2057
Replace linters-settings.gosec.excludes with properly scoped issues.exclude-rules.
linters-settings.gosec.excludes is non-standard syntax with no official support in golangci-lint; the correct approach is to use issues.exclude-rules with linters: [gosec] and rule ID patterns. Both the gosec suppressions (G115, G112) and the errcheck lock.Unlock rule currently apply repo-wide without path filtering. Move them to path-scoped rules to prevent masking future, unintended findings:
Example: path-scoped exclusion
issues:
exclude-use-default: true
exclude-rules:
- path: cmd/.*_test\.go # or appropriate test path
linters:
- gosec
text: "G115|G112"
- path: specific/location/with/mutex # scope lock.Unlock to known site
linters:
- errcheck
text: "lock.Unlock"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml around lines 16 - 26, The current golangci-lint config uses
nonstandard gosec.excludes and a repo-wide errcheck exclusion; replace the gosec
suppressions and the errcheck "lock.Unlock" exclusion by moving them under
issues.exclude-rules with explicit linters: [gosec] and linters: [errcheck]
respectively, and add path fields to scope each rule (e.g., restrict G115|G112
to test or specific directories and restrict "lock.Unlock" to the known
file/path that needs it) so the suppressions are properly recognized and not
applied repo-wide.
| if [[ "v$sortable_version" > "v$minimum_version_replica" ]] || [[ "v$sortable_version" == "v$minimum_version_replica" ]]; then | ||
| (set -x | ||
| $sandbox_dir/use_all_slaves "STOP REPLICA; SET GLOBAL replica_parallel_workers=3; START REPLICA;show processlist " | ||
| ) | ||
| else | ||
| (set -x | ||
| $sandbox_dir/use_all_slaves "STOP SLAVE; SET GLOBAL slave_parallel_workers=3; START SLAVE;show processlist " | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "replication-operations.gotxt" --exec cat -n {} \; | head -100Repository: ProxySQL/dbdeployer
Length of output: 2815
🌐 Web query:
MySQL 8.0.22 START REPLICA STOP REPLICA release notes
💡 Result:
In MySQL 8.0.22 release notes, the statements START SLAVE, STOP SLAVE, SHOW SLAVE STATUS, SHOW SLAVE HOSTS, and RESET SLAVE are deprecated. New aliases START REPLICA, STOP REPLICA, SHOW REPLICA STATUS, SHOW REPLICAS, and RESET REPLICA are introduced as replacements. These new statements function identically to the old ones, with only terminology changes to use "replica" instead of "slave". The old statements still work but emit deprecation warnings. Corresponding status variables like Com_slave_start are aliased to Com_replica_start.
Citations:
- 1: https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-22.html
- 2: https://docs.oracle.com/cd/E17952_01/mysql-8.0-relnotes-en/news-8-0-22.html
- 3: https://dev.mysql.com/blog-archive/the-mysql-8-0-22-maintenance-release-is-generally-available/
- 4: https://dev.mysql.com/blog-archive/mysql-8-0-22-replication-enhancements/
🌐 Web query:
MySQL replica_parallel_workers version 8.0.26 availability
💡 Result:
Yes, the replica_parallel_workers system variable is available in MySQL 8.0.26. It was introduced in this version as the replacement for the deprecated slave_parallel_workers variable. In MySQL 8.0.26, the default value is 0 (single-threaded replication by default). The variable enables multithreaded replication on replicas, setting the number of parallel applier worker threads (plus a coordinator thread). Values range from 0 to 1024, and it is dynamic (can be changed at runtime). For multi-channel replication, the setting applies per channel. From MySQL 8.0.27 onward, the default changed to 4.
Citations:
- 1: https://dev.mysql.com/doc/mysql-replication-excerpt/8.0/en/replication-options-replica.html
- 2: https://docs.percona.com/percona-monitoring-and-management/3/advisors/checks/sql-processing-not-multi-threaded.html
Gate replica_parallel_workers separately from START/STOP REPLICA.
START REPLICA/STOP REPLICA are available from MySQL 8.0.22, but replica_parallel_workers only exists from MySQL 8.0.26. On 8.0.22–8.0.25 this branch will switch to the new statement names correctly and then fail on SET GLOBAL replica_parallel_workers=3.
Suggested fix
minimum_version_binary_log="008002000"
minimum_version_replica="008000022"
+minimum_version_replica_parallel_workers="008000026"
@@
if [[ "v$sortable_version" > "v$minimum_version_replica" ]] || [[ "v$sortable_version" == "v$minimum_version_replica" ]]; then
+ parallel_workers_var="slave_parallel_workers"
+ if [[ "v$sortable_version" > "v$minimum_version_replica_parallel_workers" ]] || [[ "v$sortable_version" == "v$minimum_version_replica_parallel_workers" ]]; then
+ parallel_workers_var="replica_parallel_workers"
+ fi
(set -x
- $sandbox_dir/use_all_slaves "STOP REPLICA; SET GLOBAL replica_parallel_workers=3; START REPLICA;show processlist "
+ $sandbox_dir/use_all_slaves "STOP REPLICA; SET GLOBAL ${parallel_workers_var}=3; START REPLICA;show processlist "
)
else📝 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.
| if [[ "v$sortable_version" > "v$minimum_version_replica" ]] || [[ "v$sortable_version" == "v$minimum_version_replica" ]]; then | |
| (set -x | |
| $sandbox_dir/use_all_slaves "STOP REPLICA; SET GLOBAL replica_parallel_workers=3; START REPLICA;show processlist " | |
| ) | |
| else | |
| (set -x | |
| $sandbox_dir/use_all_slaves "STOP SLAVE; SET GLOBAL slave_parallel_workers=3; START SLAVE;show processlist " | |
| ) | |
| if [[ "v$sortable_version" > "v$minimum_version_replica" ]] || [[ "v$sortable_version" == "v$minimum_version_replica" ]]; then | |
| parallel_workers_var="slave_parallel_workers" | |
| if [[ "v$sortable_version" > "v$minimum_version_replica_parallel_workers" ]] || [[ "v$sortable_version" == "v$minimum_version_replica_parallel_workers" ]]; then | |
| parallel_workers_var="replica_parallel_workers" | |
| fi | |
| (set -x | |
| $sandbox_dir/use_all_slaves "STOP REPLICA; SET GLOBAL ${parallel_workers_var}=3; START REPLICA;show processlist " | |
| ) | |
| else | |
| (set -x | |
| $sandbox_dir/use_all_slaves "STOP SLAVE; SET GLOBAL slave_parallel_workers=3; START SLAVE;show processlist " | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cookbook/templates/replication-operations.gotxt` around lines 67 - 74, The
code switches to START/STOP REPLICA for MySQL >= 8.0.22 but unconditionally
tries to SET GLOBAL replica_parallel_workers (which exists only from 8.0.26),
causing failures on 8.0.22–8.0.25; update the logic around
sandbox_dir/use_all_slaves so that the SET GLOBAL replica_parallel_workers=3 is
executed only when sortable_version >= the 8.0.26 threshold (introduce a new
minimum_version_replica_parallel constant or variable) and otherwise skip that
SET or use the older slave_parallel_workers name when appropriate; keep the
START/STOP REPLICA vs START/STOP SLAVE switch unchanged, but gate the
replica_parallel_workers assignment behind the new version check (referencing
sortable_version, minimum_version_replica, minimum_version_replica_parallel,
START REPLICA/STOP REPLICA, replica_parallel_workers, slave_parallel_workers,
and sandbox_dir/use_all_slaves).
| # Directories that require MySQL binaries (sandbox, ts, ts_static) | ||
| # are skipped unless SANDBOX_BINARY is set and contains MySQL installations | ||
| SKIP_SANDBOX_TESTS="" | ||
| if [ -z "$SANDBOX_BINARY" ] || [ ! -d "$SANDBOX_BINARY" ] || [ -z "$(ls "$SANDBOX_BINARY" 2>/dev/null)" ]; then | ||
| SKIP_SANDBOX_TESTS="sandbox ts ts_static" | ||
| echo "# Skipping sandbox/ts/ts_static tests (no MySQL binaries found in SANDBOX_BINARY=$SANDBOX_BINARY)" | ||
| fi |
There was a problem hiding this comment.
Initialize SANDBOX_BINARY default before applying skip logic.
At Line 37, an unset SANDBOX_BINARY immediately skips sandbox/ts/ts_static. Since this script doesn’t source test/common.sh, it can skip those tests even when binaries exist at the default location ($HOME/opt/mysql), reducing test coverage unexpectedly.
Suggested patch
# Directories that require MySQL binaries (sandbox, ts, ts_static)
# are skipped unless SANDBOX_BINARY is set and contains MySQL installations
SKIP_SANDBOX_TESTS=""
-if [ -z "$SANDBOX_BINARY" ] || [ ! -d "$SANDBOX_BINARY" ] || [ -z "$(ls "$SANDBOX_BINARY" 2>/dev/null)" ]; then
+: "${SANDBOX_BINARY:=$HOME/opt/mysql}"
+has_mysql_binaries="$(find "$SANDBOX_BINARY" -mindepth 1 -maxdepth 1 -print -quit 2>/dev/null)"
+if [ ! -d "$SANDBOX_BINARY" ] || [ -z "$has_mysql_binaries" ]; then
SKIP_SANDBOX_TESTS="sandbox ts ts_static"
echo "# Skipping sandbox/ts/ts_static tests (no MySQL binaries found in SANDBOX_BINARY=$SANDBOX_BINARY)"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/go-unit-tests.sh` around lines 34 - 40, The skip logic currently treats
SANDBOX_BINARY as unset and skips sandbox/ts/ts_static prematurely; before that
if-block, initialize SANDBOX_BINARY to a sensible default (e.g.,
SANDBOX_BINARY="${SANDBOX_BINARY:-$HOME/opt/mysql}") so the subsequent checks
against SANDBOX_BINARY and the assignment to SKIP_SANDBOX_TESTS use the default
location when the variable wasn't exported; update the block that sets
SKIP_SANDBOX_TESTS and echo to operate on the (possibly defaulted)
SANDBOX_BINARY variable.
- replicate_from.gotxt: shell-based version detection for SHOW BINARY LOG STATUS, CHANGE REPLICATION SOURCE TO, START/STOP REPLICA - connection_info_sql.gotxt: use version-aware template variables - sandbox.go: add replication command fields to single sandbox data map - replication-operations.gotxt: version-aware SHOW MASTER STATUS and STOP/START SLAVE commands Tested with MySQL 8.4.4 and 9.1.0 — single and replication sandboxes both fully functional.
- Fix single-quote bug preventing $stop_reset_cmd expansion in remove_replication script (Gemini) - Fix boundary version comparisons to include exact version match using >= semantics (Copilot) - Remove unused change_master_cmd variable (Copilot) - Move sortable_version outside subshell in cookbook template (Copilot) - Use replicationCommands() in sandbox.go instead of duplicating version logic (Gemini) - Use named variables for magic version strings (Gemini)
6ba3569 to
671aaa8
Compare
Summary
Updates the remaining template files that still used deprecated MySQL replication SQL syntax.
Files changed
sandbox/templates/single/replicate_from.gotxt— shell-based runtime version detection for all replication commandssandbox/templates/single/connection_info_sql.gotxt— uses Go template variables for version-aware syntaxsandbox/sandbox.go— adds replication command fields to single sandbox data mapcookbook/templates/replication-operations.gotxt— version-aware SHOW MASTER STATUS and STOP/START SLAVETested
This is a companion to PR #21 which fixed the main replication templates.
Summary by CodeRabbit
New Features
Improvements
Chores