-
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/connect: misc UX improvements #64941
base: master
Are you sure you want to change the base?
Conversation
Release note: None
Release note: None
Release note: None
a5b7e20
to
a43623d
Compare
@itsbilal do you think you could still review this? (If not, we'll figure something else out) |
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.
Nice clarification of the English and errors. The 'report' argument feels wrong to me, but I want to understand your thought process.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal and @knz)
pkg/cli/connect_join.go, line 57 at r3 (raw file):
ctx context.Context, stopper *stop.Stopper, report func(string, ...interface{}),
At first glance, I liked this, but the longer I look the more it feels wrong.
I don't quite see the motivation. If it's for logging, use logging; if it's for interactive feedback, do we expect this to do anything other than print to stdout anytime soon? (And if we do expect that, it would be a system-wide thing and little pre-refactors like this might actually get in the way.)
It feels almost like you wanted the non-existent fmt.Printfln function, defined it as a utility lambda somewhere, then decided you wanted called functions to have access to it too, so you passed it as an arg instead of factoring it into a utility function.
Well, I'm speculating too much on what you were thinking when I ought to just ask you. What justifies passing a report function like this?
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.
Sorry for missing this earlier! Getting to it now.
from my end, only things I could point out in addition to what @rauchenstein has already pointed out, is a suggestion to explicitly mention connect
's interaction with start
in the help text. But otherwise, looks good.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)
pkg/cli/connect.go, line 52 at r1 (raw file):
Long: ` Connects to other instances of the 'connect init' command and negotiates a package of TLS certificates for use with secure inter-node connections.
Maybe we could add how this command is usually followed with a start
(or start-single-node
) call, and how a lot of the flags are the same?
pkg/cli/connect_join.go, line 43 at r1 (raw file):
and obtain a package of TLS certificates for use with secure inter-node connections. The TLS certificates are saved in the configured target directory.
Ditto, could repeat the part about this command preparing the certs directory for a successive call to start
.
I found these improvements useful while doing QA work for #63492.