cli: merge encode-uri into convert-url#164561
cli: merge encode-uri into convert-url#164561kev-cao wants to merge 1 commit intocockroachdb:masterfrom
Conversation
|
Merging to
|
|
This change is |
kev-cao
requested review from
a team,
andrew-r-thomas,
msbutler and
shailendra-patel
and removed request for
a team
| if options := opts.Get("options"); !strings.Contains(options, "-ccluster=") { | ||
| opts.Set("options", "-ccluster=system") | ||
| } |
There was a problem hiding this comment.
This does overwrite any existing options that may have been set (e.g. results_buffer_size). Unfortunately, Golang's encoding of url.Values encodes spaces as + instead of %20, but we expect the options to be separated with %20.
We could do a bit of replacing here on values.Encode to get the encoding correct, but not sure if it's worth it. In any case, I think this would be a separate PR.
| ldr2="$(roachprod ssh drt-ldr2:1 -- ./cockroach convert-url --inline --ca-cert ./certs/ca.crt --key ./certs/client.root.key --cert ./certs/client.root.crt --database ycsb --username root --url {pgurl:1} | tail -1)" | ||
| ldr1="$(roachprod ssh drt-ldr1:1 -- ./cockroach convert-url --inline --ca-cert ./certs/ca.crt --key ./certs/client.root.key --cert ./certs/client.root.crt --database ycsb --username root --url {pgurl:1} | tail -1)" |
There was a problem hiding this comment.
@shailendra-patel Wanted to make sure these changes work with the drt scripts — the new CLI interface does fix the bug with postgresql://postgres:// popping up, which is nice.
| fmt.Println(u.ToJDBC()) | ||
| fmt.Println() | ||
|
|
||
| fmt.Printf("# Connection URL for %[1]sCRDB%[2]s streams (%[1]sPhysical/Logical Cluster Replication%[2]s):\n", yc, rc) |
There was a problem hiding this comment.
At the moment, we're only using these URLs for PCR/LDR, but maybe the language here could be more general so that we don't have to come back and update it later? Welcoming any ideas for wording here.
| yc := cp[ttycolor.Yellow] | ||
| rc := cp[ttycolor.Reset] | ||
|
|
||
| crdbURL, err := u.ToCRDB(convertCtx.sslInline) |
There was a problem hiding this comment.
@msbutler Should the inline option be necessary here? I think it's required for all PCR/LDR streams anyway.
Although setting --inline does make the command look up those files and read from them, which means the command errors if those files aren't there. Perhaps having it behind the option gives the user a bit more flexibility (i.e. when they don't care about these URLs)
|
What does the old |
|
@dt It was deleted here, but perhaps we should just leave it and put up a deprecation notice to be deleted in the next . That way its current behavior is preserved but we let users know it'll be deleted in the next regular release. |
|
I think we should at the very least leave a stub behind to tell you to use |
This commit merges the `cockroach encode-uri` command into the `cockroach convert-url` command so that we have a unified interface for generating connection URLs. Resolves: cockroachdb#154384 Release note (cli): The `cockroach encode-uri` command has been merged into the `cockroach convert-url` command.
msbutler
left a comment
msbutler
left a comment
There was a problem hiding this comment.
one drive by comment. will provide a closer review in the morning.
| Short: "encode a CRDB connection URL", | ||
| Args: cobra.ExactArgs(1), | ||
| RunE: clierrorplus.MaybeDecorateError(encodeURI), | ||
| Deprecated: "please use convert-url instead, as encode-uri will be removed in a future release.\n", |
There was a problem hiding this comment.
nit: can you update the commit message and release note to reflect that we are deprecating this, and have unified on convert-url. i think we should also call out explicitly in the release note that PCR/LDR docs need to be updated.
4 participants
This commit merges the
cockroach encode-uricommand into thecockroach convert-urlcommand so that we have a unified interface for generating connection URLs.Resolves: #154384
Release note (cli): The
cockroach encode-uricommand has been merged into thecockroach convert-urlcommand.