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: refine error resean with invalid pipelinename in taskrunspecs #6957

Merged
merged 1 commit into from
Aug 16, 2023
Merged

fix: refine error resean with invalid pipelinename in taskrunspecs #6957

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
7 changes: 4 additions & 3 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,9 @@ const (
// ReasonInvalidWorkspaceBinding indicates that a Pipeline expects a workspace but a
// PipelineRun has provided an invalid binding.
ReasonInvalidWorkspaceBinding = "InvalidWorkspaceBindings"
// ReasonInvalidServiceAccountMapping indicates that PipelineRun.Spec.TaskRunSpecs[].TaskServiceAccountName is defined with a wrong taskName
ReasonInvalidServiceAccountMapping = "InvalidServiceAccountMappings"
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Couldn't this happen still ?
I understand that we were sending this error wrongly (as it could have bee n something different that invalid service account mapping, but.. could/would we have cases where issuing this error is still valid ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i kept this reason.

Copy link
Member

Choose a reason for hiding this comment

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

I think this reason related function is removed, and deprecated? I don't see a reason to keep it.
#3028

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed not used here. This commit will be deleted first, and if it is used again later, it will be added?

// ReasonInvalidTaskRunSpec indicates that PipelineRun.Spec.TaskRunSpecs[].PipelineTaskName is defined with
// a not exist taskName in pipelineSpec.
ReasonInvalidTaskRunSpec = "InvalidTaskRunSpecs"
Comment on lines +86 to +88
Copy link
Member

Choose a reason for hiding this comment

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

InvalidTaskRunSpecs seems a bit vague here, we are actually validating the pipeline task name from taskrunspec is defined in pipeline tasks. I'm not sure, I won't disagree if we keep this naming..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ValidateTaskRunSpecs function currently only validates the name, but there may be other validations in the future? InvalidTaskRunSpecs indicates a large error type, combined with the message field to indicate a specific error?

// ReasonParameterTypeMismatch indicates that the reason for the failure status is that
// parameter(s) declared in the PipelineRun do not have the some declared type as the
// parameters(s) declared in the Pipeline that they are supposed to override.
Expand Down Expand Up @@ -506,7 +507,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel

// Ensure that the TaskRunSpecs defined are correct.
if err := resources.ValidateTaskRunSpecs(pipelineSpec, pr); err != nil {
pr.Status.MarkFailed(ReasonInvalidServiceAccountMapping,
pr.Status.MarkFailed(ReasonInvalidTaskRunSpec,
"PipelineRun %s/%s doesn't define taskRunSpecs correctly: %s",
pr.Namespace, pr.Name, err)
return controller.NewPermanentError(err)
Expand Down
21 changes: 21 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 @@ -6884,6 +6884,24 @@ spec:
params:
- name: platform
value: linux
`),
parse.MustParseV1PipelineRun(t, `
metadata:
name: pipelinerun-with-invalid-taskrunspecs
namespace: foo
spec:
taskRunSpecs:
- metadata:
annotations:
env: test
pipelineTaskName: invalid-task-name
pipelineSpec:
tasks:
- name: pt0
taskSpec:
steps:
- image: foo:latest
serviceAccountName: test-sa
`),
}

Expand Down Expand Up @@ -6915,6 +6933,9 @@ spec:
}, {
name: "pipelinerun-matrix-param-invalid-type",
reason: ReasonInvalidMatrixParameterTypes,
}, {
name: "pipelinerun-with-invalid-taskrunspecs",
reason: ReasonInvalidTaskRunSpec,
},
}
for _, tc := range testCases {
Expand Down