-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Comments
This is the design doc for both templating and references, shared with members of knative-dev@. |
/assign @dlorenc |
In response to this:
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. |
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]>
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]>
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]>
@bobcatfish should this issue be closed? I think its done now by the above PRs |
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... |
The other thing still missing is applying any templating at a Pipeline level. |
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.
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.
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.
Can this be closed now? |
Only use case left is when you provide templating via a Pipeline:
In the end the |
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... |
At first glance, at least, that sounds like a reasonable set of tests to me. |
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. |
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 (viaPipelineRun
) be provided by the referencedPipelineParams
andResources
.This should include:
Actual Behavior
Templating is being designed in #36 but is not implemented.
Steps to Reproduce the Problem
Task
like this:Resources
andPipelineParams
like this (we're going to use the clusters):Pipeline
like this that uses templating to pass values fromResources
andPipelineParams
:PipelineRun
like this:kubectl get taskRuns
you should be able to see:template-params-task-image
should echogcr.io/somethingsomethingsomething
template-params-task-github
should echo123456789123456789
template-params-task-cluster
should echohttps://somethingsomethingsomethingsomething.com
Additional Info
Blocked by #36 (design templating) and #61 (simple PipelineRun)
The text was updated successfully, but these errors were encountered: