Release v0.11.x results cherry pick #2477
Conversation
In order to cherry pick in the fixes from tektoncd#2393 and tektoncd#2471, we first need to apply the refactoring/changes these were built on in tektoncd@a8946d4#diff-e2b4c96e5145aa139a97f864d9f9f2a1 but unfortunately this both refactored/changed the code, AND added pipeline results. Since we don't want to add a new feature in a patch release, this commit extracts the refactoring (and fix?) from that commit.
(cherry picked from commit b817a12) Removed changes from the cherry picked commit that used PipelineResults
Sometimes the PipelineTask that Deps is being executed on is actually v1beta1 instead of v1alpha1 and the old Deps function, which doesn't account for Results, is being called. This PR duplicates the logics so the Deps function is the same. After tektoncd#2410 we'll be able to assume we're always using the v1beta1 types and will not need the logic in both places and can avoid bugs like this. It also duplicates the DAG test logic which is the closest thing Deps() currently has to a set of unit tests. Part of tektoncd#2463 (cherry picked from commit 893dde2)
To make sure we don't accidentally skip the comparison between want and got (cherry picked from commit 7653dd1)
When results parsing was implemented, the set of allowed characters wasn't made to match the rest of our substitutions. This duplicates the set of characters though hopefully we can eventually consolidate this logic. I discovered this when trying to use the kaniko task in the catalog https://github.com/tektoncd/catalog/blob/v1beta1/kaniko/kaniko.yaml Part of tektoncd#2464. (cherry picked from commit fc2bf79)
This was partially addressed in tektoncd#2387 however the example from the bug (tektoncd#2387) was using runAfter, and it should be possible to determine the correct graph without runAfter. The logic that was determining the graph was returning an error if any of the substitutions present in a string along with a result were not results. Now we will instead ignore these substitutions. We might want to revisit the validation to make sure that invalid results substitutions are still caught. (cherry picked from commit 4b1b761) Updated test logic that used builder which were added / changed in other commits on master.
tekton-robot
added
the
size/XXL
(cherry picked from commit e939852) Had to remove usage of ConditionRegisterName from apply_test which hadn't been added in this release branch.
bobcatfish
force-pushed
the
release-v0.11.x-results-cherry-pick
branch
from
3d234cf
to
e827190
Compare
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbwsg 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
|
So, if I was a pain in the *** I would say, we shouldn't push feature in a bugfix release 😝. If we were in a |
|
/lgtm |
@vdemeester which feature do you mean? (want to make sure I get this right one we are 1.0+) I think the only potential "feature" this patch is support for results replacement in conditions - it could be argued that was a bug as well depending on how you look at it, but if that's the feature you're talking about i see what you mean. (i would have been willing to go back and try harder to removed it also, should have included that as an option 🤔 ) But if that's not what you mean, I don't think this includes any (other) features? |
|
@bobcatfish ah lol, my bad. I thought this added a new field in the API — I made that assumption and I browse the source code too quickly to figure out that wasn't the case. Forget I've said anything. |
|
haha kk, no worries @vdemeester :D 🤣 |
Changes
This PR "cherry picks" several fixes for results into the release branch:
I put air quotes around cherry pick because in order to pull in the fixes without adding more features (specifically pipelineresults), I had to manually update some of the cherry picked commits and in one case create my own (if #2465 wasn't enough to teach me the value of separating refactoring from feature/bug changes then this exercise certainly was!) Because so much of this was manual, it's a bit iffy, but I feel good about our existing test coverage to make sure I didn't make anything worse. However if we don't feel good about adding this to the release, our other options I think are:
cc @vdemeester @othomann
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