-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Don't modify the lister cache's copy. #4180
Conversation
/kind bug Fixes: tektoncd#4179
I'm not thrilled at the idea of discarding the error here either, but I figured it was worth leaving that alone for this change... |
The following is the coverage report on the affected files.
|
Ok, if I'm being really honest, I'm not a fan of any aspect of how the pipeline is handling retries by messing with another controller's resource's status to recycle it instead of just creating new taskruns 😂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love some more systematic way to avoid this fwiw, identifying and fixing these by hand isn't going to scale. 🤔
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: imjasonh 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 |
@imjasonh There is one! 😉 The knative table testing framework checks for this, which I think has effectively eliminated this from our controllers, but Tekton never adopted that because it landed around/after the split 😞 It'd be a pretty big undertaking, which I probably can't sign up for, but I'd be happy to consult, and I'm sure the Knative folks would be willing to field questions, since most of Knative is using it. |
/lgtm |
/kind bug
Fixes: #4179
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes