Skip to content
This repository has been archived by the owner on Jun 1, 2022. It is now read-only.

Add command to open web dashboard #11

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,14 @@
"namespace": {
"type": "string",
"description": "Namespace where Brigade is configured in your cluster"
},
"port": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

"type": "integer",
"description": "Local port for the web dashboard. Default 8081"
Copy link
Contributor Author

@radu-matei radu-matei Jun 21, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

},
"open-dashboard": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify "in browser"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"type": "boolean",
"description": "Open the dashboard in the browser. Default true"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is the same UX as brig dashboard.

}
}
}
Expand Down Expand Up @@ -62,6 +70,11 @@
"light": "images/light/refresh.svg",
"dark": "images/dark/refresh.svg"
}
},
{
"command": "brigade.openWebDashboard",
"category": "Brigade",
"title": "Open Web Dashboard"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the word 'Web' here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naming due to #12

}
],
"viewsContainers": {
Expand Down Expand Up @@ -132,4 +145,4 @@
"@types/shelljs": "^0.8.5",
"shelljs": "^0.8.3"
}
}
}
25 changes: 25 additions & 0 deletions src/brigade/brigade.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { parseTable } from './parsers';
import { Age } from '../utils/age';

const logChannel = vscode.window.createOutputChannel("Brigade");
const terminalName = "Brigade";

async function invokeObj<T>(sh: shell.Shell, command: string, args: string, opts: shell.ExecOpts, fn: (stdout: string) => T): Promise<Errorable<T>> {
const bin = config.brigPath() || 'brig';
Expand Down Expand Up @@ -83,3 +84,27 @@ export function rerun(sh: shell.Shell, buildId: string): Promise<Errorable<Proje
}
return invokeObj(sh, 'rerun', `${buildId}`, {}, parse);
}

export async function openWebDashboard(): Promise<void> {
const bin = config.brigPath() || 'brig';
const brigadeNamespace = config.getConfiguredNamespace() || 'default';
const port = config.getConfiguredPort() || 8081;
const open = config.openDashboard();
const args = `--namespace ${brigadeNamespace} --port ${port} --open-dashboard=${open}`;
const cmd = `${bin} dashboard ${args}`;

const terminal = ensureTerminal();
terminal.sendText(cmd);
radu-matei marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

terminal.show();
}

function ensureTerminal(): vscode.Terminal {
const terminals = vscode.window.terminals;
for (const term of terminals) {
if (term.name === terminalName) {
return term;
}
}

return vscode.window.createTerminal(terminalName);
}
13 changes: 13 additions & 0 deletions src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

getDashboardPort()?

return vscode.workspace.getConfiguration(EXTENSION_CONFIG_KEY)['port'];
}

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

Choose a reason for hiding this comment

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

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

if (cfg == undefined) {
return true;
}

return cfg;
}
2 changes: 2 additions & 0 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


let currentExtensionContext: vscode.ExtensionContext | null = null;

Expand All @@ -21,6 +22,7 @@ export function activate(context: vscode.ExtensionContext) {
vscode.commands.registerCommand("brigade.runProject", onCommandRunProject),
vscode.commands.registerCommand("brigade.rerunBuild", onCommandRerunBuild),
vscode.commands.registerCommand("brigade.refreshProjectExplorer", onCommandRefreshProjectExplorer),
vscode.commands.registerCommand("brigade.openWebDashboard", openWebDashboard),

// Documents
vscode.workspace.registerTextDocumentContentProvider(BrigadeResourceDocumentProvider.scheme(), new BrigadeResourceDocumentProvider()),
Expand Down