Skip to content

Conversation

@rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented Sep 12, 2018

Ⅰ. Describe what this PR did

check int type is empty when do the merge.

For example, the default value of bridge's mtu is 1500, if the daemon config haven't configed bridge's mtu, the problem will set the mtu to 0, instead of default value 1500.

Ⅱ. Does this pull request fix one issue?

NONE

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

NONE

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang [email protected]

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels Sep 12, 2018
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Ace-Tang
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Sep 12, 2018
@HusterWan
Copy link
Contributor

test failed :

--- FAIL: TestMerge (0.00s)
	Error Trace:	utils_test.go:133
	Error:		Not equal: &utils.simple{Sa:0, Sb:"hello", Sc:false, Sd:map[string]string{"go":"gogo"}, Se:utils.nestS{Na:0}, Sf:[]string(nil)} (expected)
			        != &utils.simple{Sa:1, Sb:"hello", Sc:false, Sd:map[string]string{"go":"gogo"}, Se:utils.nestS{Na:22}, Sf:[]string(nil)} (actual)
			Diff:
			--- Expected
			+++ Actual
			@@ -1,3 +1,3 @@
			-(*utils.simple)(0xc420086780)({
			- Sa: (int) 0
			+(*utils.simple)(0xc420086730)({
			+ Sa: (int) 1
			  Sb: (string) (len=5) "hello"
			@@ -8,3 +8,3 @@
			  Se: (utils.nestS) {
			-  Na: (int) 0
			+  Na: (int) 22
			  }
	Error Trace:	utils_test.go:133
	Error:		Not equal: &utils.simple{Sa:0, Sb:"world", Sc:false, Sd:map[string]string{"go":"gogo"}, Se:utils.nestS{Na:11}, Sf:[]string{"foo", "foo"}} (expected)
			        != &utils.simple{Sa:2, Sb:"world", Sc:false, Sd:map[string]string{"go":"gogo"}, Se:utils.nestS{Na:11}, Sf:[]string{"foo", "foo"}} (actual)
			Diff:
			--- Expected
			+++ Actual
			@@ -1,3 +1,3 @@
			-(*utils.simple)(0xc420086960)({
			- Sa: (int) 0
			+(*utils.simple)(0xc420086910)({
			+ Sa: (int) 2
			  Sb: (string) (len=5) "world"


// merge configurations from command line flags and config file
err = mergeConfigurations(fileConfig, cfg.delValue(flagSet, fileFlags))
return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

directly return mergeConfigurations(fileConfig, cfg.delValue(flagSet, fileFlags))?

check int type is empty when do the merge.

Signed-off-by: Rudy Zhang <[email protected]>
@codecov-io
Copy link

Codecov Report

Merging #2235 into master will decrease coverage by 0.04%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2235      +/-   ##
==========================================
- Coverage   66.56%   66.51%   -0.05%     
==========================================
  Files         208      208              
  Lines       16756    16759       +3     
==========================================
- Hits        11153    11148       -5     
- Misses       4276     4283       +7     
- Partials     1327     1328       +1
Flag Coverage Δ
#criv1alpha1test 32.88% <40%> (-0.06%) ⬇️
#criv1alpha2test 33.41% <40%> (-0.06%) ⬇️
#integrationtest 39.9% <40%> (-0.02%) ⬇️
#nodee2etest 33.68% <40%> (-0.01%) ⬇️
#unittest 23.86% <40%> (ø) ⬆️
Impacted Files Coverage Δ
pkg/utils/utils.go 85.32% <100%> (+0.16%) ⬆️
daemon/config/config.go 47.94% <66.66%> (+2.11%) ⬆️
ctrd/watch.go 78.78% <0%> (-4.55%) ⬇️
daemon/mgr/snapshot.go 89.85% <0%> (-4.35%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
daemon/containerio/container_io.go 74.58% <0%> (-1.11%) ⬇️
ctrd/container.go 59.28% <0%> (-0.48%) ⬇️
daemon/mgr/container.go 57.39% <0%> (-0.41%) ⬇️
cri/v1alpha2/cri.go 72.52% <0%> (+0.63%) ⬆️
cri/v1alpha2/cri_wrapper.go 65.69% <0%> (+1.25%) ⬆️

@fuweid
Copy link
Contributor

fuweid commented Sep 14, 2018

For this change, LGTM. We should think about the default boolean value in next PR.

ping @allencloud @Ace-Tang

}

// merge configurations from command line flags and config file
err = mergeConfigurations(fileConfig, cfg.delValue(flagSet, fileFlags))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why split return mergeConfigurations into err = ... ; return err before?

Copy link
Contributor

Choose a reason for hiding this comment

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

because the caller will check the error, no need to check before return

@Ace-Tang
Copy link
Contributor

@fuweid , current merge method is not suit for pouch config now, like we disscussed offline before.

@fuweid
Copy link
Contributor

fuweid commented Sep 17, 2018

@Ace-Tang could we merge this PR?

@Ace-Tang Ace-Tang merged commit 2e06d4e into AliyunContainerService:master Sep 17, 2018
@rudyfly rudyfly deleted the config branch October 29, 2018 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants