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

passedConstraints is confusing and non intuitive way of defining task dependency. #137

Closed
tejal29 opened this issue Oct 12, 2018 · 6 comments
Labels
area/user-study design This task is about creating and discussing a design

Comments

@tejal29
Copy link
Contributor

tejal29 commented Oct 12, 2018

Expected Behavior

A user should be able to understand task dependency in a pipeline easily when presented with a sample pipeline definition.

Actual Behavior

User did not understand what passedContraint meant. Follow up question were

  • "is there a failledConstraint` too?
  • Why do i specify my task dependency on inputSourceBindings

Steps to Reproduce the Problem

  1. As user to read a pipeline, see if can understand task dependency

Additional Info

@bobcatfish
Copy link
Collaborator

Discussed this a bit with @pivotal-nader-ziada and he pointed out that before we actually change this (or anything major) we should probably make sure that the signals we get are consistent across users, since users may find different things confusing/intuitive.

After discussion with @tejal29 + @aaron-prindle , I think we should make this Task about generating alternative designs, which we can compare and present to users to see what they find the least confusing.

Some ideas for alternatives:

  • Attach passed constraints to Tasks instead of Resources (though this may present an issue when a Task actually changes a Resource)
  • Different naming

(Alternatively it might help to have very solid examples and docs explaining why we need to have passedConstraints on resources)

@bobcatfish bobcatfish added the design This task is about creating and discussing a design label Oct 12, 2018
@bobcatfish
Copy link
Collaborator

Possible alternative names: requiredTasks, runAfter, providedBy, versionProvidedBy.

@bobcatfish
Copy link
Collaborator

@pivotal-nader-ziada based on our discussions in #168 I want to make sure we're on the same page about how passedConstraints will work. At the moment, this is the only way to create a graph.

For example, in this Pipeline, both Tasks will run simultaneously:

apiVersion: pipeline.knative.dev/v1alpha1
kind: Pipeline
metadata:
  name: pipeline
spec:
  tasks:
    - name: task-a         
      taskRef:
        name: task
    - name: task-b          
      taskRef:
        name: task

In this pipeline, the tasks will run 1. task-a, 2. task-b

apiVersion: pipeline.knative.dev/v1alpha1
kind: Pipeline
metadata:
  name: pipeline
spec:
  tasks:
    - name: task-a         
      taskRef:
        name: task
      inputSourceBindings:
        - key: workspace 
          resourceRef:
            name: someinput
    - name: task-b          
      taskRef:
        name: task
      inputSourceBindings:
        - key: workspace 
          resourceRef:
            name: someinput
          passedConstraints: [task-a]

If I wanted something more complex, e.g. start 1. Run task-a, 2. Run task-b1 and task-b2 simultaneously, I would have to do something like:

apiVersion: pipeline.knative.dev/v1alpha1
kind: Pipeline
metadata:
  name: pipeline
spec:
  tasks:
    - name: task-a         
      taskRef:
        name: task
      inputSourceBindings:
        - key: workspace 
          resourceRef:
            name: someinput
    - name: task-b1         
      taskRef:
        name: task
      inputSourceBindings:
        - key: workspace 
          resourceRef:
            name: someinput
          passedConstraints: [task-a]
    - name: task-b2         
      taskRef:
        name: task
      inputSourceBindings:
        - key: workspace 
          resourceRef:
            name: someinput
          passedConstraints: [task-a]

So in the current design, the only way to get Tasks to run in order is using passedConstraints.

This was all based on #39 which added passedConstraints and we ended up removing next and prev. We could potentially add next and/or prev back if we want to be able to express order without requiring inputs.

@pivotal-nader-ziada does this match your understanding, and your intentions in #39?

@nader-ziada
Copy link
Member

yes, this is exactly my understanding of how task dependency currently works

but I don't know why you would want to express order of tasks if inputs and outputs are not related, that should be up to the pipeline engine to optimize

@bobcatfish
Copy link
Collaborator

but I don't know why you would want to express order of tasks if inputs and outputs are not related, that should be up to the pipeline engine to optimize

I can't think of a good use case either, so maybe it's fine as is, and we can tackle this when/if we finally find one. (As long as it's an additive change to our API it wouldn't hurt anything.)

@bobcatfish bobcatfish added this to the 0.0.1 Alpha release milestone Nov 3, 2018
@nader-ziada
Copy link
Member

@bobcatfish @tejal29 how about renaming passedConstraint to dependsOn to make it more clear.

All it does is say the this input resource depends on another task

for example:

apiVersion: pipeline.knative.dev/v1alpha1
kind: Pipeline
metadata:
  name: kritis-pipeline
  namespace: default
spec:
  tasks:
  - name: unit-test-kritis          
    taskRef:
      name: make
    inputSourceBindings:
    - name: workspace
      resourceRef:
        name: kritis-resources-git
    params:
    - name: makeTarget
      value: test
  - name: push-kritis              
    taskRef:
      name: build-push
    inputSourceBindings:
    - name: workspace
      resourceRef:
        name: kritis-resources-git
      dependsOn: [unit-test-kritis]                 <- dependsOn to show dependency
    outputSourceBindings:
    - name: builtImage
      resourceRef:
        name: kritis-resources-image
    params:
    - name: pathToDockerfile
      value: deploy/Dockerfile
  - name: deploy-test-env          
    taskRef:
      name: deploy-with-helm
    inputSourceBindings:
    - name: workspace
      resourceRef:
        name: kritis-resources-git
    - name: builtImage
      resourceRef:
        name: kritis-resources-image
      dependsOn: [push-kritis]                       <- dependsOn to show dependency
    - name: testCluster
      resourceRef:
        name: kritistestcluster  
    params:
    - name: pathToHelmCharts
      value: kritis-charts

nader-ziada added a commit to nader-ziada/pipeline that referenced this issue Nov 19, 2018
 - Issue tektoncd#137
 - since the "passedConstraint" flag is for indicating that the task should
 use that input resource coming from the task(s) in the list,
 "providedBy" conveys the meaning better
knative-prow-robot pushed a commit that referenced this issue Nov 20, 2018
 - Issue #137
 - since the "passedConstraint" flag is for indicating that the task should
 use that input resource coming from the task(s) in the list,
 "providedBy" conveys the meaning better
chmouel pushed a commit to chmouel/tektoncd-pipeline that referenced this issue Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-study design This task is about creating and discussing a design
Projects
None yet
Development

No branches or pull requests

3 participants