Dispatcher updates subscriber statuses #4435
Conversation
Signed-off-by: Ville Aikas <vaikas@vmware.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vaikas 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 |
| Ready: corev1.ConditionTrue, | ||
| }) | ||
| } | ||
| patch, err := duck.CreateMergePatch(imc, after) |
There was a problem hiding this comment.
Will the merge patch correctly cause a removal of deleted subscribers from the conditions slice? (I think it depends on how the merge behaviour is configured in the CRD?)
There was a problem hiding this comment.
Of course I need to write tests for it and so forth, but yes it should because when we create the patch, we start from scratch and only add the current subscribers. So, we spin through the spec.subscribers, and if one is removed it won't get added to this list and hence it will be dropped. But "should" just work.
There was a problem hiding this comment.
I'm asking because I've seen weird behaviors with slices when the patchMergeKey isn't set. Old entries can remain in the existing list, even when removed from the "desired" list. That's also how you end up with 2 containers in a Deployment after apply-ing a manifest where containers[0].name has changed.
There was a problem hiding this comment.
Ah, yeah, we're not doing a strategic merge patch, just a merge patch, but I'll keep this in mind. I need to see if the strategic merge does something different, great point 👍
There was a problem hiding this comment.
lol @antoineco you were so right :) And I'm laughing because I now remember in the early days of duck typing we had to switch to json patch just for this reason :) I actually had to upstream a patch to get jsonpatch fakes working lol. geez... Switching to JSON patch :)
|
As I mentioned, I was expecting some UT to fail, but it's nice to see all the e2e tests passsing, here and in kind. 👍 |
|
I like the overrall direction of this patch, it should simplify patching kafkachannel too @devguyio |
Signed-off-by: Ville Aikas <vaikas@vmware.com>
Signed-off-by: Ville Aikas <vaikas@vmware.com>
| @@ -21,6 +21,10 @@ import ( | |||
| "net/http" | |||
| "testing" | |||
|
|
|||
| "k8s.io/apimachinery/pkg/types" | |||
|
|
|||
| eventingclient "knative.dev/eventing/pkg/client/injection/client" | |||
There was a problem hiding this comment.
Should this be?:
| eventingclient "knative.dev/eventing/pkg/client/injection/client" | |
| fakeeventingclient "knative.dev/eventing/pkg/client/injection/client/fake" |
Most ctors request fakes directly instead of using that indirect injection if I'm not mistaken. e.g.
etc.
Signed-off-by: Ville Aikas <vaikas@vmware.com>
Signed-off-by: Ville Aikas <vaikas@vmware.com>
|
Thanks @antoineco I think I addressed all your feedback, PTAL. |
Signed-off-by: Ville Aikas <vaikas@vmware.com>
Codecov Report
@@ Coverage Diff @@
## master #4435 +/- ##
==========================================
- Coverage 81.08% 81.06% -0.02%
==========================================
Files 281 281
Lines 7966 7981 +15
==========================================
+ Hits 6459 6470 +11
- Misses 1120 1122 +2
- Partials 387 389 +2
Continue to review full report at Codecov.
|
Signed-off-by: Ville Aikas <vaikas@vmware.com>
|
The following is the coverage report on the affected files.
|
| @@ -408,6 +495,7 @@ func TestReconciler_ReconcileKind(t *testing.T) { | |||
| }, | |||
| } | |||
| for n, tc := range testCases { | |||
| ctx, fakeEventingClient := fakeeventingclient.With(context.Background(), tc.imc) | |||
There was a problem hiding this comment.
Won't this already be injected in all tests by MakeFactory, due to the "magic" fakeeventingclient "knative.dev/eventing/pkg/client/injection/client/fake" import?
There was a problem hiding this comment.
Ahhh no, ReconcileKind is called manually in those tests.
/lgtm
There was a problem hiding this comment.
Yeah, I broke it into two tests, because what I really wanted to do was to see how the resources are handled (using the magic factory) and how the channels are configured by manually calling it. Our test infra could make this easier :)
|
not really sure what failed in the unit tests: |
|
/test pull-knative-eventing-unit-tests |
Signed-off-by: Ville Aikas vaikas@vmware.com
Fixes #
Proposed Changes
Release Note
Docs