Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cli/sql: announce the target tenant in the output #100920

Merged
merged 4 commits into from
Apr 10, 2023
Merged

cli/sql: announce the target tenant in the output #100920

merged 4 commits into from
Apr 10, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented Apr 7, 2023

Fixes #93129.
Epic: CRDB-23559

See individual commits for details.

@knz knz requested review from rafiss and a team April 7, 2023 15:41
@knz knz requested a review from a team as a code owner April 7, 2023 15:41
@knz knz added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Apr 7, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

rafiss
rafiss approved these changes Apr 7, 2023
View reviewed changes
pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled Outdated
@@ -47,28 +47,37 @@ eexpect "does not exist"
eexpect "defaultdb>"
send "\\q\r"
eexpect eof

spawn $argv sql --no-line-editor -u root -d cluster:demo-tenant
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like this test file is being skipped. should we re-enable it?

there's a lot of other disabled ones too. perhaps we can check that they don't regress with this change, even if they are not re-enabled right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I'll check them manually in this PR, then send another PR separately to re-enable them.

knz added 3 commits April 10, 2023 12:16
@knz
cli/sql: warn the user when connecting to the system tenant
095c9b3 
This change ensures that a clear warning is printed in the SQL shell
when the user connects to the system tenant and there are secondary
tenants defined.

For example:
```
 % ./cockroach sql --certs-dir=~/.cockroach-demo -d cluster:system
\#
\# Welcome to the CockroachDB SQL shell.
\[...]
\#
\#
\# ATTENTION: YOUR ARE CONNECTED TO THE SYSTEM TENANT OF A MULTI-TENANT CLUSTER.
\# PROCEED WITH CAUTION. YOU ARE RESPONSIBLE FOR ENSURING THAT YOU DO NOT
\# PERFORM ANY OPERATIONS THAT COULD DAMAGE THE CLUSTER OR OTHER TENANTS.
\#
[...]
root@localhost:26257/defaultdb>
```

Release note: None
@knz
cli/sql: include the tenant name in the prompt when known
c7a932a 
This change extends the SQL prompt with the name of the tenant the
prompt is connected to, if it was specified in the URL.

For example:
```
% ./cockroach sql --certs-dir=~/.cockroach-demo -d cluster:foo
...
root@localhost:26257/foo/defaultdb>
```

Release note: None
@knz
cli/interactive_tests: make test_client_side_checking easier to run l…
27c0a29 
…ocally

Sometimes the shell invoked by `expect` adds additional escape
characters in-between the output of a process and the next prompt.
This patch makes the test more robust in that case.

Release note: None
@knz knz force-pushed the 20230407-cli-tenant branch from 21c8ada to 27c0a29 Compare April 10, 2023 10:21
@knz
Copy link
Contributor Author

knz commented Apr 10, 2023

I have checked manually that all the disabled tests still pass. I'll send a separate PR to un-skip them.

bors r=rafiss

@knz
Copy link
Contributor Author

knz commented Apr 10, 2023

lint error

bors r-

@craig
Copy link
Contributor

craig bot commented Apr 10, 2023

Canceled.

@knz
cli/democluster: rename "demo-tenant" to "demoapp"
66755e9 
We aim to de-emphasize the word "tenant" in user-facing features.
This patch achieves this for `cockroach demo`
by making the tenant name "demoapp" instead of "demo-tenant".

Release note: None
@knz knz requested a review from a team as a code owner April 10, 2023 10:24
@knz
Copy link
Contributor Author

knz commented Apr 10, 2023

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Apr 10, 2023

Build succeeded:

@craig craig bot merged commit 6bbb865 into cockroachdb:master Apr 10, 2023
@blathers-crl blathers-crl bot mentioned this pull request Apr 10, 2023
Merged
@blathers-crl
Copy link

blathers-crl bot commented Apr 10, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error setting reviewers, but backport branch blathers/backport-release-23.1-100920 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/101049/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

Backport to branch 23.1.x failed. See errors above.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@knz knz deleted the 20230407-cli-tenant branch April 10, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafiss rafiss rafiss approved these changes

Assignees
No one assigned
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cli/sql: clearly announce the target tenant in the output
3 participants
@knz @cockroach-teamcity @rafiss