Validate nested reconcilers

[mamachanko]

• Nested validation is configured via the context with
  reconcilers#WithNestedValidation

• (Sub)Reconciler tests always do nested validation

• Moves the Validator interface from testing into reconcilers

• Sequence now implements the Validator interface. Its Validate is not
  called from its SetupWithManager to avoid duplicate validations.

fixes #583

Signed-off-by: Max Brauer [email protected]

[codecov]

Codecov Report

Attention: Patch coverage is 94.65649% with 7 lines in your changes missing coverage. Please review.

Project coverage is 59.39%. Comparing base (d0e059e) to head (37999af).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
testing/reconciler.go	0.00%	2 Missing ⚠️
testing/subreconciler.go	0.00%	2 Missing ⚠️
reconcilers/config.go	92.30%	1 Missing ⚠️
reconcilers/flow.go	98.00%	1 Missing ⚠️
reconcilers/sync.go	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #588      +/-   ##
==========================================
+ Coverage   58.26%   59.39%   +1.13%     
==========================================
  Files          33       34       +1     
  Lines        3800     3916     +116     
==========================================
+ Hits         2214     2326     +112     
- Misses       1492     1494       +2     
- Partials       94       96       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mamachanko]

@scothis I had experimented with validation handlers in the context. However, it did not convince me. It would only move the the call to t.Fatalf around in the (Sub)ReconcilerTests in the case of invalidity. I suggest we let the idea rest until we find more value. Unless, you reckon it's a good thing.

I had also found myself wanting to test (Sub)ReconcilerTests to assert they are indeed validating reconcilers and not panicking. However, since we call t.Fatalf it gets a little meta when assert against t.Failed. It's not impossible, but I let it be for now.

Lastly, AdmissionWebhookAdapater does not (yet) validate its reconciler (nested or not). That's for another PR I would say.

[scothis]

@scothis I had experimented with validation handlers in the context. However, it did not convince me. It would only move the the call to t.Fatalf around in the (Sub)ReconcilerTests in the case of invalidity. I suggest we let the idea rest until we find more value. Unless, you reckon it's a good thing.

Propagating the error and letting the caller handle it is the right call. You get better error message that hint towards the location of the error in the reconciler hierarchy as well.

I had also found myself wanting to test (Sub)ReconcilerTests to assert they are indeed validating reconcilers and not panicking. However, since we call t.Fatalf it gets a little meta when assert against t.Failed. It's not impossible, but I let it be for now.

That's not worth it. If you want you can add tests that are excluded by default, but I think it's fine to leave it so long as you have tried a couple known failure cases and there are no regressions to existing tests.

Lastly, AdmissionWebhookAdapater does not (yet) validate its reconciler (nested or not). That's for another PR I would say.

It should be fairly trivial, right? I know AdmissionWebhookAdapter doesn't have a Validate method today, but we can just check and call on the Reconciler field directly from the test case.

[mamachanko]

Lastly, AdmissionWebhookAdapater does not (yet) validate its reconciler (nested or not). That's for another PR I would say.

It should be fairly trivial, right? I know AdmissionWebhookAdapter doesn't have a Validate method today, but we can just check and call on the Reconciler field directly from the test case.

@scothis 💯

I will tackle that in a follow-up PR.

[scothis]

Looks good.

We should have each exported Validate method call init() before the validation is checked. This will provide a default reconciler name, and let the reconciler be more like what it will run as. I should have added this when exporting the method initially.

[scothis]

Use a stasher

[mamachanko]

@scothis Great idea! However, I've moved validation into its own validation package. To break a cyclic dependency on reconcilers, I would have to move stash stuff into, say, stash. That's ok I think. But before I plow on, I'd like you to acknowledge.

[scothis]

Sounds good, so long as we alias back the types to not break anyone.

[mamachanko]

@scothis After starting the work I realised it's not possible without breaking anyone, b/c generic types cannot be easily aliased (yet). Recorded #591 instead.

[mamachanko]

We should have each exported Validate method call init() before the validation is checked. This will provide a default reconciler name, and let the reconciler be more like what it will run as. I should have added this when exporting the method initially.

@scothis Every Validate is now also calling init to provide better error messages with default names. Some reconcilers haven't had an init yet. Now they do. Aggregate and Advice are defaulting fields in their init. By calling init in their Validate some of their invalid cases are removed. That's a change at test time, but runtime behaviour remains the same.