tektoncd / pipeline

A cloud-native Pipeline resource.
https://tekton.dev
Apache License 2.0
8.45k stars 1.77k forks source link

Feature: Name based Remote Resolution #7508

Closed wlynch closed 6 months ago

wlynch commented 9 months ago

Feature request

Previously you used to be able to reference OCI bundles via a simple bundle field:

taskRef:
  bundle: docker.io/myrepo/mycatalog@sha256:abc123

but this has been marked as deprecated. 😢 the new way is to use remote resolution, which looks much more verbose:

taskRef:
    resolver: bundles
    params:
    - name: bundle
      value: docker.io/myrepo/mycatalog@sha256:abc123

I liked the simplicity of the field based approach - I want to see if we can bring a version of this back (at least for simple cases), and fall back to params for everything else. 🙏

Use case

name is already a part of the remote resolver interface yet it is unused in the actual implementation.

I think we can use this to provide a short hand for remote resolution params for well-known types, e.g.:

taskRef:
  name: bundle://docker.io/myrepo/mycatalog@sha256:abc123

Using git as another example:

taskRef:
  name: git://github.com/wlynch/test@main#task.yaml?token=secret-name

which would resolve to:

taskRef:
    resolver: git
    params:
    - name: url
      value: github.com/wlynch/test
    - name: revision
      value: main
    - name: pathInRepo
      value: task.yaml
    - name: token
      value: secret-name

And you could still mix-and-match params + name for anything that doesn't conform / isn't easily represented in a name-based format.

taskRef:
  name: git://github.com/wlynch/test@main#task.yaml
  params:
    - name: token
      value: secret-name

This should be backwards compatible - :// can't be in a valid k8s object name name, so there's a clear opt-in for this behavior. This also won't effect existing remote resolution specs, since name is currently unused, which also means arbitrary resolvers can continue to be used.

cc @chitrangpatel

vdemeester commented 9 months ago

Previously you used to be able to reference OCI bundles via a simple bundle field:

taskRef:
  bundle: docker.io/myrepo/mycatalog@sha256:abc123

but this has been marked as deprecated. 😢 the new way is to use remote resolution, which looks much more verbose:

taskRef:
    resolver: bundles
    params:
    - name: bundle
      value: docker.io/myrepo/mycatalog@sha256:abc123

This is a missing some bits.

# Before
taskRef:
  name: task-or-pipeline-to-get-from-bundle # to know which "resource" to get from the bundle
  bundle: docker.io/myrepo/mycatalog@sha256:abc123
# After
taskRef:
  resolver: bundles
  params:
  - name: bundle
    value: docker.io/myrepo/mycatalog@sha256:abc123
  - name: kind # probably optional, "can be inferred"
    value: task
  - name: name 
    value: task-or-pipeline-to-get-from-bundle

I liked the simplicity of the field based approach - I want to see if we can bring a version of this back (at least for simple cases), and fall back to params for everything else. 🙏

I think you liked because it was more concise, not really more simple. The current syntax is verbose, but definitely not complex. In that regard, git://github.com/wlynch/test@main#task.yaml?token=secret-name or bundle://docker.io/myrepo/mycatalog@sha256:abc123#task:name-of-the-task is probably slightly more complex (to read or to write for the user, and to parse probably).

If we dig a bit more:

What if we had a slightly different syntax (I think that was even in the inital TEP, we just moved away from it to stay consistent with how params is in other part of the API) ? (kind of what we want to do for v2 already)

# Before
taskRef:
  name: task-or-pipeline-to-get-from-bundle # to know which "resource" to get from the bundle
  bundle: docker.io/myrepo/mycatalog@sha256:abc123
# After
taskRef:
  resolver: bundles
  params:
  - bundle: docker.io/myrepo/mycatalog@sha256:abc123
  - kind: task # probably optional, "can be inferred"
  - name: task-or-pipeline-to-get-from-bundle

I tend to like the idea of conciseness, but I would want to make sure we are not making it more confusing, and that we can treat all resolvers the same.

chmouel commented 9 months ago

I like @wlynch ideas, I think we could have both and not everything in the "concised" form need to address every key/value in the verbose one....

I understand @vdemeester arguments that may be bring some confusion but it's only if we wanted to address every resolvers and every parameters in the concise form? Maybe if we are restricting it to the most common and simple use case and leave the one who bring confusion to the more verbose form?

(as anecdote we are keeping the annotations for PAC remote tasks resolution for the simple use cases since it's easy to express and decided that for the advanced use cases the users should use tekton resolvers)

chitrangpatel commented 9 months ago

I agree with @wlynch here that we should use the name field to make it less verbose (also agree with @vdemeester that less verbose does not mean simple) for simple cases. For advanced use cases, they still have the params that they can reference.

One catch though is that the resolvers may need to interpret the name since the name format could be unique to that resolver. May be the pipeline controller only needs to understand which resolver to send the request to.

(Also, tangential so let's not discuss this in this thread, may be in the doc about what you think about upfront remote resolution. Feel free to comment in the doc.)

chitrangpatel commented 7 months ago

Hello 👋

I wanted to come back to this. Have we considered using PURL Spec for this? I think this could satisfy multiple use cases. When others write their custom resolvers, they could also parse this spec. It provides a generic schema that could work for a variety of resolvers instead of each one specifying their schema.

Today we have: git, hub, bundle and http resolvers which would fit nicely with PURL I think.

PURL also has support to parse the uri in multiple languages: https://github.com/package-url/purl-spec/tree/master so it should be possible to parse the string without much trouble as long as it satisfies the purl-spec.

chitrangpatel commented 7 months ago

Thoughts @wlynch @chmouel @vdemeester ?

I would like to drive this forward.

chitrangpatel commented 6 months ago

Superseded by TEP https://github.com/tektoncd/community/pull/1138