Skip to content
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

Merged
merged 3 commits into from
May 5, 2023

Conversation

xoscar
Copy link
Collaborator

@xoscar xoscar commented May 4, 2023

This PR adds the dashboard command that opens the configured tracetest server URL using the default browser

Changes

  • Adds new dashboard command to open default server url
  • Validates if the config is not empty

Fixes

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@xoscar xoscar linked an issue May 4, 2023 that may be closed by this pull request
@xoscar xoscar self-assigned this May 4, 2023
@xoscar xoscar added the CLI label May 4, 2023
@xoscar xoscar marked this pull request as ready for review May 4, 2023 21:23
@@ -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 {
Copy link
Contributor

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.

Copy link
Collaborator

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

Copy link
Collaborator

@schoren schoren left a 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 Show resolved Hide resolved
@@ -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 {
Copy link
Collaborator

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

cli/cmd/dashboard_cmd.go Outdated Show resolved Hide resolved
cli/cmd/dashboard_cmd.go Show resolved Hide resolved
@xoscar
Copy link
Collaborator Author

xoscar commented May 5, 2023

Hey @schoren thanks for the feedback!, I just updated the PR with the fixes and improvements you mentioned

Copy link
Collaborator

@schoren schoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great!

@xoscar xoscar merged commit 3a81cf0 into main May 5, 2023
@xoscar xoscar deleted the 2306-cli-improvements-add-dashboard-command branch May 5, 2023 19:35
schoren pushed a commit that referenced this pull request May 9, 2023
* feature(cli): adding dashboard command

* feature(cli): adding dashboard command
schoren pushed a commit that referenced this pull request Jun 5, 2023
* feature(cli): adding dashboard command

* feature(cli): adding dashboard command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CLI Improvements] Add dashboard command
4 participants