-
Notifications
You must be signed in to change notification settings - Fork 284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed windows CNI setup in case cni none is configured #6820
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6820 +/- ##
=======================================
Coverage 25.11% 25.11%
=======================================
Files 33 33
Lines 2839 2839
=======================================
Hits 713 713
Misses 2079 2079
Partials 47 47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just below another block that is switching on CNIName
, adding another if
statement checking its value feels wrong.
Just move the p.KubeConfigKubeProxy = nodeConfig.AgentConfig.KubeConfigKubeProxy
line above restConfig, err := ...
, and add a return nil
to the CNINone
case.
I didn't want to change the code too much but logically you're right. Initially the Setup was inside the switch case for calico but when flannel was added it was moved outside without considering the possible fail with |
1abe6dd
to
0e693f8
Compare
Signed-off-by: Roberto Bonafiglia <[email protected]>
0e693f8
to
654130d
Compare
Proposed Changes
Check if the cni configured is none before doing the setup on windows to fix the panic error. (#6768)
Types of Changes
Verification
Testing
Linked Issues
#6819
User-Facing Change
Further Comments