Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

factory: use customised deep compare #845

Merged
merged 1 commit into from
Oct 30, 2018

Conversation

bergwolf
Copy link
Member

reflect.DeepEqual would return false when comparing nil map/slice with
empty map/slice. We would want to return success in such case, since it
is possible for upper layers to send these kind of configs.

Fixes: #844

reflect.DeepEqual would return false when comparing nil map/slice with
empty map/slice. We would want to return success in such case, since it
is possible for upper layers to send these kind of configs.

Fixes: kata-containers#844

Signed-off-by: Peng Tao <[email protected]>
@bergwolf
Copy link
Member Author

/test

@@ -100,6 +100,71 @@ func resetHypervisorConfig(config *vc.VMConfig) {
config.ProxyConfig = vc.ProxyConfig{}
}

func compareStruct(foo, bar reflect.Value) bool {
for i := 0; i < foo.NumField(); i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing check on field counts for foo and bar? They might compare as the same but if bar has extra fields that would be incorrect surely?

Copy link
Member Author

Choose a reason for hiding this comment

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

They have to pass foo.Type() == bar.Type() so they must be of the same struct type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - good point!

@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@4c82d52). Click here to learn what that means.
The diff coverage is 85%.

@@            Coverage Diff            @@
##             master     #845   +/-   ##
=========================================
  Coverage          ?   69.29%           
=========================================
  Files             ?       88           
  Lines             ?    13436           
  Branches          ?        0           
=========================================
  Hits              ?     9310           
  Misses            ?     3240           
  Partials          ?      886

Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

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

lgtm

@jodh-intel
Copy link
Contributor

jodh-intel commented Oct 19, 2018

lgtm

Approved with PullApprove

1 similar comment
@gnawux
Copy link
Member

gnawux commented Oct 19, 2018

lgtm

Approved with PullApprove

@amshinde
Copy link
Member

/retest

@WeiZhang555
Copy link
Member

WeiZhang555 commented Oct 29, 2018

LGTM

CI retriggered, let's see if there's any real issue.

Approved with PullApprove

@jodh-intel
Copy link
Contributor

Still seeing network timeouts.

/test

@bergwolf
Copy link
Member Author

CI green now.

@bergwolf bergwolf merged commit 26cef3c into kata-containers:master Oct 30, 2018
@bergwolf bergwolf deleted the deepcompare branch March 27, 2019 07:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants