Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Pipeline Templating #64

Closed
bobcatfish opened this issue Sep 21, 2018 · 11 comments
Closed

Implement Pipeline Templating #64

bobcatfish opened this issue Sep 21, 2018 · 11 comments
Assignees
Labels
meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given

Comments

@bobcatfish
Copy link
Collaborator

bobcatfish commented Sep 21, 2018

Expected Behavior

When a user creates a Pipeline, they should be able to use templating to pass arguments to tasks, using the values that will eventually (via PipelineRun) be provided by the referenced PipelineParams and Resources.

This should include:

  • Unit test coverage
  • Docs on how to use this templating

Actual Behavior

Templating is being designed in #36 but is not implemented.

Steps to Reproduce the Problem

  1. Deploy the Pipeline CRD + related CRDs to your cluster (note: should be docs on how to do this post kubebuilder)
  2. Create a Task like this:
apiVersion: pipeline.knative.dev/v1beta1
kind: Task
metadata:
  name: template-params-task
  namespace: default
spec:
    inputs:
        params:
           - name: stuffToEcho
             value: string
    buildSpec:
        steps:
            - name: helloworld
              image: busybox
              args: ['echo', '${stuffToEcho}']
  1. Create Resources and PipelineParams like this (we're going to use the clusters):
apiVersion: pipeline.knative.dev/v1beta1
kind: Resource
metadata:
  name: template-resources
  namespace: default
spec:
  resources:
  - name: github-resource
    type: git
    params:
    - name: url
      value: https://github.com/grafeas/kritis
    - name: revision
      value: 123456789123456789
  - name: image-resource
    type: image
    params:
    - name: url
      value: gcr.io/somethingsomethingsomething
apiVersion: pipeline.knative.dev/v1beta1
kind: PipelineParams
metadata:
  name: template-params
  namespace: default
spec:
    clusters:
        - name: 'testCluster'
          type: 'gke'
          endpoint: 'https://somethingsomethingsomethingsomething.com'
  1. Create a Pipeline like this that uses templating to pass values from Resources and PipelineParams:
apiVersion: pipeline.knative.dev/v1beta1
kind: Pipeline
metadata:
  name: template-pipeline
  namespace: default
spec:
    tasks:
        - name: template-params-task-image
          taskRef:
              name: template-params-task
          params:
              - name: stuffToEcho
                value: ${resources.image-resource.url}
        - name: template-params-task-github
          taskRef:
              name: template-params-task
          params:
              - name: stuffToEcho
                value: ${resources.github-resource.revision}
        - name: template-params-task-cluster
          taskRef:
              name: template-params-task
          params:
              - name: stuffToEcho
                value: ${pipelineParams.testCluster.endpoint}
  1. Create a PipelineRun like this:
apiVersion: pipeline.knative.dev/v1beta1
kind: PipelineRun
metadata:
  name: template-param-run-1
  namespace: default
spec:
    pipelineRef:
        name: template-pipeline
    resourceRef:
        name: template-resources
    pipelineParamsRef:
        name: template-params
    triggerRef:
        type: manual
  1. Creating the PipelineRun should have created TaskRuns with all of the values filled in, so with kubectl get taskRuns you should be able to see:
  • Conditions updated appropriately
  • params (see this example) filled in with the templated values
  1. Looking at the Task logs (if this has been implemented, see Implement working "hello world" Task + TaskRun #59) you should be able to see the appropriate values echoed for each task, i.e.:
  • template-params-task-image should echo gcr.io/somethingsomethingsomething
  • template-params-task-github should echo 123456789123456789
  • template-params-task-cluster should echo https://somethingsomethingsomethingsomething.com

Additional Info

Blocked by #36 (design templating) and #61 (simple PipelineRun)

@bobcatfish
Copy link
Collaborator Author

@bobcatfish
Copy link
Collaborator Author

/assign @dlorenc
/meow space

@knative-prow-robot
Copy link

@bobcatfish: cat image

In response to this:

/assign @dlorenc
/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@bobcatfish bobcatfish added the meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given label Oct 12, 2018
nader-ziada added a commit to nader-ziada/pipeline that referenced this issue Oct 15, 2018
Turned out that the pipelinerun controller wasn't yet passing along
parameters which were supplied in Pipelines (for tektoncd#64).

Also removed unneeded `value` field from parameter delcaration.

Some remaining work that needs to be done:
- It should be an error to provide a parameter to a Task that it doesn't
need
- It should be an error to apply templating with required params not
supplied

Co-authored-by: Christie Wilson <[email protected]>
nader-ziada added a commit to nader-ziada/pipeline that referenced this issue Oct 15, 2018
Turned out that the pipelinerun controller wasn't yet passing along
parameters which were supplied in Pipelines (for tektoncd#64).

Also removed unneeded `value` field from parameter delcaration.

Some remaining work that needs to be done:
- It should be an error to provide a parameter to a Task that it doesn't
need
- It should be an error to apply templating with required params not
supplied

Co-authored-by: Christie Wilson <[email protected]>
knative-prow-robot pushed a commit that referenced this issue Oct 16, 2018
Turned out that the pipelinerun controller wasn't yet passing along
parameters which were supplied in Pipelines (for #64).

Also removed unneeded `value` field from parameter delcaration.

Some remaining work that needs to be done:
- It should be an error to provide a parameter to a Task that it doesn't
need
- It should be an error to apply templating with required params not
supplied

Co-authored-by: Christie Wilson <[email protected]>
@nader-ziada
Copy link
Member

@bobcatfish should this issue be closed? I think its done now by the above PRs

@bobcatfish
Copy link
Collaborator Author

I think that actually templating with a Task's outputs still isn't implemented 🤔 But i think @dlorenc has a branch with this fixed so we can nearly close this...

@bobcatfish
Copy link
Collaborator Author

The other thing still missing is applying any templating at a Pipeline level.

bobcatfish referenced this issue in bobcatfish/pipeline Oct 25, 2018
Before this change, Tasks were retrieving the Resources to use by
looking for PipelineResources with exactly the name that the Resource is
declared with in the Task. This means that the Resource binding in
Pipelines (and TaskRuns) was doing absolutely nothing.

Instead, we now map bound Resources inside of TaskRuns. The name of a
Resource declared in a Task is a _parameter_, which we fulfill by
providing the _key_ aka parameter of the Task, and the corresponding
Resource reference we should use for that _key_.

This allows us to use the same Task with different Resources without
having to change the Task or the Resource.

This was not caught by any tests because the tests were co-incidentally
using the same name for the Resources and the Resource
key/name/parameter in the Task. Now the tests use different names.

This came up while creating example yaml for #89 - as soon as I tried to
use two different git resources (one upstream, one my fork) I realized
that the mapping was not working.

This also partially addresses tektoncd#138 by removing the `Name` field from the
bindings, which is not required.

This addresses part of #64 by adding templating from outputs.
bobcatfish referenced this issue in bobcatfish/pipeline Oct 30, 2018
Before this change, Tasks were retrieving the Resources to use by
looking for PipelineResources with exactly the name that the Resource is
declared with in the Task. This means that the Resource binding in
Pipelines (and TaskRuns) was doing absolutely nothing.

Instead, we now map bound Resources inside of TaskRuns. The name of a
Resource declared in a Task is a _parameter_, which we fulfill by
providing the _key_ aka parameter of the Task, and the corresponding
Resource reference we should use for that _key_.

This allows us to use the same Task with different Resources without
having to change the Task or the Resource.

This was not caught by any tests because the tests were co-incidentally
using the same name for the Resources and the Resource
key/name/parameter in the Task. Now the tests use different names.

This came up while creating example yaml for #89 - as soon as I tried to
use two different git resources (one upstream, one my fork) I realized
that the mapping was not working.

This also partially addresses tektoncd#138 by removing the `Name` field from the
bindings, which is not required.

This addresses part of #64 by adding templating from outputs.
knative-prow-robot pushed a commit that referenced this issue Oct 30, 2018
Before this change, Tasks were retrieving the Resources to use by
looking for PipelineResources with exactly the name that the Resource is
declared with in the Task. This means that the Resource binding in
Pipelines (and TaskRuns) was doing absolutely nothing.

Instead, we now map bound Resources inside of TaskRuns. The name of a
Resource declared in a Task is a _parameter_, which we fulfill by
providing the _key_ aka parameter of the Task, and the corresponding
Resource reference we should use for that _key_.

This allows us to use the same Task with different Resources without
having to change the Task or the Resource.

This was not caught by any tests because the tests were co-incidentally
using the same name for the Resources and the Resource
key/name/parameter in the Task. Now the tests use different names.

This came up while creating example yaml for #89 - as soon as I tried to
use two different git resources (one upstream, one my fork) I realized
that the mapping was not working.

This also partially addresses #138 by removing the `Name` field from the
bindings, which is not required.

This addresses part of #64 by adding templating from outputs.
@bobcatfish bobcatfish added this to the 0.0.1 Alpha release milestone Nov 3, 2018
@dlorenc
Copy link
Contributor

dlorenc commented Nov 5, 2018

Can this be closed now?

@bobcatfish
Copy link
Collaborator Author

Only use case left is when you provide templating via a Pipeline:

kind: Pipeline
...
- name: deploy-bundle-test
  taskRef:
    name: deploy-with-kubectl
  inputSourceBindings:
  - name: workspace
    key: guestbook    
  - name: myImage
    key: redis-docker
    passedConstraint:
    - build-push
  params:
  - name: labels
    value: "${myImage.digest}@${workspace.revision}"

In the end the value should be expanded

@dlorenc
Copy link
Contributor

dlorenc commented Nov 9, 2018

So I'm not 100% sure of the best way to test this. I added a test case to show that the PipelineRun reconciler does not expand these and will pass them to the TaskRun controller, and we have TaskRun tests that show they get expanded. But that doesn't feel very satisifying...

@abayer
Copy link
Contributor

abayer commented Nov 9, 2018

At first glance, at least, that sounds like a reasonable set of tests to me.

@bobcatfish
Copy link
Collaborator Author

I added a test case to show that the PipelineRun reconciler does not expand these and will pass them to the TaskRun controller, and we have TaskRun tests that show they get expanded. But that doesn't feel very satisifying...

I hear you! I can't think of anything better at the moment - we could have an integration tests that passes the whole thing in and makes sure the right thing comes out, but that sounds pretty rough.

I was looking at some of the kubebuilder stuff today with @pwittrock and the integration tests it generates, which can instantiate the controllers and api server locally, could be an interesting way to test something like this.

That being said it kinda feels like overkill!

TL;DR: @dlorenc I think either add the test you've described or just decide it's not worth covering and leave it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given
Projects
None yet
Development

No branches or pull requests

6 participants