removed lock for version command #929
Conversation
| // acquire cni lock file | ||
| config := &common.PluginConfig{} | ||
| acquireLockForStore(config, plugin) | ||
| defer releaseLockForStore(plugin) |
There was a problem hiding this comment.
nope not needed, if this fails no need to unlock
| common.LogNetworkInterfaces() | ||
|
|
||
| // Initialize network manager. | ||
| err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) |
There was a problem hiding this comment.
Why did we move this? #Resolved
There was a problem hiding this comment.
we restore state inside this call which we have to do after lock
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| var err error | ||
| if err = plugin.Store.Lock(true); err != nil { |
There was a problem hiding this comment.
following this err is hard. it might be better to make this a one-line declaration/initialization (if err := ...; err != nil) and in the nested ifs below, explicitly return err or nil early instead of sometimes setting err = nil and returning err at the end of the function.
There was a problem hiding this comment.
i wanted to avoid multiple returns but i agree it affects readability. Let me redesign this a bit
There was a problem hiding this comment.
multiple returns are okay, in go it's encouraged to return early when you can. returning early on errors lets us keep the code linear and not need to nest it as deeply.
cni/network/network.go
Outdated
| // restore network state | ||
| err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) | ||
| if err != nil { | ||
| log.Printf("[cni-net] Failed to initialize network manager, err:%v.", err) |
There was a problem hiding this comment.
for readability i would like to see space in err:%v. -> err: %v., but that applies to the whole codebase so i won't block for that
This reverts commit 6079bf3.
3 participants
Reason for Change:
Issue Fixed:
Requirements:
Notes: