Move Flag upwards #1202
Move Flag upwards #1202
Conversation
| @@ -75,6 +78,28 @@ const ( | |||
| PromptYes Prompt = true | |||
| ) | |||
|
|
|||
| func NewAuthServer(handler http.Handler, port int) *httptest.Server { | |||
| // create a listener with the desired port. | |||
| l, err := net.Listen("tcp", "127.0.0.1:"+strconv.Itoa(port)) | |||
There was a problem hiding this comment.
Does this work in Docker? I'd expect we'd have to use 0.0.0.0
There was a problem hiding this comment.
We do have to use 0.0.0.0, this is just me playing with things, I am not exactly sure what the best approach, I just push onto draft PR so I don't lose the code.
| @@ -75,6 +78,28 @@ const ( | |||
| PromptYes Prompt = true | |||
| ) | |||
|
|
|||
| func NewAuthServer(handler http.Handler, port int) *httptest.Server { | |||
| @@ -17,7 +17,7 @@ func Token(ctx context.Context, client client.FabricClient, tenant types.TenantN | |||
| return ErrDryRun | |||
| } | |||
|
|
|||
| code, err := auth.StartAuthCodeFlow(ctx, true) | |||
| code, err := auth.StartAuthCodeFlow(ctx, auth.PromptYes, 0) | |||
There was a problem hiding this comment.
this command should probably honor any --auth-server flag as well.
| log.Fatal(err) | ||
| } | ||
|
|
||
| ts := httptest.NewUnstartedServer(handler) |
There was a problem hiding this comment.
Is there still value in using httptest as opposed to using the vanilla http package?
There was a problem hiding this comment.
I prefer to use httptest because its underlying server.Start() method independently manages the server goroutine lifecycle. It utilizes wait groups and wraps the connection state-tracking hook to monitor which connections are idle. Plus it non-blocking. I am just exploring which server function may be better.
| // NewUnstartedServer creates a listener. Close that listener and replace | ||
| // with the one we created. | ||
| ts.Listener.Close() | ||
| ts.Listener = l |
There was a problem hiding this comment.
This feels pretty hacky. There must be a clean way to tell http to use a given port or listener.
There was a problem hiding this comment.
hehe still WIP, but I agree. just trying things out.
| @@ -390,7 +400,7 @@ var RootCmd = &cobra.Command{ | |||
| term.ResetWarnings() // clear any previous warnings so we don't show them again | |||
|
|
|||
| defer func() { track.Cmd(nil, "Login", P("reason", err)) }() | |||
| if err = cli.InteractiveLogin(cmd.Context(), client, getCluster()); err != nil { | |||
| if err = cli.InteractiveLoginPrompt(cmd.Context(), client, getCluster(), port); err != nil { | |||
There was a problem hiding this comment.
This now opens the browser without user interaction. Not a change we want, I think.
| port, _ := cmd.Flags().GetInt("auth-server") | ||
| if port != 0 { | ||
| go func() { | ||
| if err := cli.InteractiveLoginWithDocker(cmd.Context(), getCluster(), port); err != nil { |
There was a problem hiding this comment.
only the MCP server needs this to be long running. Regular CLI can use the existing flow (Start/stop server when needed) but honoring the --auth-server port that was given.
Description
Linked Issues
Checklist