bugfix: fix can't set macaddress for endpoint #2222
Conversation
When user set the mac address in container's config: `container.Config.MacAddress`, now it will not take effect. The method is setting the mac address into the endpoint's options, it will set into network device by libnetwork or it's remote plugins. Add testcase: `TestCreateWithMacAddress` to check the mac address is set into container's device or not. Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
| if err != nil { | ||
| return nil, err | ||
| } | ||
| genericOption, _ = utils.MergeMap(genericOption, options.Generic{netlabel.MacAddress: mac}) |
There was a problem hiding this comment.
we already have Merge funtion in utils.go, can we use it ?
There was a problem hiding this comment.
It can't to merge the map as I want, I don't know and I don't want to knon what's wrong with it. 🐶
Codecov Report
@@ Coverage Diff @@
## master #2222 +/- ##
=========================================
+ Coverage 64.83% 64.9% +0.06%
=========================================
Files 208 208
Lines 16728 16744 +16
=========================================
+ Hits 10846 10868 +22
+ Misses 4526 4525 -1
+ Partials 1356 1351 -5
|
|
LGTM |
pouchrobot
added
the
LGTM
| // MergeMap merges the m2 into m1, if it has the same keys, m2 will overwrite m1. | ||
| func MergeMap(m1 map[string]interface{}, m2 map[string]interface{}) (map[string]interface{}, error) { | ||
| if m1 == nil && m2 == nil { | ||
| return nil, fmt.Errorf("all of maps are nil") |
There was a problem hiding this comment.
Why does it return an error? I think it is quite normal to merge two nil map.
I think it is incorrect to return err. Instead, the upper caller could judge whether the returned value is nil to return error or something.
MergeMap is a general function and fundamental, it should not add some business logic in the implementation. For me, judging both of them nil is business logic.
|
|
||
| c.Assert(found, check.Equals, true) | ||
|
|
||
| DelContainerForceMultyTime(c, cname) |
There was a problem hiding this comment.
We should use defer to delete container to avoid legacy container for other tests.
Ⅰ. Describe what this PR did
When user set the mac address in container's config:
container.Config.MacAddress, now it will not take effect.The method is setting the mac address into the endpoint's options, it
will set into network device by libnetwork or it's remote plugins.
Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
Add testcase:
TestCreateWithMacAddressto check the mac address is setinto container's device or not.
Ⅳ. Describe how to verify it
Using testcase
TestCreateWithMacAddressto verify itⅤ. Special notes for reviews
NONE
Signed-off-by: Rudy Zhang rudyflyzhang@gmail.com