-
Notifications
You must be signed in to change notification settings - Fork 950
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: fix map type can not be merged #1367
bugfix: fix map type can not be merged #1367
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1367 +/- ##
==========================================
+ Coverage 17.29% 17.35% +0.06%
==========================================
Files 190 189 -1
Lines 11913 11836 -77
==========================================
- Hits 2060 2054 -6
+ Misses 9706 9634 -72
- Partials 147 148 +1
|
if !srcElem.IsValid() || isEmptyValue(srcElem) { | ||
continue | ||
} | ||
if dest.IsNil() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ace-Tang Add a test case to cover this statement?
like
{
src: &simple{Sd: map[string]string{"go": "gogo"}},
dest: &simple{},
expected: &simple{Sd: map[string]string{"go": "gogo"}},
ok: true,
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already add the test. @zhuangqh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cri-test alway fails to pull image.
add more merge map test Signed-off-by: Ace-Tang <aceapril@126.com>
Signed-off-by: Ace-Tang aceapril@126.com
Ⅰ. Describe what this PR did
fix map type in daemon config can not be merged.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews