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

limayaml: add a param field for defining variables used to customize scripts and other elements within lima.yaml. #2498

Merged

Conversation

norio-nomura
Copy link
Contributor

@norio-nomura norio-nomura commented Jul 22, 2024

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 in param 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's lima.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 to limactl {create,start,edit}, the .features."containerd-snapshotter" option will be enabled in docker/daemon.json inside the VM.
  • docker.yaml: add .param.Rootful
    By passing the --set .param.Rootful=true option to limactl {create,start,edit}, Docker inside the VM will run in rootful mode.
  • docker-rootful.yaml: make everything common except for setting .param.Rootful=true in docker.yaml.

It might be better to split this into multiple parts, but for now, I’ll open it as a draft.

Thanks,

@norio-nomura
Copy link
Contributor Author

I have separated the fix for limactl start --set into #2501.

@norio-nomura norio-nomura marked this pull request as ready for review July 23, 2024 00:39
@norio-nomura
Copy link
Contributor Author

norio-nomura commented Jul 23, 2024

can not reproduce a panic in Unit Test on local machine
https://github.com/lima-vm/lima/actions/runs/10050557033/job/27778685157?pr=2498#step:5:2584

panic: test timed out after 10m0s
...
FAIL github.com/lima-vm/lima/pkg/guestagent/kubernetesservice 600.059s

@norio-nomura
Copy link
Contributor Author

I also want to include the following in lima.yaml:

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?

@norio-nomura norio-nomura force-pushed the add-param-field-to-limayaml branch 2 times, most recently from 8991fb0 to ab7f74b Compare July 24, 2024 00:31
@jandubois
Copy link
Member

This PR has 2 warnings from yamllint:

$ 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 config instead:

--- .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

@norio-nomura
Copy link
Contributor Author

updated and rebased

@AkihiroSuda AkihiroSuda modified the milestones: v0.22.1, v1.0 Jul 25, 2024
@AkihiroSuda AkihiroSuda requested a review from a team July 25, 2024 18:58
@AkihiroSuda AkihiroSuda modified the milestones: v1.0, v0.23.0 Jul 25, 2024
Copy link
Member

@jandubois jandubois left a 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.

examples/default.yaml Show resolved Hide resolved
examples/docker-rootful.yaml Outdated Show resolved Hide resolved
@norio-nomura
Copy link
Contributor Author

Updated

@norio-nomura
Copy link
Contributor Author

AkihiroSuda
AkihiroSuda previously approved these changes Jul 26, 2024
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda
Copy link
Member

@lima-vm/maintainers
Is this safe to merge in v0.23?

@afbjorklund
Copy link
Member

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

@norio-nomura
Copy link
Contributor Author

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 param: {key: value}, use {{.Param.key}}. There is no functionality to convert key to Key.

@jandubois
Copy link
Member

jandubois commented Jul 26, 2024

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 param: {key: value}, use {{.Param.key}}. There is no functionality to convert key to Key.

I believe there is no way to reference non-exported fields from Go templates, so {{.Param.key}} cannot work.

I think what @afbjorklund meant was that you have this in your lima.yaml:

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 {{.Param.Rootful}}.

Now you could argue that we are already transforming param.Param, so doing the same for the keys is consistent with that.

Personally I don't like this, and I think param and rootful are semantically different entities, and don't need to be treated the same. I would prefer that the parameters are spelled consistently across the whole lima.yaml file and not magically change their first letter to uppercase.

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 {{.Param.ROOTFUL}}. While I don't think we should enforce this, it may be something we want to use in the bundled templates.

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.

@norio-nomura
Copy link
Contributor Author

I believe there is no way to reference non-exported fields from Go templates, so {{.Param.key}} cannot work.

{{.Param.key}} works because Param is a map[string]string.

In my local tests, #2515 actually works when I change it to:

param:
  ContainerdImageStore: true
  rootful: true

and use {{.Param.rootful}} in combination.

Now you could argue that we are already transforming param.Param, so doing the same for the keys is consistent with that.

The reason it is .Param for the reference is not because text/template is converting it automatically, but because it is being passed to text/template as Param in this line of the code.

@jandubois
Copy link
Member

{{.Param.key}} works because Param is a [map[string]string]

Thanks, didn't think of that. In that case I'm even more against automatically capitalizing the first letter of params.

The reason it is .Param for the reference is not because text/template is converting it automatically, but because it is being passed to text/template as Param

I understand. And the same way we could capitalize the first letter of k before we assign the values in the param list with param[k] = v.

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.

Copy link
Member

@jandubois jandubois left a 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.

examples/default.yaml Show resolved Hide resolved
pkg/limayaml/validate.go Show resolved Hide resolved
@jandubois
Copy link
Member

@lima-vm/maintainers Is this safe to merge in v0.23?

I think it is, and would like to see it included!

Copy link
Member

@jandubois jandubois left a 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>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 18e18af into lima-vm:master Jul 27, 2024
27 checks passed
@norio-nomura norio-nomura deleted the add-param-field-to-limayaml branch July 28, 2024 00:17
@norio-nomura
Copy link
Contributor Author

Thanks! 🙏🏻

@afbjorklund
Copy link
Member

afbjorklund commented Jul 29, 2024

I understand. And the same way we could capitalize the first letter of k before we assign the values in the param list with param[k] = v.

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... :-)

@AkihiroSuda AkihiroSuda mentioned this pull request Jul 30, 2024
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Aug 21, 2024
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 ([#&#8203;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: [@&#8203;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 ([#&#8203;2498](lima-vm/lima#2498), thanks to [@&#8203;norio-nomura](https://github.com/norio-nomura))

-   vz:
    -   Prioritize rosetta over qemu-user-static ([#&#8203;2474](lima-vm/lima#2474), thanks to [@&#8203;norio-nomura](https://github.com/norio-nomura))
    -   Configura AOT caching options using an abstract socket ([#&#8203;2489](lima-vm/lima#2489), thanks to [@&#8203;norio-nomura](https://github.com/norio-nomura))

-   Templates:
    -   add `alpine-image` ([#&#8203;2360](lima-vm/lima#2360), thanks to [@&#8203;jandubois](https://github.com/jandubois))
    -   remove `centos-stream-8`, `deprecated/centos-7` ([#&#8203;2457](lima-vm/lima#2457))
    -   update to the latest revisions ([#&#8203;2553](lima-vm/lima#2553))

-   Governance:
    -   MAINTAINERS: invite Oleksandr Redko ([@&#8203;alexandear](https://github.com/alexandear)) as a Reviewer  ([#&#8203;2383](lima-vm/lima#2383))

Full changes: https://github.com/lima-vm/lima/milestone/46?closed=1
Thanks to [@&#8203;AdamKorcz](https://github.com/AdamKorcz) [@&#8203;AmedeeBulle](https://github.com/AmedeeBulle) [@&#8203;SmartManoj](https://github.com/SmartManoj) [@&#8203;afbjorklund](https://github.com/afbjorklund) [@&#8203;alexandear](https://github.com/alexandear) [@&#8203;danchr](https://github.com/danchr) [@&#8203;fwilhe2](https://github.com/fwilhe2) [@&#8203;jandubois](https://github.com/jandubois) [@&#8203;norio-nomura](https://github.com/norio-nomura) [@&#8203;tcooper](https://github.com/tcooper) [@&#8203;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=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants