-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
cli/sql: fix the handling of --set #46118
Conversation
70f36d8
to
c54347a
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.
Just to confirm from reading, this change just moves all the default settings logic into a separate function, and then runs the user provided overrides after the setup logic has run?
pkg/cli/sql.go
Outdated
sqlCtx.showTimes = true | ||
} | ||
if cliCtx.isInteractive && !sqlCtx.debugMode { | ||
// If the terminal is interactive and this was not explicitly disabled by setting the debug mode, |
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.
[nit]: long comment
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.
Fixed.
yes |
lgtm |
Release justification: Category 2: Bug fixes and low-risk updates to new functionality Release note (bug fix): The parameter `--set` for `cockroach sql` and `cockroach demo` is now properly able to override all client-side options, as advertised.
TFYR bors r=rohany |
Canceled |
bors r=rohany |
Build succeeded |
Fixes #46116
I intend to backport this to 19.2 (bug fix)
Release justification: Category 2: Bug fixes and low-risk updates to new functionality
Release note (bug fix): The parameter
--set
forcockroach sql
andcockroach demo
is now properly able to override all client-sideoptions, as advertised.