refactoring PipelineRunState to simplify logic for retrieving next tasks #3254

pritidesai · 2020-09-17T23:58:11Z

Changes

Introducing PipelineRunFacts as a combination of PipelineRunState, DAG Graph, and finally Graph. This simplifies function definitions in pipelinerun.go without having to pass graphs around.

This simplifies our implementation of retrieving next tasks, which will be further simplified in a separate PR if we decide to go this route. Also, this new struct would help simplify GetPipelineConditionStatus a whole lot (in a follow up PR).

/cc @bobcatfish

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
Release notes block has been filled in or deleted (only if no user facing changes)

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

NONE

pritidesai · 2020-09-17T23:58:40Z

/kind cleanup

tekton-robot · 2020-09-17T23:59:51Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	75.4%	69.7%	-5.7

tekton-robot · 2020-09-18T00:03:20Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	75.4%	71.9%	-3.5

pritidesai · 2020-09-18T00:07:47Z

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 75.4% 71.9% -3.5

Adding a new function DAGExecutionQueue is bringing down the coverage here. We have this covered as part of pipelinerun_test.go but will add coverage here as well.

Introducing PipelineRunFacts as a combination of PipelineRunState, DAG Graph, and finally Graph. This simplifies function definitions without having to pass graphs to PipelineRunState attributes.

tekton-robot · 2020-09-19T00:00:17Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	75.4%	72.1%	-3.3

jerop

thank you for this much-needed refactor @pritidesai 😀

jerop · 2020-09-21T15:39:23Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

+type PipelineRunFacts struct {
+	State           PipelineRunState
+	TasksGraph      *dag.Graph
+	FinalTasksGraph *dag.Graph
+}
+


Given that PipelineRunState is a list of ResolvedPipelineRunTask...

pipeline/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

Line 33 in f412b11

type PipelineRunState []*ResolvedPipelineRunTask

...I was wondering if we could make that explicit by calling it ResolvedPipelineRunTasks then we can call this object PipelineRunState instead of PipelineRunFacts, what do you think?

@jerop I like it but given ResolvedPipelineRunTask is so long and differentiating it with the slice of ResolvedPipelineRunTask by just additional s might confuse readers.

we could rename ResolvedPipelineRunTask to ResolvedPipelineTask? 😜

yes yes, I like that especially given that we refer to it as PipelineTask instead of PipelineRunTask

thanks @jerop, will have that renaming going in a separate PR. Once this is merged, would like to get the refactoring of GetPipelineConditionStatus in 🙏

afrittoli · 2020-09-22T08:41:44Z

Is this refactor solving the issue that #2821 was aiming to?
I waited on #2821 until the finally work was merged, and then it slipped behind other priorities :D

pritidesai · 2020-09-22T16:58:22Z

Is this refactor solving the issue that #2821 was aiming to?
I waited on #2821 until the finally work was merged, and then it slipped behind other priorities :D

This is the refactoring to simplify retrieving execution queue with DAG tasks, DAG tasks with when expressions, and finally tasks with minimal code changes. These changes in turn simplify GetPipelineConditionStatus.

afrittoli · 2020-09-24T16:16:41Z

Is this refactor solving the issue that #2821 was aiming to?
I waited on #2821 until the finally work was merged, and then it slipped behind other priorities :D

This is the refactoring to simplify retrieving execution queue with DAG tasks, DAG tasks with when expressions, and finally tasks with minimal code changes. These changes in turn simplify GetPipelineConditionStatus.

Got it, thank you!

Combining PipelineRunState, DAG Graph, and finally Graph is the approach I was pursuing on #2821, but in my PR I did that in a different module, because the main goal there was to move handling of the DAG away from pipelinerun.go.
I'll try to figure out if this PR is compatible with that refactor, it may be that it works as an intermediate step towards it.

pritidesai · 2020-09-25T16:18:57Z

thanks @afrittoli yes would be great to slowly get to the point where its easy to implement and review new functionalities to the existing scheduling (one very exciting near term feature coming in from @jerop as part of continueAfterSkip)

ghost · 2020-10-01T17:20:34Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

@@ -137,29 +132,53 @@ func (state PipelineRunState) checkTasksDone(d *dag.Graph) bool {
 	return true
 }

+func (facts *PipelineRunFacts) CheckDAGTasksDone() bool {


nit: these three public funcs could use comments. Could be helpful for new devs not yet familiar with the distinction between Tasks and FinalTasks

tekton-robot · 2020-10-01T17:21:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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]

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

jerop

/lgtm

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Sep 17, 2020

tekton-robot requested review from afrittoli and bobcatfish September 17, 2020 23:58

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 17, 2020

pritidesai force-pushed the pipelinerunstate branch from bf93ec5 to f61ded3 Compare September 18, 2020 00:01

refactoring pipelinerunstate

f412b11

Introducing PipelineRunFacts as a combination of PipelineRunState, DAG Graph, and finally Graph. This simplifies function definitions without having to pass graphs to PipelineRunState attributes.

pritidesai force-pushed the pipelinerunstate branch from f61ded3 to f412b11 Compare September 18, 2020 23:58

jerop reviewed Sep 21, 2020

View reviewed changes

ghost approved these changes Oct 1, 2020

View reviewed changes

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

jerop reviewed Oct 2, 2020

View reviewed changes

tekton-robot assigned jerop Oct 2, 2020

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

tekton-robot merged commit 78f5479 into tektoncd:master Oct 2, 2020

pritidesai mentioned this pull request Oct 2, 2020

refactoring get condition status for pipeline method in pipelinerunstate.go #3325

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactoring PipelineRunState to simplify logic for retrieving next tasks #3254

refactoring PipelineRunState to simplify logic for retrieving next tasks #3254

pritidesai commented Sep 17, 2020 •

edited

Loading

pritidesai commented Sep 17, 2020

tekton-robot commented Sep 17, 2020

tekton-robot commented Sep 18, 2020

pritidesai commented Sep 18, 2020

tekton-robot commented Sep 19, 2020

jerop left a comment

jerop Sep 21, 2020

pritidesai Sep 21, 2020

pritidesai Sep 21, 2020

jerop Oct 1, 2020

pritidesai Oct 1, 2020

afrittoli commented Sep 22, 2020

pritidesai commented Sep 22, 2020 •

edited

Loading

afrittoli commented Sep 24, 2020

pritidesai commented Sep 25, 2020

ghost Oct 1, 2020

tekton-robot commented Oct 1, 2020

jerop left a comment

refactoring PipelineRunState to simplify logic for retrieving next tasks #3254

refactoring PipelineRunState to simplify logic for retrieving next tasks #3254

Conversation

pritidesai commented Sep 17, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

pritidesai commented Sep 17, 2020

tekton-robot commented Sep 17, 2020

tekton-robot commented Sep 18, 2020

pritidesai commented Sep 18, 2020

tekton-robot commented Sep 19, 2020

jerop left a comment

Choose a reason for hiding this comment

jerop Sep 21, 2020

Choose a reason for hiding this comment

pritidesai Sep 21, 2020

Choose a reason for hiding this comment

pritidesai Sep 21, 2020

Choose a reason for hiding this comment

jerop Oct 1, 2020

Choose a reason for hiding this comment

pritidesai Oct 1, 2020

Choose a reason for hiding this comment

afrittoli commented Sep 22, 2020

pritidesai commented Sep 22, 2020 • edited Loading

afrittoli commented Sep 24, 2020

pritidesai commented Sep 25, 2020

ghost Oct 1, 2020

Choose a reason for hiding this comment

tekton-robot commented Oct 1, 2020

jerop left a comment

Choose a reason for hiding this comment

pritidesai commented Sep 17, 2020 •

edited

Loading

pritidesai commented Sep 22, 2020 •

edited

Loading