-
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
Design PipelineResource extensibility #238
Comments
Here is one proposal, which I am unreasonably attached to: Similar to how consourse uses Resources as an extension point, we could also use
Cons:
|
Seems reasonable at first glance - I'm putting together some thoughts on extensibility based on some design work here (i.e., we're establishing what we want to be able to accomplish to do something like automatic quality gates on |
We will probably need to have a PVC anyways to share outputs from one taskRun (in a pod) with another taskRun in the same pipeline (in a different pod) |
last time i was playing with azure's kubernetes service attaching volume to a node was taking ~2min whereas on gke it was only 10s. i think if persistent volumes are used then we should figure out how to deal with long attachment times (or how to avoid them entirely if that's possible). |
@pivotal-nader-ziada would downloading the output from a result store be a viable option ? |
@tejal29 you mean something like gcs or s3 for sharing artifacts between tasks? yes, this would be an option, but its one extra thing users have to setup and have ready. Based on @cppforlife comment and my own investigations, I want to avoid using PVC, but still investigating best options to discuss with everyone |
Something worth discussing - what exactly do we want the extensions to be able to do? Some things definitely map directly to |
I think of |
Ok, so in cases like notifications, we wouldn't be using PVC, right? |
Yes, we would not be using PVC for the notifications, we are trying to minimize any use of PVC in general for performance reasons |
It would be helpful to lay out some more examples of what we want to be extensible in the first place - notification mechanisms and SCM checkout processes are pretty clear to me, but I'm not sure what else is being considered here? |
Great point @cppforlife , thanks for sharing those stats!
@abayer that's a great point - we should try to be clear about what we want users to be able to extend. It's hard to know in advance though :S Definitely we want them to be able to provide their own:
(Maybe what I'm doing here is defining what a Resource is ... lol as it slowly becomes a Concourse resource minus polling...) I created this doc for brainstorming - knative-dev@ should have edit access.
Depends on how we design notifications! They could be viewed as a type of resource (something you put data to) like @pivotal-nader-ziada described or we could make them a top level thing in the system, e.g. we have Resources, Tasks, Pipelines + Notifications. I can see either working well. |
Another note re. using PVCs, I realized the design I proposed in #238 (comment) has a big flaw: if multiple Pipelines, or even Tasks within a Pipeline, try to use the same Resources as inputs, they'd be mounting in the same PVC, which seems like it either wouldn't work or would be asking for some terrible side effects. Here's an updated proposal (specifically for extending Resources) which I think is better:
|
Relevant comment from @mattmoor in slack:
|
Okay so I'm feeling pretty good about this plan, what do folks think: For both
|
We use the syntax `somethingRef` in several fields on our specs because we want to be able to use ducktyping down the line to switch out other types (see tektoncd#238). However `triggerRef` is a bit weird because: - If the type is `manual`, there is no actual CRD to reference - We don't know what other sorts of types we might need (e.g. when created via events) - It was inconsistent b/w TaskRun and PipelineRun (TaskRun had an extra level of indirection) This commit applies @sebgoa's suggestion to simplify this to just `trigger`. This was part of the discussion in tektoncd#320 about simplifying the interfaace.
We use the syntax `somethingRef` in several fields on our specs because we want to be able to use ducktyping down the line to switch out other types (see #238). However `triggerRef` is a bit weird because: - If the type is `manual`, there is no actual CRD to reference - We don't know what other sorts of types we might need (e.g. when created via events) - It was inconsistent b/w TaskRun and PipelineRun (TaskRun had an extra level of indirection) This commit applies @sebgoa's suggestion to simplify this to just `trigger`. This was part of the discussion in #320 about simplifying the interfaace.
Note to self in #414 we are adding rules around which types of resources produce outputs - something to take into account in a design where resources are extensible :D |
Hi @bobcatfish 👋 We would like to know, how this issue is coming along? have we started working on
Also, would you mind elaborating this point? 🤔 |
No implementation yet! I'm going to try to flesh out the design a bit further asap :D |
Design doc focusing on (I realized I actually don't have a clear idea of how to design custom |
I'm gonna downscope this issue to be just about |
Relevant discussion here: https://tektoncd.slack.com/archives/CJ62C1555/p1560974129126900 It would be nice if there was a way to specify a file/directory or even just a simple text value to be passed between tasks without needing to create a volume across tasks. Maybe even a non-typed |
This task is used to sync an Argo CD application with its underlying Git repository and to wait for it to become healthy. It takes in no resources, only parameters, and passes them to the argocd command. The application's name and the Argo CD server address are required. Some form of login is also required, either through a username/password combo or an authentication token. The specific revision to sync to and the flags to append to commands can also be passed as parameters. Currently there is no way to pass plain strings or files between tasks, or it could have been better to split the login and sync tasks. This way, there could have been two login tasks - one for username/password and one for authentication token. With the current approach, the bash command runs an if statement to see if the token's default value was overriden, and if so, it uses that token. Otherwise, it uses the username/password passed in. This issue (typing of PipelineResources) is being discussed in tektoncd/pipeline#238.
@vdemeester @pmorie what are the next steps for the design here? I think folks are pretty happy with the design as proposed and we could close this issue, and move into implementing it, what do you think? |
I'm gonna close this! @vdemeester @pmorie can we make some follow up issues to actually implement this? Happy to help if needed. |
Expected Behavior
We should have clear extension points in the Pipeline CRD system. These points should be well documented, clear, and tested.
Ideally it will be possible to extend the system without having to modify the existing controller binaries or CRD definitions.
Actual Behavior
At the moment, if a user wanted to extend the Pipeline they have these options:
In the worst case, the last option should allow a user to do pretty much anything they want, outside of the functionality provided by this system.
Additional Info
n/a
The text was updated successfully, but these errors were encountered: