feature: add envs for pouch exec #2250

knightXun · 2018-09-18T09:40:48Z

Ⅰ. Describe what this PR did

Provide new Environments for pouch exec

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

codecov · 2018-09-18T09:40:49Z

Codecov Report

❗ No coverage uploaded for pull request head (envs@8cb32d2). Click here to learn what that means.
The diff coverage is n/a.

CLAassistant · 2018-09-18T09:40:54Z

All committers have signed the CLA.

allencloud · 2018-09-18T09:48:20Z

daemon/mgr/container_exec.go

 	}

 	mgr.ExecProcesses.Put(execid, execConfig)

 	return execid, nil
 }

+// MergeEnvs merge oldEnvs with new Env
+func MergeEnvs(oldEnv, newEnv []string) []string {


I am wondering if that we have already supported this function, in https://github.com/alibaba/pouch/blob/master/daemon/mgr/container_utils.go#L262
I think this is a common function.

allencloud · 2018-09-18T09:49:16Z

apis/swagger.yml

@@ -3193,6 +3193,11 @@ definitions:
        minItems: 1
        items:
          type: "string"
+      Env:
+        type: "array"
+        description: "Env for commands"


envs for exec command in container
I think this is more specific.

allencloud · 2018-09-18T09:50:23Z

apis/types/registry_service_config.go

@@ -41,7 +41,7 @@ type RegistryServiceConfig struct {
 	// > are in compliance with any terms that cover redistributing
 	// > nondistributable artifacts.
 	//
-	AllowNondistributableArtifactsCIDRs []string `json:"AllowNondistributableArtifactsCIDRs,omitempty"`


Please revert this file when you git commit. We do not need this change. And the change done by swagger-0.12.0 is because swagger has a bug in itself.

allencloud · 2018-09-18T09:50:46Z

cli/exec.go

@@ -51,6 +52,7 @@ func (e *ExecCommand) addFlags() {
 	flagSet.BoolVarP(&e.Terminal, "tty", "t", false, "Allocate a tty device")
 	flagSet.BoolVarP(&e.Interactive, "interactive", "i", false, "Open container's STDIN")
 	flagSet.StringVarP(&e.User, "user", "u", "", "Username or UID (format: <name|uid>[:<group|gid>])")
+	flagSet.StringArrayVarP(&e.Envs,"env","e", []string{}, "Set environment variables")


Set environment variables for exec command in container

allencloud · 2018-09-18T09:51:22Z

daemon/mgr/container_exec.go

 	"github.com/pkg/errors"
+	"strings"


please package strings would be in the first part, not in the third-part one.

allencloud · 2018-09-18T09:52:20Z

daemon/mgr/container_exec.go

@@ -27,18 +29,48 @@ func (mgr *ContainerManager) CreateExec(ctx context.Context, name string, config
 		return "", fmt.Errorf("container %s is not running", c.ID)
 	}

+	Envs := MergeEnvs(c.Config.Env, config.Env)
+	logrus.Info("Envs", Envs)


This line of log is Info, and I think it is too casual. It will add more info log in production. How about changing this into Debugf?

allencloud · 2018-09-18T09:53:09Z

Please also the corresponding cli integration test for this pull request. Thanks a lot. @knightXun

knightXun · 2018-09-18T10:14:55Z

@allencloud I will add tests for this.

pouchrobot · 2018-09-18T10:47:38Z

@knightXun Thanks for your contribution. 🍻
Please sign off in each of your commits.

knightXun · 2018-09-19T01:13:08Z

@pouchrobot /test

allencloud · 2018-09-19T01:48:41Z

LGTM, and would you like to sign the cla again? Since Alibaba org has updated something, then everyone of us needs to sign it again. @knightXun

codecov · 2018-09-19T01:54:47Z

Codecov Report

Merging #2250 into master will increase coverage by 0.04%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #2250      +/-   ##
==========================================
+ Coverage    66.7%   66.74%   +0.04%     
==========================================
  Files         208      208              
  Lines       16919    16923       +4     
==========================================
+ Hits        11286    11296      +10     
- Misses       4268     4272       +4     
+ Partials     1365     1355      -10

Flag	Coverage Δ
#criv1alpha1test	`32.53% <60%> (+0.12%)`	⬆️
#criv1alpha2test	`36.15% <60%> (+0.18%)`	⬆️
#integrationtest	`39.33% <60%> (-0.18%)`	⬇️
#nodee2etest	`33.51% <60%> (+0.12%)`	⬆️
#unittest	`23.76% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
daemon/mgr/container_types.go	`83.63% <ø> (ø)`	⬆️
daemon/mgr/container_exec.go	`67.28% <60%> (-0.68%)`	⬇️
daemon/mgr/system.go	`70.49% <0%> (-7.38%)`	⬇️
daemon/mgr/container_utils.go	`81.92% <0%> (-2.41%)`	⬇️
daemon/containerio/container_io.go	`74.58% <0%> (-1.11%)`	⬇️
pkg/meta/store.go	`63.28% <0%> (-0.79%)`	⬇️
ctrd/container.go	`59.76% <0%> (-0.48%)`	⬇️
daemon/mgr/container.go	`57.59% <0%> (+0.4%)`	⬆️
cri/v1alpha2/cri.go	`67.61% <0%> (+1.19%)`	⬆️
ctrd/image.go	`78.94% <0%> (+2.19%)`	⬆️
... and 2 more

codecov · 2018-09-19T02:17:17Z

Codecov Report

Merging #2250 into master will increase coverage by 0.04%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #2250      +/-   ##
==========================================
+ Coverage    66.7%   66.74%   +0.04%     
==========================================
  Files         208      208              
  Lines       16919    16923       +4     
==========================================
+ Hits        11286    11296      +10     
- Misses       4268     4272       +4     
+ Partials     1365     1355      -10

Flag	Coverage Δ
#criv1alpha1test	`32.53% <60%> (+0.12%)`	⬆️
#criv1alpha2test	`36.15% <60%> (+0.18%)`	⬆️
#integrationtest	`39.33% <60%> (-0.18%)`	⬇️
#nodee2etest	`33.51% <60%> (+0.12%)`	⬆️
#unittest	`23.76% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
daemon/mgr/container_types.go	`83.63% <ø> (ø)`	⬆️
daemon/mgr/container_exec.go	`67.28% <60%> (-0.68%)`	⬇️
daemon/mgr/system.go	`70.49% <0%> (-7.38%)`	⬇️
daemon/mgr/container_utils.go	`81.92% <0%> (-2.41%)`	⬇️
daemon/containerio/container_io.go	`74.58% <0%> (-1.11%)`	⬇️
pkg/meta/store.go	`63.28% <0%> (-0.79%)`	⬇️
ctrd/container.go	`59.76% <0%> (-0.48%)`	⬇️
daemon/mgr/container.go	`57.59% <0%> (+0.4%)`	⬆️
cri/v1alpha2/cri.go	`67.61% <0%> (+1.19%)`	⬆️
ctrd/image.go	`78.94% <0%> (+2.19%)`	⬆️
... and 2 more

fuweid · 2018-09-25T09:59:03Z

daemon/mgr/container_exec.go

@@ -27,11 +27,18 @@ func (mgr *ContainerManager) CreateExec(ctx context.Context, name string, config
 		return "", fmt.Errorf("container %s is not running", c.ID)
 	}

+	envs, err := mergeEnvSlice(c.Config.Env, config.Env)


first argu of mergeEnvSlice should config.Env

ping @knightXun cc @HusterWan

I will correct it tonight

pouchrobot added the size/M label Sep 18, 2018

allencloud reviewed Sep 18, 2018

View reviewed changes

allencloud changed the title ~~add envs for pouch exec~~ featureadd envs for pouch exec Sep 18, 2018

pouchrobot added the kind/feature label Sep 18, 2018

allencloud changed the title ~~featureadd envs for pouch exec~~ feature: add envs for pouch exec Sep 18, 2018

knightXun force-pushed the envs branch from 01b54e8 to 62933eb Compare September 18, 2018 09:56

add envs for pouch exec

8389383

knightXun force-pushed the envs branch from 62933eb to 8389383 Compare September 18, 2018 10:10

pouchrobot added size/S and removed size/M labels Sep 18, 2018

test for pouch exec with envs

8cb32d2

pouchrobot added size/M and removed size/S labels Sep 18, 2018

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Sep 19, 2018

allencloud merged commit c2553d6 into AliyunContainerService:master Sep 19, 2018

pouchrobot mentioned this pull request Sep 22, 2018

Weekly Report of PouchContainer from 2018-09-15 to 2018-09-22 #2272

Closed

fuweid reviewed Sep 25, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add envs for pouch exec #2250

feature: add envs for pouch exec #2250

knightXun commented Sep 18, 2018 •

edited

Loading

codecov bot commented Sep 18, 2018 •

edited

Loading

CLAassistant commented Sep 18, 2018 •

edited

Loading

allencloud Sep 18, 2018

allencloud Sep 18, 2018

allencloud Sep 18, 2018

allencloud Sep 18, 2018

allencloud Sep 18, 2018

allencloud Sep 18, 2018

allencloud commented Sep 18, 2018

knightXun commented Sep 18, 2018

pouchrobot commented Sep 18, 2018

knightXun commented Sep 19, 2018 •

edited

Loading

allencloud commented Sep 19, 2018

codecov bot commented Sep 19, 2018

codecov bot commented Sep 19, 2018

fuweid Sep 25, 2018

knightXun Sep 25, 2018

feature: add envs for pouch exec #2250

feature: add envs for pouch exec #2250

Conversation

knightXun commented Sep 18, 2018 • edited Loading

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

codecov bot commented Sep 18, 2018 • edited Loading

Codecov Report

CLAassistant commented Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Sep 18, 2018

knightXun commented Sep 18, 2018

pouchrobot commented Sep 18, 2018

knightXun commented Sep 19, 2018 • edited Loading

allencloud commented Sep 19, 2018

codecov bot commented Sep 19, 2018

Codecov Report

codecov bot commented Sep 19, 2018

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knightXun commented Sep 18, 2018 •

edited

Loading

codecov bot commented Sep 18, 2018 •

edited

Loading

CLAassistant commented Sep 18, 2018 •

edited

Loading

knightXun commented Sep 19, 2018 •

edited

Loading