-
Notifications
You must be signed in to change notification settings - Fork 260
Fix for unparseable error returned by Azure CNI #212
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
Conversation
cni/network/network.go
Outdated
|
|
||
| // Output the result to stdout. | ||
| res.Print() | ||
| log.Printf("[cni-net] ADD command completed with result:%+v err:%v.", result, err) |
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.
We should still write to the log file what happened to the CNI ADD call
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.
ok
|
How soon can we get a release once this is merged? I'd like to run this through the conformance tests to see if it improves the pass rate. |
|
@sharmasushant - does the change look good? Launching tests this afternoon |
|
The fixes look good. Error handling is better. I added example logs to #195 with the fix |
|
@sharmasushant anything else needed for merge here? |
cni/network/network.go
Outdated
| plugin.Error(vererr) | ||
| } | ||
|
|
||
| if err == nil { |
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.
if err==nil && res!=nil
asking because if GetAsVersion fails, do we still want to print to stdout?
|
@daschott we will get this merged by tomorrow. |
|
ping - is this still in the works? |
|
@sharmasushant I addressed the review comment. Please approve it if everything is good. |
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #This PR fixes the unparseable error returned by Azure CNI (#195 ). Azure CNI should write result to stdout only in successful case
Special notes for your reviewer:
Release note: