-
Notifications
You must be signed in to change notification settings - Fork 730
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
Upgrade to controller-runtime v0.7.0 #1073
Conversation
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #1073 +/- ##
==========================================
+ Coverage 48.08% 48.12% +0.04%
==========================================
Files 62 62
Lines 4274 4264 -10
==========================================
- Hits 2055 2052 -3
+ Misses 1964 1959 -5
+ Partials 255 253 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
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 generally LGTM.
There is the outstanding question about the retry delay and I defer to Oren for debugging the informer cache changes. Unless he has already weighed in through pair programming or similar?
Thanks @sozercan, @maxsmythe! I'm reviewing this PR today. |
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.
@sozercan thanks for taking on this huge upgrade!
I'm posting review comments with as far as I've gotten so you can see them in the meantime. My next review task is just the rebasing of the dynamicCache.
pkg/controller/constrainttemplatestatus/constrainttemplatestatus_controller.go
Outdated
Show resolved
Hide resolved
third_party/sigs.k8s.io/controller-runtime/pkg/dynamiccache/internal/deleg_map.go
Show resolved
Hide resolved
third_party/sigs.k8s.io/controller-runtime/pkg/dynamiccache/internal/informers_map.go
Outdated
Show resolved
Hide resolved
nit: looks like the comment in |
@sozercan done reviewing - there are a few small blockers to address but overall you did a fantastic job on this! |
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
pkg/controller/constrainttemplate/constrainttemplate_controller_test.go
Outdated
Show resolved
Hide resolved
third_party/sigs.k8s.io/controller-runtime/pkg/dynamiccache/cache.go
Outdated
Show resolved
Hide resolved
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 🚀
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
third_party/sigs.k8s.io/controller-runtime/pkg/dynamiccache/cache_suite_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan sozercan@gmail.com
What this PR does / why we need it:
Upgrades controller-runtime to v0.7.0 - https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.7.0
Required PRs:
open-policy-agent/cert-controller#18
open-policy-agent/frameworks#106
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #
Special notes for your reviewer: