-
Notifications
You must be signed in to change notification settings - Fork 77
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
feature(cli): Adding dashboard command #2490
Conversation
cli/utils/common.go
Outdated
@@ -34,3 +38,21 @@ func StringReferencesFile(path string) bool { | |||
// otherwise the user also could send a directory by mistake | |||
return info != nil && !info.IsDir() | |||
} | |||
|
|||
func OpenBrowser(url string) 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.
Philosophical question: should we detect if the client has the UI enabled on the SO before opening the browser to emit a warning and stop the command?
I was thinking of something to warn the user if they are trying to execute this command in a CI container.
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 agreed on just launching without any other checks, because there is a large number of things that can go wrong and it's difficult to check them all, at least for the initial version
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.
Looks good except for a few non-go-looking bits
cli/utils/common.go
Outdated
@@ -34,3 +38,21 @@ func StringReferencesFile(path string) bool { | |||
// otherwise the user also could send a directory by mistake | |||
return info != nil && !info.IsDir() | |||
} | |||
|
|||
func OpenBrowser(url string) 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.
I think we agreed on just launching without any other checks, because there is a large number of things that can go wrong and it's difficult to check them all, at least for the initial version
Hey @schoren thanks for the feedback!, I just updated the PR with the fixes and improvements you mentioned |
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.
Looking great!
* feature(cli): adding dashboard command * feature(cli): adding dashboard command
* feature(cli): adding dashboard command * feature(cli): adding dashboard command
This PR adds the dashboard command that opens the configured tracetest server URL using the default browser
Changes
Fixes
Checklist