Skip to content

feature: support dns related flags when creating container through pouch cli #2380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 5, 2018
Merged

feature: support dns related flags when creating container through pouch cli #2380

merged 1 commit into from
Nov 5, 2018
+84 −3

Conversation

@mathspanda
Copy link
Contributor

@mathspanda mathspanda commented Oct 29, 2018

Signed-off-by: mathspanda [email protected]

Ⅰ. Describe what this PR did

Support dns related flags for pouch cli: dns, dns-option and dns-search.

Ⅱ. Does this pull request fix one issue?

Fix #2356

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

None

Ⅳ. Describe how to verify it

$ pouch run --dns 1.2.3.4 --dns-search example.com --dns-option timeout:3 busybox cat /etc/resolv.conf
search example.com
nameserver 1.2.3.4
options timeout:3

Ⅴ. Special notes for reviews

None

@codecov
Copy link

codecov bot commented Oct 29, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2380      +/-   ##
==========================================
- Coverage   68.45%   68.44%   -0.02%     
==========================================
  Files         275      275              
  Lines       18291    18245      -46     
==========================================
- Hits        12522    12488      -34     
+ Misses       4340     4335       -5     
+ Partials     1429     1422       -7
Flag Coverage Δ
#criv1alpha1test 31.84% <ø> (+0.03%) ⬆️
#criv1alpha2test 35.85% <ø> (+0.02%) ⬆️
#integrationtest 39.88% <ø> (+0.12%) ⬆️
#nodee2etest 33.13% <ø> (-0.15%) ⬇️
#unittest 25.53% <ø> (+0.01%) ⬆️
Impacted Files Coverage Δ
daemon/mgr/spec_process.go 67.01% <0%> (-12.99%) ⬇️
cri/ocicni/cni_manager.go 59.18% <0%> (-12.25%) ⬇️
daemon/containerio/cri_log_file.go 84.31% <0%> (-3.93%) ⬇️
ctrd/image.go 76.75% <0%> (-2.2%) ⬇️
pkg/meta/store.go 64.06% <0%> (-2.1%) ⬇️
daemon/mgr/container_utils.go 84.04% <0%> (-1.4%) ⬇️
ctrd/container.go 58.8% <0%> (-0.96%) ⬇️
pkg/meta/boltdb.go 68.67% <0%> (-0.74%) ⬇️
daemon/mgr/container_validation.go 44.37% <0%> (-0.63%) ⬇️
cri/v1alpha2/cri.go 67.81% <0%> (-0.48%) ⬇️
... and 20 more

ZYecho
ZYecho reviewed Oct 30, 2018
View reviewed changes
test/cli_run_dns_test.go Outdated
inspectRes := command.PouchRun("inspect", cname)
res.Assert(c, icmd.Success)

result := []types.ContainerJSON{}
Copy link
Contributor

@ZYecho ZYecho Oct 30, 2018

Choose a reason for hiding this comment

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

I add a util func named inspectFilter in pr #2372 , use it to instead of unmarshal?

Copy link
Collaborator

@allencloud allencloud Oct 31, 2018

Choose a reason for hiding this comment

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

Yeah, please rebase this pr to the latest master. And do as @ZYecho said.

Copy link
Contributor Author

@mathspanda mathspanda Oct 31, 2018

Choose a reason for hiding this comment

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

Rebased. And fixed as ZYecho said. @allencloud

test/cli_run_dns_test.go Outdated

// test if the value is in inspect result
inspectRes := command.PouchRun("inspect", cname)
res.Assert(c, icmd.Success)
Copy link
Contributor

@ZYecho ZYecho Oct 30, 2018

Choose a reason for hiding this comment

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

inspectRes.Assert(c, icmd.Success)?

test/cli_run_dns_test.go
busyboxImage,
"cat", "/etc/resolv.conf")
defer DelContainerForceMultyTime(c, cname)
res.Assert(c, icmd.Success)
Copy link
Contributor

@ZYecho ZYecho Oct 30, 2018

Choose a reason for hiding this comment

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

It's a good chioce to put command.PouchRun and Assert into one line? Since the res var didn't use after this.

Copy link
Contributor

@ZYecho ZYecho Oct 30, 2018

Choose a reason for hiding this comment

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

I saw the behavior was divergent in the cli test.

Copy link
Contributor

@ZYecho ZYecho Oct 30, 2018

Choose a reason for hiding this comment

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

@ZYecho Fixed.

LGTM, but waiting for the CI pass.

test/cli_run_dns_test.go Outdated
res.Assert(c, icmd.Success)

result := []types.ContainerJSON{}
if err := json.Unmarshal([]byte(inspectRes.Stdout()), &result); err != nil {
Copy link
Contributor

@ZYecho ZYecho Oct 30, 2018

Choose a reason for hiding this comment

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

same with above.

@allencloud
Copy link
Collaborator

allencloud commented Oct 30, 2018

Could you squash the two commits into a single one? @mathspanda
And I am afraid that the commit message 123 is not preferred since it means less.
How about changing that?

@mathspanda
Copy link
Contributor Author

mathspanda commented Oct 30, 2018 •
edited
Loading

Sorry. "123" is just a test commit. I will squash them. @allencloud

@mathspanda
Copy link
Contributor Author

mathspanda commented Oct 30, 2018 •
edited
Loading

@ZYecho Fixed.

@mathspanda mathspanda force-pushed the cli-dns branch 2 times, most recently from 30e9dfe to ec0f322 Compare October 31, 2018 07:09
@allencloud
Copy link
Collaborator

allencloud commented Oct 31, 2018

Thanks a lot for your careful review. @ZYecho
To double check, I would like to invite @Ace-Tang to take another review for this. Thanks.

HusterWan
HusterWan reviewed Nov 1, 2018
View reviewed changes
test/cli_run_dns_test.go
func (suite *PouchRunDNSSuite) TestRunWithDNSFlags(c *check.C) {
cname := "TestRunWithDNSFlags"

res := command.PouchRun("run", "--name", cname,
Copy link
Contributor

@HusterWan HusterWan Nov 1, 2018

Choose a reason for hiding this comment

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

since res not used, how about command.PouchRun(xx).(c, icmd.Success)

Copy link
Contributor Author

@mathspanda mathspanda Nov 1, 2018

Choose a reason for hiding this comment

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

Nope. res is used in line 84.

Copy link
Contributor

@HusterWan HusterWan Nov 1, 2018

Choose a reason for hiding this comment

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

OK, got it , my mistake

@allencloud
Copy link
Collaborator

allencloud commented Nov 5, 2018

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 5, 2018
@allencloud allencloud merged commit 1932630 into AliyunContainerService:master Nov 5, 2018
@pouchrobot pouchrobot mentioned this pull request Nov 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@allencloud allencloud allencloud left review comments

+2 more reviewers

@ZYecho ZYecho ZYecho left review comments

@HusterWan HusterWan HusterWan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Ace-Tang Ace-Tang

Labels

kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[feature request] support dns options when creating container through pouch cli

6 participants

@mathspanda @allencloud @ZYecho @HusterWan @Ace-Tang @pouchrobot