-
Notifications
You must be signed in to change notification settings - Fork 699
limactl help and info flag should show available plugins #3973
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
limactl help and info flag should show available plugins #3973
Conversation
b9c68fd
to
2b71c8e
Compare
I'm wondering if the plugins could also be added to tab completion (could be in a separate PR if it is more complex). |
2b71c8e
to
f81dbb6
Compare
It can be done in another PR |
f81dbb6
to
ed08b59
Compare
pkg/plugin/plugin.go
Outdated
return false | ||
} | ||
|
||
func getPluginDescription(path string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like a very useful way to find a description; the best it can deliver is "Alias for foo", which might just as well be wrong because it likely has some modified behaviour too.
If we want to provide descriptions, then I think we need to define some kind of comment pattern we can scan for. And since the plugin can be a compiled binary, we would need to scan the whole file for it because the string would be in some kind of data segment in the file.
So I would suggest to use some kind of delimiters that don't look to horrible in a shell script, and would also be unique in a compiled executable. E.g.
# <limactl> Run MCP servers in a sandbox </limactl>
Trim whitespace from the text between delimiters and you have your description.
This is just a suggestion; better ideas welcome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did think about this, but I wasn't sure if it was necessary since it's just an Alias which was why I sort of wen with the "Alias for foo" approach. But since you've mentioned it, I'll just add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've thought about this some more, and think my suggestion is too much of a hack.
I think this is better:
- Call
limactl-PLUGIN --help
- If the exit code is not zero → no description
- If the first line of the output contains the string
--help
→ no description - The description is the first line of the output
This automatically makes limactl PLUGIN --help
too, and even piggy-backs on existing commands, if they are just aliases:
❯ limactl ls --help | head -1
List instances of Lima.
So if the plugin is just an alias, it will use the first line of the help of the original command as the description.
Sorry if you already started work on the delimiter-based approach, but I think this one will be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If the first line of the output contains the string
--help
→ no description
Actually, I would reject the description if it contains any of these strings:
--
invalid
illegal
unrecognized
unexpected
unknown
missing
too many
too few
not found
not supported
usage:
error
failed
This should catch the case where the plugin just passes the --help
argument to another tool that doesn't implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be considered vulnerable to execute all limactl-*
binaries under libexec
and $PATH
by default?
Maybe we can depend on xattr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to summarize: I believe we have settled on using the --help
approach (including documentation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Call limactl-PLUGIN --help
- If the exit code is not zero → no description
- If the first line of the output contains the string --help → no description
- The description is the first line of the output
I have a little issue with step4. In the case where the plugin logs an item before before the actual command is executed, it won't be accurate, and we might end up displaying something inaccurate in the description.
Here's an example.
#!/bin/sh
# Sample limactl-shell alias that shows running instances
# <limactl> Run shell script </limactl>
echo "Running limactl-shell alias with arguments: $@"
limactl shell "$@"
The first item that'd be logged when this plugin is executed will be
Running limactl-shell alias with arguments: --help
And then we end up with something like
Available Plugins (Experimental):
sh Running limactl-shell alias with arguments: --help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, step 3 says if the line contains the string --help
then it is not a description, so this would have no description because your first line ends with --help
.
Note that matching just --help
has been replaced in #3973 (comment) with matching any string from this list:
--
invalid
illegal
unrecognized
unexpected
unknown
missing
too many
too few
not found
not supported
usage:
error
failed
So the first pattern in the list (--
) would have rejected your output as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed the --
. But it means if it's just a plain echo statement without any arg and it doesn't contain any of the restricted words, It'd treated as a description even though it's incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd treated as a description even though it's incorrect.
It is a heuristic, and by definition that means that sometimes it will be wrong.
I think this is a reasonable compromise. You are more likely to get a description if you make it easy for the plugin author.
As a side effect limactl PLUGIN --help
will automatically work too.
And typically the end user is creating/installing the plugin (it is an extension mechanism), so they can fix the issue easily.
The alternative of requiring some special markup before/after the description feels clumsy in comparison.
What other alternatives are there?
ed08b59
to
73cda0c
Compare
pkg/plugin/plugin.go
Outdated
} | ||
|
||
if prefixDir, err := usrlocalsharelima.Prefix(); err == nil { | ||
libexecDir := filepath.Join(prefixDir, "libexec", "lima") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://lima-vm.io/docs/config/plugin/cli/#how-it-works needs to be updated to cover libexec
pkg/plugin/plugin.go
Outdated
func getPluginDirectories() []string { | ||
var dirs []string | ||
|
||
if exe, err := os.Executable(); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just occurred to me that we probably want the logic from usrlocalsharelima.Dir()
here: call executableViaArgs0()
and fall back on os.Executable()
if it fails.
If limactl
is invoked via a symlink, then we want to locate additional plugins via the directory of the symlink and not via the directory where the executable itself is installed.
This is only relevant to Linux; on macOS os.Executable
already returns the symlink path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this, the CLI help strings can be addressed in another PR
Needs rebase |
73cda0c
to
6809c31
Compare
6809c31
to
9e3334f
Compare
@AkihiroSuda can you help reopen this pr. It was closed by mistake after rebase. |
Can't reopen, could you open a new one? |
I have opened another one here |
Fixes #3608 (comment)