[WIP] RFC: applying controller sdk to reconciler for Subscriptions.#525
[WIP] RFC: applying controller sdk to reconciler for Subscriptions.#525n3wscott wants to merge 13 commits into
Conversation
knative-prow-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: n3wscott If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knative-prow-robot
added
the
size/XXL
|
The following is the coverage report on pkg/.
|
neosab
left a comment
neosab
left a comment
There was a problem hiding this comment.
This is great. But if we could extend KnativeReconciler and add SDK reconcilers for Source and Channel it would go further in reducing the boilerplate. Not sure if this was indeed the intention.
| logger.Warnf("Failed to reconcile %s: %v", r.provider.Parent.GetObjectKind(), err) | ||
| } | ||
|
|
||
| if chg, err := r.statusHasChanged(ctx, original, obj); err != nil || !chg { |
There was a problem hiding this comment.
My understanding is that this would eventually check if the finalizer has been updated as well.
There was a problem hiding this comment.
yeah there are some parts missing... like finalizers
|
@n3wscott do we still want this in this repo or can it be moved to eventing-sources? |
|
Closing this one since it contains a bunch of Source stuff. @n3wscott you can create a new PR with just the Subscription controller changes if you want. |
5 participants
Note that this has #513 in the PR so it is a bit noisy. But take a look at what applying the start of the reconciler sdk did to the Subscription Controller.
Proposed Changes
Release Note