-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add support for cilium 1.9 #10695
Add support for cilium 1.9 #10695
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
602f4c8
to
886fbae
Compare
/milestone v1.20 |
NeedsRollingUpdate: "all", | ||
}) | ||
} | ||
} else if ver.Minor == 9 { |
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.
Previously there was a lone } else {
, was this acting as a default cilium version if the api field isn't set? If so, we should probably keep the } else {
here so that 1.9 is installed by default.
EDIT: I just noticed the pkg/model/components/cilium.go
change above which sets a default value so perhaps my comment doesn't apply.
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.
The code expects that the code running prior does the right thing. Added some guards against that not being the case now.
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.
Was just thinking about that error handling.
dial-timeout: | ||
retry-timeout: | ||
sort-buffer-len-max: | ||
sort-buffer-drain-timeout: |
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.
Could you remove the trailing spaces?
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.
Seems pointless to me to set empty keys like that. Removed them.
939dd11
to
44b34eb
Compare
Apply suggestions from code review Co-authored-by: Ciprian Hacman <ciprianhacman@gmail.com>
/test pull-kops-e2e-cni-cilium |
/lgtm |
No description provided.