add filter option to list command#4187
Conversation
4601c97 to
c7631d4
Compare
| --format '{{ <go template> }}' - If the format begins and ends with '{{ }}', then it is used as a go template. | ||
|
|
||
| Filtering instances: | ||
| --filter EXPR - Filter instances using yq expression (this is equivalent to --yq 'select(EXPR)') |
There was a problem hiding this comment.
Maybe we do not need this flag. We can just add --yq select() examples to --help and call it a day
There was a problem hiding this comment.
That's why I wrote in #4007:
This alone doesn't seem worthwhile, but I would want
--filterto also work with--format tableand--format '{{GO-TMPL}}'.
So you can do this (which isn't possible with --yq):
❯ cat ~/bin/limactl-ps
#!/bin/sh
limactl ls --filter '.status == "Running"' "$@"
❯ l ps
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
docker Running 127.0.0.1:60763 vz aarch64 4 4GiB 100GiB ~/.lima/docker
qemu Running 127.0.0.1:63754 qemu aarch64 4 4GiB 100GiB ~/.lima/qemu
❯ l ps -f '.config.vmType == "vz"'
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
docker Running 127.0.0.1:60763 vz aarch64 4 4GiB 100GiB ~/.lima/dockerI've not had time yet to review this PR to see if this is properly implemented.
There was a problem hiding this comment.
What if we just support --format table with --yq?
jandubois
left a comment
There was a problem hiding this comment.
Needs changes (allow --filter with --yq) and simplifications.
cmd/limactl/list.go
Outdated
| if len(yq) != 0 { | ||
| return errors.New("option --filter conflicts with option --yq") | ||
| } |
There was a problem hiding this comment.
I think you should be able to combine --filter and --yq as long as the output format is json or yaml.
So you could just drop the check here because yq is already incompatible with the other output formats.
There was a problem hiding this comment.
This isn't too clear to me but is there anything wrong with the way it currently is?
There was a problem hiding this comment.
Yes:
❯ l ls -l '.status == "Running"'
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
alpine-iso Running 127.0.0.1:50496 vz aarch64 4 4GiB 100GiB ~/.lima/alpine-iso
❯ l ls -l '.status == "Running"' --yq .name
FATA[0000] option --filter conflicts with option --yq
❯ l ls --yq 'select(.status == "Running")' --yq .name
alpine-isoI expect the last 2 commands to work exactly the same. There is no reason for the len(yq) check to exist.
404b08c to
a5ba9ad
Compare
cmd/limactl/list.go
Outdated
| isGoTemplate := strings.HasPrefix(format, "{{") && strings.HasSuffix(format, "}}") | ||
| if len(filter) != 0 && (format == "table" || isGoTemplate) { |
There was a problem hiding this comment.
| isGoTemplate := strings.HasPrefix(format, "{{") && strings.HasSuffix(format, "}}") | |
| if len(filter) != 0 && (format == "table" || isGoTemplate) { | |
| if len(filter) != 0 && format != "json" && format != "yaml") { |
Despite what the --help output says, every string except table, json, and yaml is treated as a Go template, so the check for the double braces prefix/suffix is not correct:
❯ l ls -f 'Name is: {{.Name}}'
Name is: alpine-iso
Name is: alpine-lima-3.22.2
Name is: foo
cmd/limactl/list.go
Outdated
| for _, instance := range instances { | ||
| jsonBytes, err := json.Marshal(instance) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal instance %s: %w", instance.Name, err) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("failed to marshal instance %s: %w", instance.Name, err) | |
| return nil, fmt.Errorf("failed to marshal instance %q: %w", instance.Name, err) |
Always use %q when printing use input.
Also applies to the next error message.
224e6e4 to
b3f18db
Compare
Signed-off-by: olalekan odukoya <odukoyaonline@gmail.com>
b3f18db to
bdef056
Compare
| Examples: | ||
| --filter '.status == "Running"' | ||
| --filter '.vmType == "vz"' | ||
| --filter '.status == "Running"' --filter '.vmType == "vz"' |
There was a problem hiding this comment.
Is this AND-matching or OR-matching?
There was a problem hiding this comment.
I assume you want the description updated to clarify.
It is AND so you can do this:
❯ cat ~/bin/limactl-ps
#!/bin/sh
limactl ls --filter 'select(.status == "Running")' "$@"
❯ l ls
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
alpine-iso Running 127.0.0.1:50496 vz aarch64 4 4GiB 100GiB ~/.lima/alpine-iso
alpine-lima-3.22.2 Stopped 127.0.0.1:0 vz aarch64 4 4GiB 100GiB ~/.lima/alpine-lima-3.22.2
default Running 127.0.0.1:54512 qemu aarch64 4 4GiB 100GiB ~/.lima/default
foo Stopped 127.0.0.1:0 vz aarch64 4 1.177MiB 100GiB ~/.lima/foo
❯ l ps
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
alpine-iso Running 127.0.0.1:50496 vz aarch64 4 4GiB 100GiB ~/.lima/alpine-iso
default Running 127.0.0.1:54512 qemu aarch64 4 4GiB 100GiB ~/.lima/default
❯ l ps -l '.vmType == "vz"'
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
alpine-iso Running 127.0.0.1:50496 vz aarch64 4 4GiB 100GiB ~/.lima/alpine-iso| if !valid.MatchString(f) { | ||
| return fmt.Errorf("unsafe characters in filter expression: %q", f) | ||
| } |
There was a problem hiding this comment.
I don't think this is useful. The whole point of --filter and --yq is to provide YQ expressions that are going to be evaluated. There is no vulnerability; the expression is provided by the user themselves.
And we already get an error when the YQ expression is invalid, e.g.
❯ l ls -l '.vmType =='
FATA[0000] failed to apply filter select(.vmType ==): '==' expects 2 args but there is 1So I don't understand the purpose of this "validation", especially since it might make legitimate use cases invalid (e.g. you are restricting which characters are allowed inside literal strings).
jandubois
left a comment
There was a problem hiding this comment.
The combination of --filter and --quiet is not working (the filter is ignored):
❯ l ls -l '.vmType == "qemu"'
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
default Running 127.0.0.1:54512 qemu aarch64 4 4GiB 100GiB ~/.lima/default
❯ l ls -l '.vmType == "qemu"' -q
alpine-iso
alpine-lima-3.22.2
default
foo| listCommand.Flags().BoolP("quiet", "q", false, "Only show names") | ||
| listCommand.Flags().Bool("all-fields", false, "Show all fields") | ||
| listCommand.Flags().StringArray("yq", nil, "Apply yq expression to each instance") | ||
| listCommand.Flags().StringArrayP("filter", "l", nil, "Filter instances using yq expression (equivalent to --yq 'select(EXPR)')") |
There was a problem hiding this comment.
The l shorthand should be probably reserved for --label
There was a problem hiding this comment.
what should be used?
There was a problem hiding this comment.
I don't think there is a separate --label option; it is the same as --filter, which is another reason that I think -l is appropriate. But let's wait until KubeCon is over and everyone (or at least @AkihiroSuda) is back, and continue the discussion then. Right now the 2.0 release, and potential regressions is still top-of-mind.
|
Pls what's the conclusion on the implementation for this? @jandubois @AkihiroSuda |
|
This issue is still unresolved |
|
This PR needs rebasing at least |
|
Okay. I have the intention of closing his up this week at least. I've just been swamped recently 😞 |
Fixes