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

Cannot assign params with task results to other task value #2387

Closed
r0bj opened this issue Apr 14, 2020 · 6 comments
Closed

Cannot assign params with task results to other task value #2387

r0bj opened this issue Apr 14, 2020 · 6 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug.

Comments

@r0bj
Copy link

r0bj commented Apr 14, 2020

Expected Behavior

I can assign params with task results to other task value:

      params:
      - name: arg
        value: "$(params.prefix):$(tasks.generate-suffix.results.suffix)"

Actual Behavior

Cannot assign params with task results to other task value. Error message is generated:

validation failed: invalid value: Invalid result reference expression: Must be of the form \"tasks.<taskName>.results.<resultName>\": spec.tasks.params.value

Steps to Reproduce the Problem

pipeline.yaml:

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  generateName: test-
spec:
  params:
  - name: prefix
    value: prefix
  pipelineSpec:
    params:
    - name: prefix
    tasks:
    - name: generate-suffix
      taskSpec:
        results:
        - name: suffix
        steps:
        - name: generate-suffix
          image: alpine
          script: |
            echo "suffix" > $(results.suffix.path)
    - name: do-something
      runAfter:
      - generate-suffix
      taskSpec:
        params:
        - name: arg
        steps:
        - name: do-something
          image: alpine
          script: |
            echo "$(params.arg)"
      params:
      - name: arg
        value: "$(params.prefix):$(tasks.generate-suffix.results.suffix)"

Executing this pipelinerun generates error message:

{"level":"warn","logger":"tekton.pipeline-controller","caller":"pipelinerun/pipelinerun.go:193","msg":"Failed to update PipelineRun status{error 25 0  admission webhook \"validation.webhook.pipeline.tekton.dev\" denied the request: validation failed: invalid value: Invalid result reference expression: Must be of the form \"tasks.<taskName>.results.<resultName>\": spec.tasks.params.value}","commit":"82e2758","knative.dev/controller":"pipeline-controller"}
{"level":"error","logger":"tekton.pipeline-controller","caller":"controller/controller.go:376","msg":"Reconcile error","commit":"82e2758","knative.dev/controller":"pipeline-controller","error":"1 error occurred:\n\t* admission webhook \"validation.webhook.pipeline.tekton.dev\" denied the request: validation failed: invalid value: Invalid result reference expression: Must be of the form \"tasks.<taskName>.results.<resultName>\": spec.tasks.params.value\n\n","stacktrace":"github.com/tektoncd/pipeline/vendor/knative.dev/pkg/controller.(*Impl).handleErr\n\tgithub.com/tektoncd/pipeline/vendor/knative.dev/pkg/controller/controller.go:376\ngithub.com/tektoncd/pipeline/vendor/knative.dev/pkg/controller.(*Impl).processNextWorkItem\n\tgithub.com/tektoncd/pipeline/vendor/knative.dev/pkg/controller/controller.go:362\ngithub.com/tektoncd/pipeline/vendor/knative.dev/pkg/controller.(*Impl).Run.func2\n\tgithub.com/tektoncd/pipeline/vendor/knative.dev/pkg/controller/controller.go:310"}

Additional Info

  • Kubernetes version:

    Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.4", GitCommit:"8d8aa39598534325ad77120c120a22b3a990b5ea", GitTreeState:"clean", BuildDate:"2020-03-12T23:41:24Z", GoVersion:"go1.14", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"15+", GitVersion:"v1.15.11-gke.1", GitCommit:"aa93664bafe74f3c051e021b27a64e28ac34ab40", GitTreeState:"clean", BuildDate:"2020-03-18T22:49:35Z", GoVersion:"go1.12.17b4", Compiler:"gc", Platform:"linux/amd64"}
  • Tekton Pipeline version:

    Output of tkn version or kubectl get pods -n tekton-pipelines -l app=tekton-pipelines-controller -o=jsonpath='{.items[0].metadata.labels.version}'

Client version: 0.8.0
Pipeline version: v0.11.1
@vdemeester
Copy link
Member

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 14, 2020
@ghost
Copy link

ghost commented Apr 14, 2020

I think the problem is here: https://github.com/tektoncd/pipeline/blob/master/pkg/apis/pipeline/v1alpha1/pipeline_validation.go#L143-L150

In this block we extract all variable substitutions: $(params.prefix) and $(tasks.generate-suffix.results.suffix). Then LooksLikeContainsResultRefs checks if any of those substitutions looks like a result substitution. tasks.generate-suffix.results.suffix does. So we then check if NewResultRefs parses for every substitution we extracted, returning an error if it doesn't. Here params.prefix will fail to parse because it isn't of the format tasks.<tName>.results.<rName>. So I think all we need to do here is filter out the non-result refs before attempting to call NewResultRefs on them?

@ghost ghost added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Apr 14, 2020
@othomann
Copy link
Contributor

will take a look at this.

othomann added a commit to othomann/pipeline that referenced this issue Apr 14, 2020
othomann added a commit to othomann/pipeline that referenced this issue Apr 14, 2020
othomann added a commit to othomann/pipeline that referenced this issue Apr 14, 2020
@othomann
Copy link
Contributor

@sbwsg this can be closed since #2393 has been merged. Thanks.

@vdemeester
Copy link
Member

/close

@tekton-robot
Copy link
Collaborator

@vdemeester: Closing this issue.

In response to this:

/close

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.

bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 22, 2020
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.

Also updated some tests so that they will actually compare want against
desired; previously if expression parsing failed this part would be
skipped and no error would be raised.
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 22, 2020
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 pushed a commit that referenced this issue Apr 23, 2020
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.
bobcatfish pushed a commit to bobcatfish/pipeline that referenced this issue Apr 23, 2020
(cherry picked from commit b817a12)

Removed changes from the cherry picked commit that used PipelineResults
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 23, 2020
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 pushed a commit that referenced this issue Apr 23, 2020
(cherry picked from commit b817a12)

Removed changes from the cherry picked commit that used PipelineResults
tekton-robot pushed a commit that referenced this issue Apr 23, 2020
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.

(cherry picked from commit 4b1b761)

Updated test logic that used builder which were added / changed
in other commits on master.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

4 participants