playground: Only add --comments when needed #2314
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
the
size/L
|
/cc @xhebox |
|
@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:
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. |
|
/cc @nexustar |
|
/retest |
|
maybe user run playground in an machine without mysql client？ |
Codecov ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
So if it can't execute the 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 ( Instead of
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: |
|
@mjonss any input on this? |
| if vMaj == 8 && vMin >= 1 { // 8.1.0 and newer | ||
| return "mysql" | ||
| } |
There was a problem hiding this comment.
Is --comments default on on MariaDB CLI? From reading the knowledge base it does not look like it.
Also there is no harm in always using --comments, regardless of version, just not needed for MySQL CLI ver >= 8.1 right?
I wonder if parseMysqlCommand should also return the vendor, like MySQL or MariaDB?
There was a problem hiding this comment.
--comments is not the default on MariaDB Client yet. (ref: https://mariadb.com/kb/en/mariadb-command-line-client/#-c-comments )
There is no real harm on using --comments but the commandline gets longer and more complex with this.
There was a problem hiding this comment.
Ah, I missed the vMaj == 8 which will not match any MariaDB versions.
|
@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. |
No special handling for MariaDB is needed afaik. MariaDB strips comments by default in all versions so Some commandline options in MariaDB are different (e.g. Actually.... in MariaDB the client is now |
| if vMaj == 8 && vMin >= 1 { // 8.1.0 and newer | ||
| return "mysql" | ||
| } |
There was a problem hiding this comment.
Ah, I missed the vMaj == 8 which will not match any MariaDB versions.
|
@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:
|
|
New changes are detected. LGTM label has been removed. |
What problem does this PR solve?
From https://dev.mysql.com/doc/relnotes/mysql/8.1/en/news-8-1-0.html
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
Side effects
Release notes: