-
Notifications
You must be signed in to change notification settings - Fork 116
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
feat: added optional input of remote host name #18
base: main
Are you sure you want to change the base?
Conversation
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.
This looks great! Thank you for contributing. A few small concerns, but nothing stopping this from merging once those are addressed.
local MYSQL_COMMAND_PARAMS="-h$host -uadmin" | ||
fi | ||
MYSQL_COMMAND="mysql $MYSQL_COMMAND_PARAMS -p$(cat /etc/psa/.psa.shadow)" | ||
MYSQLADMIN_COMMAND="mysqladmin $MYSQL_COMMAND_PARAMS-p$(cat /etc/psa/.psa.shadow)" |
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.
Missing space after PARAMS
.
@@ -229,10 +237,11 @@ function second_login_failed() | |||
cecho "Could not auto detect login info!" | |||
cecho "Found potential sockets: $found_socks" | |||
cecho "Using: $socket" red | |||
read -p "Would you like to provide a different socket?: [y/N] " REPLY | |||
read -p "Would you like to provide a different socket/host?: [y/N] " REPLY |
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.
Host and socket are mutually exclusive*, and the UX of the prompts that follow is strange as a result. Change y
/N
to something like s
/h
/N
. Or ask host and socket as two separate questions; both approaches strike me as valid.
*: Excepting the weird special case of -h localhost
, which is a total disaster unto itself.
@@ -242,7 +251,11 @@ function second_login_failed() | |||
read -p "User: " user | |||
read -rsp "Password: " pass | |||
|
|||
local MYSQL_COMMAND_PARAMS="-S $socket -u$user" | |||
if [ "$socket" != "" ]; then | |||
local MYSQL_COMMAND_PARAMS="-S$socket -u$user" |
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.
Prefer parameter expansion (${foo[@]}
syntax) where possible. It's fine if you'd rather not—to put it mildly, there are quite a few spots in the script that break in the face of parameters with spaces and special characters—but I'd prefer new code to follow best practices whenever practical.
@@ -117,11 +117,14 @@ function write_mycnf() { | |||
# $4: password |
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.
Update this comment to match usage.
Hi.
I tried running the script on a server with remote database and added prompting for a host as alternative to socket including saving to .my.cnf.
I know I could have manually created a .my.cnf but maybe this could be of some use ^^