Use permanent errors in the pipelinerun reconciler #2798
Conversation
tekton-robot
added
kind/cleanup
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
The following is the coverage report on the affected files.
|
| if controller.IsPermanentError(reconcileErr) { | ||
| return controller.NewPermanentError(merr) | ||
| } |
There was a problem hiding this comment.
This seems funny, does it do anything? 😅
There was a problem hiding this comment.
We collect various errors into a multierror. But if the reconcile error was a permanent one, I want the final error returned to be permanent too, so that the key is not re-queued
afrittoli
force-pushed
the
isssues/2474-pe-pipelinerun
branch
from
82d0b05 to
bad6378
Compare
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
isssues/2474-pe-pipelinerun
branch
from
bad6378 to
ae00370
Compare
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
isssues/2474-pe-pipelinerun
branch
from
ae00370 to
190ad5f
Compare
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
isssues/2474-pe-pipelinerun
branch
from
190ad5f to
2d8ed37
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
1 similar comment
|
/test pull-tekton-pipeline-integration-tests |
|
All my feedback is suuuuper minor and I don't think it should block anything. This is great!! (side note: the flakiness level of the integration tests is really starting to worry me, im starting to be concerned there might be an actual problem since this is happening so much across so many different tests - ill add something to the productivity working group agenda) /lgtm |
|
In response to this:
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish 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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
| t.Fatalf("Expected an error to be returned by Reconcile, got nil instead") | ||
| } | ||
| if controller.IsPermanentError(reconcileError) != tc.permanentError { | ||
| t.Fatalf("Expected the error to be permanent: %v but got permanent: %v", tc.permanentError, controller.IsPermanentError(reconcileError)) |
There was a problem hiding this comment.
nittiest nit, since reconcileError == nil fatals, feels like this might be a bit less code:
if reconcileError := c.Reconciler.Reconcile(context.Background(), getRunName(tc.pipelineRun)); reconcileError != nil {
if controller.IsPermanentError(reconcileError) != tc.permanentError {
t.Fatalf("Expected the error to be permanent: %v but got perman...")
}
else {
t.Fatalf("Expected an error to be returned by Reconcile, got nil instead")
}maybe that's less clear tho
im definitely over polishing here!
There was a problem hiding this comment.
I thought in a test it would be more readable the way I wrote it :)
afrittoli
force-pushed
the
isssues/2474-pe-pipelinerun
branch
from
2d8ed37 to
043bea9
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-build-tests |
Instead of returning nil on error during reconcile, if the error is transient return it, so that the key is requeue. If the error is permanent return a permanent error, so that the key is not requeued. Partially addresses tektoncd#2474
afrittoli
force-pushed
the
isssues/2474-pe-pipelinerun
branch
from
043bea9 to
6760e96
Compare
|
The following is the coverage report on the affected files.
|
|
i think something might be really wrong with the integration tests /test pull-tekton-pipeline-integration-tests |
|
/lgtm |
|
/test pull-tekton-pipeline-integration-tests |
1 similar comment
|
/test pull-tekton-pipeline-integration-tests |
Changes
Instead of returning nil on error during reconcile, if the error
is transient return it, so that the key is requeue. If the error
is permanent return a permanent error, so that the key is not
requeued.
Partially addresses #2474
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
/kind cleanup
/cc @bobcatfish @vdemeester @mattmoor