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

Inconsistency with how we use and parameterize a taskRef #1185

Closed
skaegi opened this issue Aug 12, 2019 · 11 comments · Fixed by #1725
Closed

Inconsistency with how we use and parameterize a taskRef #1185

skaegi opened this issue Aug 12, 2019 · 11 comments · Fixed by #1725
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.

Comments

@skaegi
Copy link
Contributor

skaegi commented Aug 12, 2019

In a Pipeline and PipelineRun we reference params, resources.inputs, and resources.outputs. In a Task and TaskRun we reference the same variables as inputs.params, inputs.resources, outputs.resources. This is a bit confusing and inconsistent. If we want to make a change here we better get going quick...

If we were to make a change here (and recognizing that it would be breaking) I would just want to use params, inputs, and outputs. e.g. pull params up as a child of the Task spec and make inputs and outputs an array of PipelineResources.

For example...

---
# from a Pipeline or PipelineRun
spec:
  tasks:
  - name: pipeline-task-1
    taskRef:
      name: my-task
    params:
    - name: my-param
      value: xyzzy
    resources:
      inputs:
      - name: workspace
        resource: my-repo
      outputs:
      - name: image
        resource: my-image
---
# from a TaskRun
spec:
  taskRef:
    name: my-task
  inputs:
    params:
    - name: my-param
      value: xyzzy    
    resources:
    - name: workspace
      resourceRef:
        name: my-repo
  outputs:
    resources:
    - name: image
      resourceRef:
        name: my-image
---
@bobcatfish
Copy link
Collaborator

This has been bothering me for a while now 😭

It is doubly bad b/c I'm pretty sure I did it....

@bobcatfish bobcatfish added maybe-next-milestone For consideration when planning the next milestone help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Aug 12, 2019
@dlorenc
Copy link
Contributor

dlorenc commented Aug 13, 2019

In a Pipeline and PipelineRun we reference params, resources.inputs, and resources.outputs. In a Task and TaskRun we reference the same variables as inputs.params, inputs.resources, outputs.resources. This is a bit confusing and inconsistent. If we want to make a change here we better get going quick...

I would also love to write params to disk somehow under /workspace/inputs/params for full consistency.

@vdemeester
Copy link
Member

@dlorenc what would be the benefit to write them to disk instead of passing via environment variable ? And then we are back to variable substitation and all-as-environment variables discussion 👼 (https://docs.google.com/document/d/1h_3vSApIsuiwGkrqSiegi4NVaYG4oVzBquGAhIN6qGM/edit#heading=h.api53thmnjwo)

@dlorenc
Copy link
Contributor

dlorenc commented Aug 13, 2019

@dlorenc what would be the benefit to write them to disk instead of passing via environment variable ? And then we are back to variable substitation and all-as-environment variables discussion 👼

I meant in addition to that method.

If we ever want tasks to be able to output parameters, then files would probably make sense for that, and then this would be consistent.

@bobcatfish bobcatfish added this to the Pipelines 0.8 🐱 milestone Sep 6, 2019
@bobcatfish bobcatfish removed the maybe-next-milestone For consideration when planning the next milestone label Sep 6, 2019
@bobcatfish
Copy link
Collaborator

One way of doing this in a backwards compatible way is to do this via the mutating webhook.

@bobcatfish bobcatfish removed this from the Pipelines 0.8 🐱 milestone Oct 3, 2019
@dlorenc
Copy link
Contributor

dlorenc commented Oct 7, 2019

@skaegi do you still want to pursue this or should we close it out?

@bobcatfish bobcatfish added this to the Pipelines 0.8 🐱 milestone Oct 7, 2019
@bobcatfish bobcatfish self-assigned this Oct 7, 2019
@bobcatfish
Copy link
Collaborator

I think it's important @dlorenc !

After looking into it a bit more, I think we should wait to see how #1273 plays out since that may change the way params are specified (and is a chance to make Pipeline + Task specification of params + resources more similar).

@vdemeester
Copy link
Member

/assign
This is related to #1526 and #1526, so I'll tackle this along the others 👼

@vdemeester
Copy link
Member

As of today (#1651), Task yamls for v1alpha2 look like the following:

spec:
  params:
     # params here
  resources:
    inputs:
      # inputs resources here
    outputs:
      # outputs resource here

I feel that, to close this issue, TaskRun should follow the same ? @bobcatfish @skaegi wdyt ?

@bobcatfish
Copy link
Collaborator

I agree @vdemeester let's do the same for TaskRun!

@vdemeester
Copy link
Member

Once #1725 is merged, we should be able to close this one. How it will be exposed is covered by #1526

bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 8, 2020
With tektoncd#1185 we resolved a lot
of confusion and inconsistency around how we nested things with
input/output, but we didn't update our developer docs :(

In tektoncd#2319 we pointed someone toward these docs and the person ended up
using the old syntax and being confused :(
tekton-robot pushed a commit that referenced this issue Apr 8, 2020
With #1185 we resolved a lot
of confusion and inconsistency around how we nested things with
input/output, but we didn't update our developer docs :(

In #2319 we pointed someone toward these docs and the person ended up
using the old syntax and being confused :(
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants