-
Notifications
You must be signed in to change notification settings - Fork 106
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
feat: Upgrade controller-runtime to v0.19.0 #760
feat: Upgrade controller-runtime to v0.19.0 #760
Conversation
@@ -181,7 +181,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco | |||
if err := r.client.Delete(ctx, pcu); resource.IgnoreNotFound(err) != nil { | |||
log.Debug(errDeletePCU, "error", err) | |||
r.record.Event(pc, event.Warning(reasonAccount, errors.Wrap(err, errDeletePCU))) | |||
return reconcile.Result{RequeueAfter: shortWait}, nil //nolint:nilerr // Returning err would make us requeue instantly. |
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 like golangci-lint still wanted this
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.
Fixed it. Seems there was a mismatch between the CI and my local linter.
Adjust code to apply to breaking changes in controller-runtime. There should be no breaking changes for programs using crossplane-runtime. Signed-off-by: Maximilian Blatt <[email protected]>
123124e
to
3b73e8e
Compare
addProviderConfig(evt.Object, q) | ||
} | ||
|
||
// Generic adds a NamespacedName for the supplied GenericEvent if its Object is | ||
// a ProviderConfigReferencer. | ||
func (e *EnqueueRequestForProviderConfig) Generic(_ context.Context, evt event.GenericEvent, q workqueue.RateLimitingInterface) { |
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.
🤔 Aren't these breaking changes? I guess it's fine though
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.
overall looks good, not super opinionated on the type aliases, leaving the last call to @negz 🙏
// BucketRateLimiter for a standard crossplane reconciler. | ||
type BucketRateLimiter = workqueue.TypedBucketRateLimiter[string] | ||
|
||
// RateLimiter for a standard crossplane reconciler. | ||
type RateLimiter = workqueue.TypedRateLimiter[string] |
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.
Are these type aliases necessary, or just for readability?
I think I like them - just curious.
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.
Sorry for the delay - I've been out sick.
Description of your changes
This upgrades
sigs.k8s.io/controller-runtime
tov0.19.0
. Since this library is prone to breaking changes some adjustments are made to apply to some changed interfaces. There should be no breaking changes for programs using only crossplane-runtime.I have:
earthly +reviewable
to ensure this PR is ready for review.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.y
labels to auto-backport this PR.Need help with this checklist? See the cheat sheet.