-
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
Results require runAfter when embedding Pipeline spec #2463
Labels
kind/bug
Categorizes issue or PR as related to a bug.
Milestone
Comments
Working on a fix for this now /assign bobcatfish |
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this issue
Apr 22, 2020
Yesterday I thought I would just quickly update our existing build + deploy pipelinerun example to use a git task and a kaniko task instead of PipelineResources and I immediately started stumbling over some very strange bugs. I'm not sure I've gotten 100% to the bottm of why these bugs happen, but part of it seems to be that depending on whether a pipelinespec is embedded or not, sometimes the PipelineTask instances are v1beta1 and sometimes v1alpha1. The logic to construct the graph is intentionally using a "Task" interface instead of dealing with either of thse directly, so it seems that in order to work properly, we need to have the same logic for the Deps function for both. In the long run I hope we can use exactly the same code instead of duplicating it, but in the short term, now both functions are the same. To do this I had to make it so that both v1alpha1 and v1beta1 could use the same Params and Results types. I think this is a nice way to organize the code anyway, and as a bonus, moving Params into a package outside of v1alpha1 and v1beta1 will make it so that we can more easily embed Conditions as part of tektoncd#2079. Fixes tektoncd#2463
2 tasks
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this issue
Apr 22, 2020
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
tekton-robot
pushed a commit
that referenced
this issue
Apr 23, 2020
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. Part of #2463
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this issue
Apr 23, 2020
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)
Fixed in #2471 |
bobcatfish
added a commit
to bobcatfish/pipeline
that referenced
this issue
Apr 23, 2020
The fix for tektoncd#2463 made sure that we use the same Deps logic for both v1alpha1 and v1beta1 types. This DAG test is the closest thing that Deps has to unit test. I meant to include this test in tektoncd@45ef5d7 but forgot to add it!! This test would be only temporary since after tektoncd#2410 we won't need to duplicated any of this logic, but it seems useful to have in the meantime. (Or until we add unit tests for Deps!!)
2 tasks
tekton-robot
pushed a commit
that referenced
this issue
Apr 23, 2020
The fix for #2463 made sure that we use the same Deps logic for both v1alpha1 and v1beta1 types. This DAG test is the closest thing that Deps has to unit test. I meant to include this test in 45ef5d7 but forgot to add it!! This test would be only temporary since after #2410 we won't need to duplicated any of this logic, but it seems useful to have in the meantime. (Or until we add unit tests for Deps!!)
tekton-robot
pushed a commit
that referenced
this issue
Apr 23, 2020
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. Part of #2463 (cherry picked from commit 893dde2)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Expected Behavior
Embedding a Pipeline spec into a PipelineRun should not change the behaviour of results.
Actual Behavior
If you remove the
runAfter
clause from this example, you get an error caused by the controller mistakenly starting both Tasks simultaneously.Steps to Reproduce the Problem
Additional Info
Interestingly this doesnt happen when not embedding the pipelinespec, e.g. this other example does not have the same problem https://github.com/tektoncd/pipeline/blob/master/examples/v1beta1/pipelineruns/pipelinerun-results.yaml
The text was updated successfully, but these errors were encountered: