Skip to content

Conversation

rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented Apr 9, 2019

using pouch cli to run container, command is:

 # pouch run -d -v /tmp/test:/tmp/test:rslave,ro busybox top

it will parse -v become to bind ["/tmp/test:/tmp/test:rslave", "ro"]

by default, , is separator of command, so we should refact the default
rules.

Signed-off-by: Rudy Zhang rudyflyzhang@gmail.com

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

none

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

added

Ⅳ. Describe how to verify it

ci pass

Ⅴ. Special notes for reviews

using pouch cli to run container, command is:
```bash
 # pouch run -d -v /tmp/test:/tmp/test:rslave,ro busybox top
```
it will parse `-v` become to bind ["/tmp/test:/tmp/test:rslave", "ro"]

by default, `,` is separator of command, so we should refact the default
rules.

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
@pouchrobot pouchrobot added areas/storage kind/bug This is bug report for project size/XL labels Apr 9, 2019
@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #2798 into master will decrease coverage by 0.07%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2798      +/-   ##
==========================================
- Coverage   69.41%   69.34%   -0.08%     
==========================================
  Files         277      278       +1     
  Lines       17321    17340      +19     
==========================================
+ Hits        12024    12025       +1     
- Misses       3967     3975       +8     
- Partials     1330     1340      +10
Flag Coverage Δ
#criv1alpha2_test 39.31% <0%> (ø) ⬆️
#integration_test_0 36.52% <0%> (-0.02%) ⬇️
#integration_test_1 35.3% <0%> (-0.04%) ⬇️
#integration_test_2 36.48% <0%> (+0.11%) ⬆️
#integration_test_3 35.36% <0%> (-0.04%) ⬇️
#node_e2e_test 34.99% <0%> (-0.22%) ⬇️
#unittest 28.79% <89.47%> (+0.07%) ⬆️
Impacted Files Coverage Δ
apis/opts/config/volumes.go 89.47% <89.47%> (ø)
cri/ocicni/cni_manager.go 58.82% <0%> (-11.77%) ⬇️
ctrd/container.go 53.53% <0%> (-1.92%) ⬇️
cri/v1alpha2/cri.go 70.58% <0%> (-0.9%) ⬇️
daemon/mgr/container.go 60.59% <0%> (-0.22%) ⬇️
daemon/mgr/container_utils.go 82.95% <0%> (+0.56%) ⬆️
pkg/meta/store.go 68.99% <0%> (+1.55%) ⬆️
pkg/streams/utils.go 89.28% <0%> (+7.14%) ⬆️

@rudyfly rudyfly requested review from HusterWan and fuweid April 9, 2019 15:10
@HusterWan
Copy link
Contributor

Since this pr is cherry-pick from intenal branch, reviewed! LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Apr 10, 2019
Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

Nit. but LGTM

@rudyfly please add other pr to remove the comment line.

@@ -89,7 +90,8 @@ func addCommonFlags(flagSet *pflag.FlagSet) *container {

flagSet.StringVar(&c.utsMode, "uts", "", "UTS namespace to use")

flagSet.StringSliceVarP(&c.volume, "volume", "v", nil, "Bind mount volumes to container, format is: [source:]<destination>[:mode], [source] can be volume or host's path, <destination> is container's path, [mode] can be \"ro/rw/dr/rr/z/Z/nocopy/private/rprivate/slave/rslave/shared/rshared\"")
flagSet.VarP(config.NewVolumes(&c.volume), "volume", "v", "Bind mount volumes to container, format is: [source:]<destination>[:mode], [source] can be volume or host's path, <destination> is container's path, [mode] can be \"ro/rw/dr/rr/z/Z/nocopy/private/rprivate/slave/rslave/shared/rshared\"")
//flagSet.StringSliceVarP(&c.volume, "volume", "v", nil, "Bind mount volumes to container, format is: [source:]<destination>[:mode], [source] can be volume or host's path, <destination> is container's path, [mode] can be \"ro/rw/dr/rr/z/Z/nocopy/private/rprivate/slave/rslave/shared/rshared\"")
Copy link
Contributor

Choose a reason for hiding this comment

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

could we remove the comment line if we don't need it?

@fuweid fuweid merged commit 075f071 into AliyunContainerService:master Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/storage kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants