Skip to content

Conversation

@allencloud
Copy link
Collaborator

@allencloud allencloud commented Mar 13, 2019

Signed-off-by: Allen Sun [email protected]

Ⅰ. Describe what this PR did

env management of container may have some incompatibilities there. Here is the case:

For create env:

  1. --env a=b: add env a with value b in container
  2. --env a=: add env a with empty value in container
  3. --env a: remove env a in container's envs which are inherited from image;
  4. --env a="b c": add env a with value "b c" in container, which has a whitespace there.

What I did:

  1. modify the cli instruction;
  2. modify the swagger.yml to be more clear;
  3. add test case

Ⅱ. 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

none

Ⅴ. Special notes for reviews

@wangforthinker @zjumoon01 @rudyfly

@codecov
Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #2745 into master will increase coverage by 0.19%.
The diff coverage is 91.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2745      +/-   ##
==========================================
+ Coverage   69.32%   69.52%   +0.19%     
==========================================
  Files         278      278              
  Lines       17448    17740     +292     
==========================================
+ Hits        12096    12333     +237     
- Misses       4025     4068      +43     
- Partials     1327     1339      +12
Flag Coverage Δ
#criv1alpha2_test 39.37% <68.88%> (-0.1%) ⬇️
#integration_test_0 36.67% <82.22%> (+0.08%) ⬆️
#integration_test_1 35.52% <68.88%> (-0.01%) ⬇️
#integration_test_2 36.55% <68.88%> (-0.01%) ⬇️
#integration_test_3 35.4% <77.77%> (-0.01%) ⬇️
#node_e2e_test 35.43% <68.88%> (+0.13%) ⬆️
#unittest 28.4% <55.55%> (-0.03%) ⬇️
Impacted Files Coverage Δ
daemon/mgr/container_utils.go 82.38% <88.23%> (ø) ⬆️
apis/opts/env.go 87.09% <92.85%> (+7.09%) ⬆️
pkg/streams/utils.go 82.14% <0%> (-9.53%) ⬇️
ctrd/image_commit.go 61.34% <0%> (-6.68%) ⬇️
daemon/mgr/image_load.go 45.45% <0%> (-4.55%) ⬇️
ctrd/utils.go 78.43% <0%> (-3.93%) ⬇️
daemon/mgr/image_utils.go 84.7% <0%> (-0.75%) ⬇️
cri/v1alpha2/cri.go 72.14% <0%> (+0.89%) ⬆️
ctrd/image.go 71.03% <0%> (+1.77%) ⬆️
daemon/containerio/cio.go 70.83% <0%> (+2.15%) ⬆️
... and 4 more

@allencloud
Copy link
Collaborator Author

I think this PR is going to fail, I am just testing to see whether the current implementation meets the demand.

@allencloud allencloud force-pushed the support-env branch 2 times, most recently from 2c0847d to 2b050d4 Compare March 13, 2019 15:18
@pouchrobot pouchrobot added size/XL and removed size/L labels Mar 13, 2019
@allencloud allencloud force-pushed the support-env branch 3 times, most recently from a6b86f6 to e87b14e Compare March 13, 2019 16:58
@wangforthinker
Copy link
Contributor

LGTM

@zjumoon01
Copy link
Contributor

Caution function updateContainerEnv() which writes env to /etc/profile.d/pouchenv.sh in container.
It splits env by "=", and use "parts[1]" without checking split result's length. I'm not sure whether that can cause panic if len(parts)=1 .

@allencloud
Copy link
Collaborator Author

@zjumoon01

image

@zjumoon01
Copy link
Contributor

@zjumoon01

image

all right

@allencloud
Copy link
Collaborator Author

Thanks for your review. @wangforthinker @zjumoon01
Merging...

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.

5 participants