search

weaveworks / pipeline-controller

This repository contains the Pipeline CRD and associated controller for Weave GitOps Enterprise.
1 stars 4 forks source link

Minimal level-triggered controller #200

Closed squaremo closed 1 year ago

squaremo commented 1 year ago

This does the preliminary work requested in #194, so that the implementation of level-triggered pipelines can make progress in several (or at least two) directions. After this is merged, we have a controller that checks the status of its targets, so the promotion algorithm can be implemented (#195).

This does just (about) enough to build the promotion algorithm on top, and thereby get to a usable controller. I made simplifications to get here reasonably quickly:

Removing those limitations is the work of #196.

I put the new implementation behind a feature flag, so we can work on it without having long-running branches. For many of the subsequent tasks in the umbrella issue #193 it will be enough to work in controllers/leveltriggered/ and write tests there. When we want to work on GUI or other things needing a running controller process, the flag --enable-level-triggered can be given to the binary to start the new controller code.

Fixes #194. With reference to this criterion: "objects that aren't mentioned in a pipeline don't get watched" -- this implementation limits support to a specific type, and the local cluster, and uses the convenience builder API in controller-runtime, so avoids creating informers and watchers and so on dynamically. The expanded implementation (#196) will need to pay careful attention to this, though.

squaremo commented 1 year ago

When/if we drop the original logic, make sure we move the controller under controlers.

Or even better if we move it now and rename it controllers.NewLevelTriggeringPipelineReconciler. It's a common code structure to store controllers under controllers and it bothers me to see the new one under a different module. controllers directory is plural.

We can move the code when the old one is fully deprecated, yep.

The forked controller went in its own package mainly because the tests rely on a TestMain which sets the controller running, and it is easier and cleaner if the two controller implementations have their own TestMain.

If the implementations being in controllers/ and a subdirectory of controllers/ is the objectionable bit, I could move the old one to e.g., controllers/legacy. It's pretty common to see subdirectories for different controllers that run in the same manager; this is a different situation but the pattern seems about right.