implementing TEP-0150 - `displayName` in `childReferences` #7683

pritidesai · 2024-02-16T23:54:44Z

Changes

Implementing proposal TEP-0150.

Adding a functionality where the existing displayName can be used to configure unique names for each instance using the params from each combination such that the matrix instances in the Tekton Dashboard are rendered to distinguishable names.

Also, repurposing an existing name field as part of the matrix.include[].name. If specified, a fully resolved name field is available in the childReferences.

Fixes #7082

/kind feature

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
pre-commit Passed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

A fully resolved displayName is now available in childReferences along with the pipelineTaskName. This is mainly beneficial to parameterize and easily distinguish matrix instances of the task.

tekton-robot · 2024-02-17T00:00:07Z

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/resources/pipelinerunstate.go	99.5%	99.6%	0.1

tekton-robot · 2024-02-17T00:03:37Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

tekton-robot · 2024-02-17T00:32:37Z

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/resources/pipelinerunstate.go	99.5%	99.6%	0.1

tekton-robot · 2024-02-17T00:35:23Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

docs/matrix.md

tekton-robot · 2024-02-22T17:16:36Z

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/resources/apply.go	98.7%	98.4%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

tekton-robot · 2024-02-22T17:19:20Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.7%	98.4%	-0.3
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

tekton-robot · 2024-02-22T18:05:45Z

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/resources/apply.go	98.7%	98.7%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

tekton-robot · 2024-02-22T18:08:24Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.7%	98.7%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

afrittoli · 2024-02-23T10:37:58Z

/test pull-tekton-pipeline-integration-tests

pkg/reconciler/pipelinerun/pipelinerun_test.go

pritidesai · 2024-02-26T17:21:50Z

The display name here (and in the next ones) seems to be related to how a specific matrix combination was extended through the matrix.include section. Some are only extended with common-package so they all that name. the the common-package include did not exists, would they have an empty displayName?

Thanks @afrittoli! That is correct. The displayName in childReferences will have the matrix.include[].name only for the combinations which were added as part of that include otherwise the displayName is not initialized. For the names from multiple include sections, we are concatenating them with a space. Any specific comment/feedback on that?

afrittoli · 2024-02-28T14:58:39Z

The display name here (and in the next ones) seems to be related to how a specific matrix combination was extended through the matrix.include section. Some are only extended with common-package so they all that name. the the common-package include did not exists, would they have an empty displayName?

Thanks @afrittoli! That is correct. The displayName in childReferences will have the matrix.include[].name only for the combinations which were added as part of that include otherwise the displayName is not initialized. For the names from multiple include sections, we are concatenating them with a space. Any specific comment/feedback on that?

Thanks @pritidesai. I think concatenation makes sense. As I mentioned, I find the current matrix API a bit hard to parse, but that's not something for this to address. Given the current matrix API, the proposed display name makes sense. Perhaps we should have one case in the test list where display name remains empty to check that case too?

pritidesai · 2024-02-28T17:17:03Z

Thanks @pritidesai. I think concatenation makes sense. As I mentioned, I find the current matrix API a bit hard to parse, but that's not something for this to address. Given the current matrix API, the proposed display name makes sense. Perhaps we should have one case in the test list where display name remains empty to check that case too?

We have a variety of tests written under pipelinerunstate_test.go which include the case where the displayName remains empty when include params does not make it as part of the combinations:

https://github.com/tektoncd/pipeline/pull/7683/files#diff-22f0dfe65e555d6833c559a7a03613fa9cfec74ddf0962109a8b7ed810ccc850R4014-R4021

Let me know if we want to add more tests. Thanks!

tekton-robot · 2024-02-28T17:21:56Z

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/resources/apply.go	98.5%	98.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

tekton-robot · 2024-02-28T17:26:27Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.5%	98.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

pritidesai · 2024-02-28T18:11:22Z

/retest

Hey @AlanGreene @AverageMarcus, this is ready for review, PTAL! Thanks!

pritidesai · 2024-02-29T17:06:32Z

/retest

pritidesai · 2024-03-05T16:47:25Z

/retest

chitrangpatel · 2024-03-05T17:37:32Z

Pipeline WG: @AlanGreene @AverageMarcus PTAL again since we are hoping to get this merged this month.

AverageMarcus

/lgtm

tekton-robot · 2024-03-05T17:44:37Z

@AverageMarcus: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

AlanGreene

Pipeline WG: @AlanGreene @AverageMarcus PTAL again since we are hoping to get this merged this month.

The latest updates to the docs and examples look good based on our discussions and previous feedback, and as far as I can tell the test updates make sense too.

However, I'm not familiar enough with the codebase to determine whether the implementation has any potential impact on other features / functionality, in particular the changes in ApplyReplacements. A Pipelines maintainer or contributor would need to review and comment on that.

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

tekton-robot · 2024-03-05T18:27:58Z

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/resources/apply.go	98.5%	98.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

tekton-robot · 2024-03-05T18:30:10Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.5%	98.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

pritidesai · 2024-03-08T01:48:09Z

@tektoncd/core-maintainers please help review this PR. Thanks in advance!

pritidesai · 2024-03-11T16:48:30Z

This is a very simple change in adding human readable name in the childReferences. As an end user, we are waiting for this feature. Will appreciate if we can get the community to review on this PR. Thanks in advance!

pkg/apis/pipeline/v1beta1/pipelinerun_types.go

Implementing proposal TEP-0150. Adding a functionality where the existing displayName can be used to configure unique names for each instance using the params from each combination such that the matrix instances in the Tekton Dashboard are rendered to distinguishable names. Also, repurposing an existing name field as part of the matrix.include[].name. If specified, a fully resolved name field is available in the childReferences. Signed-off-by: Priti Desai <pdesai@us.ibm.com>

tekton-robot · 2024-03-11T18:28:38Z

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/apis/pipeline/v1beta1/pipelinerun_conversion.go	95.7%	95.7%	0.0
pkg/reconciler/pipelinerun/resources/apply.go	98.5%	98.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

chitrangpatel · 2024-03-11T18:30:22Z

/approve

tekton-robot · 2024-03-11T18:30:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chitrangpatel

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~~ [chitrangpatel]

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

tekton-robot · 2024-03-11T18:31:26Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/pipelinerun_conversion.go	95.7%	95.7%	0.0
pkg/reconciler/pipelinerun/resources/apply.go	98.5%	98.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.6%	0.1

afrittoli

Thanks @pritidesai for the extra test and updates.
I only left a NIT comment, but nothing blocking really - maybe something for another PR.

/lgtm

docs/matrix.md

afrittoli · 2024-03-15T12:41:53Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go

+				TypeMeta: runtime.TypeMeta{
+					APIVersion: "tekton.dev/v1",
+					Kind:       "TaskRun",
+				},
+				Name:             "matrixed-task-run-2",
+				PipelineTaskName: "matrixed-task",


afrittoli · 2024-03-15T12:47:43Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

@@ -240,8 +242,53 @@ func (facts *PipelineRunFacts) GetChildReferences() []v1.ChildStatusReference {
 	return childRefs
 }

+func (t *ResolvedPipelineTask) getDisplayName(customRun *v1beta1.CustomRun, taskRun *v1.TaskRun, c v1.ChildStatusReference) v1.ChildStatusReference {


NIT: I find the name/structure of this function slightly confusing - it does not return the display name, instead it sets it on a ChildStatusReference that must be passed as input.

Since the ChildStatusReference is not used to build the displayName, I thought it would be better to have a getDisplayName function that returns a string - any particular reason for having it this way?

tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 16, 2024

tekton-robot requested review from bobcatfish and dibyom February 16, 2024 23:54

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 16, 2024

pritidesai force-pushed the displayName-in-matrix-single-api branch from bb37fc9 to b84b8a2 Compare February 17, 2024 00:27

AlanGreene reviewed Feb 17, 2024

View reviewed changes

docs/matrix.md Show resolved Hide resolved

pritidesai added this to the Pipelines v0.58 milestone Feb 21, 2024

pritidesai force-pushed the displayName-in-matrix-single-api branch from b84b8a2 to c6a0bed Compare February 22, 2024 17:11

pritidesai force-pushed the displayName-in-matrix-single-api branch from c6a0bed to d5256ea Compare February 22, 2024 18:00

afrittoli reviewed Feb 23, 2024

View reviewed changes

pkg/reconciler/pipelinerun/pipelinerun_test.go Show resolved Hide resolved

pritidesai force-pushed the displayName-in-matrix-single-api branch from d5256ea to 4fbb1e5 Compare February 28, 2024 17:17

AverageMarcus reviewed Mar 5, 2024

View reviewed changes

AlanGreene reviewed Mar 5, 2024

View reviewed changes

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go Outdated Show resolved Hide resolved

pritidesai force-pushed the displayName-in-matrix-single-api branch from 4fbb1e5 to 39daaf7 Compare March 5, 2024 18:22

This was referenced Mar 11, 2024

Add support for displayName from childReferences tektoncd/dashboard#3345

Merged

Support pipeline matrix feature tektoncd/dashboard#2410

Closed

chitrangpatel reviewed Mar 11, 2024

View reviewed changes

pkg/apis/pipeline/v1beta1/pipelinerun_types.go Show resolved Hide resolved

pritidesai force-pushed the displayName-in-matrix-single-api branch from 39daaf7 to a9f4b61 Compare March 11, 2024 18:22

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2024

JeromeJu self-assigned this Mar 15, 2024

afrittoli reviewed Mar 15, 2024

View reviewed changes

tekton-robot assigned afrittoli Mar 15, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2024

tekton-robot merged commit 62cddf7 into tektoncd:main Mar 15, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implementing TEP-0150 - `displayName` in `childReferences` #7683

implementing TEP-0150 - `displayName` in `childReferences` #7683

pritidesai commented Feb 16, 2024 •

edited

Loading

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 22, 2024

tekton-robot commented Feb 22, 2024

tekton-robot commented Feb 22, 2024

tekton-robot commented Feb 22, 2024

afrittoli commented Feb 23, 2024

pritidesai commented Feb 26, 2024

afrittoli commented Feb 28, 2024

pritidesai commented Feb 28, 2024

tekton-robot commented Feb 28, 2024

tekton-robot commented Feb 28, 2024

pritidesai commented Feb 28, 2024

pritidesai commented Feb 29, 2024

pritidesai commented Mar 5, 2024

chitrangpatel commented Mar 5, 2024

AverageMarcus left a comment

tekton-robot commented Mar 5, 2024

AlanGreene left a comment

tekton-robot commented Mar 5, 2024

tekton-robot commented Mar 5, 2024

pritidesai commented Mar 8, 2024

pritidesai commented Mar 11, 2024

tekton-robot commented Mar 11, 2024

chitrangpatel commented Mar 11, 2024

tekton-robot commented Mar 11, 2024

tekton-robot commented Mar 11, 2024

afrittoli left a comment

afrittoli Mar 15, 2024

afrittoli Mar 15, 2024

implementing TEP-0150 - displayName in childReferences #7683

implementing TEP-0150 - displayName in childReferences #7683

Conversation

pritidesai commented Feb 16, 2024 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 22, 2024

tekton-robot commented Feb 22, 2024

tekton-robot commented Feb 22, 2024

tekton-robot commented Feb 22, 2024

afrittoli commented Feb 23, 2024

pritidesai commented Feb 26, 2024

afrittoli commented Feb 28, 2024

pritidesai commented Feb 28, 2024

tekton-robot commented Feb 28, 2024

tekton-robot commented Feb 28, 2024

pritidesai commented Feb 28, 2024

pritidesai commented Feb 29, 2024

pritidesai commented Mar 5, 2024

chitrangpatel commented Mar 5, 2024

AverageMarcus left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 5, 2024

AlanGreene left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 5, 2024

tekton-robot commented Mar 5, 2024

pritidesai commented Mar 8, 2024

pritidesai commented Mar 11, 2024

tekton-robot commented Mar 11, 2024

chitrangpatel commented Mar 11, 2024

tekton-robot commented Mar 11, 2024

tekton-robot commented Mar 11, 2024

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Mar 15, 2024

Choose a reason for hiding this comment

afrittoli Mar 15, 2024

Choose a reason for hiding this comment

implementing TEP-0150 - `displayName` in `childReferences` #7683

implementing TEP-0150 - `displayName` in `childReferences` #7683

pritidesai commented Feb 16, 2024 •

edited

Loading