Fix Task(Run)/Pipeline(Run) metadata propagation

[vdemeester]

Changes

For annotations and labels, do not override the Run metadata with the
definition (Pipeline/Task) if they are the same. If an annotation or
label is present in both Pipeline and PipelineRun, the value in the
Run type takes precedence (same for Task and TaskRun).

Closes #4636

Signed-off-by: Vincent Demeester vdemeest@redhat.com

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• Docs included if any changes are user facing
• Tests included if any functionality added or changed
• Follows the commit message standard
• Meets the Tekton contributor standards (including
  functionality, content, code)
• Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Fix Task(Run)/Pipeline(Run) metadata propagation,  If an annotation or
label is present in both Pipeline and PipelineRun, the value in the
Run type takes precedence (same for Task and TaskRun)

[vdemeester]

/hold
Needs a small update of the docs and a test update.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	83.6%	-0.3
pkg/reconciler/taskrun/taskrun.go	80.5%	80.0%	-0.5

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	83.9%	0.1
pkg/reconciler/taskrun/taskrun.go	80.5%	80.6%	0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	83.9%	0.1
pkg/reconciler/taskrun/taskrun.go	80.5%	80.6%	0.1

[vdemeester]

We don't have a doc for the same behavior on labels, should we rewrite this one to be labels and annotations ?
/cc @pritidesai @afrittoli @bobcatfish

[pritidesai]

yup indeed, lets be specific and document labels and annotations both. Also the doc can be updated to include an example!

[Tomcli]

+1

[pritidesai]

@Tomcli @rafalbigaj please confirm this new behavior for your use cases 🙏

[Tomcli]

For Kubeflow pipeline on Tekton, we were expecting PipelineRun labels/annotations to take precedences over its child resources. @rafalbigaj I believe that was your expected behavior as well?

[vdemeester]

/hold cancel

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	83.9%	0.1
pkg/reconciler/taskrun/taskrun.go	80.5%	80.6%	0.1

[vdemeester]

/retest

[vdemeester]

/retest

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	83.9%	0.1
pkg/reconciler/taskrun/taskrun.go	80.5%	80.6%	0.1

[vdemeester]

/retest

[pritidesai]

one last copy past issue here 🤣

[pritidesai]

thank you @vdemeester 👍

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.9%	83.9%	0.1
pkg/reconciler/taskrun/taskrun.go	80.5%	80.6%	0.1

[jerop]

thanks @vdemeester 😀

/lgtm

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, pritidesai

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:

• OWNERS [jerop,pritidesai]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hrivera-ntap]

@vdemeester : I noticed a side effect of this change that I was wondering if it was expected behavior.

It's stated that PipelineRuns take precedence over Pipeline labels and annotations (L&As) and TaskRuns take precedence over Task labels and annotations. In v0.34.1, I'm seeing that the TaskRuns that are created by the PipelineRuns inherit the L&As of the respective PipelineRun that created it, which has the side effect of overriding any of the explicit L&As that i set on the associated Tasks. The following is my general workflow.

TaskObj.Metadata.Labels["entity"] = "task"
PipelineObj.Metadata.Labels["entity"] = "pipeline"
PipelineObj.Spec.Tasks = [TaskObj]

PipelineRunObj.Metadata.Labels["entity"] = "pipelinerun"
PipelineRunObj.Spec.PipelineRef = PipelineObj

When i inspect the pod associated to the TaskRun that is produced in reference to TaskObj, I see that the pod has the "entity" label set to the value "pipelinerun", instead of "task".

I could probably code around this, but I wanted to confirm whether the cross-over metadata propagation between PipelineRun -> TaskRun was by design and was not overlooked as a potential bug.

Thanks!
-R

[vdemeester]

Hey @hrivera-ntap, yes this is by design, the value from the Run taks precedence over the one defined in the Task.