- Notifications
You must be signed in to change notification settings - Fork18
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
First pass. Good thinking with keeping the full URL in the config.
| // From this point, the commandline is correct. | ||
| // Don't return errors as it would print the usage. |
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.
I see what you're getting at here... not sure this is the best approach though. Because usage errors seem like the outlier, we could setSilenceUsage: true and only print the usage when needed.
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.
This seem a bit outside the scope of this PR as it is the way all commands are set. We can create a new PR to improve error management everywhere at once.
| case <-ctx.Done(): | ||
| t.Fatal("Timeout waiting for the input.") | ||
| case err := <-errChan: | ||
| t.Fatalf("ReadLine returned before we got the token (%v).", err) |
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.
slogtest.Fatal(t, "ReadLine returned before we got the token", slog.Error(err))... same for all other logs.
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.
I think we should stick with stdlib and standard widely used and supported instead of using custom ones. Unless there is some value I am missing? Integration with our co/monitoring/alerting maybe?
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.
I enjoy using slog since it's able to print the stack frames from the provided error, though I can't say there's much consistency in either direction atm.
| case <-ctx.Done(): | ||
| t.Fatal("Timeout waiting for readline to finish.") | ||
| case err := <-errChan: | ||
| require.NoError(t, err, "Error reading the line.") |
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.
assert.Success
| return xerrors.Errorf("store config: %w", err) | ||
| } | ||
| flog.Success("Logged in.") |
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.
Should be precede this with a newline? Right now, the output is:
$ coder login https://master.cdr.devor enter token manually: 2020-09-08 16:04:30 SUCCESS Logged in.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.
Added a newline in the prompt before this line. Otherwise, flog's prefix still is on he previous line.
internal/loginsrv/input.go Outdated
| buf := bufio.NewReader(r) | ||
| retry: | ||
| _, _ = fmt.Fprintf(w, "or enter token manually: ") // Best effort. Can only fail on custom writers. |
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.
This message doesn't seem descriptive enough... maybe we should include specific direction so the user knows to paste the contents of the URL bar.
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.
This is the same message that concourse ci uses, with the same formatting. We expect most people to not require this part, so I think it might be good enough? I am not really good with words ^^, If you have a suggestion for better wording, I'd be happy to update.
| case <-ctx.Done(): | ||
| t.Fatal("Timeout waiting for the input.") | ||
| case err := <-errChan: | ||
| t.Fatalf("ReadLine returned before we got the token (%v).", err) |
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.
I enjoy using slog since it's able to print the stack frames from the provided error, though I can't say there's much consistency in either direction atm.
internal/cmd/login.go Outdated
| if err != nil { | ||
| return xerrors.Errorf("parse url: %v", err) | ||
| if l, err = net.Listen("tcp6", "[::1]:0"); err != nil { | ||
| return nil, xerrors.Errorf("failed to listen on a port: %s", err) |
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.
| returnnil,xerrors.Errorf("failed tolisten on a port: %s",err) | |
| returnnil,xerrors.Errorf("listen on a port: %w",err) |
Uh oh!
There was an error while loading.Please reload this page.
09a3d77 toaafc42aCompare- Support path based reverse proxy.Signed-off-by: Guillaume J. Charmes <guillaume@coder.com>
aafc42a to8b35b02CompareThere 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.
The language ofor enter token manually is a bit off, but I don't have any better ideas at the moment. I'd be curious to hear what others think when they try it out after this lands.
Loosely based on how concourse ci handles it.
Fixes#107. This makes for a better experience when using the cli over ssh.
Notable change: use the full URL given by the user instead of trimming down the path. This allows for path based reverse proxies.