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.

Sign up for GitHub

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

Jump to bottom

ls: no-trunc opt #2138

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

ls: no-trunc opt #2138

wants to merge 1 commit into from
+147 −11

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Nov 23, 2023 •
edited

follow-up #830 with the no-trunc opt.

When using ls, platforms can get wieldy easily and make output in the terminal difficult to read:

image

NAME/NODE              DRIVER/ENDPOINT                              STATUS    BUILDKIT                                PLATFORMS
builder                docker-container
 \_ builder0            \_ unix:///var/run/docker.sock              running   v0.12.3                                 linux/amd64, linux/amd64/v2, linux/amd64/v3, linux/arm64, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/mips64le, linux/mips64, linux/arm/v7, linux/arm/v6
macos                  docker-container
 \_ macos0              \_ tcp://mac-mini-m1.home.fooooo.com:2376   error                                             linux/arm64*
sifive                 docker-container
 \_ sifive0             \_ tcp://192.168.0.65:2375                  running   v0.12.2                                 linux/riscv64*
default*               docker
 \_ default             \_ default                                  running   v0.11.7+d3e6c1360f6e                    linux/amd64, linux/amd64/v2, linux/amd64/v3, linux/arm64, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/mips64le, linux/mips64, linux/arm/v7, linux/arm/v6
docker-test            docker
 \_ docker-test         \_ docker-test                              running   v0.11.6                                 linux/amd64, linux/386
docker20               docker
 \_ docker20            \_ docker20                                 running   v0.8+unknown                            linux/amd64, linux/386
docker23               docker
 \_ docker23            \_ docker23                                 running   v0.10.6+d52b2d5                         linux/amd64, linux/386
docker24               docker
 \_ docker24            \_ docker24                                 running   v0.11.6                                 linux/amd64, linux/386
docker24-arm64         docker
 \_ docker24-arm64      \_ docker24-arm64                           running   v0.11.7-0.20230525183624-798ad6b0ce9f   linux/arm64, linux/arm/v7, linux/arm/v6
remote-ssh             docker
 \_ remote-ssh          \_ remote-ssh                               running   v0.11.6+0a15675913b7                    linux/arm64, linux/arm/v7, linux/arm/v6

Only platforms are truncated by default and only when using the table format. We only display "major/common" platforms with a limitation to 4:

image

NAME/NODE              DRIVER/ENDPOINT                              STATUS    BUILDKIT                                PLATFORMS
builder                docker-container
 \_ builder0            \_ unix:///var/run/docker.sock              running   v0.12.3                                 linux/amd64, linux/arm64, linux/riscv64, linux/ppc64le, (+8)
macos                  docker-container
 \_ macos0              \_ tcp://mac-mini-m1.home.fooooo.com:2376   error                                             linux/arm64*
sifive                 docker-container
 \_ sifive0             \_ tcp://192.168.0.65:2375                  running   v0.12.2                                 linux/riscv64*
default*               docker
 \_ default             \_ default                                  running   v0.11.7+d3e6c1360f6e                    linux/amd64, linux/arm64, linux/riscv64, linux/ppc64le, (+8)
docker-test            docker
 \_ docker-test         \_ docker-test                              running   v0.11.6                                 linux/amd64, linux/386
docker20               docker
 \_ docker20            \_ docker20                                 running   v0.8+unknown                            linux/amd64, linux/386
docker23               docker
 \_ docker23            \_ docker23                                 running   v0.10.6+d52b2d5                         linux/amd64, linux/386
docker24               docker
 \_ docker24            \_ docker24                                 running   v0.11.6                                 linux/amd64, linux/386
docker24-arm64         docker
 \_ docker24-arm64      \_ docker24-arm64                           running   v0.11.7-0.20230525183624-798ad6b0ce9f   linux/arm64, linux/arm/v7, linux/arm/v6
remote-ssh             docker
 \_ remote-ssh          \_ remote-ssh                               running   v0.11.6+0a15675913b7                    linux/arm64, linux/arm/v7, linux/arm/v6

crazy-max
crazy-max commented Nov 23, 2023
View reviewed changes
commands/ls.go Outdated
Comment on lines 295 to 304
majorPlatforms := map[string]struct{}{
"linux/amd64": {},
"linux/arm64": {},
"linux/arm/v7": {},
"linux/mips64": {},
"linux/ppc64le": {},
"linux/riscv64": {},
"linux/s390x": {},
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Following platforms take priority but let me know if we should change that.

@crazy-max crazy-max force-pushed the ls-notrunc branch 2 times, most recently from db1dd93 to 94cdae5 Compare November 23, 2023 14:18
crazy-max
crazy-max commented Nov 23, 2023
View reviewed changes
commands/ls_test.go Outdated
name: "all preferred",
platforms: []string{"darwin/arm64*", "linux/arm64*", "linux/arm/v5*", "linux/arm/v6*", "linux/arm/v7*", "windows/arm64*"},
expected: []string{"linux/arm64*", "linux/arm/v7*", "darwin/arm64*", "linux/arm/v5*"},
truncated: true,
max: 4,
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we always display all preferred platforms even if it exceeds the limit?

crazy-max
crazy-max commented Nov 23, 2023
View reviewed changes
commands/ls_test.go Outdated
Comment on lines 57 to 56
name: "no major preferred",
platforms: []string{"linux/amd64/v2*", "linux/arm/v6*", "linux/mips64le*", "linux/amd64", "linux/amd64/v3", "linux/386", "linux/arm64", "linux/riscv64", "linux/ppc64le", "linux/s390x", "linux/mips64", "linux/arm/v7"},
expected: []string{"linux/amd64", "linux/arm64", "linux/riscv64", "linux/ppc64le"},
truncated: true,
max: 4,
Copy link
Member Author

Choose a reason for hiding this comment

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

Should preferred platforms take priority even if not part of major ones?

@crazy-max crazy-max mentioned this pull request Nov 23, 2023
Closed
@crazy-max crazy-max marked this pull request as ready for review November 23, 2023 14:23
tonistiigi
tonistiigi reviewed Nov 28, 2023
View reviewed changes
Copy link
Member

@tonistiigi tonistiigi left a comment •
edited

Choose a reason for hiding this comment

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

I think a better output could be linux/arm64, linux/amd64 (N more)

For the amd64 variants, I guess we could just show the maximum variant with truncated mode.

@crazy-max crazy-max force-pushed the ls-notrunc branch 2 times, most recently from cda6a69 to 3a19fa5 Compare November 30, 2023 15:12
@crazy-max
Copy link
Member Author

I think a better output could be linux/arm64, linux/amd64 (N more)

👍

For the amd64 variants, I guess we could just show the maximum variant with truncated mode.

Hum so for linux/amd64, linux/amd64/v2, linux/amd64/v3 we should only display linux/amd64/v3 or linux/amd64, linux/amd64/v3? User might be confused if we just display max variant only. I guess this is similar to linux/arm and linux/arm/v7 🤔

@tonistiigi
Copy link
Member

User might be confused if we just display max variant only.

If you think it is confusing, then I think better to not show variant at all in truncated view. We should not show linux/amd64/v1 or linux/amd64/v2 if that is not the max variant. The question is though if linux/amd64 (2 more) untruncated to linux/amd64 linux/amd64/v2 linux/amd64/v3` makes much sense.

@crazy-max
ls: no-trunc opt
494f85f 
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
crazy-max
crazy-max commented Dec 12, 2023
View reviewed changes
commands/ls_test.go
Comment on lines +18 to +19
platforms: []string{"linux/arm64*", "linux/amd64", "linux/amd64/v2", "linux/riscv64", "linux/ppc64le", "linux/s390x", "linux/386", "linux/mips64le", "linux/mips64", "linux/arm/v7", "linux/arm/v6"},
expected: []string{"linux/arm64*", "linux/amd64", "linux/arm/v7", "linux/ppc64le"},
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the priority over popular platforms to iterate over them until finding a match instead of using a map so we add some weight from the list.

I think this truncated output makes sense based on popular CPU architectures. I will take a look about stripping the variant.

@crazy-max crazy-max marked this pull request as draft December 23, 2023 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/cli
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@crazy-max @tonistiigi