cli: warn the user for common mistakes around options=-ccluster
#97588
Conversation
|
This change is |
| u.warnFn("\nwarning: found raw URL parameter \"%[1]s\"; "+ | ||
| "are you sure you did not mean to use \"options=%[1]s\" instead?\n\n", optName) | ||
| } |
There was a problem hiding this comment.
If you wanted, some of the surrounding formatting could go into the warnFn to keep it consistent as we add more checks. But I don't feel strongly about it.
pkg/cli/clienturl/client_url.go
Outdated
| // For tenant selection, the option is `-ccluster=`, not `-cluster=`. | ||
| if extraOpts := opts.Get("options"); extraOpts != "" { | ||
| if strings.Contains(extraOpts, "-cluster=") { | ||
| u.warnFn("\nwarning: found \"-cluster=\" in URL \"options\" field; " + |
There was a problem hiding this comment.
perhaps somewhat confusingly, all of these are allowed:
-c cluster=-ccluster=--cluster=
i am not sure if we want to mention the other allowed syntaxes here. wdyt?
There was a problem hiding this comment.
Can you clarify your question? -c cluster= and -ccluster= are equivalent (and valid)
I did not know --cluster=. Is this one valid?
There was a problem hiding this comment.
the point i am making is that all three of the syntaxes i listed are valid and equivalent. my question is: should our informational message mention all three of them?
(we matched the PG behavior documented at https://www.postgresql.org/docs/current/app-psql.html - look for -c command)
There was a problem hiding this comment.
a separate point i just thought of: if we have to pick only one to recommend, then i think options=-ccluster= is less clear than options=-c cluster=
There was a problem hiding this comment.
-ccluster= can be typed in without escapes (e.g. the user can literally type in &options=-ccluster=...) whereas if we recommend a space they will have to use %20 which strains the eye.
I did not know that --cluster= was accepted. i'll add tests for this.
There was a problem hiding this comment.
i've ended up reusing the same code as pgwire so we have consistent behavior.
knz
force-pushed
the
20230223-url-warn
branch
2 times, most recently
from
de0f7f1 to
76a63d5
Compare
Examples: ``` % ./cockroach sql --url='postgres://localhost:26257/?-ccluster=foo' warning: found raw URL parameter "-ccluster"; are you sure you did not mean to use "options=-ccluster" instead? % ./cockroach sql --url='postgres://localhost:26257/?options=-cluster=foo' warning: found "-cluster=" in URL "options" field; are you sure you did not mean to use "options=-ccluster" instead? ``` Release note: None
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz)
pkg/cli/clienturl/client_url.go line 299 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
i've ended up reusing the same code as pgwire so we have consistent behavior.
that sgtm! thank you
|
TFYR! bors r=rafiss |
|
Build failed: |
|
bors r=rafiss |
|
Build failed: |
|
bors r=rafiss |
|
Build failed: |
|
bors r=rafiss |
|
Build succeeded: |
Fixes #97586.
Epic: CRDB-23559
Examples:
Release note: None