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

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

Merged
merged 1 commit into from
Jul 3, 2024
+81 −0
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1322,6 +1322,20 @@ func propagatePipelineNameLabelToPipelineRun(pr *v1.PipelineRun) error {
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@khrm khrm Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should we even have PipelineLabelKey for resolvers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

var kind, name string
for _, param := range pr.Spec.PipelineRef.Params {
if param.Name == "kind" {
kind = param.Value.StringVal
}
if param.Name == "name" {
name = param.Value.StringVal
}
}
if kind == "pipeline" {
pr.ObjectMeta.Labels[pipeline.PipelineLabelKey] = name
}
default:
return fmt.Errorf("pipelineRun %s not providing PipelineRef or PipelineSpec", pr.Name)
}
Expand Down
67 changes: 67 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8244,6 +8244,73 @@ spec:
}
}

func TestReconciler_ReconcileKind_PipelineRunLabels(t *testing.T) {
names.TestingSeed()

pipelineName := "p-pipelinetask"
pipelineRunName := "pr-pipelinetask"

ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, `
metadata:
name: p-pipelinetask
namespace: foo
spec:
tasks:
- name: task1
taskRef:
name: mytask
`)}

prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, `
metadata:
name: pr-pipelinetask
namespace: foo
spec:
pipelineRef:
params:
- name: kind
value: pipeline
- name: name
value: p-pipelinetask
- name: namespace
value: foo
resolver: cluster
`)}

ts := []*v1.Task{{ObjectMeta: baseObjectMeta("mytask", "foo")}}

trs := []*v1.TaskRun{mustParseTaskRunWithObjectMeta(t,
taskRunObjectMeta(pipelineRunName+"-task1-xxyy", "foo", pipelineRunName, pipelineName, "task1", false),
`
spec:
serviceAccountName: test-sa
taskRef:
name: mytask
status:
conditions:
- reason: "done"
status: "True"
type: Succeeded
`)}

d := test.Data{
PipelineRuns: prs,
Pipelines: ps,
Tasks: ts,
TaskRuns: trs,
}
prt := newPipelineRunTest(t, d)
defer prt.Cancel()

actualPipelineRun, _ := prt.reconcileRun("foo", pipelineRunName, []string{}, false)
if actualPipelineRun.Labels == nil {
t.Fatalf("Pelinerun should have labels")
}
if v, ok := actualPipelineRun.Labels[pipeline.PipelineLabelKey]; !ok || v != pipelineName {
t.Fatalf("The expected name of the pipeline is %s, but the actual name is %s", pipelineName, v)
}
}

// newPipelineRunTest returns PipelineRunTest with a new PipelineRun controller created with specified state through data
// This PipelineRunTest can be reused for multiple PipelineRuns by calling reconcileRun for each pipelineRun
func newPipelineRunTest(t *testing.T, data test.Data) *PipelineRunTest {
Expand Down