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

Closed
wants to merge 10 commits into from

Conversation

austinzhao-go
Copy link
Contributor

@austinzhao-go austinzhao-go commented Apr 20, 2022

The Related Issue: Pipeline Issue #4105
Design Doc: Support Specifying Metadata per Task in Runtime

TODO

  • Collect feedback and confirm details about Metadata Precedence
  • Add a section about Metadata Precedence

Re-raised PR as here:
#695

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 20, 2022
@tekton-robot tekton-robot requested review from dlorenc and hrishin April 20, 2022 18:53
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 20, 2022
@vdemeester
Copy link
Member

/assign

@austinzhao-go austinzhao-go changed the title [WIP] TEP-0106: Support Specifying Metadata per Task in Runtime TEP-0106: Support Specifying Metadata per Task in Runtime Apr 21, 2022
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 21, 2022
@dibyom
Copy link
Member

dibyom commented Apr 21, 2022

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Apr 21, 2022
@dibyom
Copy link
Member

dibyom commented Apr 25, 2022

/assign

@austinzhao-go
Copy link
Contributor Author

austinzhao-go commented Apr 27, 2022

Hi @pritidesai, was reviewing the point you mentioned for metadata precedence:
think I want to propose the order will be as (+[] for the addition):
+[PipelineTaskRunSpec(of PipelineRun)] > PipelineRun (metadata field) > TaskRun (metadata field) > TaskSpec (of
PipelineTask in Pipeline type)

Reasons:

  • Current order gave higher precedence for annotations/labels in runtime as PipelineRun/TaskRun > TaskSpec in Pipeline, so think it's reasonable to have the runtime metadata from TaskRunSpec of PipelineRun with a higher precedence
  • As annotations specified in runtime depend on the execution context and are closely related to a specific usage case, so a higher precedence will ensure they were propagated down to the target Pod to take effect (so perhaps overwriting the ones statically defined in Task or Pipeline).

perhaps some comments?

@dibyom
Copy link
Member

dibyom commented Apr 27, 2022

+[PipelineTaskRunSpec(of PipelineRun)] > PipelineRun (metadata field) > TaskRun (metadata field) > TaskSpec (of
PipelineTask in Pipeline type)

This sounds good to me.

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 27, 2022
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

This generally looks good to me, but I'll wait for more folks to give it a pass first. Thanks for working on this! 👍


## Motivation

The required metadata currently can be added under a `Task` entity while a user is authoring/configuring the template for a `TaskRun`. As two contexts are considered for Tetkon - the “authoring time” and “runtime”, a stretch of thinking will lead to if the metadata could be defined “dynamically” under the runtime.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The required metadata currently can be added under a `Task` entity while a user is authoring/configuring the template for a `TaskRun`. As two contexts are considered for Tetkon - the “authoring time” and “runtime”, a stretch of thinking will lead to if the metadata could be defined “dynamically” under the runtime.
The required metadata currently can be added under a `Task` entity while a user is authoring/configuring the template for a `TaskRun`. As two contexts are considered for Tekton - the “authoring time” and “runtime”, a stretch of thinking will lead to if the metadata could be defined “dynamically” under the runtime.

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 @imjasonh for the check-in!
parked in community slack for few days, seems no further feedback at this moment.

could I have have a lgtm label when good with u? will keep a tracking and update with the coming changes PR on this TEP.

cc @dibyom

@dibyom
Copy link
Member

dibyom commented May 2, 2022

Hey @austinzhao-go before we can merge, a couple of housekeeping items:

  1. Let's fix the typo that Jason mentioned
  2. Let's squash up the commits to 1 commit with a commit message that follows the guidelines

@austinzhao-go
Copy link
Contributor Author

@dibyom bad me. missed the typo line.
thought there could be a "squash" checkbox when merging (worked with Gitlab...)

closed this PR and raise a new one as there are several merge commits making it troublesome to "squash" down
#695

@afrittoli
Copy link
Member

thought there could be a "squash" checkbox when merging (worked with Gitlab...)

We don't merge PR manually, we let "tide" merge PRs once all CI jobs are green and required labels ("approved", "lgtm") are set. Automatic squash of commits is not enabled in our tide, as in some cases one might want to have more than one commit per PR and in generally leads to a cleaner git log.
I hope this is not causing too much inconvenience, let me know if you need assistance with manual squashing.

closed this PR and raise a new one as there are several merge commits making it troublesome to "squash" down

This works too, thanks @austinzhao-go.

Should you be in this situation again, you can re-use the same PR and keep the context of comments and previous discussion. You can prepare your code in a dedicated local branch (a more convenient option compared to using main) and then push that branch to the one on your fork that you used originally for the PR, e.g.

git push <remote-of-your-fork> <local-branch-name>:<pr-branch-name> --force

For this PR the <pr-branch-name> would have been main, which I would not recommend in future, because if you have more than one PR against the same repo, they cannot share the main branch of your fork.

@austinzhao-go
Copy link
Contributor Author

these are much clear and helpful context and actions. thanks a lot @afrittoli!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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). size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: @ dibyom
Development

Successfully merging this pull request may close these issues.

6 participants