tiup
tiup copied to clipboard
pingcap
playground: Only add --comments when needed
What problem does this PR solve?
From https://dev.mysql.com/doc/relnotes/mysql/8.1/en/news-8-1-0.html
Comments in the mysql client are now enabled by default. To disable them, start mysql with the --skip-comments option.
Our thanks to Daniël van Eeden for the contribution. (Bug #109972, Bug #35061087, WL #15597)
This means we can shorten the commandline to connect to TiDB if the client is newer than 8.1.0.
What is changed and how it works?
Check List
Tests
- Unit test
- Manual test (add detailed scripts or steps below)
Side effects
- Increased code complexity
Release notes:
TiUP Playground now only prints `--comments` for older versions of MySQL Client.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign kaaaaaaang for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
![ti-chi-bot[bot] avatar](https://avatars.githubusercontent.com/in/214286?v=4 "Published by a pub.dev verified publisher"){kind=small}
@dveeden: GitHub didn't allow me to request PR reviews from the following users: xhebox.
Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.
In response to this:
/cc @xhebox
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
Codecov Report
Attention: Patch coverage is 53.65854% with 19 lines in your changes missing coverage. Please review.
Project coverage is 50.73%. Comparing base (
ced2d6d) to head (db97f60). Report is 62 commits behind head on master.
:exclamation: Current head db97f60 differs from pull request most recent head 3dc6fe2
Please upload reports for the commit 3dc6fe2 to get more accurate results.
| Files | Patch % | Lines |
|---|---|---|
| components/playground/playground.go | 53.66% | 14 Missing and 5 partials :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## master #2314 +/- ##
==========================================
+ Coverage 47.65% 50.73% +3.08%
==========================================
Files 307 324 +17
Lines 33819 34748 +929
==========================================
+ Hits 16116 17628 +1512
+ Misses 15715 14940 -775
- Partials 1988 2180 +192
| Flag | Coverage Δ | |
|---|---|---|
| playground | 15.30% <53.66%> (?) |
|
| tiup | 33.48% <ø> (ø) |
|
| unittest | 22.30% <24.39%> (+0.01%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
maybe user run playground in an machine without mysql client？
$ sudo mv /usr/bin/mysql /usr/bin/mysqlabc
$ ./bin/tiup-playground --tiproxy 1 --tiflash 0 v7.1.2
...
Connect TiDB: mysql --comments --host 127.0.0.1 --port 4000 -u root
Connect TiProxy: mysql --comments --host 127.0.0.1 --port 6000 -u root
...
So if it can't execute the mysql command or if it fails to parse the version string it will output mysql --comments ....
The only risk is that the user might use SSH forwarding from another host or use another mysql client to connect. However playground is mostly used locally.
I now also notice that we use a mix between long (--comments, --host and --port) and short options (-u), which isn't very consistent.
Instead of mysql --comments --host 127.0.0.1 --port 4000 -u root we could use one of these:
-
mysql --comments --host 127.0.0.1 --port 4000 --user root -
mysql -c -h 127.0.0.1 -P 4000 -u root
The long options are more descriptive, but the short options are more concise.
Not something we should do now, but I like how MySQL Shell is doing this:
mysqlsh --sql mysql://[email protected]:4000
@mjonss: adding LGTM is restricted to approvers and reviewers in OWNERS files.
In response to this:
LGTM, but I assume some special handling for MariaDB is also needed?
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
LGTM, but I assume some special handling for MariaDB is also needed?
No special handling for MariaDB is needed afaik. MariaDB strips comments by default in all versions so --comments is still needed. (ref: https://mariadb.com/kb/en/mariadb-command-line-client/#-c-comments )
Some commandline options in MariaDB are different (e.g. --ssl-mode), but I don't think we use any of these right now.
Actually.... in MariaDB the client is now bin/mariadb instead of bin/mysql. I think as a MySQL compatible database we should suggest/recommend a MySQL Client. Note that there is a bin/mysql in MariaDB, but that reports: "Deprecated program name. It will be removed in a future release, use '/usr/bin/mariadb' instead"
@mjonss: adding LGTM is restricted to approvers and reviewers in OWNERS files.
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
[LGTM Timeline notifier]
Timeline:
-
2023-12-11 17:32:07.188212495 +0000 UTC m=+291018.225439436: :ballot_box_with_check: agreed by srstack. -
2023-12-19 06:14:53.353633702 +0000 UTC m=+941584.390860629: :heavy_multiplication_x::repeat: reset by breezewish.
[LGTM Timeline notifier]
Timeline:
-
2023-12-11 17:32:07.188212495 +0000 UTC m=+291018.225439436: :ballot_box_with_check: agreed by srstack. -
2023-12-19 06:14:53.353633702 +0000 UTC m=+941584.390860629: :heavy_multiplication_x::repeat: reset by breezewish. -
2024-07-15 02:05:01.751929191 +0000 UTC m=+233123.742870661: :ballot_box_with_check: agreed by xhebox.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: xhebox
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [xhebox]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment