feature: pouch copy in client and daemon side #2182

knightXun · 2018-08-31T01:55:52Z

Ⅰ. Describe what this PR did

copy file from container
copy file to container

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

allencloud · 2018-08-31T07:05:32Z

apis/types/container_copy.go

@@ -0,0 +1,30 @@
+package types


We do not manually edit this file, but generate it by swagger, you could see the reference: https://github.com/alibaba/pouch/blob/master/apis/README.md

allencloud · 2018-08-31T07:06:29Z

cli/cp.go

+	"strings"
+	"path/filepath"
+
+	"github.com/spf13/cobra"


We should improve the import sequence by https://github.com/alibaba/pouch/blob/master/docs/contributions/code_styles.md#rule003---import-packages
And please keep this in mind in imports of all this pull request.

allencloud · 2018-08-31T07:07:04Z

daemon/mgr/container.go

+	// We need to rebase the archive entries if the last element of the
+	// resolved path was a symlink that was evaluated and is now different
+	// than the requested path. For example, if the given path was "/foo/bar/",
+	// but it resolved to "/var/lib/docker/containers/{id}/foo/baz/", we want


I do not think the details here are proper.

allencloud · 2018-08-31T07:08:08Z

apis/server/container_bridge.go

@@ -19,6 +19,9 @@ import (
 	"github.com/gorilla/mux"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	"encoding/base64"


import sequence.

allencloud · 2018-08-31T07:11:14Z

client/container_copy.go

+	return getContainerPathStatFromHeader(response.Header)
+}
+
+func ensureReaderClosed(response *Response) {


This function is redudant with https://github.com/alibaba/pouch/blob/master/client/utils.go#L18.
Maybe we can re-use that.

allencloud · 2018-08-31T07:13:58Z

pkg/httputils/http_utils.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 )

+


Please remove this blank line.

allencloud · 2018-08-31T07:18:23Z

I only add some initial comment. For this valuable part, I think we need to spend more time and patience to make this move on. @knightXun
We will definitely help to make it work. Let us carry on. Thanks a lot.

allencloud · 2018-09-04T13:37:23Z

Any update on this? @knightXun

codecov-io · 2018-09-07T03:55:42Z

Codecov Report

Merging #2182 into master will decrease coverage by 0.37%.
The diff coverage is 42.17%.

@@            Coverage Diff             @@
##           master    #2182      +/-   ##
==========================================
- Coverage    66.5%   66.12%   -0.38%     
==========================================
  Files         208      208              
  Lines       16755    16985     +230     
==========================================
+ Hits        11143    11232      +89     
- Misses       4282     4403     +121     
- Partials     1330     1350      +20

Flag	Coverage Δ
#criv1alpha1test	`32.35% <1.3%> (-0.38%)`	⬇️
#criv1alpha2test	`33.04% <1.3%> (-0.54%)`	⬇️
#integrationtest	`39.95% <42.17%> (+0.01%)`	⬆️
#nodee2etest	`33.09% <1.3%> (-0.59%)`	⬇️
#unittest	`23.5% <0%> (-0.36%)`	⬇️

Impacted Files	Coverage Δ
apis/server/router.go	`92.1% <100%> (+0.15%)`	⬆️
daemon/mgr/container.go	`53.06% <27.95%> (-4.53%)`	⬇️
apis/server/container_bridge.go	`78.24% <71.05%> (-1.02%)`	⬇️
daemon/mgr/container_state.go	`85.24% <78.57%> (-1.99%)`	⬇️
cri/ocicni/cni_manager.go	`65% <0%> (-15%)`	⬇️
ctrd/watch.go	`78.78% <0%> (-4.55%)`	⬇️
cri/v1alpha2/cri.go	`71.24% <0%> (-0.64%)`	⬇️
cri/v1alpha2/cri_wrapper.go	`64.43% <0%> (ø)`	⬆️
daemon/containerio/container_io.go	`75.69% <0%> (+1.1%)`	⬆️
... and 3 more

knightXun · 2018-09-07T04:07:50Z

now working on e2e test

knightXun · 2018-09-10T10:35:25Z

ping @allencloud

allencloud · 2018-09-10T11:44:26Z

I am afraid that we need to add more details in swagger.yml to define the API side. For adding new APIs, we should add definition in swagger.yml, like https://github.com/alibaba/pouch/blob/master/apis/README.md .

So please help to add that as well. I think we are almost there. Keep going. Thanks a lot. @knightXun

knightXun · 2019-01-23T04:46:12Z

I will split this pr into 4 or 5 parts.

pouchrobot · 2019-01-28T06:58:02Z

ping @knightXun
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

fuweid · 2019-01-28T09:21:51Z

thanks @knightXun . And I close it and looking forward to the new PR :)

pouchrobot added the size/XXL label Aug 31, 2018

knightXun changed the title ~~[WIP] pouch copy~~ [WIP] [feature] pouch copy Aug 31, 2018

pouchrobot added the kind/feature label Aug 31, 2018

allencloud reviewed Aug 31, 2018

View reviewed changes

pkg/httputils/http_utils.go Outdated

@@ -5,6 +5,7 @@ import (

"strings"

)

Copy link

Collaborator

allencloud Aug 31, 2018

Choose a reason for hiding this comment

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

Please remove this blank line.

allencloud changed the title ~~[WIP] [feature] pouch copy~~ [WIP] feature: and pouch copy in client and daemon side Aug 31, 2018

knightXun force-pushed the pouch-ls branch 8 times, most recently from 2bfa6b1 to fa32481 Compare September 7, 2018 03:02

knightXun force-pushed the pouch-ls branch 4 times, most recently from a726b1b to b1fe729 Compare September 10, 2018 08:12

knightXun changed the title ~~[WIP] feature: and pouch copy in client and daemon side~~ feature: and pouch copy in client and daemon side Sep 10, 2018

knightXun force-pushed the pouch-ls branch from b1fe729 to 73b911f Compare September 10, 2018 09:10

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Jan 23, 2019

weekly-digest bot mentioned this pull request Jan 27, 2019

Weekly Digest (20 January, 2019 - 27 January, 2019) #2695

Closed

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Jan 28, 2019

fuweid closed this Jan 28, 2019

ZYecho mentioned this pull request May 30, 2019

feature: add pouch cp cmd #2879

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: pouch copy in client and daemon side #2182

feature: pouch copy in client and daemon side #2182

knightXun commented Aug 31, 2018

allencloud Aug 31, 2018

knightXun Aug 31, 2018

allencloud Aug 31, 2018 •

edited

allencloud Aug 31, 2018

allencloud Aug 31, 2018

allencloud Aug 31, 2018

allencloud Aug 31, 2018

allencloud commented Aug 31, 2018

allencloud commented Sep 4, 2018

codecov-io commented Sep 7, 2018 •

edited

knightXun commented Sep 7, 2018

knightXun commented Sep 10, 2018

allencloud commented Sep 10, 2018

knightXun commented Jan 23, 2019

pouchrobot commented Jan 28, 2019

fuweid commented Jan 28, 2019

feature: pouch copy in client and daemon side #2182

feature: pouch copy in client and daemon side #2182

Conversation

knightXun commented Aug 31, 2018

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

allencloud Aug 31, 2018

Choose a reason for hiding this comment

knightXun Aug 31, 2018

Choose a reason for hiding this comment

allencloud Aug 31, 2018 • edited

Choose a reason for hiding this comment

allencloud Aug 31, 2018

Choose a reason for hiding this comment

allencloud Aug 31, 2018

Choose a reason for hiding this comment

allencloud Aug 31, 2018

Choose a reason for hiding this comment

allencloud Aug 31, 2018

Choose a reason for hiding this comment

allencloud commented Aug 31, 2018

allencloud commented Sep 4, 2018

codecov-io commented Sep 7, 2018 • edited

Codecov Report

knightXun commented Sep 7, 2018

knightXun commented Sep 10, 2018

allencloud commented Sep 10, 2018

knightXun commented Jan 23, 2019

pouchrobot commented Jan 28, 2019

fuweid commented Jan 28, 2019

allencloud Aug 31, 2018 •

edited

codecov-io commented Sep 7, 2018 •

edited