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

Add builtin yq support to lima start command #1359

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

afbjorklund
Copy link
Member

@afbjorklund afbjorklund commented Feb 12, 2023

Allows modifying the yaml template "inplace" for a single instance,
if you don't want to change the default or override for all instances.

This is similar to running yq on the template, from the command-line.
Except it does it on a temporary file, and for all different kinds of templates.

$ yq < examples/default.yaml > default.yaml
$

Currently it removes empty lines, some quirk with the yaml parser used.
It also seems to change the indentation on some comments (only), but OK:

$ diff -B -w examples/default.yaml default.yaml
$

This can be used as a general fallback, similar to jq, for modifying templates.
Instead of adding the usual --cpus --memory --arch, or other parameters ?


See https://github.com/mikefarah/yq for details

limactl start config.yaml --yq '.arch = "aarch64"'

It is not YAML syntax, but it is rather straightforward.

@@ -56,6 +60,7 @@ $ cat template.yaml | limactl start --name=local -
// TODO: "survey" does not support using cygwin terminal on windows yet
startCommand.Flags().Bool("tty", isatty.IsTerminal(os.Stdout.Fd()), "enable TUI interactions such as opening an editor, defaults to true when stdout is a terminal")
startCommand.Flags().String("name", "", "override the instance name")
startCommand.Flags().String("yq", "", "modify the template inplace")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe:

Suggested change
startCommand.Flags().String("yq", "", "modify the template inplace")
startCommand.Flags().String("set", "", "modify the template inplace, using yq syntax")

Copy link
Member

Choose a reason for hiding this comment

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

  • I guess we have to prohibit modifying images?
  • Maybe we want to treat this as an experimental?
  • Please update README.md too

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is it not allowed to modified images, from the templates ?

Copy link
Member

Choose a reason for hiding this comment

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

Might be ok for new instances, but not allowable for existing instances, obviously

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. But this new tool is not very different, from doing it yourself in the editor...

@AkihiroSuda AkihiroSuda added this to the v0.15.0 milestone Feb 12, 2023
@AkihiroSuda AkihiroSuda added enhancement New feature or request area/cli limactl CLI user experience labels Feb 12, 2023
AkihiroSuda
AkihiroSuda previously approved these changes Feb 14, 2023
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

Instead of adding the usual --cpus --memory --arch, or other parameters ?

We should add them too, for typechecking, shell completion, etc. (In separate PRs)

jandubois
jandubois previously approved these changes Feb 14, 2023
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.

I wish the option would also be added to limactl edit, but that could be done in a followup PR.

It also makes me a little sad that this little feature increases the size of the limactl binary by 20% (20MB → 24MB), but I guess that can't be helped, and doesn't really matter when you compare it to the size of QEMU. 😄

Comment on lines +179 to +183
To create an instance "default" with modified parameters:
```console
$ limactl start --set='.cpus = 2 | .memory = "2GiB"'
```

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we need to include a reference to the yq documentation, so people can learn to construct more powerful expressions for e.g. scripting:

$ limactl start --set '.env.HOSTPATH=strenv(PATH)' template://alpine

Copy link
Member

Choose a reason for hiding this comment

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

I expected the same syntax to be available for the limactl edit command as well.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I should have specified: I would expect limactl edit --set ... to just edit the config, but not to invoke an editor.

It would just be a mechanism to modify the config programmatically without starting the instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can use something like --eval, since that is the name of the yq command being invoked ?

Available Commands:
  completion       Generate the autocompletion script for the specified shell
  eval             (default) Apply the expression to each document in each yaml file in sequence
  eval-all         Loads _all_ yaml documents of _all_ yaml files and runs expression once
  help             Help about any command
  shell-completion Generate completion script

I agree that --set is somewhat misleading, and that --yq might be a bit cryptic for lima users...

Adding it to edit should be straight-forward.

Copy link
Member

Choose a reason for hiding this comment

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

I would expect limactl edit --set ... to just edit the config, but not to invoke an editor.

👍

--eval sounds much more misleading

Copy link
Member

Choose a reason for hiding this comment

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

limactl edit --set can be another PR if complicated

Copy link
Member Author

Choose a reason for hiding this comment

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

The error handling can be a bit tricky to do, so I'd like that.

pkg/yqutil/yqutil_test.go Show resolved Hide resolved
@afbjorklund
Copy link
Member Author

afbjorklund commented Feb 14, 2023

It also makes me a little sad that this little feature increases the size of the limactl binary by 20% (20MB → 24MB), but I guess that can't be helped, and doesn't really matter when you compare it to the size of QEMU. smile

I noticed that as well.

It would be possible to make a more conservative option, either some special-case for cpus/memory/arch/etc.
Or doing the --set approach, that would create a special override.yaml just for the particular instance.

This feature (yq) as such has a lot of "overkill" in it, like using jq (or even go templates) to change the list output...

@AkihiroSuda
Copy link
Member

The binary footprint seems acceptable for me

@jandubois
Copy link
Member

It would be possible to make a more conservative option,

No, I think I would prefer to have the expressiveness of yq expressions. I was just commenting that I noticed the size increase, but over time bloat is inevitable, and I think most users won't care (or even notice).

Idle curiosity: yq is 30× the size of jq

$ ls -lh /usr/local/Cellar/jq/1.6/bin/jq
-r-xr-xr-x  1 jan  admin   296K  2 Jan 18:54 /usr/local/Cellar/jq/1.6/bin/jq
$ ls -lh /usr/local/Cellar/yq/4.30.8/bin/yq
-r-xr-xr-x  1 jan  admin   9.0M 14 Jan 16:37 /usr/local/Cellar/yq/4.30.8/bin/yq

@afbjorklund
Copy link
Member Author

It also does XML and JSON, in addition to just YAML. But I don't think it had any build flags ?

@jandubois
Copy link
Member

It also does XML and JSON, in addition to just YAML. But I don't think it had any build flags ?

Of course it does JSON, as JSON is just a subset of YAML; every valid JSON file is also a valid YAML file. I didn't know yq also did XML though.

@afbjorklund
Copy link
Member Author

I meant that it links with both encoding/json and encoding/xml libraries:

1813	gopkg.in/yaml.v3
1478	syscall
1083	encoding/json
994	encoding/xml
917	time
830	os
773	fmt
677	regexp
650	github.com/magiconair/properties
552	math
524	github.com/alecthomas/participle/v2/lexer
503	unicode
497	strconv
487	strings
430	gopkg.in/op/go-logging.v1
386	bytes
368	github.com/goccy/go-json
343	bufio
315	io
302	net/url
292	sort
289	github.com/jinzhu/copier
273	io/fs
269	github.com/a8m/envsubst/parse
250	github.com/fatih/color
205	math/rand
151	github.com/goccy/go-yaml/printer
135	encoding/csv
122	encoding/base64
94	container/list
84	github.com/elliotchance/orderedmap
81	golang.org/x/net/html/charset
55	github.com/dimchansky/utfbom
44	errors
14	github.com/goccy/go-yaml/lexer

Sizes in kiB (build-size).

@afbjorklund
Copy link
Member Author

Idle curiosity: yq is 30× the size of jq

It is written in C (not Go), so this is normal.

@AkihiroSuda
Copy link
Member

Needs rebase.

Are we ready to merge this after rebasing?

@jandubois
Copy link
Member

Are we ready to merge this after rebasing?

I'm fine with merging.

Allows modifying the template inplace for a single instance, if you
don't want to change the default or override for all instances.

There are currently some quirks with the modified yaml syntax,
like removing empty lines or adding indentation on sequences...

Signed-off-by: Anders F Björklund <[email protected]>
@afbjorklund
Copy link
Member Author

I ran go mod tidy on it, so I hope those dependencies (go sum) are correct...
The logging is a direct dependency, due to the "translation" over to logrus.

For what it is worth, I made a PR to yq in order to make json and xml optional.
So that should cut the library size in half, if merged. -tags yq_nojson,yq_noxml

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 eb2a4cd into lima-vm:master Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli limactl CLI user experience enhancement New feature or request impact/changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants