feature: modify defaut bridge mode #1424
Conversation
rudyfly
changed the title
rudyfly
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1424 +/- ##
==========================================
+ Coverage 40.31% 40.34% +0.02%
==========================================
Files 251 251
Lines 16416 16466 +50
==========================================
+ Hits 6618 6643 +25
- Misses 8956 8967 +11
- Partials 842 856 +14
|
rudyfly
changed the title
| return nil | ||
| } | ||
|
|
||
| //MergeConfigurations merges flagSet flags and config file flags into Config. |
There was a problem hiding this comment.
Please update this comment as well.
There was a problem hiding this comment.
I think these comments is right 😄
| } | ||
|
|
||
| if !types.CompareIPNet(addrv4.IPNet, config.AddressIPv4) { | ||
| if os.Getenv("SetBridgeIP") == "true" && !types.CompareIPNet(addrv4.IPNet, config.AddressIPv4) { |
There was a problem hiding this comment.
This change should add something into the document, right?
For this kind of work, the pouchd's source code relies on some outter specific environment variables. If no guidance is here, it is quite hard to software user to notice the usage of env. WDYT?
How to solve the problem for user is what we need to take into consideration. @rudyfly
There was a problem hiding this comment.
I will change it in my next job that vendor libnetwork into alibaba group.
| // network config | ||
| flagSet.StringVar(&cfg.NetworkConfig.BridgeConfig.Name, "bridge-name", "", "Set default bridge name") | ||
| flagSet.StringVar(&cfg.NetworkConfig.BridgeConfig.IP, "bip", "", "Set bridge ip") | ||
| flagSet.StringVar(&cfg.NetworkConfig.BridgeConfig.GatewayIPv4, "default-gateway", "", "Set default bridge gateway") |
There was a problem hiding this comment.
Set IP address for default bridge gateway?
In addition, when we use IP in description or document, we must use IP with both letters uppercased. Please update the block of code.
MTU and CIDR as well.
There was a problem hiding this comment.
Set IP address for default bridge gateway?
yes, it is bridge config, so set bridge ip address
In addition, when we use IP in description or document, we must use IP with both letters uppercased. Please update the block of code.
MTU and CIDR as well.
OK, I will modify
| IP string `json:"bip"` | ||
| FixedCIDR string `json:"fixed-cidr"` | ||
| GatewayIPv4 string `json:"default-gateway"` | ||
| PreferredIP string `json:"preferred-ip"` |
There was a problem hiding this comment.
I think it is quite hard to understand this field without annotation.
Change the default nat bridge network from 172.17.0.1/24 to 192.168.5.1/24 Complete the bridge network configure. Don't to change bridge device ip address. Signed-off-by: Rudy Zhang <[email protected]>
|
LGTM |
pouchrobot
added
the
LGTM
5 participants
Ⅰ. Describe what this PR did
Change the default nat bridge network from 172.17.0.1/24 to 192.168.5.1/24
Complete the bridge network configure.
Don't to change bridge device ip address.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
Signed-off-by: Rudy Zhang [email protected]