-
Notifications
You must be signed in to change notification settings - Fork 226
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
TEP-0086 Changing the way result parameters are stored #660
Conversation
- Design details updated - 5th option based on API WG Alternate discussion - Updated based on TEP review comments - Updated based on Result Collector Sidecar - Added Result Collector Sidecar author - Updated with additional PipelineResource alternative' - Updated based on PR comments - Moved the result sidecar solution to alternative and marked as TBD - Updated based on WG review feedback
…on 📓 In [a recent API working group meeting](https://docs.google.com/document/d/17PodAxG8hV351fBhSu7Y_OIPhGTVgj6OJ2lPphYYRpU/edit#heading=h.ux4qhojeot6k) we talked through the proposal that was currently looking the most appealing (sidecar + HTTP service) and @skaegi brought up some potential requirements that might change this, and also volunteered to find someone to work on a POC for a logs based solution before we disregard that entirely. Personally I agree that it would be best if we could keep the TaskRun to be the source of truth for the results themselves, but that would mean agreeing to an upper limit on result size (at least). I've tried to reflect the above and generally organize the TEP a bit, and add some missing stuff: * Moved some points from Goals to requirements (really one goal overall, rest of the items are constraints on how we accomplish that goal) * Add requirements we've been discussing, trying to highlight the ones that are newer and other collaborators might disagree with * Expanding the risks section a bit * Added some high level performance details * Suggested potential next steps to unblock this * Added more detials to auth/encryption/defined interface notes from our previous one off meeting [notes](https://hackmd.io/YU_g27vRS2S5DwfBXDGpYA?view) * Filled in the design evaluation * Added some drawbacks * Separated "SideCar Result References" into two alternatives (which are not mutually exclusive) * Added using PVCs as an alternative * Added some cons to log based solution
I noticed this TEP is not yet accepted and merged. This TEP is stuck in the review process. Dropping the proposal and design details based on the recent discussion in the API WG. This TEP instead now contains a list of alternatives. If we can merge this without proposal, it opens up an opportunity for the community to create a PoC and evaluate which solution fits best for the set of the requirements. Moving the proposal and design details under the first alternative, making it more like a TEP within TEP. One major change here is the requirement section in which task results as part of the taskRun status is listed as a requirement with the flexible design for the use cases which would like to avoid it. Updated the toc.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @afrittoli |
Hi @pritidesai what do we need to do so you can permissions to push to the existing TEP-0086 that we have used for the last 6 or so months. #521. Happy to adjust rights or share the branch or merge in this one in draft so anyone can branch from main (i think thats what I had discussed with @bobcatfish) I'd like to retain the original goal if possible around large result sets and a pluggable abstracted storage mechanism. |
Alternatively, if that doesn't work, you could give @pritidesai push access to your fork. |
Never knew that existed! It is ticked though. What do I need to do to give @pritidesai access to push to the fork? or is there some other check box i need to look into? |
Hey @tlawrie I don't see the similar check box in your PR #521 🤔 |
Hey @pritidesai yep, that's the one that's showed it for me. Maybe its only shows the checkbox to the author? |
Yes, it only shows the checkbox to the author. Then the "maintainer" is in the GitHub sense of the word (not prow's sense), and this mean "people who have the maintainer or write role" if I remember correctly — this is why @pritidesai probably cannot edit the PR but I can 😅 . We need to update the github teams and roles on this repository a bit or you @tlawrie need to add @pritidesai as a member on your fork. |
Well invited @pritidesai to be a collaborator on my Fork. But i also like the idea of updating the repositories roles @vdemeester |
Thanks @tlawrie I have pushed the changes to your fork 🚀 We have the PR open to create community maintainers team which should resolve this issue (maintainers will be able to push to the PR). |
I do not have permission to push to the existing PR 😞
I am opening a new PR for now, please feel free to close this if someone can help pushing commits to the existing PR #521 🙏
This TEP lists the problem statement, use cases, requirements, and a list of possible solutions.
I noticed this TEP is not yet accepted and merged. This TEP is stuck in the review process. Dropping the proposal and design details based on the recent discussion in the API WG. If we can merge this without proposal, it opens up an opportunity for the community to create a PoC and evaluate which solution fits best for the set of the requirements.
Moving the proposal and design details under the first alternative, making it more like a TEP within TEP.
One major change here is the requirement section in which task results as part of the taskRun status is listed as a requirement with the flexible design for the use cases which would like to avoid it.
Easier way to review the changes I have applied: 5a00865?diff=split&w=1
tektoncd/pipeline#4012
/kind tep