feature: cli supports --env-file #2870

lang710 · 2019-05-27T05:59:23Z

Signed-off-by: Lang Chi 21860405@zju.edu.cn

Ⅰ. Describe what this PR did

pouch command supports --env-file

Ⅱ. Does this pull request fix one issue?

fixes #2857

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

added

Ⅳ. Describe how to verify it

cat /home/abc

env_var_name=another_value

pouch run --env-file=/home/abc -d busybox top

b2f0df7176e9459441276c7470ad95ae701329b49ff2709344706316597ce639

pouch exec b2f0df7176e9459441276c7470ad95ae701329b49ff2709344706316597ce639 env | grep env_var_name

env_var_name=another_value

Ⅴ. Special notes for reviews

codecov · 2019-05-27T05:59:25Z

Codecov Report

Merging #2870 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2870      +/-   ##
==========================================
- Coverage   68.87%   68.85%   -0.02%     
==========================================
  Files         291      291              
  Lines       18249    18249              
==========================================
- Hits        12569    12566       -3     
- Misses       4223     4227       +4     
+ Partials     1457     1456       -1

Flag	Coverage Δ
#criv1alpha2_test	`38.36% <ø> (+0.02%)`	⬆️
#integration_test_0	`36.2% <ø> (+0.1%)`	⬆️
#integration_test_1	`35.69% <ø> (+0.02%)`	⬆️
#integration_test_2	`36.42% <ø> (+0.38%)`	⬆️
#integration_test_3	`35.51% <ø> (-0.15%)`	⬇️
#node_e2e_test	`34.16% <ø> (-0.05%)`	⬇️
#unittest	`28.03% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
apis/server/utils.go	`71.15% <0%> (-3.85%)`	⬇️
cri/ocicni/netns.go	`58.1% <0%> (-2.71%)`	⬇️
ctrd/supervisord/daemon.go	`49.32% <0%> (-1.36%)`	⬇️
ctrd/container.go	`54.3% <0%> (ø)`	⬆️
ctrd/watch.go	`78.37% <0%> (+5.4%)`	⬆️

pouchrobot · 2019-05-27T05:59:27Z

We found this is your first time to contribute to pouch, @lang710
👏 We really appreciate it.
Just remind that you have read the contribution guide: https://github.com/alibaba/pouch/blob/master/CONTRIBUTING.md
If you didn't, you should do that first. If done, welcome again and please enjoy hacking! 🍻

CodeJuan · 2019-05-27T07:20:44Z

@lang710 Would you please squash the commits into a single one and set a readable commit message .

CodeJuan · 2019-05-27T07:25:45Z

Test cases are high priority, would you please add unit test and integration test?

lang710 · 2019-05-27T07:27:25Z

@lang710 Would you please add unit test and integration test?

I have written the test, but not sure if it's right.

cli/envfile.go

yeya24 · 2019-05-27T14:20:41Z

cli/envfile.go

+	return lines, scanner.Err()
+}
+
+var whiteSpaces = " \t"


Is it better to make this variable a constant?

I think so and thank you for you advise!

CodeJuan · 2019-05-28T09:47:12Z

@lang710 CI failed, please take a look at build log.

FAIL: cli_create_test.go:364: PouchCreateSuite.TestCreateWithEnvfile
cli_create_test.go:385:
    c.Fatalf("container env in inspect result should have %s in %s while no\n", env1, envs)
... Error: container env in inspect result should have TEST1=value1 in [PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin TEST2=value2] while no

fuweid · 2019-05-29T02:34:24Z

cli/envfile.go

+			}
+
+			if len(data) > 1 {
+


duplicate empty line

fuweid · 2019-05-29T02:39:48Z

cli/envfile.go

+			data := strings.SplitN(line, "=", 2)
+
+			// trim the front of a variable, but nothing else
+			variable := strings.TrimLeft(data[0], whiteSpaces)


do we actually need this line? I think the strings.TrimLeft(scanner.Text(), whiteSpaces) is good enough.

fuweid · 2019-05-29T02:43:41Z

cli/envfile.go

+	scanner := bufio.NewScanner(fh)
+	for scanner.Scan() {
+		// trim the line from all leading whitespace first
+		line := strings.TrimLeft(scanner.Text(), whiteSpaces)


how about unicode.IsSpace?

fuweid · 2019-05-29T02:46:00Z

cli/envfile_test.go

+
+// Test ParseEnvFile for a file with a few well formatted lines
+func TestParseEnvFileGoodFile(t *testing.T) {
+	content := `foo=bar


I am not sure that the behaviour in Chinese 😂
could you please add some test for this?

fuweid · 2019-05-29T02:47:24Z

test/cli_run_test.go

+	c.Assert(strings.TrimSpace(ret.Stdout()), check.Equals, "value1-")
+
+	// Test three
+	validfile = "fixtures/valid.env"


I think we can use tmp file to contain the content. How do you think?

test/util/util.go

fuweid · 2019-06-04T05:38:35Z

cli/envfile_test.go

+with.dots=working
+and_underscore=working too
+地址=杭州
+	ADDRESS=杭州


I think the ADDRESS=杭州 is valid env.

root@ubuntu-xenial /h/vagrant# cat /tmp/1 1 地址=杭州 ADDRESS=杭州 root@ubuntu-xenial /h/vagrant# docker run -d --env-file /tmp/1 busybox:1.25 top edd1c870b3be3727d87ffe320c1af218f67a2f148f4989a4039f81ea397fc3d8 root@ubuntu-xenial /h/vagrant# docker exec edd1c870b3be3727d87ffe320c1af218f67a2f148f4989a4039f81ea397fc3d8 env PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin HOSTNAME=edd1c870b3be 地址=杭州 ADDRESS=杭州 HOME=/root

Yes, there is no problem with testing Chinese characters.

fuweid · 2019-06-04T05:40:04Z

test/cli_create_test.go

+	res = command.PouchRun("start", name)
+	res.Assert(c, icmd.Success)
+
+	ret := command.PouchRun("exec", name, "env")


missing ret.Assert(c, icmd.Success)

I think it's no need to add this instruction.

yes. But it can help us to know the command status. If there is output wrong, we need to know the command is correct or not. what do you think?

fuweid · 2019-06-04T06:14:24Z

apis/server/container_bridge.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"github.com/docker/docker/pkg/stdcopy"


please put this package into next third group

Signed-off-by: Lang Chi <21860405@zju.edu.cn>

fuweid

LGTM

pouchrobot added kind/feature size/L labels May 27, 2019

lang710 force-pushed the fixenv branch from 80bac0d to a42a398 Compare May 27, 2019 06:08

lang710 force-pushed the fixenv branch from b6d7d51 to c5a6e8b Compare May 27, 2019 07:47

CodeJuan reviewed May 27, 2019

View reviewed changes

cli/envfile.go Outdated Show resolved Hide resolved

lang710 force-pushed the fixenv branch from c5a6e8b to a563749 Compare May 27, 2019 08:04

pouchrobot added size/XL and removed size/L labels May 27, 2019

yeya24 reviewed May 27, 2019

View reviewed changes

lang710 force-pushed the fixenv branch from a563749 to 8d61eac Compare May 28, 2019 04:01

pouchrobot added size/M and removed size/XL labels May 28, 2019

lang710 force-pushed the fixenv branch from 8d61eac to e62c14b Compare May 28, 2019 04:08

pouchrobot added size/S and removed size/M labels May 28, 2019

lang710 force-pushed the fixenv branch from e62c14b to a09fdef Compare May 28, 2019 04:50

pouchrobot added size/XL and removed size/S labels May 28, 2019

lang710 force-pushed the fixenv branch 3 times, most recently from 7644b4c to dabfce5 Compare May 28, 2019 08:09

lang710 force-pushed the fixenv branch 3 times, most recently from e6b4612 to 3d160f9 Compare May 28, 2019 12:47

lang710 force-pushed the fixenv branch 3 times, most recently from 22e5ccb to a387cd1 Compare May 29, 2019 01:40

fuweid reviewed May 29, 2019

View reviewed changes

lang710 force-pushed the fixenv branch 5 times, most recently from 0491081 to 764b8b6 Compare May 29, 2019 11:47

CodeJuan reviewed May 29, 2019

View reviewed changes

test/util/util.go Outdated Show resolved Hide resolved

lang710 force-pushed the fixenv branch 5 times, most recently from 66f0861 to 8b3d26b Compare May 29, 2019 13:54

weekly-digest bot mentioned this pull request Jun 2, 2019

Weekly Digest (26 May, 2019 - 2 June, 2019) #2884

Closed

fuweid reviewed Jun 4, 2019

View reviewed changes

lang710 force-pushed the fixenv branch from 8b3d26b to d7b6f78 Compare June 4, 2019 06:10

fuweid reviewed Jun 4, 2019

View reviewed changes

lang710 force-pushed the fixenv branch 4 times, most recently from 7d98da9 to a825538 Compare June 4, 2019 08:07

feature: cli supports --env-file

d23578d

Signed-off-by: Lang Chi <21860405@zju.edu.cn>

lang710 force-pushed the fixenv branch from a825538 to d23578d Compare June 4, 2019 08:49

fuweid reviewed Jun 4, 2019

View reviewed changes

fuweid merged commit 7094e7e into AliyunContainerService:master Jun 4, 2019

pouchrobot mentioned this pull request Jun 7, 2019

WeeklyReport of PouchContainer from 2019-05-31 to 2019-06-07 #2896

Closed

weekly-digest bot mentioned this pull request Jun 9, 2019

Weekly Digest (2 June, 2019 - 9 June, 2019) #2898

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: cli supports --env-file #2870

feature: cli supports --env-file #2870

lang710 commented May 27, 2019

codecov bot commented May 27, 2019 •

edited

Loading

pouchrobot commented May 27, 2019

CodeJuan commented May 27, 2019 •

edited

Loading

CodeJuan commented May 27, 2019

lang710 commented May 27, 2019

yeya24 May 27, 2019

lang710 May 28, 2019

CodeJuan commented May 28, 2019

fuweid May 29, 2019

fuweid May 29, 2019

fuweid May 29, 2019

fuweid May 29, 2019

fuweid May 29, 2019

fuweid Jun 4, 2019

lang710 Jun 4, 2019

fuweid Jun 4, 2019

lang710 Jun 4, 2019

fuweid Jun 4, 2019

lang710 Jun 4, 2019

fuweid Jun 4, 2019

fuweid left a comment

feature: cli supports --env-file #2870

feature: cli supports --env-file #2870

Conversation

lang710 commented May 27, 2019

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

cat /home/abc

pouch run --env-file=/home/abc -d busybox top

pouch exec b2f0df7176e9459441276c7470ad95ae701329b49ff2709344706316597ce639 env | grep env_var_name

Ⅴ. Special notes for reviews

codecov bot commented May 27, 2019 • edited Loading

Codecov Report

pouchrobot commented May 27, 2019

CodeJuan commented May 27, 2019 • edited Loading

CodeJuan commented May 27, 2019

lang710 commented May 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeJuan commented May 28, 2019

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

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

fuweid left a comment

Choose a reason for hiding this comment

codecov bot commented May 27, 2019 •

edited

Loading

CodeJuan commented May 27, 2019 •

edited

Loading