Add spinner loading to docker cp command #2708

maximillianfx · 2020-09-04T22:04:57Z

- What I did

When a user executes docker cp command, a Copying message is showed on terminal and a spinner is printed out together, with a small animation.

- How I did it

Add animation using a channel and an array of characters to simulate a spinner in cp.go.

- How to verify it

Run the environment container with make -f docker.Makefile shell, then run the command ./build/docker-linux-amd64 cp file container:path to see.

- Description for the changelog

Addition of animated spinner when copying files using the docker cp command

- A picture of a cute animal (not mandatory but encouraged)

closes #2564
closes #1281

codecov-commenter · 2020-09-05T00:12:27Z

Codecov Report

Merging #2708 into master will decrease coverage by 1.32%.
The diff coverage is 72.39%.

@@            Coverage Diff             @@
##           master    #2708      +/-   ##
==========================================
- Coverage   58.42%   57.10%   -1.33%     
==========================================
  Files         295      297       +2     
  Lines       21201    18690    -2511     
==========================================
- Hits        12387    10672    -1715     
+ Misses       7906     7153     -753     
+ Partials      908      865      -43

silvin-lubecki · 2020-09-08T08:47:41Z

Hello @maximillianfx, thank you for opening this PR, but can I ask what is the rational behind adding a spinner to this command? I guess the issue is that you don't really know what's going on and the command looks blocked (like if you cp a large file to a remote engine)?
Maybe then we shouldn't add a spinner (I don't think there's one yet in the CLI UX, so I don't think we should introduce one here), but instead have a counter of the bytes sent or received, like we do when we send the context to docker build or when we pull an image?

maximillianfx · 2020-09-08T09:15:02Z

Hi @silvin-lubecki, how are you? Thanks for the quick help! Yeah, the reason for the spinner is to show to user a little animation/progression about the command, but I understand that this have some UI restrictions.

Your idea about a progress bar looks great and achieve the same (or even better) goal. Can I adjust this PR to this new approach?

silvin-lubecki · 2020-09-08T09:30:08Z

cc @thaJeztah any thoughts?

thaJeztah · 2020-09-10T13:41:28Z

Chatted with @silvin-lubecki and I agree that printing the amount of data that we copied would be more informative than just a spinner.

We weren't sure if it's possible to know up-front how much data will be copied (in case you're copying a directory structure, the API will probably not return the total size?), so if we don't know the total, a progress-bar won't make much sense; in that case, only printing the amount to data that was copied (copied xx kB ... copied xx mB) would work

maximillianfx · 2020-09-10T14:25:33Z

Yes, I understand, the directory scenario shows exactly the problem when displaying the amount of data that will be copied. Therefore, displaying a simple message after the copying process seems more effective. I will make these changes to this PR and we can return to the discussion soon.

maximillianfx · 2020-09-18T21:10:27Z

Hey guys. I was working in this item yesterday, and I faced the content variable inside the copyToContainer and copyFromContainer functions. Getting the size from content from copyToContainer function returns more bytes than the real file size. Is this behavior right?

func getCopySize(data io.ReadCloser) (int, io.Reader) {
	content, err := ioutil.ReadAll(data.Body)
	if err != nil {
		fmt.Println(err)
	}
	
    return len(content), bytes.NewReader(content)
}

In my tests, the file size is 6 bytes, but len(content) returns 2048.

maximillianfx · 2020-09-18T21:35:27Z

I made a mistake, the file is transformed to a Tar file. So, I'll made the size evaluation before this transformation.

maximillianfx · 2020-09-19T00:54:22Z

Alright @silvin-lubecki @thaJeztah ! Can you run these changes and give me some feedback?

silvin-lubecki · 2020-09-21T07:07:54Z

Hello @maximillianfx, thanks for the update! But I think there's an issue with your last rebase, the PR now includes all the last commits from master, so it's pretty hard to follow which are your commits/modifications 😅 Could you rebase properly your commits? 🙏

maximillianfx · 2020-09-21T11:06:48Z

Yes, I'll update this.

maximillianfx · 2020-09-21T12:51:06Z

@silvin-lubecki the current rebase is wrong? Because, I made the rebase to put my commits in front of the last commits of master, this is wrong? Can you help with the correct rebase?

silvin-lubecki · 2020-09-21T14:04:45Z

Ok I reviewed the code using master...maximillianfx:2564-add-spinner-loading
If I understood it correctly, it waits for the cp to be done, and then prints the size ? But it means that during the cp you won't see anything as it was before? I think we should have something more dynamic, updating in real time the content size transferred. WDYT?

maximillianfx · 2020-09-21T14:13:16Z

Thanks for reviewing. I don't know, because the cp command generally takes some ms or seconds to complete, different from docker pull, where the download speed interfere in the progress (some seconds up to minutes). Could be a better experience. I'll investigate if this change will be affect more files than cp.go and the complexity to implement.

silvin-lubecki · 2020-09-21T14:49:28Z

I guess that if you are targeting a remote daemon the cp will take more time 🤔

maximillianfx · 2020-09-21T15:16:29Z

Yes, this scenario could occur. I'll investigate.

maximillianfx · 2020-09-26T15:30:22Z

Hi @silvin-lubecki how are you? The behavior of these dynamically copying should be implemented on the engine project itself, WDYT? Because, looking into the pull command, the animation for the download is implemented there. In the cli project could be a more generic implementation, I think.

albers · 2020-09-26T15:47:43Z

Removing myself as a reviewer because completion is not touched by this PR.

silvin-lubecki · 2020-10-05T06:53:49Z

Hello @maximillianfx, I think that if you replace the io.Copy https://github.com/docker/cli/blob/master/cli/command/container/cp.go#L163 here with a "dynamic" printer, it should be enough 🤔 . WDYT?

maximillianfx · 2020-10-05T13:42:22Z

Could be a solution, but this will affect only the copy from container and not to container, isn't it?

maximillianfx · 2020-10-15T14:43:30Z

@silvin-lubecki did you manage to check my last question?

silvin-lubecki · 2020-10-15T16:27:44Z

Sorry @maximillianfx I missed your question, thank you for the ping 😅
I think we can do it the other way too https://github.com/docker/cli/blob/master/cli/command/container/cp.go#L232
Content is an io.Reader, so we could implement the interface, counting how much data was transferred here. WDYT?

maximillianfx · 2020-10-15T18:01:56Z

Yeah, this looks great @silvin-lubecki. I'll read more about this, and make the changes 😄

codecov-io · 2020-10-16T17:32:02Z

Codecov Report

Merging #2708 into master will decrease coverage by 1.32%.
The diff coverage is 72.39%.

@@            Coverage Diff             @@
##           master    #2708      +/-   ##
==========================================
- Coverage   58.42%   57.10%   -1.33%     
==========================================
  Files         295      297       +2     
  Lines       21201    18690    -2511     
==========================================
- Hits        12387    10672    -1715     
+ Misses       7906     7153     -753     
+ Partials      908      865      -43

codecov-commenter · 2020-10-16T18:08:12Z

Codecov Report

Merging #2708 into master will decrease coverage by 1.27%.
The diff coverage is 74.88%.

@@            Coverage Diff             @@
##           master    #2708      +/-   ##
==========================================
- Coverage   58.42%   57.15%   -1.28%     
==========================================
  Files         295      297       +2     
  Lines       21201    18721    -2480     
==========================================
- Hits        12387    10700    -1687     
+ Misses       7906     7157     -749     
+ Partials      908      864      -44

codecov-io · 2020-10-16T18:08:15Z

Codecov Report

Merging #2708 (9c7bdc4) into master (ed8ce81) will decrease coverage by 1.53%.
The diff coverage is 30.30%.

@@            Coverage Diff             @@
##           master    #2708      +/-   ##
==========================================
- Coverage   58.42%   56.89%   -1.54%     
==========================================
  Files         295      295              
  Lines       21201    18534    -2667     
==========================================
- Hits        12387    10544    -1843     
+ Misses       7906     7129     -777     
+ Partials      908      861      -47

maximillianfx · 2020-10-16T18:12:18Z

I have made some changes. Added a little progress bar to indicate the copy process. Both copies, from and into container, are working. I have made this because only displaying the amount of bytes transfered could be unreadable 😕. WDYT @silvin-lubecki ?

thaJeztah · 2021-02-16T13:03:34Z

I was making changes to the code while I wrote those comments, and will open a pull request against your branch if that's easier for you 😅

I opened maximillianfx#1 on your fork 👍

maximillianfx · 2021-02-16T13:13:00Z

Thanks @thaJeztah ! Your comments are very hepful and necessary. Your PR is welcome 😄

thaJeztah · 2021-02-16T13:28:02Z

You're welcome! Be sure to check my changes with the control-codes, because I'm horrible at those 😂

maximillianfx · 2021-02-16T14:11:45Z

Of course, thanks!

cli/command/container/cp.go

maximillianfx · 2021-02-22T19:32:15Z

@thaJeztah Some problems arose, I'll made more tests.

maximillianfx · 2021-02-22T20:21:24Z

I think that's ok now, PTAL @thaJeztah 😄

maximillianfx · 2021-03-02T11:37:02Z

Hi @thaJeztah , how are you? Could you review again?
Thanks.

thaJeztah · 2021-04-22T09:39:06Z

Ah, welp.. this one slipped off my radar again 😬. Let me have another look.

Before merging, we should squash the commits (let me have a quick look at doing a rebase and squash)

thaJeztah · 2021-04-24T11:22:04Z

I rebased (will push in a minute); and found some last small things, which I added in an extra commit (but will squash);

Remove the "progress" line after copy is complete

After copying, the "progress" line was kept:

 docker cp foo:/usr/share/nginx bla
 Preparing to copy...

 docker cp foo:/usr/share/nginx bla
 Copying from container - 512B

 docker cp foo:/usr/share/nginx bla
 Copying from container - 4.608kB
 Successfully copied 4.608kB to /go/src/github.com/docker/cli/bla

The "Copying from container" line is a bit redundant after it's completed, so
removing the line afterwards:

 docker cp foo:/usr/share/nginx bla
 Preparing to copy...

 docker cp foo:/usr/share/nginx bla
 Copying from container - 512B

 docker cp foo:/usr/share/nginx bla
 Successfully copied 4.608kB to /go/src/github.com/docker/cli/bla

Print progress lines with a trailing newline

Without a newline, the cursor would be visible at the end of the progress
line, which was a bit distracting, as it would be jumping if the progress
line was not the same width for each refresh.
Swapped fmt.Fprint for fmt.Fprintln in some places

Same result, just prevents having to add \n in the strings, and it automatically
joins arguments with a space in between, so a slight bit cleaner.

Here's a before/after:

before.mov

after.mov

Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Maximillian Fan Xavier <maximillianfx@gmail.com>

codecov-commenter · 2021-04-24T11:25:56Z

Codecov Report

Merging #2708 (b4ade48) into master (daf5f12) will decrease coverage by 0.24%.
The diff coverage is 13.95%.

❗ Current head b4ade48 differs from pull request most recent head 12370ad. Consider uploading reports for the commit 12370ad to get more accurate results

@@            Coverage Diff             @@
##           master    #2708      +/-   ##
==========================================
- Coverage   57.08%   56.83%   -0.25%     
==========================================
  Files         299      295       -4     
  Lines       18689    18544     -145     
==========================================
- Hits        10668    10540     -128     
+ Misses       7155     7145      -10     
+ Partials      866      859       -7

thaJeztah · 2021-04-24T11:28:22Z

Pushed the squashed and rebased version; also squashed my last changes; diff below for posterity;

From c5e7af600e90b1793c789219773cdbf6ed1948a1 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 24 Apr 2021 13:14:13 +0200
Subject: [PATCH] Some small fix-ups / improvements

1. Remove the "progress" line after copy is complete

After copying, the "progress" line was kept:

    docker cp foo:/usr/share/nginx bla
    Preparing to copy...

    docker cp foo:/usr/share/nginx bla
    Copying from container - 512B

    docker cp foo:/usr/share/nginx bla
    Copying from container - 4.608kB
    Successfully copied 4.608kB to /go/src/github.com/docker/cli/bla

The "Copying from container" line is a bit redundant after it's completed, so
removing the line afterwards:

    docker cp foo:/usr/share/nginx bla
    Preparing to copy...

    docker cp foo:/usr/share/nginx bla
    Copying from container - 512B

    docker cp foo:/usr/share/nginx bla
    Successfully copied 4.608kB to /go/src/github.com/docker/cli/bla

2. Print progress lines with a trailing newline

Without a newline, the cursor would be visible at the end of the progress
line, which was a bit distracting, as it would be jumping if the progress
line was not the same width for each refresh.

3. Swapped `fmt.Fprint` for `fmt.Fprintln` in some places

Same result, just prevents having to add `\n` in the strings, and it automatically
joins arguments with a space in between, so a slight bit cleaner.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 cli/command/container/cp.go | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/cli/command/container/cp.go b/cli/command/container/cp.go
index 071becf94b..01940d5616 100644
--- a/cli/command/container/cp.go
+++ b/cli/command/container/cp.go
@@ -61,9 +61,9 @@ func (pt *copyProgressPrinter) Read(p []byte) (int, error) {
 		fmt.Fprint(pt.writer, aec.Restore)
 		fmt.Fprint(pt.writer, aec.EraseLine(aec.EraseModes.All))
 		if pt.toContainer {
-			fmt.Fprint(pt.writer, "Copying to container - "+units.HumanSize(*pt.total))
+			fmt.Fprintln(pt.writer, "Copying to container - "+units.HumanSize(*pt.total))
 		} else {
-			fmt.Fprint(pt.writer, "Copying from container - "+units.HumanSize(*pt.total))
+			fmt.Fprintln(pt.writer, "Copying from container - "+units.HumanSize(*pt.total))
 		}
 	}
 
@@ -229,10 +229,11 @@ func copyFromContainer(ctx context.Context, dockerCli command.Cli, copyConfig cp
 	}
 
 	fmt.Fprint(dockerCli.Err(), aec.Save)
-	fmt.Fprint(dockerCli.Err(), "Preparing to copy...\n")
+	fmt.Fprintln(dockerCli.Err(), "Preparing to copy...")
 	res := archive.CopyTo(preArchive, srcInfo, dstPath)
-	fmt.Fprint(dockerCli.Err(), "\n")
-	fmt.Fprint(dockerCli.Err(), "Successfully copied ", units.HumanSize(copiedSize), " to ", dstPath, "\n")
+	fmt.Fprint(dockerCli.Err(), aec.Restore)
+	fmt.Fprint(dockerCli.Err(), aec.EraseLine(aec.EraseModes.All))
+	fmt.Fprintln(dockerCli.Err(), "Successfully copied", units.HumanSize(copiedSize), "to", dstPath)
 
 	return res
 }
@@ -351,10 +352,11 @@ func copyToContainer(ctx context.Context, dockerCli command.Cli, copyConfig cpCo
 	}
 
 	fmt.Fprint(dockerCli.Err(), aec.Save)
-	fmt.Fprint(dockerCli.Err(), "Preparing to copy...\n")
+	fmt.Fprintln(dockerCli.Err(), "Preparing to copy...")
 	res := client.CopyToContainer(ctx, copyConfig.container, resolvedDstPath, content, options)
-	fmt.Fprint(dockerCli.Err(), "\n")
-	fmt.Fprint(dockerCli.Err(), "Successfully copied ", units.HumanSize(copiedSize), " to ", copyConfig.container, ":", dstInfo.Path, "\n")
+	fmt.Fprint(dockerCli.Err(), aec.Restore)
+	fmt.Fprint(dockerCli.Err(), aec.EraseLine(aec.EraseModes.All))
+	fmt.Fprintln(dockerCli.Err(), "Successfully copied", units.HumanSize(copiedSize), "to", copyConfig.container+":"+dstInfo.Path)
 
 	return res
 }

thaJeztah

LGTM, thanks!!!

thaJeztah · 2021-04-24T15:29:00Z

@silvin-lubecki @cpuguy83 PTAL

silvin-lubecki

LGTM

silvin-lubecki · 2021-04-28T16:24:32Z

Thanks a lot @maximillianfx for this awesome work 👍

maximillianxavier · 2021-04-28T17:53:57Z

Thank you guys!

`container cp` docker/cli#2708 `container create/run` docker/cli#3377

Azratosh · 2023-02-24T14:50:29Z

May I ask what the rationale behind suddenly adding output to a previously silent command is? This didn't really break anything for me, but it's instead just very annoying, because docker cp now clogs up my terminal when it didn't before:

What's obviously not shown in the screenshot are the thousands of other lines that the command now prints.

Therefore, why not just add this kind of behaviour through a --progress flag or similar? Why does the default behaviour for a command need to change? And moreover, if it now does have a progress indicator, fine - but why does it only show up when it's connected to a TTY? Why can I not pipe the progress output to a log file in case I explicitly want to see it, in order to, for example, see at which point a large transfer gets aborted?

Edit: Phrasing.

timokz · 2023-03-01T01:46:57Z

May I ask what the rationale behind suddenly adding output to a previously silent command is? This didn't really break anything for me, but it's instead just very annoying, because docker cp now clogs up my terminal when it didn't before:

What's obviously not shown in the screenshot are the thousands of other lines that the command now prints.

Therefore, why not just add this kind of behaviour through a --progress flag or similar? Why does the default behaviour for a command need to change? And moreover, if it now does have a progress indicator, fine - but why does it only show up when it's connected to a TTY? Why can I not pipe the progress output to a log file in case I explicitly want to see it, in order to, for example, see at which point a large transfer gets aborted?

Edit: Phrasing.

This seems very annoying and doesn't appear to be expected behavior.
Maybe there is an error in the implementation of the new progress indication?

maximillianfx requested review from albers and thaJeztah as code owners September 18, 2020 23:55

albers removed their request for review September 26, 2020 15:47

thaJeztah mentioned this pull request Feb 16, 2021

Review suggestions maximillianfx/cli#1

Merged

thaJeztah reviewed Feb 22, 2021

View reviewed changes

cli/command/container/cp.go Outdated Show resolved Hide resolved

maximillianfx requested a review from thaJeztah April 20, 2021 17:36

Add progress bar to copy into and from container

12370ad

Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Maximillian Fan Xavier <maximillianfx@gmail.com>

thaJeztah force-pushed the 2564-add-spinner-loading branch from b4ade48 to 12370ad Compare April 24, 2021 11:25

thaJeztah approved these changes Apr 24, 2021

View reviewed changes

silvin-lubecki approved these changes Apr 28, 2021

View reviewed changes

silvin-lubecki merged commit 87add19 into docker:master Apr 28, 2021

thaJeztah added impact/changelog kind/enhancement labels Apr 28, 2021

thaJeztah added this to the 21.xx milestone Apr 28, 2021

matt9ucci added a commit to matt9ucci/DockerCompletion that referenced this pull request Feb 9, 2023

CLI 23.0.0: add --quiet

49a6b6d

`container cp` docker/cli#2708 `container create/run` docker/cli#3377

matt9ucci added a commit to matt9ucci/DockerCompletion that referenced this pull request Feb 9, 2023

CLI 23.0.0: add --quiet

ba3efa6

`container cp` docker/cli#2708 `container create/run` docker/cli#3377

dtronche mentioned this pull request Mar 29, 2023

docker cp spinner does not work #4135

Closed

Add spinner loading to docker cp command #2708

Add spinner loading to docker cp command #2708

Conversation

maximillianfx commented Sep 4, 2020 • edited by thaJeztah Loading

codecov-commenter commented Sep 5, 2020 • edited Loading

Codecov Report

silvin-lubecki commented Sep 8, 2020

maximillianfx commented Sep 8, 2020

silvin-lubecki commented Sep 8, 2020

thaJeztah commented Sep 10, 2020

maximillianfx commented Sep 10, 2020

maximillianfx commented Sep 18, 2020

maximillianfx commented Sep 18, 2020

maximillianfx commented Sep 19, 2020

silvin-lubecki commented Sep 21, 2020

maximillianfx commented Sep 21, 2020

maximillianfx commented Sep 21, 2020

silvin-lubecki commented Sep 21, 2020

maximillianfx commented Sep 21, 2020 • edited Loading

silvin-lubecki commented Sep 21, 2020

maximillianfx commented Sep 21, 2020

maximillianfx commented Sep 26, 2020

albers commented Sep 26, 2020

silvin-lubecki commented Oct 5, 2020

maximillianfx commented Oct 5, 2020

maximillianfx commented Oct 15, 2020

silvin-lubecki commented Oct 15, 2020

maximillianfx commented Oct 15, 2020 • edited Loading

codecov-io commented Oct 16, 2020

Codecov Report

codecov-commenter commented Oct 16, 2020

Codecov Report

codecov-io commented Oct 16, 2020 • edited Loading

Codecov Report

maximillianfx commented Oct 16, 2020

thaJeztah commented Feb 16, 2021

maximillianfx commented Feb 16, 2021

thaJeztah commented Feb 16, 2021

maximillianfx commented Feb 16, 2021

maximillianfx commented Feb 22, 2021

maximillianfx commented Feb 22, 2021

maximillianfx commented Mar 2, 2021

thaJeztah commented Apr 22, 2021

thaJeztah commented Apr 24, 2021

codecov-commenter commented Apr 24, 2021

Codecov Report

thaJeztah commented Apr 24, 2021

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Apr 24, 2021

silvin-lubecki left a comment

Choose a reason for hiding this comment

silvin-lubecki commented Apr 28, 2021

maximillianxavier commented Apr 28, 2021

Azratosh commented Feb 24, 2023 • edited Loading

timokz commented Mar 1, 2023

maximillianfx commented Sep 4, 2020 •

edited by thaJeztah

Loading

codecov-commenter commented Sep 5, 2020 •

edited

Loading

maximillianfx commented Sep 21, 2020 •

edited

Loading

maximillianfx commented Oct 15, 2020 •

edited

Loading

codecov-io commented Oct 16, 2020 •

edited

Loading

Azratosh commented Feb 24, 2023 •

edited

Loading