- Notifications
You must be signed in to change notification settings - Fork18
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Co-authored-by: Armin <arminaaki@users.noreply.github.com>
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.
Code looks great to me, appreciate the extra log! Thanks for the contribution! 👍🏻
| @@ -66,7 +69,8 @@ func login(cmd *cobra.Command, workspaceURL *url.URL) error { | |||
| q.Add("show_token", "true") | |||
| authURL.RawQuery = q.Encode() | |||
| if err := browser.OpenURL(authURL.String()); err != nil { | |||
| if err := openURL(authURL.String()); err != nil { | |||
| clog.LogWarn(err.Error()) | |||
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.
Thanks for the additional warning log 👍🏻
internal/cmd/login.go Outdated
| // isWSL determines if coder-cli is running within Windows Subsystem for Linux | ||
| func isWSL() (bool, error) { | ||
| if runtime.GOOS == "darwin" || runtime.GOOS == "windows" { |
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.
it looks like we should make use of constants that are already defined:
we should addgoosDarwin as a const as well
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.
Thank you for your review. Updated!
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 may resolve →#448
internal/cmd/login.go Outdated
| if wsl { | ||
| cmd = "cmd.exe" | ||
| args = []string{"/c", "start"} | ||
| url = strings.Replace(url, "&", "^&", -1) |
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.
| url=strings.Replace(url,"&","^&",-1) | |
| url=strings.ReplaceAll(url,"&","^&") |
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.
Done!
| var cmd string | ||
| var args []string | ||
| wsl, err := isWSL() |
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 this is good, but I do wonder whether we could propose this upstream to pkg/browser
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.
Thank you for the review. I agree, this work should ideally be part of upstreampkg/browser. But Thoughts on moving forward with the work for Coder until I have the patch ready for upstream?
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.
Yeah, I think it makes sense to make this change now for sure! No objection to it :)
Thanks for your contribution!
Issue
coder loginfails to open user's browser while running fromWindows Subsystem for Linux.Changes
coder loginto work while running from WSL.microsoftin/proc/versionfile.