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

TEP-0086 Changing the way result parameters are stored #660

Closed
wants to merge 4 commits into from

Conversation

pritidesai
Copy link
Member

@pritidesai pritidesai commented Mar 18, 2022

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

tlawrie and others added 4 commits February 1, 2022 11:56
- 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.
@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Mar 18, 2022
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign pritidesai
You can assign the PR to them by writing /assign @pritidesai in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 18, 2022
@dibyom
Copy link
Member

dibyom commented Mar 21, 2022

/assign @afrittoli
/assign @jerop

@tlawrie
Copy link
Contributor

tlawrie commented Mar 28, 2022

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.

@afrittoli
Copy link
Member

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

I'd like to retain the original goal if possible around large result sets and a pluggable abstracted storage mechanism.

Some docs from GitHub: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

Alternatively, if that doesn't work, you could give @pritidesai push access to your fork.

@tlawrie
Copy link
Contributor

tlawrie commented Apr 4, 2022

Never knew that existed! It is ticked though.

image

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?

@pritidesai
Copy link
Member Author

Never knew that existed! It is ticked though.

image

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 🤔
Screen Shot 2022-04-04 at 8 54 02 AM

@tlawrie
Copy link
Contributor

tlawrie commented Apr 5, 2022

Hey @pritidesai yep, that's the one that's showed it for me. Maybe its only shows the checkbox to the author?

@vdemeester
Copy link
Member

vdemeester commented Apr 5, 2022

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.

@vdemeester
Copy link
Member

Like this:
2022-04-05-174035

@bobcatfish bobcatfish added the area/s3c Issues or PRs that are related to Secure Software Supply Chain (S3C) label Apr 5, 2022
@tlawrie
Copy link
Contributor

tlawrie commented Apr 7, 2022

Well invited @pritidesai to be a collaborator on my Fork. But i also like the idea of updating the repositories roles @vdemeester

@pritidesai
Copy link
Member Author

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).

@pritidesai
Copy link
Member Author

Closing this PR since I was able to push the changes to @tlawrie's fork and can be reviewed in #521

@pritidesai pritidesai closed this Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/s3c Issues or PRs that are related to Secure Software Supply Chain (S3C) kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: NEW
Status: UnAssigned
Development

Successfully merging this pull request may close these issues.

8 participants