-
Notifications
You must be signed in to change notification settings - Fork 601
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
limayaml: add a param
field for defining variables used to customize scripts and other elements within lima.yaml
.
#2498
limayaml: add a param
field for defining variables used to customize scripts and other elements within lima.yaml
.
#2498
Conversation
937b3ad
to
4a2f79c
Compare
I have separated the fix for |
can not reproduce a panic in Unit Test on local machine
|
I also want to include the following in mounts:
- location: "~/.cache/ubuntu/{{.Name}}/var/cache/apt"
mountPoint: "/var/cache/apt"
writable: true
- location: "~/.cache/ubuntu/{{.Name}}/var/lib/apt"
mountPoint: "/var/lib/apt"
writable: true Should this be a separate PR? |
8991fb0
to
ab7f74b
Compare
This PR has 2 warnings from $ yamllint examples/docker.yaml
examples/docker.yaml
58:14 warning too few spaces before comment (comments)
$ yamllint examples/docker-rootful.yaml
examples/docker-rootful.yaml
58:14 warning too few spaces before comment (comments) You can fix by adding another space before the comments, like -- mode: user # configure docker under non-root user
+- mode: user # configure docker under non-root user Personally I don't really agree with the requirement for 2 spaces between content and comments, and would change --- .yamllint
+++ .yamllint
@@ -7,6 +7,8 @@ rules:
indent-sequences: false
truthy:
allowed-values: ['true', 'false', 'on', 'off']
+ comments:
+ min-spaces-from-content: 1
comments-indentation: disable
document-start: disable
line-length: disable |
ab7f74b
to
ae9c4bf
Compare
updated and rebased |
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.
Not sure if I get the time to review this today, but if I do, then I will skip the examples/docker*
changes; I think they should go into a separate PR.
ae9c4bf
to
762d46a
Compare
Updated |
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
@lima-vm/maintainers |
I am not a big fan of mixing in upper case, with the rest of the yaml that uses lower case... Even if it means having to "translate" a param: key: value into a {{Param.Key}} so that they can each look their best |
To reference |
I believe there is no way to reference non-exported fields from Go templates, so I think what @afbjorklund meant was that you have this in your param:
containerdImageStore: false
rootful: false And we would automatically capitalize the first letter when putting it into the template data struct, so you could reference it as Now you could argue that we are already transforming Personally I don't like this, and I think I would maybe go a completely different way, and use the same conventions I would use for environment variables for params: param:
CONTAINERD_IMAGE_STORE: false
ROOTFUL: false And then reference them as What do other people think? Assuming we are not implementing @afbjorklund's suggestion to auto-capitalize param names (I would vote against it), we should catch parameters starting with a lowercase letter in validation. |
In my local tests, #2515 actually works when I change it to: param:
ContainerdImageStore: true
rootful: true and use
The reason it is |
Thanks, didn't think of that. In that case I'm even more against automatically capitalizing the first letter of params.
I understand. And the same way we could capitalize the first letter of I just think we shouldn't. It is also possible that I misunderstand what @afbjorklund is asking for. Just to be clear: I think the code should be left the way it is right now. |
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, LGTM, except it needs a comment to show that probes now do template expansion, and the regex for checking if a param is used needs to loosen up.
I think it is, and would like to see it included! |
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, looks good, but can you please squash commits?
…e scripts and other elements within `lima.yaml`. The defined variables can be referenced within `lima.yaml` as `{{.Param.Key}}`. The fields where this can be utilized are as follows: - `provision[%d].script` - `probes[%d].script` - `copyToHost[%d].{guest,host}` - `portForwards[%d].{guestSocket,hostSocket}` Signed-off-by: Norio Nomura <norio.nomura@gmail.com> limayaml: add a check to verify whether the variables defined in `param` are actually used This check needs to be performed before the template is expanded, so it should be added before calling `FillDefault()`. Signed-off-by: Norio Nomura <norio.nomura@gmail.com> limayaml: add `{{if .Param.rootful}}{{else}}{{end}}` pattern to `TestValidateParamIsUsed` Signed-off-by: Norio Nomura <norio.nomura@gmail.com> default.yaml: add a comment on the `probes` that supports variable substitution Signed-off-by: Norio Nomura <norio.nomura@gmail.com>
39b9791
to
6331b56
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
Thanks! 🙏🏻 |
That was indeed my suggestion, and it was purely aesthetical (there was something "wrong" to me, with Key = value) Now the params will work like environment variables, and I suppose there is some Lowest Common Denominator in that. But I seem to always be able to make things worse... :-) |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [lima-vm/lima](https://github.com/lima-vm/lima) | minor | `v0.22.0` -> `v0.23.1` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>lima-vm/lima (lima-vm/lima)</summary> ### [`v0.23.1`](https://github.com/lima-vm/lima/releases/tag/v0.23.1) [Compare Source](lima-vm/lima@v0.23.0...v0.23.1) #### Changes - Fixed the CI to generate the release note ([#​2555](lima-vm/lima#2555)) #### Usage ```console [macOS]$ limactl create [macOS]$ limactl start ... INFO[0029] READY. Run `lima` to open the shell. [macOS]$ lima uname Linux ``` *** The binaries were built automatically on GitHub Actions. The build log is available for 90 days: https://github.com/lima-vm/lima/actions/runs/10441930092 The sha256sum of the SHA256SUMS file itself is `e93a48f3a011c25367da50ab3609bb28437fcde259371f005f8b234caa46efff` . *** Release manager: [@​AkihiroSuda](https://github.com/AkihiroSuda) ### [`v0.23.0`](https://github.com/lima-vm/lima/releases/tag/v0.23.0) [Compare Source](lima-vm/lima@v0.22.0...v0.23.0) - YAML: - Add a `param` field for defining variables ([#​2498](lima-vm/lima#2498), thanks to [@​norio-nomura](https://github.com/norio-nomura)) - vz: - Prioritize rosetta over qemu-user-static ([#​2474](lima-vm/lima#2474), thanks to [@​norio-nomura](https://github.com/norio-nomura)) - Configura AOT caching options using an abstract socket ([#​2489](lima-vm/lima#2489), thanks to [@​norio-nomura](https://github.com/norio-nomura)) - Templates: - add `alpine-image` ([#​2360](lima-vm/lima#2360), thanks to [@​jandubois](https://github.com/jandubois)) - remove `centos-stream-8`, `deprecated/centos-7` ([#​2457](lima-vm/lima#2457)) - update to the latest revisions ([#​2553](lima-vm/lima#2553)) - Governance: - MAINTAINERS: invite Oleksandr Redko ([@​alexandear](https://github.com/alexandear)) as a Reviewer ([#​2383](lima-vm/lima#2383)) Full changes: https://github.com/lima-vm/lima/milestone/46?closed=1 Thanks to [@​AdamKorcz](https://github.com/AdamKorcz) [@​AmedeeBulle](https://github.com/AmedeeBulle) [@​SmartManoj](https://github.com/SmartManoj) [@​afbjorklund](https://github.com/afbjorklund) [@​alexandear](https://github.com/alexandear) [@​danchr](https://github.com/danchr) [@​fwilhe2](https://github.com/fwilhe2) [@​jandubois](https://github.com/jandubois) [@​norio-nomura](https://github.com/norio-nomura) [@​tcooper](https://github.com/tcooper) [@​why168](https://github.com/why168) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
The defined variables can be referenced within
lima.yaml
as{{.Param.Key}}
.The fields where this can be utilized are as follows:
provision[%d].script
probes[%d].script
copyToHost[%d].{guest,host}
portForwards[%d].{guestSocket,hostSocket}
It also changes:
limayaml
: add a check to verify whether the variables defined inparam
are actually used.Following change has been merged as a separated PR: #2501
limactl start
: fix the issue where using--set
would overwrite the existing instance'slima.yaml
without validation.Following changes will be opened as a separated PR: #2515
docker.yaml
: add.param.ContainerdImageStore
By passing the
--set .param.ContainerdImageStore=true
option tolimactl {create,start,edit}
, the.features."containerd-snapshotter"
option will be enabled indocker/daemon.json
inside the VM.docker.yaml
: add.param.Rootful
By passing the
--set .param.Rootful=true
option tolimactl {create,start,edit}
, Docker inside the VM will run in rootful mode.docker-rootful.yaml
: make everything common except for setting.param.Rootful=true
indocker.yaml
.It might be better to split this into multiple parts, but for now, I’ll open it as a draft.
Thanks,