Allow results to work when pipeline spec is embedded ⛏️ #2465
Conversation
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
googlebot
added
the
cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
size/XXL
|
The following is the coverage report on pkg/.
|
|
@bobcatfish: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/cc @othomann |
There was a problem hiding this comment.
Few remarks
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.
Yes. The sometimes v1alpha1 and sometimes v1beta1 part is a bit confusing right now, but should go away soon-ish (see below, and #2413 + #2410)
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 #2079.
I would rather go the duplication route for this (sorry 😓). The main reason is that with #2413 and #2410 fixed, the duplication won't be needed. Spliting the params in a separate package is adding more complexity than I feel it is worth I feel. Also this makes the review really hard there (I have trouble seeing what is the real fix in all those changes).
The issue is on PipelineTask, so fixing the issue should be only on PipelineTask. And a follow-up refactoring could do whatever but… doing both at the same time is.. tricky to review.
Also, I am not sure what is there to duplicate really. All the param types in v1alpha1 are just type alias to the v1beta1 for now, so anything that is defined in v1beta1 is also in v1alpha1.
// ParamSpec defines arbitrary parameters needed beyond typed inputs (such as
// resources). Parameter values are provided by users as inputs on a TaskRun
// or PipelineRun.
type ParamSpec = v1beta1.ParamSpec
// Param declares an ArrayOrString to use for the parameter called name.
type Param = v1beta1.Param
// ParamType indicates the type of an input parameter;
// Used to distinguish between a single string and an array of strings.
type ParamType = v1beta1.ParamType
// Valid ParamTypes:
const (
ParamTypeString ParamType = v1beta1.ParamTypeString
ParamTypeArray ParamType = v1beta1.ParamTypeArray
)
// AllParamTypes can be used for ParamType validation.
var AllParamTypes = v1beta1.AllParamTypes
// ArrayOrString is modeled after IntOrString in kubernetes/apimachinery:
// ArrayOrString is a type that can hold a single string or string array.
// Used in JSON unmarshalling so that a single JSON field can accept
// either an individual string or an array of strings.
type ArrayOrString = v1beta1.ArrayOrString
So in a gist, I would rather see a small PR that duplicates the code for the fix (in PipleineTask), and maybe reduce the duplication on that part in a follow-up and maybe later extract params in its own package, but I really don't see the value of it as of now – same apply for results.
| for _, param := range cond.Params { | ||
| expressions, ok := results.GetVarSubstitutionExpressionsForParam(param) | ||
| if ok { | ||
| resultRefs := v1beta1.NewResultRefs(expressions) | ||
| for _, resultRef := range resultRefs { | ||
| deps = append(deps, resultRef.PipelineTask) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // Add any dependents from task results | ||
| for _, param := range pt.Params { | ||
| expressions, ok := results.GetVarSubstitutionExpressionsForParam(param) | ||
| if ok { | ||
| resultRefs := results.NewResultRefs(expressions) | ||
| for _, resultRef := range resultRefs { | ||
| deps = append(deps, resultRef.PipelineTask) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the real fix, right ?
There was a problem hiding this comment.
This is the change to handle the dependencies.
|
Spoke with @vdemeester at length about this - gonna see if i can fix it without moving the params + results types into their own packages, but might still do that in a separate PR! |
| @@ -44,6 +44,7 @@ func (source *PipelineSpec) ConvertTo(ctx context.Context, sink *v1beta1.Pipelin | |||
| sink.Workspaces = source.Workspaces | |||
| sink.Description = source.Description | |||
| if len(source.Tasks) > 0 { | |||
| // todo: why is this failing? | |||
Changes
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 #2079.
Fixes #2463
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