-
Notifications
You must be signed in to change notification settings - Fork 707
Add a plugin mechanism to provide custom locator URL schemes #3937
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
Conversation
00bb585
to
824daed
Compare
I just realized that the ❯ limactl url-github lima-vm/lima
https://raw.githubusercontent.com/lima-vm/lima/master/lima.yaml
❯ limactl url-my default
https://raw.githubusercontent.com/lima-vm/lima/master/templates/default.yaml If we don't want this, then we need to use a longer prefix for plugins, like |
I've just tested, and you can use tags or commit ids instead of branch names too. These all work: ❯ limactl tmpl copy github:lima-vm/lima/examples/[email protected] -
❯ limactl tmpl copy github:lima-vm/lima/examples/opensuse@74e2fda81 - |
This looks great!
I did say "something like" in the request to leave room for improvement! |
824daed
to
6e87a32
Compare
I've updated the PR by refactoring the code to run subcommands with the
|
6e87a32
to
ad26a40
Compare
What is the remaining task to merge this PR? CI? |
I was planning to move to Either or both could happen in separate PRs to keep the PR easier to review. |
ad26a40
to
f6ceb58
Compare
I think it will be best to create separate PRs for further changes, to keep things easy for review. I've made one more change in this PR: I've appended I've noticed that we also use |
5a1ab38
to
6032d5f
Compare
cmd/limactl/main.go
Outdated
if err != nil || cmd == rootCmd { | ||
// Function calls os.Exit() if it found and executed the plugin | ||
runExternalPlugin(rootCmd.Context(), args[0], args[1:]) | ||
_ = executil.WithExecutablePath(func() error { |
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.
Why ignore err?
cmd/limactl/template.go
Outdated
return err | ||
} | ||
|
||
func newTemplateURLCommand() *cobra.Command { |
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.
Needs docs and tests
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.
Docs can be added under https://deploy-preview-3972--lima-vm.netlify.app/docs/config/plugin after merging:
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 just created another URL scheme for testing: #!/bin/bash
echo "${LIMA_HOME:-$HOME/.lima}/$1/lima.yaml" With this you can create another instance with identical config like this (but unlike the ❯ limactl create -y --name clone instance:default And of course it works all the usual places: Not sure though if it is useful enough to be builtin. |
This is quite confusing; |
Yeah, let's not include it in Lima. I think the only scheme we want to have builtin is |
Needs rebase |
Extension ideas:
|
5dde020
to
5d72cc0
Compare
I think this PR is ready for review. Still missing, but could be done in several separate PRs:
|
Also fix the global option processing so that they are not passed on to the plugin commands. Signed-off-by: Jan Dubois <[email protected]>
d306d12
to
97a9d84
Compare
The template reader will attempt to call `limactl-url-$SCHEME` to rewrite a custom URL as either a local filename, or a URL with a supported URL like https. Signed-off-by: Jan Dubois <[email protected]>
97a9d84
to
8ed0180
Compare
I've made that change as well now (in a separate commit), but I think this PR is getting too big. Can we get this reviewed and merged, and do the remaining tasks in separate PRs? |
8ed0180
to
e46309e
Compare
README.md
Outdated
To run containers with Docker: | ||
```bash | ||
limactl start template://docker | ||
limactl start template:docker |
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.
The old form should be kept until people actually begin using v2
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 kind of disagree. The website should document the released version, but the README.md
should correspond to the state of the branch.
Otherwise we could also not close issues until their PRs have been included in a new release, which would make the whole workflow really awkward.
I'll revert for now, but after 2.0 we should rethink our workflow, and also figure out how to have a version of the docs for the last release, and a different version for the head of master
.
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.
Regardless to our intention, people will just refer to the README in the default branch and will continue opening "I followed README but it doesn't work" issues when the content conflicts with the latest release, so we have to care about them
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.
Regardless to our intention, people will just refer to the README in the default branch
I think the way to deal with that is to remove end-user instructions from the README and point them to the docs. There is no reason to repeat usage documentation in it.
README
Lima is Linux Machines running in a VM...
How to install and use?
Please read the [Documentation] and follow the [Installation] instructions.
Community channels
- New releases on GitHub
- GitHub Discussions and Issues
- Slack
- Community meetings (maybe?)
- Social media (if we start posting there)
Developing Lima
Required tools and their version
How to build
How to run tests
License
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 still prefer to keep the "TLDR" abstract in README.md
.
First comers do not want to access the website when they do not even know what it is all about.
e46309e
to
942bb17
Compare
The old style template://name URLs are malformed because they do not contain a valid AUTHORITY (host name), but treat the host name as part of the path. They are still fully supported, but will emit a warning. Signed-off-by: Jan Dubois <[email protected]>
942bb17
to
677fa1f
Compare
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.
Thanks
This is a reimplementation of the bash code in lima-vm#3937 (comment) Signed-off-by: Jan Dubois <[email protected]>
This is a reimplementation of the bash code in lima-vm#3937 (comment) Signed-off-by: Jan Dubois <[email protected]>
This is a reimplementation of the bash code in lima-vm#3937 (comment) Signed-off-by: Jan Dubois <[email protected]>
On the second thought, a locator plugin should return the YAML contents, not HTTPS URL, so as to support non-HTTP and non-local remotes |
This is a reimplementation of the bash code in lima-vm#3937 (comment) Signed-off-by: Jan Dubois <[email protected]>
This is a reimplementation of the bash code in lima-vm#3937 (comment) Signed-off-by: Jan Dubois <[email protected]>
This is a reimplementation of the bash code in lima-vm#3937 (comment) Signed-off-by: Jan Dubois <[email protected]>
This is a reimplementation of the bash code in lima-vm#3937 (comment) Signed-off-by: Jan Dubois <[email protected]>
This is a reimplementation of the bash code in lima-vm#3937 (comment) Signed-off-by: Jan Dubois <[email protected]>
The template reader will attempt to call
limactl-url-$SCHEME
to rewrite a custom URL as either a local filename, or a URL with a supported scheme likehttps
.This change allows us to experiment with custom schemes, like the one requested in #3930.
Example implementation for the
github:
scheme is at the end. You can use:FILE
defaults tolima
, and files without an extension will have.yaml
appended.The default branch is determined by the GitHub API if no
BRANCH
is specified.This implementation has a few differences to the one requested in #3930:
master
lima.yaml
instead oftemplates/default.yaml
as the default fileI believe this implementation is more versatile, but we can discuss this below. The plugin format makes it easy to experiment.
Here is a minimalistic
my:
scheme to fetch default templates from the Lima repo:And here is
limactl-url-github
. Which should become a builtin scheme (implemented in Go) once we agree on the specific semantics: