REC-57: open browser automatically #54

phoenixuprising · 2024-10-16T21:36:09Z

Add the pkg/browser module to handle cross platform opening of urls in the user's default browser.

When calling `engflow_auth login $cluster`, the browser will now automatically open to the correct url with the necessary query params appended.

rogerhu · 2024-10-17T19:52:00Z

Ready for review? Looks good!

minor-fixes

Thanks for the fix! I'm certainly excited to not have to click the link as part of the flow anymore :)

cmd/engflow_auth/main.go

minor-fixes · 2024-10-17T19:57:07Z

internal/browser/browser.go

 func (p *StderrPrint) Open(u *url.URL) error {
 	fmt.Fprintf(
-		os.Stderr,
-		"Please open the following URL in your web browser to authenticate:\n\n\t%s\n\n",
+		os.Stdout,


Why the switch to stdout? It does mean the object should probably be renamed, as StderrPrint now no longer only prints to stderr

Originally this was stderr because I figured it followed UNIX best practices (stdout for machine-consumable output, stderr for human-only output)

yeah I was wondering about the choice for stderr originally. You make a good point about stdout being used for machine output. Will revert it back to stderr.

jayconrod

Verified this works on Windows. I have a couple small comments, but this is looking good!

jayconrod · 2024-10-17T19:53:26Z

internal/browser/browser.go

+	var err error
+	switch runtime.GOOS {
+	case "darwin": // macOS
+		err = exec.Command("open", url).Start()


Use Run rather than Start. Run waits for the subprocess to terminate, so its error will be more useful. On all platforms, these commands complete pretty quickly, so it's fine to block.

jayconrod · 2024-10-17T19:55:14Z

internal/browser/browser.go

+		// One of them is xdg-open, another is x-www-browser, then there's www-browser, etc.
+		// Look for one that exists and run it
+		for _, provider := range providers {
+			if _, err := exec.LookPath(provider); err == nil {


Nit: assign the result of LookPath and use that in the call to exec.Command to avoid repeating the lookup.

jayconrod · 2024-10-17T19:56:29Z

internal/browser/browser.go

+		// Look for one that exists and run it
+		for _, provider := range providers {
+			if _, err := exec.LookPath(provider); err == nil {
+				err = exec.Command(provider, url).Start()


break or return nil if there was no error. If for some reason all of these commands are installed, we don't want to open the browser three times.

jayconrod · 2024-10-17T19:59:34Z

internal/browser/browser.go

+	default:
+		err = fmt.Errorf("unsupported platform")
+	}
+	return err


Nit: err will be nil on linux if none of those commands are installed. Let's either add a err = fmt.Errorf("unsupported platform") at the end of the loop or change each of these case to return their error directly.

internal/browser/browser.go

jayconrod · 2024-10-17T20:03:32Z

internal/browser/browser.go

-		os.Stderr,
-		"Please open the following URL in your web browser to authenticate:\n\n\t%s\n\n",
+		os.Stdout,
+		"Opening the following URL in your web browser to authenticate:\n\n\t%s\n\n",


The user might be running this command on a remote machine whether there is no browser or where they can't see the browser. I think it still makes sense to tell them to open the link manually in the case.

For reference, this is what aws sso login prints:

Attempting to automatically open the SSO authorization page in your default browser. If the browser does not open or you wish to use a different device to authorize this request, open the following URL: https://device.sso.us-east-2.amazonaws.com/ Then enter the code:

jayconrod · 2024-10-17T20:05:41Z

cmd/engflow_auth/main.go

@@ -47,7 +47,7 @@ const (
 )

 type appState struct {
-	// These vars are initialized by `build()` iff they are not pre-populated;
+	// These vars are initialized by `build()` if they are not pre-populated;


I think this was meant to be an abbreviation rather than a misspelling. Let's expand it.

Suggested change

// These vars are initialized by `build()` if they are not pre-populated;

// These vars are initialized by `build()` if and only if they are not pre-populated;

Co-authored-by: Scott Minor <[email protected]>

Co-authored-by: Jay Conrod <[email protected]>

jayconrod · 2024-10-17T22:53:58Z

internal/browser/browser.go

-		"Please open the following URL in your web browser to authenticate:\n\n\t%s\n\n",
+		`Attempting to automaticaly open the authentication URL in your web browser.
+If the browser does not open or you wish to use a different device to authorize this request, open the following URL:
+		%s`,


Two tabs ends up being a lot of space. Let's take it back to one.

Also make sure to keep the newline at the end here.

phoenixuprising requested review from jayconrod and rogerhu October 17, 2024 18:28

REC-57: open browser automatically

f8a2a44

When calling `engflow_auth login $cluster`, the browser will now automatically open to the correct url with the necessary query params appended.

phoenixuprising force-pushed the chayes/auto-open-browser branch from 11f357d to f8a2a44 Compare October 17, 2024 18:30

minor-fixes reviewed Oct 17, 2024

View reviewed changes

jayconrod reviewed Oct 17, 2024

View reviewed changes

fix comment

a09fb8f

Co-authored-by: Scott Minor <[email protected]>

phoenixuprising force-pushed the chayes/auto-open-browser branch 2 times, most recently from 4858c83 to d88e55e Compare October 17, 2024 21:08

pr feedback

6e814b9

phoenixuprising force-pushed the chayes/auto-open-browser branch from d88e55e to 6e814b9 Compare October 17, 2024 21:09

assign openURL return to _

eb047b7

Co-authored-by: Jay Conrod <[email protected]>

phoenixuprising marked this pull request as ready for review October 17, 2024 21:11

phoenixuprising requested review from jayconrod and minor-fixes October 17, 2024 21:11

jayconrod approved these changes Oct 17, 2024

View reviewed changes

fix indentation of message

950adf0

phoenixuprising enabled auto-merge (squash) October 18, 2024 05:57

phoenixuprising disabled auto-merge October 18, 2024 05:57

phoenixuprising enabled auto-merge (squash) October 18, 2024 05:58

phoenixuprising merged commit 2d27df2 into main Oct 18, 2024
5 checks passed

phoenixuprising deleted the chayes/auto-open-browser branch October 18, 2024 05:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REC-57: open browser automatically #54

REC-57: open browser automatically #54

phoenixuprising commented Oct 16, 2024

rogerhu commented Oct 17, 2024 •

edited

Loading

minor-fixes left a comment

minor-fixes Oct 17, 2024

phoenixuprising Oct 17, 2024

jayconrod left a comment

jayconrod Oct 17, 2024

jayconrod Oct 17, 2024

jayconrod Oct 17, 2024

jayconrod Oct 17, 2024

jayconrod Oct 17, 2024

jayconrod Oct 17, 2024

jayconrod Oct 17, 2024

	// These vars are initialized by `build()` if they are not pre-populated;
	// These vars are initialized by `build()` if and only if they are not pre-populated;

REC-57: open browser automatically #54

REC-57: open browser automatically #54

Conversation

phoenixuprising commented Oct 16, 2024

rogerhu commented Oct 17, 2024 • edited Loading

minor-fixes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayconrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerhu commented Oct 17, 2024 •

edited

Loading