Add support for `from` usage in Pipeline Conditions #1527

dibyom · 2019-11-05T20:45:46Z

Changes

Resources in Pipeline Conditions can now declare that they depend on the
output of previous tasks using the from clause. Using from in a
conditional resource implies ordering for the pipeline task i.e. if
task B has a condition (say, C) that takes in an output resource from
task A, task A will run first, followed by the conditional C, and then B

Fixes #1256

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the 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

Resources in Pipeline Conditions can now declare that they depend on the
output of previous tasks using the `from` clause.

tekton-robot · 2019-11-05T20:48:26Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/dag.go	100.0%	98.2%	-1.8
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.9%	98.0%	0.1

tekton-robot · 2019-11-06T16:16:17Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/dag.go	100.0%	98.1%	-1.9
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.9%	98.0%	0.1

tekton-robot · 2019-11-06T16:53:58Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.9%	98.0%	0.1

dibyom · 2019-11-12T20:13:11Z

/cc @sbwsg @afrittoli @bobcatfish @vdemeester I haven't fully kept up with all the cool new resource stuff but do you think adding support for the from syntax for conditions still makes sense in the new world?

ghost · 2019-11-12T20:17:15Z

Yes I think so! Supporting it for the existing pipeline resources implementation makes sense to me.

ghost · 2019-11-12T20:24:37Z

pkg/apis/pipeline/v1alpha1/dag_test.go

@@ -326,3 +337,82 @@ func TestBuild_Invalid(t *testing.T) {
 		})
 	}
 }
+
+func TestBuild_ConditionResources(t *testing.T) {
+	// a,b, c, d := regularTask


remove dead code?

Ahh, more like a outdated comment 😅

ghost · 2019-11-12T20:27:59Z

/lgtm

tekton-robot · 2019-11-12T22:16:40Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.9%	98.0%	0.1

dibyom · 2019-11-13T15:43:50Z

/test pull-tekton-pipeline-integration-tests

ghost · 2019-11-13T16:01:18Z

/lgtm

afrittoli

Thank you, this is really useful!

afrittoli · 2019-12-07T11:15:05Z

examples/pipelineruns/conditional-pipelinerun.yaml

 spec:
-  inputs:
+  outputs:


oh, I though we deprecated git pipeline resource as output, at least until we have an actual output resource - only holding off the LGTM to clarify this

Ahh, did not know that....I can switch the example to a GCS resource or something

afrittoli · 2019-12-07T11:19:28Z

pkg/apis/pipeline/v1alpha1/dag_test.go

+		Conditions: []v1alpha1.PipelineTaskCondition{cond2DependsOnB},
+	}
+
+	//  a   b   c


afrittoli · 2019-12-07T11:33:38Z

pkg/apis/pipeline/v1alpha1/pipeline_types.go

@@ -152,15 +152,6 @@ type PipelineDeclaredResource struct {
 	Type PipelineResourceType `json:"type"`
 }

-// PipelineConditionResource allows a Pipeline to declare how its DeclaredPipelineResources
-// should be provided to a Condition as its inputs.
-type PipelineConditionResource struct {


As far as I understand this was removed to make the logic in the code simpler, since you have to deal with PipelineTaskInputResource only; I wonder if we may need different behaviours for task input resources and condition resources, but we can still add this back if becomes necessary.

Yeah, I don't think we do at the moment; but if we do we can add this back later on. Right now, this is basically an unused/extra level of indirection

dibyom · 2019-12-09T17:45:01Z

Thanks for the review @afrittoli Looks like there have been some major changes to the DAG....so the rebase will need some work..I'll give that shot on Friday!

vdemeester · 2019-12-10T10:00:29Z

Thanks for the review @afrittoli Looks like there have been some major changes to the DAG....so the rebase will need some work..I'll give that shot on Friday!

I don't think they are that major 😅 but ping me if you need help 😝

vdemeester

Naive question, can I create a deadlock with that from ? Like refering to a task that has a from on the current task. Something like the following ?

tasks:
  - name: first-create-file
    taskRef:
      name: create-file
    resources:
      outputs:
        - name: workspace
          resource: source-repo
  - name: then-check
    conditions:
      - conditionRef: "file-exists"
        resources:
          - name: workspace
            resource: source-repo
            from: [then-again]
    taskRef:
      name: echo-hello
  - name: then-again
    resources:
      outputs:
        - name: workspace
          resource: source-repo
          from: [then-check]
    taskRef:
      name: echo-hello

Asking because I think we should try to prevent that from happening.

dibyom · 2020-01-09T18:33:25Z

Naive question, can I create a deadlock with that from ? Like refering to a task that has a from on the current task. Something like the following ?

I'd hope not 😅 I think we detect the cycles in the DAG...let me verify

dibyom · 2020-01-09T18:42:28Z

Tried this out:

apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: conditional-pipeline
spec:
  resources:
    - name: source-repo
      type: git
  params:
    - name: "path"
      default: "README.md"
  tasks:
   - name: task-1
     conditions:
       - conditionRef: "file-exists"
         params:
           - name: "path"
             value: "$(params.path)"
         resources:
           - name: workspace
             resource: source-repo
             from: [task-2]
     taskRef:
       name: create-readme-file-1
     resources:
       inputs:
       - name: workspace1
         resource: source-repo
       outputs:
       - name: workspace
         resource: source-repo
   - name: task-2
     taskRef:
      name: create-readme-file-1
     resources:
       inputs:
       - name: workspace1
         resource: source-repo
         from: [task-1]
       outputs:
       - name: workspace
         resource: source-repo

webhook validation failed:

invalid value: couldn't add link between task-2 and task-1: couldn't create link from task-1 to task-2: cycle detected: task-2 -> task-1 -> task-2: spec.tasks

dibyom · 2020-01-09T19:23:59Z

/test pull-tekton-pipeline-integration-tests

dibyom · 2020-01-09T19:56:10Z

/test pull-tekton-pipeline-build-tests

ghost · 2020-01-09T20:29:37Z

/approve

Resources in Pipeline Conditions can now declare that they depend on the output of previous tasks using the `from` clause. Using `from` in a conditional resource implies ordering for the pipeline task i.e. if task B has a condition (say, C) that takes in an output resource from task A, task A will run first, followed by the conditional C, and then B Signed-off-by: Dibyo Mukherjee <[email protected]>

dibyom · 2020-01-09T21:33:46Z

/test pull-tekton-pipeline-integration-tests

vdemeester

/lgtm

tekton-robot · 2020-01-10T08:32:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbwsg,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dibyom · 2020-01-10T15:39:38Z

/test pull-tekton-pipeline-integration-tests

dibyom · 2020-01-10T17:24:33Z

/test pull-tekton-pipeline-integration-tests

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Nov 5, 2019

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 5, 2019

tekton-robot requested review from bobcatfish and imjasonh November 5, 2019 20:46

dibyom changed the title ~~Add support for from usage in Pipeline Conditions~~ WIP: Add support for from usage in Pipeline Conditions Nov 5, 2019

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2019

dibyom force-pushed the cond-from branch from 5561d94 to c07acc8 Compare November 6, 2019 16:14

dibyom force-pushed the cond-from branch from c07acc8 to c8c2d73 Compare November 6, 2019 16:50

dibyom changed the title ~~WIP: Add support for from usage in Pipeline Conditions~~ Add support for from usage in Pipeline Conditions Nov 6, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 6, 2019

ghost reviewed Nov 12, 2019

View reviewed changes

tekton-robot assigned ghost Nov 12, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2019

dibyom force-pushed the cond-from branch from c8c2d73 to 551c7bb Compare November 12, 2019 22:14

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2019

afrittoli reviewed Dec 7, 2019

View reviewed changes

vdemeester added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2019

dibyom force-pushed the cond-from branch from 551c7bb to 96710af Compare January 7, 2020 21:29

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2020

dibyom force-pushed the cond-from branch from 96710af to ef9ea40 Compare January 7, 2020 22:15

vdemeester reviewed Jan 8, 2020

View reviewed changes

dibyom force-pushed the cond-from branch from ef9ea40 to 2785554 Compare January 9, 2020 18:44

dibyom removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2020

dibyom force-pushed the cond-from branch from 2785554 to 684d166 Compare January 9, 2020 21:01

vdemeester approved these changes Jan 10, 2020

View reviewed changes

tekton-robot assigned vdemeester Jan 10, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2020

tekton-robot merged commit 4b6a390 into tektoncd:master Jan 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `from` usage in Pipeline Conditions #1527

Add support for `from` usage in Pipeline Conditions #1527

dibyom commented Nov 5, 2019 •

edited

Loading

tekton-robot commented Nov 5, 2019

tekton-robot commented Nov 6, 2019

tekton-robot commented Nov 6, 2019

dibyom commented Nov 12, 2019

ghost commented Nov 12, 2019

ghost Nov 12, 2019

dibyom Nov 12, 2019

ghost commented Nov 12, 2019

tekton-robot commented Nov 12, 2019

dibyom commented Nov 13, 2019

ghost commented Nov 13, 2019

afrittoli left a comment

afrittoli Dec 7, 2019 •

edited

Loading

dibyom Dec 9, 2019

afrittoli Dec 7, 2019

afrittoli Dec 7, 2019

dibyom Dec 9, 2019

dibyom commented Dec 9, 2019

vdemeester commented Dec 10, 2019

vdemeester left a comment

dibyom commented Jan 9, 2020

dibyom commented Jan 9, 2020

dibyom commented Jan 9, 2020

dibyom commented Jan 9, 2020

ghost commented Jan 9, 2020

dibyom commented Jan 9, 2020

vdemeester left a comment

tekton-robot commented Jan 10, 2020

dibyom commented Jan 10, 2020

dibyom commented Jan 10, 2020

Add support for from usage in Pipeline Conditions #1527

Add support for from usage in Pipeline Conditions #1527

Conversation

dibyom commented Nov 5, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Nov 5, 2019

tekton-robot commented Nov 6, 2019

tekton-robot commented Nov 6, 2019

dibyom commented Nov 12, 2019

ghost commented Nov 12, 2019

ghost Nov 12, 2019

Choose a reason for hiding this comment

dibyom Nov 12, 2019

Choose a reason for hiding this comment

ghost commented Nov 12, 2019

tekton-robot commented Nov 12, 2019

dibyom commented Nov 13, 2019

ghost commented Nov 13, 2019

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Dec 7, 2019 • edited Loading

Choose a reason for hiding this comment

dibyom Dec 9, 2019

Choose a reason for hiding this comment

afrittoli Dec 7, 2019

Choose a reason for hiding this comment

afrittoli Dec 7, 2019

Choose a reason for hiding this comment

dibyom Dec 9, 2019

Choose a reason for hiding this comment

dibyom commented Dec 9, 2019

vdemeester commented Dec 10, 2019

vdemeester left a comment

Choose a reason for hiding this comment

dibyom commented Jan 9, 2020

dibyom commented Jan 9, 2020

dibyom commented Jan 9, 2020

dibyom commented Jan 9, 2020

ghost commented Jan 9, 2020

dibyom commented Jan 9, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jan 10, 2020

dibyom commented Jan 10, 2020

dibyom commented Jan 10, 2020

Add support for `from` usage in Pipeline Conditions #1527

Add support for `from` usage in Pipeline Conditions #1527

dibyom commented Nov 5, 2019 •

edited

Loading

afrittoli Dec 7, 2019 •

edited

Loading