use AutoHandlers instead of custom enablement logic #1585
Conversation
Codecov Report
@@ Coverage Diff @@
## next #1585 +/- ##
==========================================
- Coverage 62.12% 62.04% -0.08%
==========================================
Files 83 81 -2
Lines 7326 7272 -54
==========================================
- Hits 4551 4512 -39
+ Misses 2443 2431 -12
+ Partials 332 329 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
also changes the signature of AutoHandler to use client.Client because client.Reader was too narrow for the refactored implementation
shaneutt
force-pushed
the
refactor/cleanup-enablement
branch
from
7c3facd to
3871221
Compare
|
As per #1591 I rebased this onto the structural changes now in |
There was a problem hiding this comment.
Approving the content currently here per #1580 (comment). If we ultimately decide to use booleans, we can refactor that separately. We want the change to AutoHander implementation regardless.
As far as I understand the WIP status, there are only some further documentation changes and test changes pending, and the code is not expected to change further. Less interested in those/reasonably confident that the additions will be fine, so going ahead and approving this on the basis of the implementation changes. Please dismiss and re-request review if the subsequent changes are large enough that you want another pass.
One question though: this changes the config defaults such that everything is enabled or auto by default, but doesn't remove all controller flags from the tests. Is there a reason we're still leaving in some of the overrides? Most look now irrelevant, and the disabled kongconsumer controller looks outright wrong/left over from when it wasn't implemented.
Tests pass on next with all of the flags removed from the test, and will presumably pass with them all removed when we make whatever remaining tests changes are needed here.
Lastly, integration testing with some CRDs absent seems a bit excessive, since it'd require a completely different cluster environment. Can we mock the K8S API and resource availability in it? Unit tests for CRDExists and the Ingress version negotiator sound more reasonable.
* refactor(mgr) use booleans for controller flags Change flags for managing controllers from a custom enabled/auto/disabled type to booleans. By default, all controllers are enabled. If a controller's enable flag is set to false, it is disabled. Enabled controllers that have an auto loader are loaded if they are enabled and their auto loader indicates they should load. Remove the KongStateEnabled flag. This was a leftover from an earlier design where the controller managed Kong state information via a special Secret. It had no remaining associated code and was only present in configuration. Rename controller flags to reflect that they enable controllers. Remove enablement status type, associated utility functions, and custom flag handler. * fix(test) only defer body close if exists Move deferred body close checks after the error checkers. If an error is present, no body exists, and the deferred attempt to close it segfaults. Co-authored-by: Michał Flendrich <michal@flendrich.pro>
mflendrich
changed the title
part of #1580
This PR:
KongClusterPluginsupport under anautosetting (as opposed toenabled, thus providing explicit distinction between "enabled unconditionally, please fail when not working" and "enabled if the cluster supports it and silently disabled otherwise")auto(ditto)auto(ditto)auto(wasenabled)client.Clientinstead ofclient.Reader- a wider interface - because the existing implementation ofCRDExists()makes use of raw REST calls that are not possible withclient.Reader