Skip to content
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.

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

bugfix: check int type is empty #2235

Merged
merged 1 commit into from
Sep 17, 2018

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 rudyflyzhang@gmail.com

@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"

@@ -193,7 +198,6 @@ func (cfg *Config) MergeConfigurations(flagSet *pflag.FlagSet) error {
// 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 <rudyflyzhang@gmail.com>
@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

@@ -191,9 +196,7 @@ func (cfg *Config) MergeConfigurations(flagSet *pflag.FlagSet) error {
}

// 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