Add command to open web dashboard #11

radu-matei · 2019-06-21T04:44:37Z

This PR adds a command to open the web dashboard, by calling brig dashboard --port {config} --open-dashboard {config}.

Ensure Brigade terminal exists Signed-off-by: Radu M <[email protected]>

radu-matei · 2019-06-21T04:46:57Z

package.json

@@ -33,6 +33,14 @@
                        "namespace": {
                            "type": "string",
                            "description": "Namespace where Brigade is configured in your cluster"
+                        },
+                        "port": {


I debated whether to add this as a picker, or as a config - and ultimately, I don't think this is something you want to configure every time.

radu-matei · 2019-06-21T04:47:42Z

package.json

+                            "type": "integer",
+                            "description": "Local port for the web dashboard. Default 8081"
+                        },
+                        "open-dashboard": {


Should this be a picker?
I feel about it the same way as for the port, but I can be persuaded otherwise.

If you wanted to make it optional, you could have a notification: Dashboard is available on http://localhost:8081. with a button Open in Browser?

Currently, the browser-opening functionality is baked into brig - so you would either have to do it before starting the tunnel, or reimplement the functionality.

By default it is enabled though.

src/brigade/brigade.ts

radu-matei · 2019-06-21T05:17:02Z

package.json

+                        },
+                        "port": {
+                            "type": "integer",
+                            "description": "Local port for the web dashboard. Default 8081"


Is the default value actually used by VS Code if no configuration is provided?
The description of the default field says: A default value. Used by suggestions, which makes me think the value is not actually used.

itowlson

Aaugh, sorry to nitpick another of your PRs, but I have some concerns about the naming and about the UI of the port-forwarding process...

itowlson · 2019-06-21T05:18:57Z

package.json

+                        },
+                        "open-dashboard": {
+                            "type": "boolean",
+                            "description": "Open the dashboard in the browser. Default true"


I would skip this. What does false mean anyway - forward the local port but don't open the browser?

Yes, it is the same UX as brig dashboard.

itowlson · 2019-06-21T05:19:35Z

package.json

+                        },
+                        "port": {
+                            "type": "integer",
+                            "description": "Local port for the web dashboard. Default 8081"


Shouldn't really be the user's problem should it? Can't we use something like port-finder to pick an arbitrary open port?

itowlson · 2019-06-21T05:20:06Z

package.json

+            {
+                "command": "brigade.openWebDashboard",
+                "category": "Brigade",
+                "title": "Open Web Dashboard"


Do we need the word 'Web' here?

Naming due to #12

itowlson · 2019-06-21T05:27:02Z

src/brigade/brigade.ts

+    const cmd = `${bin} dashboard ${args}`;
+
+    const terminal = ensureTerminal();
+    terminal.sendText(cmd);


We had this in the k8s extension and I have come to be less and less keen on it. For the API I ended up not doing port-forwarding in a terminal, because the purpose of the port forward API was to enable extensions to open their Web dashboards, and port forwarding was an implementation detail that shouldn't take over that much of the screen. Instead, I put an icon in the status bar to indicate that one or more ports were being forwarded. The user could click that icon to see what ports were being forwarded, and why, and to close ones that were no longer needed. I'd prefer to see us adopt something like that approach here, rather than the in-yer-face terminal. k8s implementation is at https://github.com/Azure/vscode-kubernetes-tools/blob/795246a8b9f29cdd7ac8d0d9004aad3eb5972a3f/src/api/implementation/kubectl/v1.ts#L21 though not directly portable!

An alternative approach would be to just keep the port open until the end of the editor session with no UI at all, but a Close Dashboard command for those who really really care about the port being kept open (or the brig process being kept alive).

itowlson · 2019-06-21T05:27:22Z

src/config/config.ts

@@ -17,3 +17,16 @@ export function toolPath(tool: string): string | undefined {
 export function getConfiguredNamespace(): string | undefined {
    return vscode.workspace.getConfiguration(EXTENSION_CONFIG_KEY)['namespace'];
 }
+
+export function getConfiguredPort(): number | undefined {


getDashboardPort()?

itowlson · 2019-06-21T05:28:00Z

package.json

@@ -33,6 +33,14 @@
                        "namespace": {
                            "type": "string",
                            "description": "Namespace where Brigade is configured in your cluster"
+                        },
+                        "port": {


From the name, this could be any port. Can we make it clearer that it's the port to use when opening the dashboard?

itowlson · 2019-06-21T05:28:22Z

package.json

+                            "type": "integer",
+                            "description": "Local port for the web dashboard. Default 8081"
+                        },
+                        "open-dashboard": {


Clarify "in browser"?

Initially, I wanted to preserve the CLI flags in the config, which might have not been the ideal choice.

itowlson · 2019-06-21T05:29:33Z

package.json

+                            "type": "integer",
+                            "description": "Local port for the web dashboard. Default 8081"
+                        },
+                        "open-dashboard": {


If you wanted to make it optional, you could have a notification: Dashboard is available on http://localhost:8081. with a button Open in Browser?

itowlson · 2019-06-21T05:30:50Z

src/config/config.ts

+}
+
+export function openDashboard(): boolean {
+    const cfg: boolean = vscode.workspace.getConfiguration(EXTENSION_CONFIG_KEY)['open-dashboard'];


This reads like "open the dashboard"; consider e.g. "shouldOpenDashboardInBrowser".

itowlson · 2019-06-21T05:35:01Z

src/extension.ts

@@ -7,6 +7,7 @@ import { BrigadeResourceDocumentProvider } from './documents/brigaderesource.doc
 import { hasResourceURI } from './projectexplorer/node.hasresourceuri';
 import { onCommandRunProject } from './commands/runproject';
 import { onCommandRerunBuild } from './commands/rerunbuild';
+import { openWebDashboard } from './brigade/brigade';


I would rather command handlers not go straight into the CLI wrapper. I know it works, but it feels like a mixing of abstraction levels. The brigade module's job is to surface the brig CLI to the extension UI; although in this case there may be nothing for the UI to do other than invoke the CLI (because the CLI provides all the UI!), it feels uncomfortable not to have a separate place for the command to land. Maybe I am being silly though.

Add command to open web dashboard

5df3072

Ensure Brigade terminal exists Signed-off-by: Radu M <[email protected]>

radu-matei force-pushed the web-dashboard branch from 4185c50 to 24bbf67 Compare June 21, 2019 04:46

radu-matei commented Jun 21, 2019

View reviewed changes

src/brigade/brigade.ts Show resolved Hide resolved

radu-matei force-pushed the web-dashboard branch from 24bbf67 to 5df3072 Compare June 21, 2019 05:09

radu-matei requested a review from itowlson June 21, 2019 05:10

radu-matei commented Jun 21, 2019

View reviewed changes

radu-matei mentioned this pull request Jun 21, 2019

[WIP] Brigadeterm #12

Open

itowlson suggested changes Jun 21, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command to open web dashboard #11

Add command to open web dashboard #11

radu-matei commented Jun 21, 2019

radu-matei Jun 21, 2019

radu-matei Jun 21, 2019

itowlson Jun 21, 2019

radu-matei Jun 21, 2019

radu-matei Jun 21, 2019 •

edited

Loading

itowlson left a comment

itowlson Jun 21, 2019

radu-matei Jun 21, 2019

itowlson Jun 21, 2019

itowlson Jun 21, 2019

radu-matei Jun 21, 2019

itowlson Jun 21, 2019

itowlson Jun 21, 2019

itowlson Jun 21, 2019

itowlson Jun 21, 2019

radu-matei Jun 21, 2019

itowlson Jun 21, 2019

itowlson Jun 21, 2019

itowlson Jun 21, 2019

Add command to open web dashboard #11

Are you sure you want to change the base?

Add command to open web dashboard #11

Conversation

radu-matei commented Jun 21, 2019

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

radu-matei Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

itowlson 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

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

radu-matei Jun 21, 2019 •

edited

Loading