Improve `limactl edit` #2593

norio-nomura · 2024-09-05T14:24:49Z

yqutil: use yamlfmt to fix indentation of sequence
limactl edit: support editing lima.yaml file
This allows limactl edit --set "..." <limayaml> to be used instead of yq -i eval "..." <limayaml> in scripts like hack/inject-cmdline-to-template.sh.

The changes made to examples/ubuntu.yaml using limactl edit --set in hack/inject-cmdline-to-template.sh are as follows:

diff --git forkSrcPrefix/examples/ubuntu.yaml forkDstPrefix/examples/ubuntu.yaml
index a7fb0b3246e71bd9f98df2e8849185ed1339c76b..9f19540104091b13e08cf255bd074831560af654 100644
--- forkSrcPrefix/examples/ubuntu.yaml
+++ forkDstPrefix/examples/ubuntu.yaml
@@ -7,13 +7,19 @@ images:
 - location: "https://cloud-images.ubuntu.com/releases/24.04/release-20240821/ubuntu-24.04-server-cloudimg-arm64.img"
   arch: "aarch64"
   digest: "sha256:5ecac6447be66a164626744a87a27fd4e6c6606dc683e0a233870af63df4276a"
+  kernel:
+    location: https://cloud-images.ubuntu.com/releases/24.04/release-20240821/unpacked/ubuntu-24.04-server-cloudimg-arm64-vmlinuz-generic
+    digest: sha256:69e66230d6acc503082f098fd072a672a804774007209110b353d25bb7007669
+    cmdline: root=LABEL=cloudimg-rootfs ro console=tty1 console=ttyAMA0 console=hvc0
+  initrd:
+    location: https://cloud-images.ubuntu.com/releases/24.04/release-20240821/unpacked/ubuntu-24.04-server-cloudimg-arm64-initrd-generic
+    digest: sha256:92acf9f2976b7a8b3b5e9e3f22211d17a6fdb3f2aed981467f91cd6dbfa16ae5
 # Fallback to the latest release image.
 # Hint: run `limactl prune` to invalidate the cache
 - location: "https://cloud-images.ubuntu.com/releases/24.04/release/ubuntu-24.04-server-cloudimg-amd64.img"
   arch: "x86_64"
 - location: "https://cloud-images.ubuntu.com/releases/24.04/release/ubuntu-24.04-server-cloudimg-arm64.img"
   arch: "aarch64"
-
 mounts:
 - location: "~"
 - location: "/tmp/lima"

The issue with line breaks disappearing cannot be fixed with yamlfmt, so it still remains. However, the sequence is no longer indented, and I personally think it has become usable.

I'm considering making a separate PR to change hack/inject-cmdline-to-template.sh from yq -i eval to limactl edit --set after #2562, which is likely to have overlapping changes, is resolved.

AkihiroSuda · 2024-09-05T18:09:46Z

go.mod

@@ -58,6 +59,8 @@ require (
 	github.com/VividCortex/ewma v1.2.0 // indirect
 	github.com/a8m/envsubst v1.4.2 // indirect
 	github.com/alecthomas/participle/v2 v2.1.1 // indirect
+	github.com/bmatcuk/doublestar/v4 v4.6.0 // indirect
+	github.com/braydonk/yaml v0.7.0 // indirect


Not a huge fan of importing multiple YAML libraries

Is this the fifth YAML library?

Oh, is this the first time a YAML library forked from https://github.com/go-yaml/yaml is being added?

I want to express my gratitude to Go for allowing the use of both a forked library and the original library simultaneously without causing issues. 👏🏻

I think both of yqlib and yamlfmt brings enough value, to be allowed to bring-their-own-yaml to the party

jandubois · 2024-09-05T20:55:20Z

Tests are failing on Windows because it is adding \r\n line endings to the YAML file instead of sticking to plain newlines.

afbjorklund · 2024-09-09T18:25:16Z

I like this new library, with some configuration it can actually keep the examples/default.yaml more or less as-is:

.yamlfmt

line_ending: lf
formatter:
    indentless_arrays: true
    retain_line_breaks: true

The only change is some alignment on the comments, and I don't think that is too bad (and yamllint is OK too)

yamlfmt examples/default.yaml

--- a/examples/default.yaml
+++ b/examples/default.yaml
@@ -264,10 +264,10 @@ containerd:
 # Setting of instructions is supported like this: "qemu64,+ssse3".
 # 🟢 Builtin default: hard-coded arch map with type (see the output of `limactl info | jq .defaultTemplate.cpuType`)
 cpuType:
-  # aarch64: "cortex-a72" # (or "host" when running on aarch64 host)
-  # armv7l: "cortex-a7" # (or "host" when running on armv7l host)
-  # riscv64: "rv64" # (or "host" when running on riscv64 host)
-  # x86_64: "qemu64" # (or "host,-pdpe1gb" when running on x86_64 host)
+# aarch64: "cortex-a72" # (or "host" when running on aarch64 host)
+# armv7l: "cortex-a7" # (or "host" when running on armv7l host)
+# riscv64: "rv64" # (or "host" when running on riscv64 host)
+# x86_64: "qemu64" # (or "host,-pdpe1gb" when running on x86_64 host)
 
 rosetta:
   # Enable Rosetta for Linux (EXPERIMENTAL; will graduate from experimental in Lima v1.0).
@@ -456,8 +456,8 @@ hostResolver:
   # predefined to specify the gateway address to the host.
   # 🟢 Builtin default: null
   hosts:
-    # guest.name: 127.1.1.1
-    # host.name: host.lima.internal
+  # guest.name: 127.1.1.1
+  # host.name: host.lima.internal
 
 # If hostResolver.enabled is false, then the following rules apply for configuring dns:
 # Explicitly set DNS addresses for qemu user-mode networking. By default qemu picks *one*

jandubois

I agree with @afbjorklund that the new libraries provide sufficient value to justify adding one more YAML library to the project.

I think the fact that limactl edit can now edit templates in addition to instances should be noted in the command descriptions:

-		Use:               "edit INSTANCE",
-		Short:             "Edit an instance of Lima",
+		Use:               "edit INSTANCE|FILE.yaml",
+		Short:             "Edit an instance of Lima or a template",

pkg/yqutil/yqutil_test.go

jandubois · 2024-09-14T06:39:29Z

Please squash commits!

Signed-off-by: Norio Nomura <[email protected]> `limactl edit`: support editing lima.yaml file This allows `limactl edit --set "..." <limayaml>` to be used instead of `yq -i eval "..." <limayaml>` in scripts like `hack/inject-cmdline-to-template.sh`. Signed-off-by: Norio Nomura <[email protected]> yqutil: Use `LF` for line endings regardless of the platform. Signed-off-by: Norio Nomura <[email protected]> `limactl edit`: update command description Signed-off-by: Norio Nomura <[email protected]> yqutil_test: fix typo Signed-off-by: Norio Nomura <[email protected]>

jandubois

Thanks, LGTM

jandubois · 2024-09-15T04:53:35Z

@AkihiroSuda Do you have any objections to merging this?

jandubois · 2024-09-15T04:57:39Z

The issue with line breaks disappearing cannot be fixed with yamlfmt, so it still remains.

Super-hacky idea: Replace all empty lines with ### this is an empty line ### before editing the file, and then replace them back with empty lines after yamlfmt is done.

norio-nomura · 2024-09-15T07:29:41Z

Super-hacky idea: Replace all empty lines with ### this is an empty line ### before editing the file, and then replace them back with empty lines after yamlfmt is done.

That's exactly the idea that yamlfmt adopts.
https://github.com/google/yamlfmt/blob/main/internal/hotfix/retain_line_break.go

norio-nomura · 2024-09-15T08:27:39Z

That's exactly the idea that yamlfmt adopts

Oh, it seems that the following hack works well:

diff --git forkSrcPrefix/pkg/yqutil/yqutil.go forkDstPrefix/pkg/yqutil/yqutil.go
index 139d9fcbce3ff6c7ca3df38a04a1fb3ef36ccc1b..8787797f40b6e0a8fcaec1f06b203b25e8ba3c06 100644
--- forkSrcPrefix/pkg/yqutil/yqutil.go
+++ forkDstPrefix/pkg/yqutil/yqutil.go
@@ -1,6 +1,7 @@
 package yqutil
 
 import (
+	"bufio"
 	"bytes"
 	"fmt"
 	"os"
@@ -15,13 +16,17 @@ import (
 // EvaluateExpression evaluates the yq expression, and returns the modified yaml.
 func EvaluateExpression(expression string, content []byte) ([]byte, error) {
 	logrus.Debugf("Evaluating yq expression: %q", expression)
+	contentModified, err := replaceLineBreaksWithMagicString(content)
+	if err != nil {
+		return nil, err
+	}
 	tmpYAMLFile, err := os.CreateTemp("", "lima-yq-*.yaml")
 	if err != nil {
 		return nil, err
 	}
 	tmpYAMLPath := tmpYAMLFile.Name()
 	defer os.RemoveAll(tmpYAMLPath)
-	_, err = tmpYAMLFile.Write(content)
+	_, err = tmpYAMLFile.Write(contentModified)
 	if err != nil {
 		tmpYAMLFile.Close()
 		return nil, err
@@ -94,3 +99,41 @@ func yamlfmt(content []byte) ([]byte, error) {
 	}
 	return formatter.Format(content)
 }
+
+const yamlfmtLineBreakPlaceholder = "#magic___^_^___line"
+
+type paddinger struct {
+	strings.Builder
+}
+
+func (p *paddinger) adjust(txt string) {
+	var indentSize int
+	for i := 0; i < len(txt) && txt[i] == ' '; i++ { // yaml only allows space to indent.
+		indentSize++
+	}
+	// Grows if the given size is larger than us and always return the max padding.
+	for diff := indentSize - p.Len(); diff > 0; diff-- {
+		p.WriteByte(' ')
+	}
+}
+
+func replaceLineBreaksWithMagicString(content []byte) ([]byte, error) {
+	// hotfix: yq does not support line breaks in the middle of a string.
+	var buf bytes.Buffer
+	reader := bytes.NewReader(content)
+	scanner := bufio.NewScanner(reader)
+	var padding paddinger
+	for scanner.Scan() {
+		txt := scanner.Text()
+		padding.adjust(txt)
+		if strings.TrimSpace(txt) == "" { // line break or empty space line.
+			buf.WriteString(padding.String()) // prepend some padding incase literal multiline strings.
+			buf.WriteString(yamlfmtLineBreakPlaceholder)
+			buf.WriteString("\n")
+		} else {
+			buf.WriteString(txt)
+			buf.WriteString("\n")
+		}
+	}
+	return buf.Bytes(), scanner.Err()
+}
diff --git forkSrcPrefix/pkg/yqutil/yqutil_test.go forkDstPrefix/pkg/yqutil/yqutil_test.go
index fcf1260ec24519a847a9305d978144f0bbdbe7cc..287c51c6be2d13bd75a7984712f0d4aad492fb0b 100644
--- forkSrcPrefix/pkg/yqutil/yqutil_test.go
+++ forkDstPrefix/pkg/yqutil/yqutil_test.go
@@ -19,6 +19,7 @@ memory: null
 	expected := `
 # CPUs
 cpus: 2
+
 # Memory size
 memory: 2GiB
 `

🤔

(edited: add yqutil_test.go to patch)

norio-nomura · 2024-09-15T13:49:18Z

The changes after running limactl edit templates/default.yaml --memory 8 using the above patch are as follows:

diff --git forkSrcPrefix/examples/default.yaml forkDstPrefix/examples/default.yaml
index 5b269ff7e25530c16ce7a9c4fd00251a35850b0d..c46886072da9ff064754da989698e618daa787e8 100644
--- forkSrcPrefix/examples/default.yaml
+++ forkDstPrefix/examples/default.yaml
@@ -43,7 +43,7 @@ cpus: null
 
 # Memory size
 # 🟢 Builtin default: min("4GiB", half of host memory)
-memory: null
+memory: 8GiB
 
 # Disk size
 # 🟢 Builtin default: "100GiB"
@@ -264,10 +264,10 @@ containerd:
 # Setting of instructions is supported like this: "qemu64,+ssse3".
 # 🟢 Builtin default: hard-coded arch map with type (see the output of `limactl info | jq .defaultTemplate.cpuType`)
 cpuType:
-  # aarch64: "cortex-a72" # (or "host" when running on aarch64 host)
-  # armv7l: "cortex-a7" # (or "host" when running on armv7l host)
-  # riscv64: "rv64" # (or "host" when running on riscv64 host)
-  # x86_64: "qemu64" # (or "host,-pdpe1gb" when running on x86_64 host)
+# aarch64: "cortex-a72" # (or "host" when running on aarch64 host)
+# armv7l: "cortex-a7" # (or "host" when running on armv7l host)
+# riscv64: "rv64" # (or "host" when running on riscv64 host)
+# x86_64: "qemu64" # (or "host,-pdpe1gb" when running on x86_64 host)
 
 rosetta:
   # Enable Rosetta for Linux (EXPERIMENTAL; will graduate from experimental in Lima v1.0).
@@ -456,8 +456,8 @@ hostResolver:
   # predefined to specify the gateway address to the host.
   # 🟢 Builtin default: null
   hosts:
-    # guest.name: 127.1.1.1
-    # host.name: host.lima.internal
+  # guest.name: 127.1.1.1
+  # host.name: host.lima.internal
 
 # If hostResolver.enabled is false, then the following rules apply for configuring dns:
 # Explicitly set DNS addresses for qemu user-mode networking. By default qemu picks *one*

jandubois · 2024-09-16T00:08:23Z

Oh, it seems that the following hack works well:

If you create a PR with that hack (which I would like to see once the current PR is merged), then I would suggest that we should also fix up the templates so that the comments round-trip correctly (all comments should be in front of the setting they apply to, and not after it).

Then we can add a check to CI to make sure they remain round-trippable. Something like

find examples -name '*.yaml' -exec limactl edit --set 'del(.nothing)' {} \;
git diff-index --exit-code HEAD

jandubois · 2024-09-16T00:13:29Z

@AkihiroSuda I'll take the fact that you assigned this PR to the 1.0 milestone as agreement to merge. I was waiting for your feedback because you originally commented:

Not a huge fan of importing multiple YAML libraries

So I wasn't sure if you still had objections to merging it or not. But I guess you are ok with it now.

norio-nomura · 2024-09-16T02:39:21Z

Thanks! 🙏🏻

Then we can add a check to CI to make sure they remain round-trippable.

Before doing that, I want to finalize #2532.

AkihiroSuda · 2024-09-16T03:14:41Z

@AkihiroSuda I'll take the fact that you assigned this PR to the 1.0 milestone as agreement to merge. I was waiting for your feedback because you originally commented:

Not a huge fan of importing multiple YAML libraries

So I wasn't sure if you still had objections to merging it or not. But I guess you are ok with it now.

Yes, ok to merge, sorry for delay
🙇‍♂️

AkihiroSuda reviewed Sep 5, 2024

View reviewed changes

afbjorklund mentioned this pull request Sep 9, 2024

Avoid importing multiple YAML libraries #2597

Open

jandubois requested changes Sep 14, 2024

View reviewed changes

pkg/yqutil/yqutil_test.go Outdated Show resolved Hide resolved

norio-nomura force-pushed the improve-limactl-edit branch from 6cfba80 to a10a8bb Compare September 14, 2024 09:16

jandubois approved these changes Sep 15, 2024

View reviewed changes

AkihiroSuda added this to the v1.0 milestone Sep 15, 2024

AkihiroSuda added the area/cli limactl CLI user experience label Sep 15, 2024

jandubois merged commit 133979a into lima-vm:master Sep 16, 2024
27 checks passed

norio-nomura deleted the improve-limactl-edit branch September 16, 2024 00:23

norio-nomura mentioned this pull request Sep 17, 2024

yqutil: fix to preserve line breaks #2625

Merged

norio-nomura mentioned this pull request Oct 8, 2024

Add hack/update-template-ubuntu.sh #2702

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `limactl edit` #2593

Improve `limactl edit` #2593

norio-nomura commented Sep 5, 2024

AkihiroSuda Sep 5, 2024

norio-nomura Sep 5, 2024

norio-nomura Sep 6, 2024

This comment was marked as off-topic.

norio-nomura Sep 9, 2024

afbjorklund Sep 9, 2024

jandubois commented Sep 5, 2024

afbjorklund commented Sep 9, 2024 •

edited

Loading

jandubois left a comment

jandubois commented Sep 14, 2024

jandubois left a comment

jandubois commented Sep 15, 2024

jandubois commented Sep 15, 2024

norio-nomura commented Sep 15, 2024

norio-nomura commented Sep 15, 2024 •

edited

Loading

norio-nomura commented Sep 15, 2024

jandubois commented Sep 16, 2024 •

edited

Loading

jandubois commented Sep 16, 2024

norio-nomura commented Sep 16, 2024

AkihiroSuda commented Sep 16, 2024

Improve limactl edit #2593

Improve limactl edit #2593

Conversation

norio-nomura commented Sep 5, 2024

AkihiroSuda Sep 5, 2024

Choose a reason for hiding this comment

norio-nomura Sep 5, 2024

Choose a reason for hiding this comment

norio-nomura Sep 6, 2024

Choose a reason for hiding this comment

This comment was marked as off-topic.

norio-nomura Sep 9, 2024

Choose a reason for hiding this comment

afbjorklund Sep 9, 2024

Choose a reason for hiding this comment

jandubois commented Sep 5, 2024

afbjorklund commented Sep 9, 2024 • edited Loading

jandubois left a comment

Choose a reason for hiding this comment

jandubois commented Sep 14, 2024

jandubois left a comment

Choose a reason for hiding this comment

jandubois commented Sep 15, 2024

jandubois commented Sep 15, 2024

norio-nomura commented Sep 15, 2024

norio-nomura commented Sep 15, 2024 • edited Loading

norio-nomura commented Sep 15, 2024

jandubois commented Sep 16, 2024 • edited Loading

jandubois commented Sep 16, 2024

norio-nomura commented Sep 16, 2024

AkihiroSuda commented Sep 16, 2024

Improve `limactl edit` #2593

Improve `limactl edit` #2593

afbjorklund commented Sep 9, 2024 •

edited

Loading

norio-nomura commented Sep 15, 2024 •

edited

Loading

jandubois commented Sep 16, 2024 •

edited

Loading