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.

Sign up for GitHub

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

Jump to bottom

TEP 0109: Better structured provenance retrieval in Tekton Chains #697

Merged
merged 0 commits into from
Aug 22, 2022
Merged

TEP 0109: Better structured provenance retrieval in Tekton Chains #697

merged 0 commits into from
Aug 22, 2022

Conversation

ywluogg
Copy link
Contributor

@ywluogg ywluogg commented May 3, 2022

Start the discussions and reviews regarding to support structured results and params in Tekton Chains

@tekton-robot tekton-robot requested review from sm43 and vtereso May 3, 2022 18:52
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 3, 2022
@ywluogg
Copy link
Contributor Author

ywluogg commented May 3, 2022

/assign @wlynch @afrittoli

@ywluogg
Copy link
Contributor Author

ywluogg commented May 3, 2022

This is a draft that I have. I'm currently working on changes to add PipelineRun into scope as well.

@ywluogg ywluogg changed the title TEP 0107: Better structured provenance retrieval in Tekton Chains TEP 0108: Better structured provenance retrieval in Tekton Chains May 3, 2022
@ywluogg ywluogg marked this pull request as draft May 3, 2022 19:08
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2022
@ywluogg ywluogg changed the title TEP 0108: Better structured provenance retrieval in Tekton Chains TEP 0109: Better structured provenance retrieval in Tekton Chains May 4, 2022
@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 4, 2022
@ywluogg ywluogg mentioned this pull request May 5, 2022
Merged
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 5, 2022
@ywluogg
Copy link
Contributor Author

ywluogg commented May 9, 2022

/assign @dibyom

@pritidesai
Copy link
Member

/kind tep

@tekton-robot tekton-robot added kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 9, 2022
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2022
@tekton-robot tekton-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 10, 2022
@ywluogg ywluogg force-pushed the main branch 2 times, most recently from bf871c5 to d120eea Compare August 8, 2022 16:54
@ywluogg ywluogg requested review from afrittoli and removed request for lcarva August 8, 2022 18:03
wlynch
wlynch reviewed Aug 8, 2022
View reviewed changes
teps/0109-better-structured-provenance-retrieval-in-tekton-chains.md Outdated Show resolved Hide resolved
teps/0109-better-structured-provenance-retrieval-in-tekton-chains.md Outdated Show resolved Hide resolved
teps/0109-better-structured-provenance-retrieval-in-tekton-chains.md Outdated
Comment on lines 350 to 352
- name: ARTIFACT_INPUTS
value: ["ARTIFACT-INPUT-NAME-1", "ARTIFACT-INPUT-NAME-1"]
- name: ARTIFACT_OUTPUTS
value: ["ARTIFACT-OUTPUT-NAME-1"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this over requiring task authors to plumb through result names, though a few caveats:

Putting data in object keys means that we're breaking from the intoto spec - e.g. what happens if a user just outputs a vanilla intoto material? Is intoto compatibility a goal for this proposal (it feels like this is what we're aiming for, but it's not explicitly called out as a goal)?

By putting the field in the result spec, this has the nice property of not having to need to modify the payload the Task needs to output (e.g. it can be a vanilla intoto spec), but if we'll need a spec change it might make more sense to just move forward with the status-based provenance.

@ywluogg ywluogg force-pushed the main branch 2 times, most recently from 4b3f310 to b9a0aa2 Compare August 8, 2022 19:44
@ywluogg ywluogg requested a review from wlynch August 8, 2022 19:45
wlynch
wlynch reviewed Aug 8, 2022
View reviewed changes
teps/0109-better-structured-provenance-retrieval-in-tekton-chains.md Outdated Show resolved Hide resolved
teps/0109-better-structured-provenance-retrieval-in-tekton-chains.md Outdated
description: |
The source distribution
* uri: resource uri of the artifact.
* digest: revision digest in form of algorithm:digest.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a way of representing variable length outputs? e.g. how the IMAGES field works today.

I'm trying to think through how things where we may not know the exact # of artifacts produced like multiarch images can be supported. 🤔

teps/0109-better-structured-provenance-retrieval-in-tekton-chains.md Outdated
Comment on lines 350 to 352
- name: ARTIFACT_INPUTS
value: ["ARTIFACT-INPUT-NAME-1", "ARTIFACT-INPUT-NAME-1"]
- name: ARTIFACT_OUTPUTS
value: ["ARTIFACT-OUTPUT-NAME-1"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can live with it, though I think we should change the arrays - I'm not sure if there's an easy way to have the Task provide these without hardcoding them. I don't think there's a way to derive the values from the replacement context.

@ywluogg ywluogg requested a review from wlynch August 9, 2022 18:34
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 9, 2022
@ywluogg
Copy link
Contributor Author

ywluogg commented Aug 9, 2022 •
edited
Loading

I can live with it, though I think we should change the arrays - I'm not sure if there's an easy way to have the Task provide these without hardcoding them. I don't think there's a way to derive the values from the replacement context.

I just updated the schemas and removed the arrays. Let me know what you think @wlynch @afrittoli

@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 ask for approval from afrittoli after the PR has been reviewed.

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 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2022
@ywluogg
Copy link
Contributor Author

ywluogg commented Aug 22, 2022

/retest

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2022
@tekton-robot
Copy link
Contributor

@ywluogg: PR needs rebase.

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.

wlynch
wlynch requested changes Aug 22, 2022
View reviewed changes
teps/0109-better-structured-provenance-retrieval-in-tekton-chains.md Outdated

## Proposal

**_Please note that the below proposal will be acted as a middle ground proposal, as it on one side definitely provides more organized structure for users to provide provenance data in Tekton TaskRuns and PipelineRuns, but on the other side that it still has a bunch of trade offs that can be resolved by another proposal mentioned in Alternatives section._**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might help to refocus this TEP on the desired end state, and move the middle ground impl to Implementation Plan and/or Risks and Mitigations.

We should also look out to fill out the sections for Drawbacks - though I think this should focus on the end goal rather than in-between steps.

@ywluogg ywluogg merged commit 134f031 into tektoncd:main Aug 22, 2022
@ywluogg
Copy link
Contributor Author

ywluogg commented Aug 22, 2022

/reopen

@tekton-robot
Copy link
Contributor

@ywluogg: Failed to re-open PR: state cannot be changed. The pull request cannot be reopened.

In response to this:

/reopen

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.

@ywluogg ywluogg mentioned this pull request Aug 22, 2022
Merged
@ywluogg
Copy link
Contributor Author

ywluogg commented Aug 22, 2022

/assign @wlynch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcarva lcarva lcarva left review comments

@chuangw6 chuangw6 chuangw6 left review comments

@wlynch wlynch wlynch requested changes

@sm43 sm43 Awaiting requested review from sm43

@vtereso vtereso Awaiting requested review from vtereso

@dibyom dibyom Awaiting requested review from dibyom

@afrittoli afrittoli Awaiting requested review from afrittoli

Assignees

@vdemeester vdemeester

@wlynch wlynch

@afrittoli afrittoli

@dibyom dibyom

Labels
kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
TEP Steward Assignments
Status: UnAssigned
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ywluogg @pritidesai @tekton-robot @wlynch @afrittoli @dibyom @lcarva @chuangw6 @vdemeester