feat: Add sandbox integration - #436
Conversation
0320422 to
0ff74af
Compare
Signed-off-by: Dragos Gheorghioiu <dragosg@unikraft.com>
f38a068 to
69ab70e
Compare
|
Can you update the description with info on how to run the plugin? Also probably worth looking at https://linear.app/unikraft/issue/TOOL-1319/publish-plugins-images. |
There was a problem hiding this comment.
when i type the screen flickers 🤔
❯ gnome-terminal --version
# GNOME Terminal 3.60.0 using VTE 0.84.0 +BIDI +GNUTLS +ICU +SYSTEMD
There was a problem hiding this comment.
If the plugin isn't loaded, it should give a reasonable error, not:
error: failed on "staging" client: failed to start command: performing the request: request failed: 404 Not Found
parsing response: invalid character '<' looking for beginning of value
| } | ||
|
|
||
| if line == "clear" { | ||
| print("\033[H\033[2J") |
There was a problem hiding this comment.
We should use ansi / lipgloss, instead of manually constructed escape sequences.
There was a problem hiding this comment.
Let's move this and all other shell stuff into internal/shell. shell is a complicated command, so let's keep all of it together.
| ) | ||
|
|
||
| type ( | ||
| SandboxInstanceCmd []string |
There was a problem hiding this comment.
just a personal preference here
i will remove it
There was a problem hiding this comment.
Generally, we're missing full-on integration tests. I don't really think we need tests that the parsing is correct, I'm more interested that this works against a deployed instance with a sandbox.
| fields := shellFields(line) | ||
| if len(fields) == 0 { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Because we're not using the evaluator things in mvdan.sh, it means that things like volumes mounted > out don't work. Also echo $(volumes mounted) won't work, because it's not present in the shell.
If we really don't want to use the evaluator (and want to directly rely on the host's shell), we need to be clear that these are special commands (some sort of prefix, maybe /, like /volumes mounted)?
There was a problem hiding this comment.
the builtins are documented in the help menu but i can make the fact clear that they cannot run on the sandbox directly
maybe some different syntax highlighting for them can also be a plus
There was a problem hiding this comment.
Indeed, but we should be very clear that they are evaluated differently - different syntaxing is a plus, but also additional syntax to really clarify is useful too. Otherwise, users will attempt to use it like I showed, and will be surprised that it doesn't.
| var knownBuiltins = map[string]bool{ | ||
| "get": true, "help": true, "restart": true, "start": true, | ||
| "stop": true, "suspend": true, "mount": true, "unmount": true, | ||
| "edit": true, "volumes": true, "history": true, | ||
| } |
There was a problem hiding this comment.
Can't we derive this from shell_builtins.go? Or determine it based on error?
| var cmd ShellCmd | ||
| parser, err := kong.New(&cmd, | ||
| kong.Name(""), | ||
| kong.NoDefaultHelp(), | ||
| kong.Exit(func(int) {}), | ||
| kong.Writers(sctx.Out, sctx.ErrOut), | ||
| ) | ||
| if err != nil { | ||
| fmt.Fprintf(sctx.ErrOut, "%s failed to initialize shell parser: %v\n", shell.ShellErrorStyle.Render("error:"), err) | ||
| return true | ||
| } |
There was a problem hiding this comment.
I think we may be able to cache this between calls. It's the same each time we run it right? (except for Out and ErrOut, but I think you can set those after)
Just trying to avoid needing to do reflection on every single command run.
| func builtinHelp(out io.Writer) { | ||
| fmt.Fprintln(out, shell.ShellTitleStyle.Render("Builtins:")) | ||
| fmt.Fprintln(out) | ||
| fmt.Fprintf(out, " %s %s\n", shell.ShellValueStyle.Render("cd"), shell.ShellHintStyle.Render("change the current remote directory")) | ||
| fmt.Fprintf(out, " %s %s\n", shell.ShellValueStyle.Render("get"), shell.ShellHintStyle.Render("inspect the current instance")) | ||
| fmt.Fprintf(out, " %s %s\n", shell.ShellValueStyle.Render("edit"), shell.ShellHintStyle.Render("edit instance fields (env, args, memory, vcpus, tags)")) | ||
| fmt.Fprintf(out, " %s %s\n", shell.ShellValueStyle.Render("volumes"), shell.ShellHintStyle.Render("list volumes mounted on this instance (alias for volumes mounted)")) | ||
| fmt.Fprintf(out, " %s %s\n", shell.ShellValueStyle.Render("volumes mounted"), shell.ShellHintStyle.Render("list volumes mounted on this instance")) |
There was a problem hiding this comment.
Let's build this as a table. Or even use a help printer, similar to how kingkong does it, instead of repeating it over and over.
| // Sandbox serves the sandbox plugin API of the same metro. It cannot be | ||
| // embedded alongside platform.Client, since both name their interface | ||
| // Client. |
2 participants
No description provided.