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

Initial Implementation of conditionals #1093

Merged
merged 3 commits into from
Jul 30, 2019

Conversation

dibyom
Copy link
Member

@dibyom dibyom commented Jul 16, 2019

Changes

This commit adds an initial implementation for conditionals and depends on #1031.

In this implementation, condition evaluations aka ConditionChecks are
backed by TaskRuns. All conditionChecks associated with a PipelineTask
have to succeed before the task is executed. If a ConditionCheck fails,
the PipelineTask's associated TaskRun is marked failed i.e. its
Status.ConditionSucceeded is False. However, the PipelineRun itself
is not marked as failed.

Future PRs will add full support for passing in parameters, resource contents, and ability to read pipelinerun status from within the conditional container.

Part of #1019

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:

Release Notes

Adds a Condition type that can be used to conditionally execute tasks in a pipeline. Docs are in ./docs/condtions.md and ./docs/pipelines.md

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2019
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jul 16, 2019
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 16, 2019
@dibyom dibyom changed the title Conditional init Initial Implementation of conditionals Jul 16, 2019
@tekton-robot
Copy link
Collaborator

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/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/condition_validation.go Do not exist 87.5%
pkg/reconciler/v1alpha1/pipelinerun/controller.go Do not exist 100.0%
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go Do not exist 81.9%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 100.0%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 84.0% 80.6% -3.4
test/controller.go 36.2% 34.6% -1.6

@bobcatfish
Copy link
Collaborator

Pretty nice coverage boost! :D It looks like the coverage for test/builder/pipeline.go went down which I think means we are missing the test for what you've added to the builder (we do test the tests in this case XD). Would be nice to cover the additions to test/controller.go but that can be hard 😅

@m1093782566
Copy link

Nice work!
/cc

@tekton-robot
Copy link
Collaborator

@m1093782566: GitHub didn't allow me to request PR reviews from the following users: m1093782566.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Nice work!
/cc

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
Copy link
Collaborator

@dibyom let me know if you want me to review this now - I'm assuming we want to resolve the whole TaskRun question from #1031 before getting too deep into this one.

@dibyom dibyom changed the title Initial Implementation of conditionals WIP: Initial Implementation of conditionals Jul 22, 2019
@tekton-robot 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 Jul 22, 2019
@dibyom dibyom force-pushed the conditional_init branch 2 times, most recently from 3f316b0 to d519652 Compare July 22, 2019 19:11
@tekton-robot
Copy link
Collaborator

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/reconciler/v1alpha1/pipelinerun/controller.go Do not exist 100.0%
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go Do not exist 81.9%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 84.0% 83.1% -1.0
test/controller.go 36.2% 34.6% -1.6

dibyom added a commit to dibyom/pipeline that referenced this pull request Jul 22, 2019
This commit refactors the labels, annotations, default timeouts, and
service account fields from taskruns into helper functions. This will
allow us to easily reuse some common defaults for regular taskruns as
well as those created for conditionals in tektoncd#1093
@tekton-robot
Copy link
Collaborator

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/reconciler/v1alpha1/pipelinerun/controller.go Do not exist 100.0%
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go Do not exist 81.9%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 84.0% 83.1% -1.0
test/controller.go 36.2% 34.6% -1.6

dibyom added a commit to dibyom/pipeline that referenced this pull request Jul 22, 2019
This commit refactors the labels, annotations, default timeouts, and
service account fields from taskruns into helper functions. This will
allow us to easily reuse some common defaults for regular taskruns as
well as those created for conditionals in tektoncd#1093
dibyom added a commit to dibyom/pipeline that referenced this pull request Jul 23, 2019
This commit refactors the labels, annotations, default timeouts, and
service account fields from taskruns into helper functions. This will
allow us to easily reuse some common defaults for regular taskruns as
well as those created for conditionals in tektoncd#1093

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/pipeline that referenced this pull request Jul 23, 2019
This commit refactors the labels, annotations, default timeouts, and
service account fields from taskruns into helper functions. This will
allow us to easily reuse some common defaults for regular taskruns as
well as those created for conditionals in tektoncd#1093

Also, this replaces some of the TestReconcile_ funcs that were testing
only the functionality in these new helper functions with their own
dedicated unit tests

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/pipeline that referenced this pull request Jul 24, 2019
This commit refactors the labels, annotations, default timeouts, and
service account fields from taskruns into helper functions. This will
allow us to easily reuse some common defaults for regular taskruns as
well as those created for conditionals in tektoncd#1093

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/pipeline that referenced this pull request Jul 24, 2019
This commit refactors the labels, annotations, default timeouts, and
service account fields from taskruns into helper functions. This will
allow us to easily reuse some common defaults for regular taskruns as
well as those created for conditionals in tektoncd#1093

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
@dibyom dibyom changed the title WIP: Initial Implementation of conditionals Initial Implementation of conditionals Jul 24, 2019
@tekton-robot 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 Jul 24, 2019
@tekton-robot
Copy link
Collaborator

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/reconciler/v1alpha1/pipelinerun/controller.go Do not exist 100.0%
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go Do not exist 81.9%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 84.0% 83.1% -1.0
test/controller.go 36.2% 34.6% -1.6

@tekton-robot
Copy link
Collaborator

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/condition_types.go 0.0% 33.3% 33.3
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go 74.2% 82.0% 7.8
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 84.0% 83.1% -1.0
test/controller.go 36.2% 34.6% -1.6

@tekton-robot
Copy link
Collaborator

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/condition_types.go 0.0% 33.3% 33.3
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go 74.2% 82.0% 7.8
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 84.0% 83.1% -1.0
test/controller.go 36.2% 34.6% -1.6

@tekton-robot
Copy link
Collaborator

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/condition_types.go 0.0% 33.3% 33.3
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go 74.2% 82.0% 7.8
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 84.0% 83.1% -1.0
test/controller.go 36.2% 34.6% -1.6

@dibyom
Copy link
Member Author

dibyom commented Jul 29, 2019

/test pull-tekton-pipeline-build-tests

@dibyom
Copy link
Member Author

dibyom commented Jul 29, 2019

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2019
Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! I think all of my comments are pretty minor things about docs or adding comments, generally looks like it's pretty much ready to go! 🎉 🎉 🎉

test/builder/pipeline.go Show resolved Hide resolved
docs/conditions.md Outdated Show resolved Hide resolved
docs/conditions.md Outdated Show resolved Hide resolved
docs/conditions.md Outdated Show resolved Hide resolved
docs/pipelines.md Outdated Show resolved Hide resolved
if !cc.IsSuccessful() {
t.Fatal("Expected conditionCheck status to be done")
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice :D

c.Recorder.Eventf(pr, corev1.EventTypeWarning, "ConditionCheckCreationFailed", "Failed to create TaskRun %q: %v", rcc.ConditionCheckName, err)
return xerrors.Errorf("error creating ConditionCheck container called %s for PipelineTask %s from PipelineRun %s: %w", rcc.ConditionCheckName, rprt.PipelineTask.Name, pr.Name, err)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll play around with this some more, but what do you think about refactoring this in a follow-up?

Sounds good to me :D Thanks for looking into it :)


func (e *ConditionNotFoundError) Error() string {
return fmt.Sprintf("Couldn't retrieve Condition %q: %s", e.Name, e.Msg)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResolvePipelineRun definitely does a lot (e.g. resolving tasks, taskruns resources, and now conditions and checks). I'm open to ideas on simplifying/modularizing this. At the moment though, we need this in order to build the full PipelineRunState in one call which is then used by the reconciler.

kk maybe we can refactor it in follow up PRs! For me the most appealing option is to have multiple Resolve functions, each resolving a different type of thing, each being called by the Reconciler - then the Reconciler will know what kind of error message to set cuz it will know what kind of thing its trying to reconcile. Not super important tho!

@@ -113,6 +114,32 @@ func TestPipelineRun(t *testing.T) {
// 1 from PipelineRun and 1 from Tasks defined in pipelinerun
expectedNumberOfEvents: 2,
pipelineRunFunc: getHelloWorldPipelineRun,
}, {
name: "pipeline succeeds when task skipped due to failed condition",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding this! i think i derped a little bit when I suggested this be an end to end test vs. an example: i though the pipelinerun would end up failing (therefore wouldnt work as an example) but i forgot that a failing condition actually doesnt affect the success of the PipelineRun 🤦‍♀ anyway tho i think either is fine :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this led me down a rabbit hole which led to #1130 which I think was what finally fixed the issue where creating and using a Condition immediately would always fail with a NotFound error. So, definitely worth it 👼

@tekton-robot
Copy link
Collaborator

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/condition_types.go 0.0% 33.3% 33.3
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go 80.9% 82.3% 1.4
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 83.8% 82.8% -1.0
test/controller.go 36.2% 34.6% -1.6

@tekton-robot
Copy link
Collaborator

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/condition_types.go 0.0% 33.3% 33.3
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go 80.9% 82.3% 1.4
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 83.8% 82.8% -1.0
test/controller.go 36.2% 34.6% -1.6

@tekton-robot
Copy link
Collaborator

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/condition_types.go 0.0% 33.3% 33.3
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go 80.9% 82.3% 1.4
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 83.8% 86.1% 2.3
test/controller.go 36.2% 34.6% -1.6

In this implementation, condition evaluations aka ConditionChecks are
backed by TaskRuns. All conditionChecks associated with a `PipelineTask`
have to succeed before the task is executed. If a ConditionCheck fails,
the PipelineTask's associated TaskRun is marked failed i.e. its
`Status.ConditionSucceeded` is False. However, the PipelineRun itself
is not marked as failed.

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
Instead of modifying the passed in pipelinerun's status, return the
status to be updated. Also, updates the multiple status tests with
conditionChecks to a single one with multiple inputs
@tekton-robot
Copy link
Collaborator

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/condition_types.go 0.0% 33.3% 33.3
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go 80.9% 82.3% 1.4
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 83.8% 86.1% 2.3
test/controller.go 36.2% 34.6% -1.6

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
@tekton-robot
Copy link
Collaborator

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/condition_types.go 0.0% 33.3% 33.3
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go 80.9% 82.3% 1.4
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Do not exist 76.7%
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go 90.2% 90.5% 0.3
test/builder/pipeline.go 83.8% 86.1% 2.3
test/controller.go 36.2% 34.6% -1.6

Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great to me! Thanks for responding to everything @dibyom 🙏🙏🙏

/lgtm
/approve
/meow space


This document defines `Conditions` and their capabilities.

*NOTE*: This feature is currently a WIP being tracked in [#1137](https://github.com/tektoncd/pipeline/issues/1137)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

PipelineTaskLabelKey = "/pipelineTask"

// ConditionCheck is used as the label identifier for a ConditionCheck
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding these!!

@tekton-robot
Copy link
Collaborator

@bobcatfish: cat image

In response to this:

This is looking great to me! Thanks for responding to everything @dibyom 🙏🙏🙏

/lgtm
/approve
/meow space

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.

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, dibyom

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2019
@tekton-robot tekton-robot merged commit f880ffa into tektoncd:master Jul 30, 2019
@dibyom dibyom deleted the conditional_init branch July 31, 2019 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants