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

Hide columns in the list command #1266

Merged
merged 2 commits into from
Feb 1, 2023
Merged

Conversation

afbjorklund
Copy link
Member

@afbjorklund afbjorklund commented Dec 27, 2022

  • Hide identical columns in the list command

    If all the instances have the same type or arch,
    then don't display the column - unless requested.

  • Hide the directory column on narrow terminal

    If the terminal width does not fit all the list
    columns, then hide "dir" column - unless requested.

before

NAME    STATUS     SSH            VMTYPE    ARCH      CPUS    MEMORY    DISK   
   DIR
k8s     Stopped    127.0.0.1:0    qemu      x86_64    4       4GiB      100GiB  
  ~/.lima/k8s

after

NAME    STATUS     SSH            CPUS    MEMORY    DISK      DIR
k8s     Stopped    127.0.0.1:0    4       4GiB      100GiB    ~/.lima/k8s
NAME    STATUS     SSH            VMTYPE    ARCH      CPUS    MEMORY    DISK
k8s     Stopped    127.0.0.1:0    qemu      x86_64    4       4GiB      100GiB

One can use the --all-fields flag, to get the old behaviour back again.

On a wide enough terminal, the table output is the same as before.

Closes #1204

@AkihiroSuda AkihiroSuda added the area/cli limactl CLI user experience label Dec 28, 2022
cmd/limactl/list.go Outdated Show resolved Hide resolved
cmd/limactl/list.go Outdated Show resolved Hide resolved
@AkihiroSuda
Copy link
Member

Thanks, but needs rebase

@afbjorklund afbjorklund force-pushed the hide-columns branch 2 times, most recently from 4b74383 to 0839624 Compare January 5, 2023 17:28
@AkihiroSuda
Copy link
Member

Found a regression: The ARCH field is no longer printed even when the terminal width is enough.

LGTM other than that.

@AkihiroSuda
Copy link
Member

I'd also like to see unit tests

@AkihiroSuda AkihiroSuda added this to the v0.15 (tentative) milestone Jan 7, 2023
@afbjorklund
Copy link
Member Author

Found a regression: The ARCH field is no longer printed even when the terminal width is enough.

No regression, it was separate features. But we can make the arch/type dedup depend on width too ?

@jandubois
Copy link
Member

Found a regression: The ARCH field is no longer printed even when the terminal width is enough.

I think I'm find with the ARCH field being removed, even when there is enough space, as long as all machines use the native arch:

hideArch = len(archs) == 1 && archs[0] == runtime.GOARCH

I don't think it adds any value in that situation, which I expect is going to be rather common.

@AkihiroSuda
Copy link
Member

Lints are failing

@afbjorklund
Copy link
Member Author

Lints are failing

Needed to upgrade my golangci-lint

@afbjorklund
Copy link
Member Author

afbjorklund commented Jan 11, 2023

I don't think it adds any value in that situation

I never found any value from the SSH column, but maybe it has some ?

Especially the 127.0.0.1:0 is weird, but also others.

You need to use show-ssh anyway, to get something actually usable.

But we can hide the arch and vmtype again, sure.

@jandubois
Copy link
Member

Especially the 127.0.0.1:0 is weird, but also others.

I agree, but maybe we should leave this for future discussion / another PR, so we can get this change wrapped up and merged?

@afbjorklund
Copy link
Member Author

Ok, so let's autohide again and see how that goes.

@jandubois
Copy link
Member

Always hide identical columns any width

That's not what we agreed on, we agreed to auto-hide the ARCH column if all values are identical to the local arch.

I thought that was the only remaining change, and that's why I didn't want to open the discussion to include the SSH column as well.

@afbjorklund
Copy link
Member Author

Just that it always shows the vm type, even if there is only one vm type (on Linux)

AkihiroSuda
AkihiroSuda previously approved these changes Jan 13, 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

Sorry needs rebase

cmd/limactl/list.go Outdated Show resolved Hide resolved
pkg/store/instance.go Outdated Show resolved Hide resolved
pkg/store/instance.go Outdated Show resolved Hide resolved
pkg/store/instance.go Outdated Show resolved Hide resolved
pkg/store/instance.go Outdated Show resolved Hide resolved
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.

ARCH should only be hidden when all instances use the host arch.

@afbjorklund
Copy link
Member Author

Did the rebase and the simple edits, others are left todo.

@AkihiroSuda
Copy link
Member

Did the rebase and the simple edits, others are left todo.

Should this PR be marked as a draft?

@afbjorklund afbjorklund marked this pull request as draft January 18, 2023 17:25
If all the instances have the same type or arch,
then don't display the column - unless requested.

Only hide the arch if it is the same as host arch.
Emulated instances all show their arch by default.

Signed-off-by: Anders F Björklund <[email protected]>
@afbjorklund afbjorklund marked this pull request as ready for review January 18, 2023 22:04
If the terminal width does not fit all the list
columns, then hide "dir" column - unless requested.

Without terminal (or unknown), hide the identical.
If the terminal is wide enough, then don't hide.

Signed-off-by: Anders F Björklund <[email protected]>
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

@jandubois LGTY?

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!

@jandubois jandubois merged commit f7e7add into lima-vm:master Feb 1, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

limactl list: dynamically reduces the column, depending on the terminal width
3 participants