-
-
Notifications
You must be signed in to change notification settings - Fork 498
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
Smart routing for sql command execution against running local servers #5966
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.
Generally looking good; the command startup feels like it's getting complex, but I guess that's unavoidable to some extent since we're including more logic. I held off from approving just to make sure I understood a couple questions/thoughts first.
Looks like there's just one failing BATS test left, too! 🎉
b8f2033
to
4bd64a7
Compare
…where Use the ABS dataDir rather than the user provided value when looking for config and permissions files
Not all commands, or even all SQL statements need databases.
Renamed a variable to be more clear, and updated documentation for the --use-db flag.
Also, avoid using /tmp in tests. Could get polluted
48f1778
to
d5214af
Compare
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.
Logic looks good. I still think it's worth a little more time on the error messaging. Ideally the error message would be crystal clear about what's happening and include a concrete next step for how the user can correct it. Otherwise we get more support requests and customer get blocked in their usage.
When
dolt sql
commands are executed against a database which is in used by a sql server, connect to it through the sql port. Currently this does not support authenticated transport. If the root user is still active, we'll use that be default. Otherwise, the--user
flag can be used to specify a user, but no authentication token is supported. That will come in the next round of changes.A step towards: #3922