RFC: Add template field for runnable

[waciumawanjohi]

Changes proposed by this PR

RFC to change Runnable
Readable

Release Note

PR Checklist

Note: Please do not remove items. Mark items as done [x] or use strikethrough if you believe they are not relevant

• [ ] Linked to a relevant issue. Eg: Fixes #123 or Updates #123
• Removed non-atomic or wip commits
• [ ] Filled in the Release Note section above
• [ ] Modified the docs to match changes

[netlify]

✅ Deploy Preview for elated-stonebraker-105904 ready!

Name	Link
🔨 Latest commit	6661f26
🔍 Latest deploy log	https://app.netlify.com/sites/elated-stonebraker-105904/deploys/624b0f435e041600082ea884
😎 Deploy Preview	https://deploy-preview-787--elated-stonebraker-105904.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[cirocosta]

what about runnable.spec.selector? even if there's no templating to be done for what used to be the inputs field, I imagine there'd still be the need of templating properties of the selected object? or is part of the proposal to get rid of it?

[scothis]

template is a very common field/term in k8s when a significant portion of one resource is inlined into another resource. For example, Deployments have .spec.template which is a PodTemplateSpec, which has its own nested .spec.template which is a PodSpec.

[cirocosta]

from what I recall, there were two big reasons for having a ClusterRunTemplate:

1. making Runnable useful on its own
2. preventing conflicting templating w/ carto's Cluster*Template
3. preventing conflicting templating w/ the interpolation syntax of the children

1. runnable useful on its own

the idea there was that one could find Runnable useful outside a cartographer supply chain so that, as long as they have a way of updating runnable.spec.inputs with the inputs they care about, Runnable would do the work of creating new objects according to that "driver" that the clusterruntemplate provides (with enough "drivers", maybe you'd never need to write a ClusterRunTemplate - you'd pick one from the catalog and go with it).

looking at the questions we receive, I don't believe this has been the case - Runnable is not as promoted as the main CRDs and I don't think there are many ways of using it by itself out there outside cartographer anyway, so I don't think this is much relevant

2. preventing conflicting templating w/ carto's Cluster*Template

a big reason for having a clusterruntemplate was to prevent those embedding a Runnable inside something like Cluster(Source|Image|Config|_)Template of having multiple levels of templating leveraging the same interpolation syntax. for instance:

kind: ClusterImageTemplate
spec:
  template:    #       $()$ interpolation
    kind: Runnable
    spec:
      template:       # which interpolation? $$()$$?

by breaking into a separate object, we were able to then keep that single level of templating

kind: ClusterImageTemplate
spec:
  template:    #       $()$ interpolation
    kind: Runnable
    spec:
      runTemplateRef: <>   

with this RFC, if we keep runnable.template w/ the ability of templating something out, then we'll be facing this problem

3. preventing conflicting templating w/ the interpolation syntax of the children

another point of "multiple templating" being involved is in regards to the definition of the object itself - while tekton provides us primitives where we can separate the execution of a pipeline (PipelineRun) from the Pipeline itself (via pipelinerun.spec.pipelineRef), other targets we were aiming at the time (like Job) don't, forcing you to provide some spec that makes use of their native interpolation. this particular case is not solved by ClusterRunTemplate as there we could possibly still have a collision anyway

to illustrate that, consider a ClusterImageTemplate using a slightly modified version of the Runnable proposed here but making use of pipelinerun.spec.pipelineSpec (to avoid having to have a Pipeline object in the cluster, putting us closer to the example of a Job):

apiVersion: carto.run/v1alpha1
kind: ClusterImageTemplate
metadata:
  name: testing-pipeline
spec:
  imagePath: status.outputs.image

  template:
    apiVersion: carto.run/v1alpha1
    kind: Runnable
    metadata:
      name: $(workload.metadata.name)$
    spec:
      outputs:
        image: status.results[?(@.name=="image")].value
      template:
        apiVersion: tekton.dev/v1beta1
        kind: PipelineRun
        metadata:
          generateName:  $(workload.metadata.name)$-
        spec:
          params:
            - name: tag
              value: $(params.tag)$
          pipelineSpec:
            params:
              - name: tag
            tasks:
              - name: build-image
                taskRef: {name: kaniko}
                params:
                - name: tag
                  value: $(params.tag)                         #! tekton's $() syntax
            results:
              - name: image
                value: $(tasks.build-image.results.image)      #! tekton's $() syntax

here we can see how the $() syntax from tekton is veeeerrrryyy close to the $()$ used by Cartographer, which can be quite confusing. again, this is not solved with ClusterRunTemplate, and I don't think this proposal would solve either (not that it's its desire I believe)

[cirocosta]

left some context in #787 (comment), but overall I do like the idea of trying to simplify Runnable with some form of inlining. For me the biggest issue I see at the moment is

• if we indeed want to inline what clusterruntemplate provides, what do we do about runnnable.spec.selector? I believe that requires some form of interpolation from Runnable itself

[sclevine]

if we indeed want to inline what clusterruntemplate provides, what do we do about runnnable.spec.selector? I believe that requires some form of interpolation from Runnable itself

Maybe selector actually belongs in ClusterSupplyChain and not in Runnable? If we combine ClusterSupplyChain and ClusterDelivery into one resource that requires an explicit descriptor (e.g., Workload or Deliverable), maybe selector is a generalization of descriptor matching logic (e.g., maybe a Pipeline is a "non-primary descriptor")?

I think the changes in #787 and #705 might be better candidates for inclusion in the larger work around #766, rather than iterative changes.

[scothis]

template is a very common field/term in k8s when a significant portion of one resource is inlined into another resource. For example, Deployments have .spec.template which is a PodTemplateSpec, which has its own nested .spec.template which is a PodSpec.

[scothis]

Is this different than today's behavior for Runnable?

[scothis]

Preserving backwards compatibility is important. It's fine to deprecate the old behavior if the eventual goal is to remove it. Having a window where the old and new models both work gives users time to migrate.

[scothis]

Bumping the version isn't required for additive changes. If we did update the version, the old and new fields would need to have a home on both the v1alpha1 and v1alpha2 versions, in order for the CRD conversion support to function.

[cirocosta]

marking it as blocked as we'd like to tackle #815 first in order to be able to get rid of the templating at clusterruntemplate level w/ regards to selector