Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate DB CLI commands for Teleterm from tsh daemon #11835
Generate DB CLI commands for Teleterm from tsh daemon #11835
Changes from 14 commits
b59ea3a
aca1d79
dea26cd
b2bdf71
f085de1
5d3017c
ef014a4
3f83a00
3a77875
be1b615
b7f2759
0437882
d60bf57
fc76d32
d0dbc65
a6d434b
916ec58
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way for Teleterm to deal with that error?
tsh
will printmysql not found not mentioning that
mariadb` is also a supported clientCLIBuildder
then another person working on that code can easily return an error and break the Teleterm. If this is the case I'd suggest removing theerror
from returned values.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
The fundamental issue is that
tsh db connect
expectsdbcmd
to return a command that will work and if it can't provide one, it should either return an appropriate error or attempt to execute the command which will result in the<binary> not found
error.tsh db connect
also executesdbcmd
only when the command is actually needed. So if someone has a cluster with a Postgres db but they don't havepsql
installed it and they never try to access it, nothing bad ever happens.Teleterm on the other hand doesn't expect this command to work. It's more of a "if you have the CLI client installed on your system, this command should connect you to that db". It also needs this command any time it creates a local proxy for the db, but the user might not even want to use that command. They might want to use a GUI app. So again, in this case we don't really care if the binary is actually on the system.
@jakule What if we added a
tolerateMissingCLIClient
flag toconnectionCommandOpts
? If it's present, dbcmd would not return an error here, but ratherc.getMySQLOracleCommand()
.#11843 added the
printFormat
flag so thattsh db config
can wrap Postgres connection strings in quotes. WithtolerateMissingCLIClient
, the option usage would look like this:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakule I added a comment which adds said option, please let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me. I'd just change the name of that flag to
ignoreMissingBin
orskipBinCheck
maybe?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a test with this flag set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I thought about a name like this. However, for all the other protocols, dbcmd doesn't actually check if the binary is there or not – attempting to run the resulting command is just going to fail with a regular shell error about a missing binary.
At the moment it really is just a special case for MySQL where instead of telling the user that
mysql
doesn't exist we also want to point out thatmariadb
is a valid option too. So I didn't want to use a field name that would suggest that if the field is not present then the check will be done.In that sense it's more of a caller telling dbcmd "I'm okay with the CLI missing on user's system".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I already added one that makes mysql and mariadb not available and expects that the method will not return an error.