-
Notifications
You must be signed in to change notification settings - Fork 13
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
Controller: Set CNI args on the delegate request #156
Conversation
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.
Looks good, but can you add a unit test ?
c8ef869
to
98e3d20
Compare
I just added a unit test |
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.
You need to address the linting issues.
Code wise, looks good. I can provide some nits if you're interested, but I would go for a merge once this passes CI.
Thanks for the contribution.
a969a17
to
0603293
Compare
pkg/controller/pod_test.go
Outdated
ifaceStatus(namespace, networkName, "net0", ""), | ||
ifaceStatus(namespace, networkToAdd, "net1", macAddr))) |
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.
given the linter's complaints, I would just define a const within the ifaceStatus func, and rename it to ifaceStatusForDefaultNamespace
.
... not the most elegant solution but it would unlock your PR.
I would refactor the tests afterwards to address that.
The CNI args has to be set in the delegate request so the cni-args in the network attachment annotation are considered. Signed-off-by: Lionel Jouin <[email protected]>
0603293
to
4aee468
Compare
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.
Thank you for this contribution.
Thank you for the quick review and help! |
Do you need a new release with this patch ? |
Not really, after #158 it would be nice yes, I might also update dependencies and the golang version if it's ok with you. |
Let's leave that out for now, since we have dependabot doing it for us. ... I've just not done a good job lately at paying attention to it. |
What this PR does / why we need it:
The CNI args has to be set in the delegate request so the cni-args in the network attachment annotation are considered.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer (optional):