fix: the label is set to the wrong pipeline name #7051

0xff-dev · 2023-08-14T05:26:41Z

Changes

when PipelineRun uses ClusterResolver to parse the Pipeline, the name of the Pipeline set in the label is wrong.

/kind bug

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  annotations:
    tekton.dev/displayName: component-rbac-gen
    tekton.dev/platforms: linux/amd64
    tekton.dev/tags: component-rbac-gen
  creationTimestamp: "2023-08-14T02:52:12Z"
  generation: 1
  labels:
    description: A_PipelineRun
    tekton.dev/pipeline: component-rbac-gen-run
  name: component-rbac-gen-run
  namespace: kube-system
  resourceVersion: "2534051"
  uid: fe33bcc4-f28d-4171-93f9-87cc73ff8510
spec:
  pipelineRef:
    params:
    - name: kind
      value: pipeline
    - name: name
      value: component-rbac-gen
    - name: namespace
      value: default
    resolver: cluster

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
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

set the correct pipeline name in the pipelinerun's label.

linux-foundation-easycla · 2023-08-14T05:26:45Z

The committers listed above are authorized under a signed CLA.

✅ login: 0xff-dev / name: smoky (91b7499)

tekton-robot · 2023-08-14T05:26:50Z

Hi @0xff-dev. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

tekton-robot · 2023-08-14T05:35:03Z

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/pipelinerun.go	92.4%	91.9%	-0.4

0xff-dev · 2023-08-14T05:49:43Z

/kind bug

khrm · 2023-08-14T05:54:32Z

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -1169,7 +1169,13 @@ func propagatePipelineNameLabelToPipelineRun(pr *v1.PipelineRun) error {
 	case pr.Spec.PipelineSpec != nil:
 		pr.ObjectMeta.Labels[pipeline.PipelineLabelKey] = pr.Name
 	case pr.Spec.PipelineRef != nil && pr.Spec.PipelineRef.Resolver != "":
-		pr.ObjectMeta.Labels[pipeline.PipelineLabelKey] = pr.Name
+		// https://tekton.dev/docs/pipelines/cluster-resolver/#pipeline-resolution


This code caters to every resolver. So removing will mean no value for git-resolver, etc.

Sorry, didn't consider Git's resolver. If it is universal, then this PR should not be necessary.

Should we even have PipelineLabelKey for resolvers?

According to the documentation, resolver is a pod that parses some data according to the configuration. So, I don't know much about adding the PipelineName tag to the resolver.

For other resolvers, is it possible that they are not associated with a pipeline, so is it reasonable to not have this label?

Or we can determine if the kind field is a pipeline, if not, still use the default name of PipelineRun, otherwise, we can choose the correct name of the pipeline.

spec: pipelineRef: params: - name: kind value: pipeline

Yes, but it would seem like depending upon the resolver's param in the pipeline controllers code. So I am fine with not having this also.

@vdemeester What do you think? Do we need this or not?

I don't think we need this. cc @lbernick @abayer

Hmmm, I agree putting a pipelinerun name to tekton.dev/pipeline is a bit confusing. Today, this logic happens before remote resolution. I'm thinking if we can propagate this value after resolving the pipeline when using a resolver.

khrm

Can you also add a unit test case or test it via an existing test?

khrm

/ok-to-test

tekton-robot · 2023-08-14T07:39:06Z

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	92.4%	91.9%	-0.4

tekton-robot · 2023-08-15T05:52:35Z

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	92.4%	91.7%	-0.7

tekton-robot · 2023-08-15T05:53:59Z

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/pipelinerun.go	92.4%	91.7%	-0.7

0xff-dev · 2023-08-20T08:41:54Z

friendly ping @khrm .
Do I need to make any other changes?

khrm · 2023-08-21T06:29:54Z

I don't think so.
/kind bug

jerop

@0xff-dev thanks for the contribution! please add tests 🙏🏾

jerop · 2023-09-20T16:55:29Z

/assign

afrittoli · 2024-02-26T12:32:14Z

@0xff-dev thank you for your PR. Are you still interested in pursuing this? As @jerop mentioned, we would need unit tests to be added.

0xff-dev · 2024-02-26T12:47:59Z

@0xff-dev thank you for your PR. Are you still interested in pursuing this? As @jerop mentioned, we would need unit tests to be added.

I'm very sorry, I really didn't have time to work on it before, I've been working on it for the last couple days. Very sorry for not pushing for PR in time.

afrittoli · 2024-02-26T12:50:01Z

@0xff-dev thank you for your PR. Are you still interested in pursuing this? As @jerop mentioned, we would need unit tests to be added.

I'm very sorry, I really didn't have time to work on it before, I've been working on it for the last couple days. Very sorry for not pushing for PR in time.

@0xff-dev no need to be sorry, I didn't mean to put pressure 🙏
I'm just going through all open PRs to check which are active and which are not. Thank you again for your contribution!

0xff-dev · 2024-03-03T13:36:31Z

Hi @afrittoli The test case has been added, but I don't know how to test it locally, so I can only ask you to review the code and then test it through github action.

tekton-robot · 2024-03-03T13:39:31Z

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	91.8%	91.9%	0.1

tekton-robot · 2024-03-03T13:43:12Z

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/pipelinerun.go	91.8%	91.9%	0.1

pkg/reconciler/pipelinerun/pipelinerun_test.go

afrittoli

/approve

when PipelineRun uses ClusterResolver to parse the Pipeline, the name of the Pipeline set in the label is wrong. Signed-off-by: 0xff-dev <[email protected]>

tekton-robot · 2024-06-27T15:21:55Z

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	91.8%	91.9%	0.1

tekton-robot · 2024-06-27T15:33:43Z

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/pipelinerun.go	91.8%	91.9%	0.1

vdemeester

We may want a release note for this 🙏🏼

tekton-robot · 2024-07-02T12:15:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, 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:

~~OWNERS~~ [afrittoli,vdemeester]

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

0xff-dev · 2024-07-02T13:49:33Z

We may want a release note for this 🙏🏼

Done. Thanks for review.

vdemeester

/lgtm

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Aug 14, 2023

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 14, 2023

tekton-robot requested review from imjasonh and jerop August 14, 2023 05:26

khrm reviewed Aug 14, 2023

View reviewed changes

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 14, 2023

0xff-dev force-pushed the main branch from 613869c to 4083bd9 Compare August 15, 2023 05:46

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 15, 2023

tekton-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Aug 21, 2023

jerop 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 Sep 20, 2023

jerop reviewed Sep 20, 2023

View reviewed changes

tekton-robot assigned jerop Sep 20, 2023

jerop removed their assignment Nov 9, 2023

0xff-dev force-pushed the main branch from 4083bd9 to 8d92502 Compare March 3, 2024 13:33

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 3, 2024

afrittoli reviewed Jun 27, 2024

View reviewed changes

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

afrittoli reviewed Jun 27, 2024

View reviewed changes

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

fix: the label is set to the wrong pipeline name

91b7499

when PipelineRun uses ClusterResolver to parse the Pipeline, the name of the Pipeline set in the label is wrong. Signed-off-by: 0xff-dev <[email protected]>

0xff-dev force-pushed the main branch from 8d92502 to 91b7499 Compare June 27, 2024 15:12

vdemeester approved these changes Jul 2, 2024

View reviewed changes

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Jul 2, 2024

vdemeester reviewed Jul 3, 2024

View reviewed changes

tekton-robot assigned vdemeester Jul 3, 2024

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

tekton-robot merged commit ba7cb10 into tektoncd:main Jul 3, 2024
14 checks passed

fix: the label is set to the wrong pipeline name #7051

fix: the label is set to the wrong pipeline name #7051

Conversation

0xff-dev commented Aug 14, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

linux-foundation-easycla bot commented Aug 14, 2023 • edited Loading

tekton-robot commented Aug 14, 2023

tekton-robot commented Aug 14, 2023

0xff-dev commented Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khrm Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khrm left a comment

Choose a reason for hiding this comment

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Aug 14, 2023

tekton-robot commented Aug 15, 2023

tekton-robot commented Aug 15, 2023

0xff-dev commented Aug 20, 2023

khrm commented Aug 21, 2023 • edited Loading

jerop left a comment

Choose a reason for hiding this comment

jerop commented Sep 20, 2023

afrittoli commented Feb 26, 2024

0xff-dev commented Feb 26, 2024

afrittoli commented Feb 26, 2024

0xff-dev commented Mar 3, 2024

tekton-robot commented Mar 3, 2024

tekton-robot commented Mar 3, 2024

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 27, 2024

tekton-robot commented Jun 27, 2024

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 2, 2024

0xff-dev commented Jul 2, 2024 • edited Loading

vdemeester left a comment

Choose a reason for hiding this comment

0xff-dev commented Aug 14, 2023 •

edited

Loading

linux-foundation-easycla bot commented Aug 14, 2023 •

edited

Loading

0xff-dev commented Aug 14, 2023 •

edited

Loading

khrm Aug 14, 2023 •

edited

Loading

khrm commented Aug 21, 2023 •

edited

Loading

0xff-dev commented Jul 2, 2024 •

edited

Loading