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-0106: Support Specifying Metadata per Task in Runtime #695

Merged
merged 1 commit into from
May 9, 2022
Merged

TEP-0106: Support Specifying Metadata per Task in Runtime #695

merged 1 commit into from
May 9, 2022

Conversation

austinzhao-go
Copy link
Contributor

@austinzhao-go austinzhao-go commented May 3, 2022 •
edited
Loading

The Related Issue

Design Doc

Implementation PR:

Original PR was closed due to merge commits affecting a squash-down

@tekton-robot tekton-robot requested review from dibyom and sm43 May 3, 2022 01:36
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 3, 2022
@austinzhao-go austinzhao-go mentioned this pull request May 3, 2022
Closed
2 tasks
@austinzhao-go austinzhao-go changed the title TEP-0106 TEP-0106: Support Specifying Metadata per Task in Runtime May 3, 2022
@austinzhao-go
Copy link
Contributor Author

/assign @vdemeester
/assign @dibyom
/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label May 3, 2022
@austinzhao-go austinzhao-go force-pushed the tep-0106-support-metadata-runtime branch 2 times, most recently from 63e08ca to bd6d894 Compare May 3, 2022 14:55
@austinzhao-go austinzhao-go mentioned this pull request May 3, 2022
Closed
@afrittoli
Copy link
Member

Design Doc: Support Specifying Metadata per Task in Runtime

The design doc is not shared with the tekton-dev / tekton-users google groups as I don't have access to it.
Could you please share it?

@austinzhao-go
Copy link
Contributor Author

updated the access. sry for this. @afrittoli
I rewrote most of ideas in design doc into the TEP which should have updated details there.

afrittoli
afrittoli reviewed May 4, 2022
View reviewed changes
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thank you for this.
I only have a small comment on preventing the use of *.tekton.dev labels and annotations, otherwise it looks good!

teps/0106-support-specifying-metadata-per-task-in-runtime.md
## References

- The related [Tekton Pipeline Issue #4105](https://github.com/tektoncd/pipeline/issues/4105)
- Design Doc [Support Specifying Metadata per Task in Runtime](https://docs.google.com/document/d/1JyeE_TEKDpnqr1uygxkALJyPKXMOypAwPfnEAx7HKyY/edit?usp=sharing)
Copy link
Member

Choose a reason for hiding this comment

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

This should be shared with the tekton community

teps/0106-support-specifying-metadata-per-task-in-runtime.md
The proposed order will be as (the addition marked with +[]):

```markdown
+[PipelineTaskRunSpec (of PipelineRun)] > PipelineRun (metadata field) -> TaskRun (metadata field) > TaskSpec (of PipelineTask in Pipeline type)
Copy link
Member

Choose a reason for hiding this comment

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

This looks good. We might want to prevent overriding of Tekton defined labels and annotations though.
For instance, in this example:

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  annotations:
    chains.tekton.dev/retries: "3"
    chains.tekton.dev/signed: failed.   
    pipeline.tekton.dev/affinity-assistant: affinity-assistant-24428a19af
    pipeline.tekton.dev/release: 6b5710c
    results.tekton.dev/record: default/results/5ffb52cc-ce6f-466b-8ddf-827b955cf6c5/records/43f5e888-b935-4264-91c2-7ff54fd09b3e
    results.tekton.dev/result: default/results/5ffb52cc-ce6f-466b-8ddf-827b955cf6c5
  labels:
    tekton.dev/memberOf: tasks
    tekton.dev/pipeline: catalog-publish
    tekton.dev/pipelineRun: publish-catalog-tekton-ghn4b
    tekton.dev/pipelineTask: git-clone
    tekton.dev/task: git-clone
    triggers.tekton.dev/eventlistener: tekton-cd
    triggers.tekton.dev/trigger: catalog
    triggers.tekton.dev/triggers-eventid: 5ffb52cc-ce6f-466b-8ddf-827b955cf6c5
  • pipeline.tekton.dev annotations are set by the pipeline controller
  • chains.tekton.dev annotations are set by chains
  • results.tekton.dev annotations are set by results
  • tekton.dev labels are set by the pipeline controller or coming from the catalog
  • triggers.tekton.dev labels are set by triggers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, thanks Andrea! thinking where do we do the validation(check if overwriting the Tekton metadata) on current logic?

working on my changes with this fuc, perhaps also could be a place which needs to ensure "not overwriting tekton metadata" right
https://github.com/tektoncd/pipeline/blob/d5ae54ac4d978a142ad1ddd26921bb761dd0bfff/pkg/reconciler/pipelinerun/pipelinerun.go#L1010

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if validation logic exists on current code, think I can do this to prevent the overwriting:

  • add the similar validation on the added field, so the tekton overwritten metadata can't be inputed
  • keep changes (populating metadata with the mentioned precedence) in the combineTaskRunAndTaskSpecAnnotations()

Copy link
Member

Choose a reason for hiding this comment

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

I thought tektoncd/pipeline#4366 had been implemented but in fact it's still open - in which case I would say we don't need to implement it here - yet.
When we implement the restriction, we will update the runtime metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks this context. I will update this point into TEP and mark a follow-up action to revisit this work after the constraint is applied. (also let me update the linked context in that issue)

This was referenced May 4, 2022
Open
Merged
@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2022
@austinzhao-go
TEP-0106: Support Specifying Metadata per Task in Runtime
a1d25de 
Add Design Details, Clean Comments

Improve Idea Expression

Add Note for a Possible Feature

Add Section for Metadata Precedence
@austinzhao-go austinzhao-go force-pushed the tep-0106-support-metadata-runtime branch from bd6d894 to a1d25de Compare May 5, 2022 14:52
@austinzhao-go
Copy link
Contributor Author

austinzhao-go commented May 5, 2022 •
edited
Loading

Updated actions to handle with the mentioned 2 concerns:

  • Metadata Conflicts as here
  • Tekton Metadata Overriding as here

afrittoli
afrittoli reviewed May 9, 2022
View reviewed changes
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, dibyom, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@pritidesai
Copy link
Member

API WG - @austinzhao-go to address the conflict concern in the follow up PR, this is in proposed state and ready to merge.

Issue - tektoncd/pipeline#4366

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2022
@tekton-robot tekton-robot merged commit d7c91fa into tektoncd:main May 9, 2022
@lbernick lbernick mentioned this pull request May 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afrittoli afrittoli afrittoli left review comments

@dibyom dibyom dibyom approved these changes

@vdemeester vdemeester vdemeester approved these changes

@sm43 sm43 Awaiting requested review from sm43

Assignees

@vdemeester vdemeester

@pritidesai pritidesai

@dibyom dibyom

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
TEP Steward Assignments
Status: UnAssigned
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@austinzhao-go @afrittoli @tekton-robot @pritidesai @vdemeester @dibyom