Added new CLI command dapr dashboard #383

gdhuper · 2020-06-25T01:54:47Z

Description

Extended dapr cli to launch dapr dashboard in kubernetes.

Runs the Dapr dashboard on a Kubernetes cluster

Usage:
  dapr dashboard [flags]

Flags:
  -h, --help         help for dashboard
  -k, --kubernetes   Start Dapr dashboard in local browser
  -p, --port int     The local port on which to serve dashboard (default 8080)

Issue reference

This PR will close: #341

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Added code comments
Created/updated tests
Extended the documentation

…ashboard

shalabhms · 2020-06-25T19:13:53Z

pkg/kubernetes/portforward.go

+	"k8s.io/client-go/transport/spdy"
+)
+
+type PortForward struct {


Please add comments for exported methods. It is applicable for other methods.

shalabhms · 2020-06-25T19:15:08Z

pkg/kubernetes/portforward.go

+	host string, localPort, remotePort int,
+	emitLogs bool,
+) (*PortForward, error) {
+	time.Sleep(10 * time.Second)


I think this sleep is not needed here, almost it is quite long 10 secs.

Agree, 10 secs feel a bit long. I had to introduce this 10 secs delay because portforwarding would fail since the pod takes couple of seconds to be up and running. Any ideas how to do this idiomatically using client API?

Just looked into it, does this help?

https://gianarb.it/blog/programmatically-kube-port-forward-in-go

Yes, that's the way to do it. Sleep is not needed.

Generally, we should only connect if the dashboard pod is in Ready status.

shalabhms · 2020-06-25T19:17:16Z

pkg/kubernetes/dashboard.go

+
+func InitDashboard() error {
+
+	var dashboardManifestPath string = "https://raw.githubusercontent.com/dapr/dashboard/master/deploy/dashboard.yaml"


It would be better to create const and use that const.

shalabhms · 2020-06-25T19:25:08Z

cmd/dashboard.go

+			false,
+		)
+		if err != nil {
+			print.FailureStatusEvent(os.Stderr, "Failed to initialize port forwarding: %s\n", err)


It would be better if either you can change the error message here to "creating.." or change Line 70 to "error in port forwarding.."
because both messages conveying the same message for 2 different related things..

shalabhms

Thanks for your contribution !

shalabhms · 2020-06-25T19:27:18Z

cmd/dashboard.go

+
+func init() {
+	DashboardCmd.Flags().BoolVarP(&kubernetesMode, "kubernetes", "k", false, "Deploy Dapr dashboard to a Kubernetes cluster")
+	DashboardCmd.Flags().BoolVarP(&open, "open", "o", false, "Open Dapr dashboard in a browser")


Change "open" to "start" .

shalabhms · 2020-06-25T19:32:10Z

@gdhuper , will you also be considering in your changes for invoking dashboard in standalone mode ?

yaron2 · 2020-06-25T19:59:09Z

@gdhuper , will you also be considering in your changes for invoking dashboard in standalone mode ?

We discussed this in the issue, this PR will tackle Kubernetes first. Once we agree on a packaging and distribution format of the dashboard for self-hosted mode, we can later enable it in the CLI.

yaron2 · 2020-06-25T20:00:53Z

This overall looks great!

Yesterday in a maintainers meeting, the following decisions were made:

The dashboard will be installed by default with Helm
The dashboard will be installed by default with dapr init.

Thus, I think we can remove the deployment of the dashboard from this PR, and have dapr dashboard -k open the browser locally IF the dashboard pod is located in the cluster and is in a Ready state.

/cc @gdhuper @shalabhms @willdavsmith

gdhuper · 2020-06-25T20:01:08Z

@gdhuper , will you also be considering in your changes for invoking dashboard in standalone mode ?

yeah so that was the plan initially to support both standalone and kubernetes mode, but turns out standalone mode is a bit tricky with containers. However, as Yaron mentioned in this comment, we can download dashboard as a zip and spin it up in a local dir. IIRC, dapr/dashboard does not have a release pipeline. Any plans on setting one up for dashboard so standalone mode could be supported?

EDIT: I think my question was answered as I was writing this :). I'll go ahead an remove dashboard deployment from this PR. Thanks

shalabhms · 2020-06-25T20:13:03Z

This overall looks great!

Yesterday in a maintainers meeting, the following decisions were made:

The dashboard will be installed by default with Helm

The dashboard will be installed by default with dapr init.

Thus, I think we can remove the deployment of the dashboard from this PR, and have dapr dashboard -k open the browser locally IF the dashboard pod is located in the cluster and is in a Ready state.

/cc @gdhuper @shalabhms @willdavsmith

@yaron2 , that's right. We can remove deployment from dapr dashboard --k but we need to have similar changes in dapr init for kubernetes also for development mode. I think @gdhuper can still bring changes directly in dapr init code path.
Thoughts ?

shalabhms · 2020-06-25T20:35:51Z

This overall looks great!
Yesterday in a maintainers meeting, the following decisions were made:

The dashboard will be installed by default with Helm

The dashboard will be installed by default with dapr init.

Thus, I think we can remove the deployment of the dashboard from this PR, and have dapr dashboard -k open the browser locally IF the dashboard pod is located in the cluster and is in a Ready state.
/cc @gdhuper @shalabhms @willdavsmith

@yaron2 , that's right. We can remove deployment from dapr dashboard --k but we need to have similar changes in dapr init for kubernetes also for development mode. I think @gdhuper can still bring changes directly in dapr init code path.
Thoughts ?

@gdhuper , dapr init -k will take care of deploying dashboard.yaml. So yes, you can remove dashboard deployment from this PR.

Thanks @yaron2 for clarification.

willdavsmith

Looks great! Thanks for doing this.

shalabhms

Thanks for your valuable contribution !

yaron2

LGTM.

Thank you for this.

gdhuper added 7 commits May 31, 2020 00:31

Added files for new command, added cobra command line code for dapr d…

927505e

…ashboard

Merge branch 'master' into gdhuper/dapr_dashboard

dc37ed2

Janky first implementation

852bcdf

WIP: added default host, port for dashboard

f3fd7a4

implemented port-forwarding for dashboard

4009ceb

Resolving merge conflicts

9709844

fixed lint errors

b989134

yaron2 self-requested a review June 25, 2020 02:25

gdhuper changed the title ~~Gdhuper/dapr dashboard~~ Added new CLI command dapr dashboard Jun 25, 2020

amanbha requested review from willdavsmith and shalabhms June 25, 2020 19:00

shalabhms reviewed Jun 25, 2020

View reviewed changes

shalabhms suggested changes Jun 25, 2020

View reviewed changes

removed dashboard deployment, fixed PR comments

8f9882b

gdhuper requested a review from shalabhms June 29, 2020 08:47

willdavsmith approved these changes Jun 29, 2020

View reviewed changes

shalabhms approved these changes Jun 29, 2020

View reviewed changes

yaron2 approved these changes Jun 29, 2020

View reviewed changes

yaron2 merged commit 0a1a0bd into dapr:master Jun 29, 2020

gdhuper mentioned this pull request Nov 30, 2020

REQUEST: New membership for gdhuper dapr/community#40

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new CLI command dapr dashboard #383

Added new CLI command dapr dashboard #383

gdhuper commented Jun 25, 2020 •

edited

Loading

shalabhms Jun 25, 2020

shalabhms Jun 25, 2020

gdhuper Jun 25, 2020 •

edited

Loading

willdavsmith Jun 25, 2020

yaron2 Jun 25, 2020

shalabhms Jun 25, 2020

shalabhms Jun 25, 2020

shalabhms left a comment

shalabhms Jun 25, 2020

shalabhms commented Jun 25, 2020

yaron2 commented Jun 25, 2020

yaron2 commented Jun 25, 2020 •

edited

Loading

gdhuper commented Jun 25, 2020 •

edited

Loading

shalabhms commented Jun 25, 2020 •

edited

Loading

shalabhms commented Jun 25, 2020

willdavsmith left a comment

shalabhms left a comment

yaron2 left a comment


		func InitDashboard() error {

		var dashboardManifestPath string = "https://raw.githubusercontent.com/dapr/dashboard/master/deploy/dashboard.yaml"

Added new CLI command dapr dashboard #383

Added new CLI command dapr dashboard #383

Conversation

gdhuper commented Jun 25, 2020 • edited Loading

Description

Issue reference

Checklist

shalabhms Jun 25, 2020

Choose a reason for hiding this comment

shalabhms Jun 25, 2020

Choose a reason for hiding this comment

gdhuper Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

willdavsmith Jun 25, 2020

Choose a reason for hiding this comment

yaron2 Jun 25, 2020

Choose a reason for hiding this comment

shalabhms Jun 25, 2020

Choose a reason for hiding this comment

shalabhms Jun 25, 2020

Choose a reason for hiding this comment

shalabhms left a comment

Choose a reason for hiding this comment

shalabhms Jun 25, 2020

Choose a reason for hiding this comment

shalabhms commented Jun 25, 2020

yaron2 commented Jun 25, 2020

yaron2 commented Jun 25, 2020 • edited Loading

gdhuper commented Jun 25, 2020 • edited Loading

shalabhms commented Jun 25, 2020 • edited Loading

shalabhms commented Jun 25, 2020

willdavsmith left a comment

Choose a reason for hiding this comment

shalabhms left a comment

Choose a reason for hiding this comment

yaron2 left a comment

Choose a reason for hiding this comment

gdhuper commented Jun 25, 2020 •

edited

Loading

gdhuper Jun 25, 2020 •

edited

Loading

yaron2 commented Jun 25, 2020 •

edited

Loading

gdhuper commented Jun 25, 2020 •

edited

Loading

shalabhms commented Jun 25, 2020 •

edited

Loading