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

runsc: Fix newline for cmd usage display #11324

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

vax-r
Copy link
Contributor

@vax-r vax-r commented Dec 27, 2024

Summary

Originally, some of the commands' usage display didn't achieved to show newline, while some of them does, which are more user-friendly, and won't confuse the message if there're more which should be in the nextline.

Fix this issue by returning usage string with a newline.

Tests

Before the change some commands display their usage like the following, for example runsc start

vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc start
start <container id> - start a secure container.vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$

Or runsc wait

vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc wait
wait [flags] <container id>  -checkpoint
    	wait for the next checkpoint to complete
  -pid int
    	select a PID in the container's PID namespace to wait on instead of the container's root process (default -1)
  -rootpid int
    	select a PID in the sandbox root PID namespace to wait on instead of the container's root process (default -1)

We can see in the case of runsc wait, the flag -checkpoint is expected to be shown in a way which is aligned with other flags, like

vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc wait
wait [flags] <container id>  
  -checkpoint
    	wait for the next checkpoint to complete
  -pid int
    	select a PID in the container's PID namespace to wait on instead of the container's root process (default -1)
  -rootpid int
    	select a PID in the sandbox root PID namespace to wait on instead of the container's root process (default -1)

After the change , we can see those commands with this issue are being fixed.

vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc start
start <container id> - start a secure container.
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc wait
wait [flags] <container id>
  -checkpoint
    	wait for the next checkpoint to complete
  -pid int
    	select a PID in the container's PID namespace to wait on instead of the container's root process (default -1)
  -rootpid int
    	select a PID in the sandbox root PID namespace to wait on instead of the container's root process (default -1)

@milantracy
Copy link
Contributor

how about replacing backtick strings with double quota strings, and using the escape character\n for newlines.

for example

diff --git a/runsc/cmd/wait.go b/runsc/cmd/wait.go
index eab74c283..e5bfedf27 100644
--- a/runsc/cmd/wait.go
+++ b/runsc/cmd/wait.go
@@ -51,7 +51,7 @@ func (*Wait) Synopsis() string {
 
 // Usage implements subcommands.Command.Usage.
 func (*Wait) Usage() string {
-       return `wait [flags] <container id>`
+       return "wait [flags] <container id>\n"
 }

@vax-r
Copy link
Contributor Author

vax-r commented Dec 27, 2024

how about replacing backtick strings with double quota strings, and using the escape character\n for newlines.

@milantracy -
Thanks for your review !
Yeah that should be better and make the code more understandable, I should modify all the usage string to match the format.

I'll push it up ASAP , thanks !

Originally, some of the commands' usage display didn't achieved to show
newline, while some of them does, which are more user-friendly, and
won't confuse the message if there're more which should be in the
nextline.

Fix this issue by returning usage string with a newline.

Signed-off-by: I Hsin Cheng <richard120310@gmail.com>
@vax-r vax-r force-pushed the fix_usage_display branch from fcafe76 to 83f9f18 Compare December 28, 2024 19:50
@vax-r
Copy link
Contributor Author

vax-r commented Dec 28, 2024

@milantracy -
I've made the requested changes, please have a look while you're available, thanks !

copybara-service bot pushed a commit that referenced this pull request Dec 30, 2024
## Summary
Originally, some of the commands' usage display didn't achieved to show newline, while some of them does, which are more user-friendly, and won't confuse the message if there're more which should be in the nextline.

Fix this issue by returning usage string with a newline.

## Tests
Before the change some commands display their usage like the following, for example `runsc start`
```
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc start
start <container id> - start a secure container.vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$
```
Or `runsc wait`
```
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc wait
wait [flags] <container id>  -checkpoint
    	wait for the next checkpoint to complete
  -pid int
    	select a PID in the container's PID namespace to wait on instead of the container's root process (default -1)
  -rootpid int
    	select a PID in the sandbox root PID namespace to wait on instead of the container's root process (default -1)
```
We can see in the case of `runsc wait`, the flag `-checkpoint` is expected to be shown in a way which is aligned with other flags, like
```
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc wait
wait [flags] <container id>
  -checkpoint
    	wait for the next checkpoint to complete
  -pid int
    	select a PID in the container's PID namespace to wait on instead of the container's root process (default -1)
  -rootpid int
    	select a PID in the sandbox root PID namespace to wait on instead of the container's root process (default -1)
```

After the change , we can see those commands with this issue are being fixed.
```
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc start
start <container id> - start a secure container.
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$
```
```
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc wait
wait [flags] <container id>
  -checkpoint
    	wait for the next checkpoint to complete
  -pid int
    	select a PID in the container's PID namespace to wait on instead of the container's root process (default -1)
  -rootpid int
    	select a PID in the sandbox root PID namespace to wait on instead of the container's root process (default -1)
```

FUTURE_COPYBARA_INTEGRATE_REVIEW=#11324 from vax-r:fix_usage_display 83f9f18
PiperOrigin-RevId: 710756149
copybara-service bot pushed a commit that referenced this pull request Dec 30, 2024
## Summary
Originally, some of the commands' usage display didn't achieved to show newline, while some of them does, which are more user-friendly, and won't confuse the message if there're more which should be in the nextline.

Fix this issue by returning usage string with a newline.

## Tests
Before the change some commands display their usage like the following, for example `runsc start`
```
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc start
start <container id> - start a secure container.vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$
```
Or `runsc wait`
```
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc wait
wait [flags] <container id>  -checkpoint
    	wait for the next checkpoint to complete
  -pid int
    	select a PID in the container's PID namespace to wait on instead of the container's root process (default -1)
  -rootpid int
    	select a PID in the sandbox root PID namespace to wait on instead of the container's root process (default -1)
```
We can see in the case of `runsc wait`, the flag `-checkpoint` is expected to be shown in a way which is aligned with other flags, like
```
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc wait
wait [flags] <container id>
  -checkpoint
    	wait for the next checkpoint to complete
  -pid int
    	select a PID in the container's PID namespace to wait on instead of the container's root process (default -1)
  -rootpid int
    	select a PID in the sandbox root PID namespace to wait on instead of the container's root process (default -1)
```

After the change , we can see those commands with this issue are being fixed.
```
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc start
start <container id> - start a secure container.
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$
```
```
vax-r@vaxr-ASUSPRO-D840MB-M840MB:~/gvisor$ runsc wait
wait [flags] <container id>
  -checkpoint
    	wait for the next checkpoint to complete
  -pid int
    	select a PID in the container's PID namespace to wait on instead of the container's root process (default -1)
  -rootpid int
    	select a PID in the sandbox root PID namespace to wait on instead of the container's root process (default -1)
```

FUTURE_COPYBARA_INTEGRATE_REVIEW=#11324 from vax-r:fix_usage_display 83f9f18
PiperOrigin-RevId: 710756149
@copybara-service copybara-service bot merged commit 1db44ea into google:master Dec 30, 2024
5 checks passed
@EtiennePerot
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants