Apply replacements in tasks and finally tasks separately #3244
Conversation
tekton-robot
added
release-note-none
|
I am still mostly convinced that a "functional" side effect free approach is the long term way to go, however trying to mix the approaches here has definitely caused some problems. |
|
In the long run I think we should decide whether to always mutate or always return brand new instances and be consistent about it; in the short term I think it's good to remove the append() from this function since the differences in behavior between when it makes a new slice and when it doesn't are pretty extreme so I think we should go ahead with this PR: /lgtm Want to hear @pritidesai 's thoughts too tho before we merge! |
Previously, we created a slice containing both tasks and finally tasks then applied replacements across all of them. However, the variable replacements we make in conditions and when expressions in tasks don't apply to finally tasks. Moreover, this joined slice caused strange memory allocation side effects when finally tasks were present vs when they were missing.
jerop
force-pushed
the
separate-finally
branch
from
4cc7f03 to
931a2a6
Compare
|
this looks great, yes, consistency is must, let's evaluate this further in a separate PR while refactoring |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pritidesai 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
|
TestTaskRunTimeout is flakey maybe? 🤔 /test pull-tekton-pipeline-integration-tests |
|
/test pull-tekton-pipeline-integration-tests |
Changes
Previously, we created a slice containing both tasks and finally tasks then applied replacements across all of them. However, the variable replacements we make in conditions and when expressions in tasks don't apply to finally tasks. Moreover, this joined slice caused strange memory allocation side effects when finally tasks were present vs when they were missing.
We explored reverting the mutation of when expressions when applying replacements, as discussed in #3217 (comment) but decided to remove that because it introduces inconsistencies as discussed in #3244 (comment) and #3244 (comment).
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.
Release Notes
/cc @pritidesai @bobcatfish
/kind cleanup