Show copy progress for CopyFileToMachine on Shell Service #4918
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
| return pr.reader.Stat() | ||
| } | ||
|
|
||
| func (pr *progressReader) Close() error { |
There was a problem hiding this comment.
consider adding final \n somewhere (potentially in Close)
as-is, the next shell prompt appears on the same line as the progress output
~/repo/rdk$ go run ./cli/viam machine part cp --part PART README.md machine:README.md
Copying README.md...
Progress: 100% (5577/5577 bytes)~/repo/rdk$There was a problem hiding this comment.
Oops. Good catch. I had this but then I refactored to handle multiple files.
| } | ||
| uploadPercent := int(math.Ceil(100 * float64(bytes) / float64(fileSize))) | ||
| //nolint:errcheck // progress display is non-critical | ||
| _, _ = os.Stdout.WriteString(fmt.Sprintf("\rProgress: %d%% (%d/%d bytes)", uploadPercent, bytes, fileSize)) |
There was a problem hiding this comment.
note this will work differently in headless shells
would not worry too much about it, but maybe add a --no-progress field for reload + cp, so that people can skip this behavior if it's messing with their shell?
(or assign this to me as afterwork so this PR can get merged; I feel bad that I took so long to start this review)
There was a problem hiding this comment.
Good idea. Was easy.
viambot
added
safe to test
viambot
added
safe to test
|
Hey @cheukt can you give this a review? |
cli/app.go
Outdated
| @@ -75,6 +75,7 @@ const ( | |||
| moduleFlagForce = "force" | |||
| moduleFlagBinary = "binary" | |||
| moduleFlagLocal = "local" | |||
| moduleFlagNoProgress = "no-progress" | |||
There was a problem hiding this comment.
(minor) can we collapse this and cpFlagNoProgress into a single flag (generalFlagNoProgress or something like that)? Would prefer to not have two identical flags.
There was a problem hiding this comment.
Sure thing! I did not see the general flags above earlier. Makes total sense.
viambot
added
safe to test
It was hard to see the progress of the two CLI operations that copy files to machines:
viam module reloadviam machine part cpThis PR adds wrappers around the CopyFileToMachine
NewCopyFileToMachineFactoryto print out the status of the file transfer.The
viam machine part cpcommand now outputs this (works for both single and multiple files):And
viam module reload: