fix: replace \G with --vertical in replication templates (MySQL 9.5 macOS compat)#76
Conversation
….5 compat) MySQL 9.5 client rejects \G in -e mode with "Unknown command '\G'". Replace all instances across templates with --vertical flag which produces the same vertical output format. Templates fixed: - test_replication.gotxt — \G on show master/slave status - check_slaves.gotxt — \G on show master/slave status - check_multi_source.gotxt — \G on show master/slave status - replicate_from.gotxt — \G on show master/replica status Also: - Made master binlog/position grep more robust (handles File/Log_name) - Added guard for empty binlog name in source_pos_wait call - Updated export test for admin ui subcommand count
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds MySQL 9.5.0 to GitHub Actions CI test matrices, updates sandbox replication templates to use MySQL's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request updates several MySQL replication templates to use the --vertical flag instead of the \G terminator and increments the expected subcommand count in the export tests. It also enhances test_replication.gotxt with more robust parsing for master status and a safety check for missing binlog coordinates. Review feedback suggests applying these robust parsing improvements to replicate_from.gotxt and using grep -E for better consistency and readability across the templates.
| fi | ||
|
|
||
| $master_use_script -e "$show_master_status_cmd\G" > $master_status | ||
| $master_use_script --vertical -e "$show_master_status_cmd" > $master_status |
There was a problem hiding this comment.
For consistency with the improvements made in test_replication.gotxt (lines 33-34), the extraction of binlog_file and binlog_pos in the subsequent lines (168-169) should also be updated to use the more robust regex. This ensures compatibility with different MySQL versions or forks that might use Log_name instead of File, and correctly handles potential leading whitespace in the vertical output.
| fi | ||
| $SBDIR/use -v -e "$start_replica_cmd" | ||
| $SBDIR/use -v -e "$show_replica_cmd\G" | grep "\(Running:\|Master_Log_Pos\|Source_Log_Pos\|\<Master_Log_File\|\<Source_Log_File\|Retrieved\|Executed\|Auto_Position\)" | ||
| $SBDIR/use --vertical -e "$show_replica_cmd" | grep "\(Running:\|Master_Log_Pos\|Source_Log_Pos\|\<Master_Log_File\|\<Source_Log_File\|Retrieved\|Executed\|Auto_Position\)" |
There was a problem hiding this comment.
Use grep -E for consistency with other replication templates (e.g., check_slaves.gotxt and check_multi_source.gotxt). This also improves readability by avoiding the need for multiple backslash escapes for the alternation operator.
$SBDIR/use --vertical -e "$show_replica_cmd" | grep -E "(Running:|Master_Log_Pos|Source_Log_Pos|\<Master_Log_File|\<Source_Log_File|Retrieved|Executed|Auto_Position)"
There was a problem hiding this comment.
Pull request overview
This PR updates dbdeployer’s replication-related shell templates to avoid MySQL client \G usage in -e mode (notably failing on MySQL 9.5 on macOS), while also adjusting a few related parsing/CI/test expectations.
Changes:
- Replaced
\Gquery terminator usage withmysql --verticalacross replication templates to restore cross-platform compatibility. - Improved binlog filename parsing in
test_replicationand added a guard against empty binlog/position values before callingmaster_pos_wait/source_pos_wait. - Added MySQL
9.5.0to the GitHub Actions integration test matrix and updated an export structure test expectation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sandbox/templates/single/replicate_from.gotxt | Switches master/replica status output to --vertical for compatibility. |
| sandbox/templates/replication/test_replication.gotxt | Uses --vertical, tweaks binlog parsing, and guards pos-wait invocation. |
| sandbox/templates/replication/check_slaves.gotxt | Uses --vertical for master/slave status output. |
| sandbox/templates/replication/check_multi_source.gotxt | Uses --vertical for multi-source status output. |
| cmd/export_test.go | Updates expected admin subcommand count. |
| .github/workflows/integration_tests.yml | Extends CI matrix to include MySQL 9.5.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| binlog_file=$(grep File < $master_status | awk '{print $2}') | ||
| binlog_pos=$(grep Position < $master_status | awk '{print $2}') |
There was a problem hiding this comment.
show master status / show binary log status vertical output can expose the binlog filename under Log_name: (as already handled in test_replication.gotxt). Here parsing only looks for File, so binlog_file can become empty and the script will exit even though the master status output is valid. Update the grep to accept both field names (and ideally anchor to the label).
| binlog_file=$(grep File < $master_status | awk '{print $2}') | |
| binlog_pos=$(grep Position < $master_status | awk '{print $2}') | |
| binlog_file=$(grep -E '^[[:space:]]*(File|Log_name):' < $master_status | awk '{print $2}') | |
| binlog_pos=$(grep -E '^[[:space:]]*Position:' < $master_status | awk '{print $2}') |
| master_binlog=$(grep -E '^\s*(File|Log_name):' $master_status | awk '{print $2}' ) | ||
| master_pos=$(grep -E '^\s*Position:' $master_status | awk '{print $2}' ) |
There was a problem hiding this comment.
The regexes use \s* with grep -E, but \s is not portable in ERE (GNU/BSD grep -E treats it as a literal s). If the intent is to allow leading whitespace in the --vertical output, this won’t work reliably; use [[:space:]]* (or remove the whitespace matcher if not needed).
| master_binlog=$(grep -E '^\s*(File|Log_name):' $master_status | awk '{print $2}' ) | |
| master_pos=$(grep -E '^\s*Position:' $master_status | awk '{print $2}' ) | |
| master_binlog=$(grep -E '^[[:space:]]*(File|Log_name):' $master_status | awk '{print $2}' ) | |
| master_pos=$(grep -E '^[[:space:]]*Position:' $master_status | awk '{print $2}' ) |
| echo "# WARNING: could not determine master binlog/position, using GTID wait" | ||
| sleep 3 | ||
| S_READY=0 |
There was a problem hiding this comment.
When master_binlog/master_pos can’t be determined, the script prints “using GTID wait” but then only sleeps and forces S_READY=0. This can make the test flaky (or mask a real parsing/replication issue) because it skips waiting for the slave to catch up. Either implement an actual GTID-based wait (e.g., wait for the master’s executed GTID set on the slave) or treat missing binlog/position as a hard failure so the root cause is surfaced.
| echo "# WARNING: could not determine master binlog/position, using GTID wait" | |
| sleep 3 | |
| S_READY=0 | |
| echo "# ERROR: could not determine {{.MasterLabel}} binlog/position; cannot verify replication wait" | |
| exit 1 |
Problem
MySQL 9.5 client on macOS rejects
\Gin-emode:This breaks
test_replication,check_slaves,check_multi_source, andreplicate_fromscripts.Fix
Replace
\Gwith--verticalflag in all 4 templates.--verticalproduces identical output and works on all platforms/versions.Also:
FileorLog_name)source_pos_waitcallTest Plan
test_replicationpasses for 8.0.42, 8.4.4, 9.1.0, 9.5.0Summary by CodeRabbit
Tests
Improvements