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.

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 3 bugs with results #2471

Merged
merged 4 commits into from
Apr 23, 2020
Merged

Conversation

bobcatfish
Copy link
Collaborator

@bobcatfish bobcatfish commented Apr 22, 2020

Changes

Okay it's one PR with 3 fixes BUT they are in different commits! And also - this is much more reasonable than my first attempt (#2465)

  • Me 🐱: I fixed it! Review my giant PR where I also moved params and results and touched nearly every file!
  • Vincent 😺: I think you can fix it without all that refactoring, maybe merge that first?
  • Me 😡: Then I will have to duplicate everything! NOOOOOOOO
  • Vincent 😺: Sometimes a bit of duplication is worth it
  • Me 😡: FINE I WILL TRY IT RELUCTANTLY
  • tries it
  • actually needs no additional duplication at all
  • Me 😳: ... perhaps I should have tried that first

Lesson: try the quicker fix first and THEN refactor

Okay so here are the 3 things I'm trying to address:

(To review separately plz look at the individual commits https://github.com/tektoncd/pipeline/pull/2471/commits 🙏 )

1. Use the same Deps logic in v1alpha1 and v1beta1

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 #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.

2. Allow results to contain underscores

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

3. Determine correct graph when params and results are mixed 🥄

This was partially addressed in #2387 however the example from the
bug (#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.

Fixes #2464

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:

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

Fix 3 bugs with results:
- Determine correct graph when results variable replacement is mixed with other types
- Allow results replacement to work with all valid characters
- Use the same graph logic when using embedded Pipeline specs vs referring to Pipeline CRDs

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
To make sure we don't accidentally skip the comparison between want and
got
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.
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.
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 22, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/pipeline_types.go 65.5% 69.0% 3.4
pkg/apis/pipeline/v1beta1/pipeline_types.go 82.4% 72.4% -9.9
pkg/apis/pipeline/v1beta1/pipeline_validation.go 92.2% 91.7% -0.5
pkg/apis/pipeline/v1beta1/resultref.go 93.9% 93.8% -0.2

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/meow
/cc @othomann

@tekton-robot tekton-robot requested a review from othomann April 23, 2020 06:43
@tekton-robot
Copy link
Collaborator

@vdemeester: cat image

In response to this:

/meow
/cc @othomann

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.

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2020
@vdemeester vdemeester added the kind/bug Categorizes issue or PR as related to a bug. label Apr 23, 2020
@vdemeester vdemeester added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Apr 23, 2020
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, vdemeester

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ghost
Copy link

ghost commented Apr 23, 2020

/lgtm

@tekton-robot tekton-robot assigned ghost Apr 23, 2020
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2020
@tekton-robot tekton-robot merged commit 4b1b761 into tektoncd:master Apr 23, 2020
bobcatfish added a commit to bobcatfish/pipeline that referenced this pull request Apr 23, 2020
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.
@bobcatfish
Copy link
Collaborator Author

Fixes #2463

tekton-robot pushed a commit that referenced this pull request Apr 23, 2020
In order to cherry pick in the fixes from #2393 and #2471, we first need
to apply the refactoring/changes these were built on in
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.
@ghost ghost removed the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Results regex should be the same as other substitution regexes
3 participants