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

Add command flags as quick picks #13

Merged
merged 27 commits into from
Jun 6, 2021
Merged

Add command flags as quick picks #13

merged 27 commits into from
Jun 6, 2021

Conversation

arknable
Copy link
Contributor

No description provided.

@arknable arknable self-assigned this May 30, 2021
@arknable arknable linked an issue May 30, 2021 that may be closed by this pull request
let args = ["build", filePath];

if (configPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we losing the ability to use a config file to build images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh sorry, that code is currently not being used because build command is not asking config file. I will add it back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added back by 86b372b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably we should create new issue to add "Build with Config" command ?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. It's a useful command.

src/cmd/cmdHandler.ts Show resolved Hide resolved
src/cmd/cmdHandler.ts Outdated Show resolved Hide resolved

let mounts = await vscode.window.showInputBox({
title: "Mount Points",
placeHolder: "Comma-separated [volume_id:mount_path]"
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires users to know the volume id beforehand. It would be easier for users to select a volume from the list.

Check this example as inspiration https://github.com/microsoft/vscode-extension-samples/tree/main/quickinput-sample. This would facilitate a lot the selection of a volume.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by 069a777

Copy link
Contributor

Choose a reason for hiding this comment

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

I could select an existing volume but wasn't able to mount it on the unikernel. This is what console output shows after executing Ops: Run with a volume selected args: run,/mnt/d/Go/src/github.com/fabiodmferreira/walk-server/main,--imagename,lorem. The argument seems to not be added on executing ops.

UUIDs aren't human friendly. It would be nice to add the volume name on the select options.

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 can see the volume_id:mount_path passed correctly:

args: run,/home/arknable/Development/Project/nanovms/example/hello/hello,--imagename,hello,--mounts,b2858b75-cd8e-631b-9f90-af0d89524dc8:/home/arknable/Development/Project/nanovms/tmp/mounts/a

probably you forgot to select volume id or input the mount path @fabioDMFerreira. anyway I will add volume name also.

the flow is we select volumes then for each selected volume, the extension will ask user to input mount point path. if we don't select any volume, --mounts will not be passed to cli. for each selected volume, if we don't input the mount path, that volume will me omitted.

Copy link
Contributor Author

@arknable arknable Jun 4, 2021

Choose a reason for hiding this comment

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

I wish there is a way to customize quick pick form but for now this is the only option provided by vscode afaik.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Volume name added by f5696eb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mount flag usage fixed by f6290db

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, I wasn't selecting the volume. I thought it was enough to click enter on the volumes list, but we need to click on the checkbox. I think this happens because it is a multiselect.

src/lib/ops_default/index.ts Outdated Show resolved Hide resolved
src/cmd/cmdHandler.ts Outdated Show resolved Hide resolved
@fabioDMFerreira
Copy link
Contributor

The code seems good though. After deleting the console.logs and fixing the mounts argument we can merge. Heads up this branch requires rebasing.

@arknable arknable merged commit 26daa1e into master Jun 6, 2021
@arknable arknable deleted the 3-flags-quickpick branch June 6, 2021 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept config file path and other flags on run command
2 participants