playground: Only add --comments when needed

dveeden commented 1 year ago

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.

dveeden commented 1 year ago

/cc @xhebox

ti-chi-bot[bot] commented 1 year ago

@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](https://github.com/pingcap/tiup/pull/2314#issuecomment-1809847062): >/cc @xhebox Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

dveeden commented 1 year ago

/cc @nexustar

dveeden commented 1 year ago

/retest

kaaaaaaang commented 1 year ago

maybe user run playground in an machine without mysql client？

codecov-commenter commented 1 year ago

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

```diff @@ 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](https://app.codecov.io/gh/pingcap/tiup/pull/2314/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pingcap) | Coverage Δ | | |---|---|---| | [playground](https://app.codecov.io/gh/pingcap/tiup/pull/2314/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pingcap) | `15.30% <53.66%> (?)` | | | [tiup](https://app.codecov.io/gh/pingcap/tiup/pull/2314/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pingcap) | `33.48% <ø> (ø)` | | | [unittest](https://app.codecov.io/gh/pingcap/tiup/pull/2314/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pingcap) | `22.30% <24.39%> (+0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pingcap#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dveeden commented 1 year ago

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://root@127.0.0.1:4000

dveeden commented 1 year ago

@mjonss any input on this?

ti-chi-bot[bot] commented 1 year ago

@mjonss: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to [this](https://github.com/pingcap/tiup/pull/2314#pullrequestreview-1754700137): >LGTM, but I assume some special handling for MariaDB is also needed? Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

dveeden commented 1 year ago

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"

ti-chi-bot[bot] commented 12 months ago

@mjonss: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to [this](https://github.com/pingcap/tiup/pull/2314#pullrequestreview-1762361601): > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

ti-chi-bot[bot] commented 4 months ago

[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.

xhebox commented 4 months ago

/approve

ti-chi-bot[bot] commented 4 months ago

[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

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/pingcap/tiup/blob/master/OWNERS)~~ [xhebox] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

pingcap / tiup