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.

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: hint client flags in the output of cockroach start #28198

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

knz
Copy link
Contributor

@knz knz commented Aug 2, 2018

Fixes #14325.
Requested by @sploiselle.

When one starts a node with the --host/--advertise flag, all future
clients commands require the same value in --host. However, there is
no in-app messaging that communicates this requirement, nor is there a
command that you can run to find out what the --host flag was set to.

Ditto for --certs-dir.

This patch enhances the situation by announcing the command prefix to
use for client commands in the output of cockroach start. For
example:

% ./cockroach start --certs-dir=certs --host=localhost
CockroachDB node starting [...]
...
client flags:        ./cockroach --host=localhost --port=26257 --certs-dir=certs
...
% ./cockroach start --certs-dir=certs --host=localhost --advertise-host=kenax
CockroachDB node starting [...]
...
client flags:        ./cockroach --host=kenax --port=26257 --certs-dir=certs
...

Release note (cli change): cockroach start now informs the user of
which command-line flags to use to access the newly started node in
client commands (e.g. cockroach quit etc.).

@knz knz requested review from bdarnell and a-robinson August 2, 2018 13:07
@knz knz requested a review from a team as a code owner August 2, 2018 13:07
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm: modulo nit.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/cli/start.go, line 569 at r1 (raw file):

			fmt.Fprintf(tw, "webui:\t%s\n", serverCfg.AdminURL())
			fmt.Fprintf(tw, "sql:\t%s\n", pgURL)
			fmt.Fprintf(tw, "client cmd:\t%s\n", clientFlags())

Nit: the commit message says client flags, but here you have client cmd. I wonder if this should be cli flags. @piyush-singh ?

@piyush-singh
Copy link

I agree with cli flags to eliminate any ambiguity.

@knz knz force-pushed the 20180802-inform-flags branch from 71c0dea to fbaa826 Compare August 2, 2018 16:27
@knz
Copy link
Contributor Author

knz commented Aug 2, 2018

Done.

Used the word "client" because "cli flag" would be inclusive of "cockroach start".

@petermattis
Copy link
Collaborator

Used the word "client" because "cli flag" would be inclusive of "cockroach start".

client feels confusing to me because I think of the client application. console flags? We could bike shed this all day.

@knz
Copy link
Contributor Author

knz commented Aug 2, 2018

What do you want me to do here?

  • cli flags is incorrect because other commands (cockroach gen, cockroach debug etc) cannot use the flags
  • console flags is confusing because the flags work also when used in scripts in a non-interactive fashion
  • terminal flags ditto

Now consider this: I used client flags: but I also placed the word ./cockroach directly to the right of it. This is a reminder we're talking about CockroachDB client commands, not anything else. Is that reminder insufficient?

@petermattis
Copy link
Collaborator

What do you want me to do here?

I'm not thrilled with any of the options. client flags is fine for now given the problems with the other options.

Copy link
Contributor

@a-robinson a-robinson left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/cli/start.go, line 794 at r2 (raw file):

	flags := []string{
		os.Args[0],
		"--host=" + serverAdvertiseHost,

If I just run cockroach start without specifying --host or --advertise-host, won't serverAdvertiseHost be empty? I'd think we'd want to omit --host in such cases (or always use localhost or the hostname or something)..

Copy link
Contributor

@a-robinson a-robinson left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale)


pkg/cli/start.go, line 794 at r2 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

If I just run cockroach start without specifying --host or --advertise-host, won't serverAdvertiseHost be empty? I'd think we'd want to omit --host in such cases (or always use localhost or the hostname or something)..

Although I guess that's ok, so nevermind.

Copy link
Contributor

@a-robinson a-robinson left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)

@knz
Copy link
Contributor Author

knz commented Aug 2, 2018

@a-robinson good catch I wasn't looking at the right var to generate the flags. Fixed.

Requested by @sploiselle.

When one starts a node with the --host/--advertise flag, all future
clients commands require the same value in --host. However, there is
no in-app messaging that communicates this requirement, nor is there a
command that you can run to find out what the --host flag was set to.

Ditto for --certs-dir.

This patch enhances the situation by announcing the command prefix to
use for client commands in the output of `cockroach start`. For
example:

```
% ./cockroach start --certs-dir=certs --host=localhost
CockroachDB node starting [...]
...
client flags:        ./cockroach --host=localhost --port=26257 --certs-dir=certs
...
```

```
% ./cockroach start --certs-dir=certs --host=localhost --advertise-host=kenax
CockroachDB node starting [...]
...
client flags:        ./cockroach --host=kenax --port=26257 --certs-dir=certs
...
```

Release note (cli change): `cockroach start` now informs the user of
which command-line flags to use to access the newly started node in
client commands (e.g. `cockroach quit` etc.).
@knz knz force-pushed the 20180802-inform-flags branch from fbaa826 to 39d8b03 Compare August 2, 2018 17:15
Copy link
Contributor

@a-robinson a-robinson left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale)

@knz
Copy link
Contributor Author

knz commented Aug 2, 2018

thanks!

bors r+

craig bot pushed a commit that referenced this pull request Aug 2, 2018
28198: cli: hint client flags in the output of `cockroach start` r=knz a=knz

Fixes #14325.
Requested by @sploiselle.

When one starts a node with the --host/--advertise flag, all future
clients commands require the same value in --host. However, there is
no in-app messaging that communicates this requirement, nor is there a
command that you can run to find out what the --host flag was set to.

Ditto for --certs-dir.

This patch enhances the situation by announcing the command prefix to
use for client commands in the output of `cockroach start`. For
example:

```
% ./cockroach start --certs-dir=certs --host=localhost
CockroachDB node starting [...]
...
client flags:        ./cockroach --host=localhost --port=26257 --certs-dir=certs
...
```

```
% ./cockroach start --certs-dir=certs --host=localhost --advertise-host=kenax
CockroachDB node starting [...]
...
client flags:        ./cockroach --host=kenax --port=26257 --certs-dir=certs
...
```

Release note (cli change): `cockroach start` now informs the user of
which command-line flags to use to access the newly started node in
client commands (e.g. `cockroach quit` etc.).

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 2, 2018

Build succeeded

@craig craig bot merged commit 39d8b03 into cockroachdb:master Aug 2, 2018
@knz knz deleted the 20180802-inform-flags branch February 14, 2019 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli: Improve UX around running node with --host flag
5 participants