`finalizer` ergonomics

[LukeMathWalker]

Given finalizer's current signature, the user is forced to use a closure to pass the ctx object coming from the top-level reconciliation routine (the first argument of Controller::run) into the function that is going to handle finalizer::Event - see:

kube_runtime::finalizer(
      &api_client,
      FINALIZER_NAME,
      object,
      // We must use a closure otherwise we cannot pass `ctx` down to our `apply` and `cleanup`
      // routines.
      |event| async move {
          match event {
              kube_runtime::finalizer::Event::Apply(c) => creation::apply(c, ctx).await,
              kube_runtime::finalizer::Event::Cleanup(c) => deletion::cleanup(c, ctx).await,
          }
      },
  )
  .await

It feels like modifying the signature of the closure of the last argument of finalizer to be impl FnMut(Event<K>, Context<T>) -> ReconcilerFut instead of impl FnOnce(Event<K>) -> ReconcileFut would improve the API ergonomics. This requires, as a side-effect, to take T as an additional generic parameter for finalizer.

[nightkr]

To be honest, it kind of feels like we're just "infecting" more and more aspects of the library with having to thread through Context, while requiring the user to pay for it even if they have no use for it.

Personally, I'd rather just burn Context completely, and make closures The Way to pass state into the reconciler (as it is for the rest of the language).

[LukeMathWalker]

while requiring the user to pay for it even if they have no use for it.

I appreciate where you are coming from, but I find it difficult to believe you can write anything non-trivial without any piece of shared state - at the very least a Kubernetes client. I feel almost all users will end up having the need for a shared state container. This can explicit (as it is now) or implicit (closures or task locals), but the need remains. Explicit feels nicer because it nudges the user into a well-trodden path.

[clux]

I'd rather just burn Context completely, and make closures The Way to pass state into the reconciler

The Context is the only way to lift these closures away though, and without it we are stuck with these deeply nested structures that's incredibly hard for new users to grok without understanding everything. I'd much rather see people have a standardised reconciler fn as a unit when scanning code, rather than have to ingest a whole pyramid.

Personally, I don't think having a single simple struct is that hard to thread that it's worthy of concern.

[nightkr]

I appreciate where you are coming from, but I find it difficult to believe you can write anything non-trivial without any piece of shared state - at the very least a Kubernetes client.

Absolutely agreed.

The Context is the only way to lift these closures away though, and without it we are stuck with these deeply nested structures that's incredibly hard for new users to grok without understanding everything. I'd much rather see people have a standardised reconciler fn as a unit when scanning code, rather than have to ingest a whole pyramid.

Not really, the question ends up being between:

struct MyCtx {
    http: reqwest::Client,
    config: Config,
}

async fn reconciler(obj: ..., ctx: Arc<MyCtx>) {
    ctx.http.get(&ctx.config.base_url).await.unwrap().text().unwrap();
}

async fn main() {
    let ctx = Arc::new(MyCtx {...});
    Controller::new(...).run(|obj| reconciler(obj, ctx.clone())).await
}

Versus

struct MyCtx {
    http: hyper::Client,
    config: Config,
}

async fn reconciler(obj: ..., ctx: Context<MyCtx>) {
    ctx.get_ref().http.get(&ctx.get_ref().config.base_url).await.unwrap().text().unwrap();
}

async fn main() {
    Controller::new(...).run(reconciler, Context::new(MyCtx {...})).await
}

I wouldn't call the lambda in the first variant significantly more complex, and it also makes it clear that the data is ref-counted (since Arc is a pretty common Rust type that works the same everywhere).

It's also slightly more convenient in the reconciler itself, since Arc implements Deref, so that MyCtx fields are usable directly (although there isn't really anything preventing us from implementing that for Context either, to be fair).

[clux]

Oh, right. For some reason I forgot you could just factor out closures :dumpling: That's definitely pretty similar in the base case :thinking:

The secrets_syncer example writes them all inline though, and if we need "3 standardised functions", the maths probably changes a bit on what's nicest.