Add new CNI Spec and Type to Installation #699
Conversation
pkg/render/config.go
Outdated
| } | ||
|
|
||
| // If CalicoNetwork is specified, then use Calico networking. | ||
| if install.Spec.CalicoNetwork == nil { |
There was a problem hiding this comment.
I'm confused, the comment says If CalicoNetwork is specified, but then the function checks if it's nil...
There was a problem hiding this comment.
I'm not sure what I was thinking here. I'm removing that if and the associated code block.
There was a problem hiding this comment.
I found why that was needed. Need to skip the rest of the processing if no CalicoNetwork exists.
There was a problem hiding this comment.
Ok. One other concern I have about this function:
Based on the single switch case, it looks like we intend on this function growing as we add more CNI plugins. And it seems a bit fragile that developers need to know whether to add that logic before or after this nil check / return depending on the situation. The comment you've added does help, but worth considering if we can make this less fragile. Not sure what I'd suggest to accomplish that, so I'm fine merging as is and we can re-evaluate as those new cases appear.
There was a problem hiding this comment.
I could switch the check to if install.Spec.CalicoNetwork != nil and then move all the remainder of the code into the code block.
| PluginAzureVNET CNIPluginType = "AzureVNET" | ||
| ) | ||
|
|
||
| var CNIPluginTypes []CNIPluginType = []CNIPluginType{ |
There was a problem hiding this comment.
I'm not sure if it is. But perhaps I should use it to validate the PluginType.
tmjd
force-pushed
the
api-new-cni-options
branch
from
3ac61cc
to
bc26ea4
Compare
Description
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.