-
Notifications
You must be signed in to change notification settings - Fork 138
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
Update K8s pins to 0.27.9 and controller-runtime to 0.15.3 / fix incompatibilities #3146
Update K8s pins to 0.27.9 and controller-runtime to 0.15.3 / fix incompatibilities #3146
Conversation
bb0c784
to
15ac5f2
Compare
37397e1
to
f6f674b
Compare
8acfaf7
to
737243f
Compare
K8s v1.27.10 is also released. Might be a good time to update to the latest v1.27 release. |
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.
Just a few thoughts but looks good.
pkg/controller/amazoncloudintegration/amazoncloudintegration_controller.go
Outdated
Show resolved
Hide resolved
test/client.go
Outdated
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package 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.
Would it be better to move this under the pkg/runtime
and keep it named fake or maybe go with fakeclient?
It would keep the context of being runtime related.
It could be added as pkg/runtime/client/fake
.
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.
I dunno, I like the line of thinking, I'm just not sure if it's better to have test utils packaged under a test directory or in line with a similar package.
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 controller-runtime folks nested the fake package at a sub-directory to the client package so if we've got our own controller runtime package then it makes sense to me to duplicate the structure.
If you don't want to move this to under our runtime/client directory I'd probably at least suggest a sub-directory/package so it doesn't some how get mixed up with anything else under 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.
OK I updated the location, definitely feels like better organization.
737243f
to
5aaef02
Compare
…mpatibilities This commit updates the pins to 0.27.9 (and controller-runtime to 0.15.3, where most of the incompabitilities lie) and fixes the incompatibilities with the new versions, specifically: - updating the manager creation, replacing the removed cache builder and mapper provider inputs with the replacements - Implementing a new Controller layer to cache the manager cache to deal with the new source.Kind requirement where you must provide a cache to the source.Kind function (previously a struct) when creatin a watch - Replace bits of deprecated functionality from the apiserver wait package. - Automatically register types that have statuses with the fake client using utility function (types with status now must be registered) - updated egress controller test to not wait for proper calico version, just expect the first reconcile to fail and the next one to succeed after setting the right version For the second bullet, I've introduced our own Controller interface which extends the controller.Controller interface so the individual controller / controller utilities don't need to deal with cache management. As outlined in some of the comments for that package, I intendd to use this layer to house future adaptations for cache management (and possibly even multi tenancy) to the controller runtime package.
a3088a0
to
308b45c
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.
LGTM
This commit updates the pins to 0.27.9 (and controller-runtime to 0.15.3, where most of the incompabitilities lie) and fixes the incompatibilities with the new versions, specifically:
For the second bullet, I've introduced our own Controller interface which extends the controller.Controller interface so the individual controller / controller utilities don't need to deal with cache management. As outlined in some of the comments for that package, I intendd to use this layer to house future adaptations for cache management (and possibly even multi tenancy) to the controller runtime package.
Description
For PR author
make gen-files
make gen-versions
For PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bug
if this is a bugfix.kind/enhancement
if this is a a new feature.enterprise
if this PR applies to Calico Enterprise only.