Add createPipelineRun to Pipeline Controller #93
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tejal29 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 |
knative-prow-robot
added
size/L
tejal29
force-pushed
the
invoke_tasks
branch
3 times, most recently
from
bc36991 to
a903657
Compare
There was a problem hiding this comment.
nice!
should the PipelineRun just create the first taskRun and let the taskRun controller create the following ones if the first passes? it works also if all taskRun are created, but this will make the testRun controller more complicated since it will have to wait for the one taskRun to finish before creating a build for the next one
| } | ||
|
|
||
| // TODO fetch the taskruns status for this pipeline run. |
There was a problem hiding this comment.
should probably call createPipelineRun() here
| // TODO fetch the taskruns status for this pipeline run. | ||
|
|
||
| // TODO check status of tasks and update status of PipelineRuns | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (c *Reconciler) createPipelineRun(ctx context.Context, p *v1alpha1.Pipeline) error { |
There was a problem hiding this comment.
this func is actually not creating the pipelineRun, its creating the contents of the pipelineRun, so maybe we can add something to the name to make it more clear. Maybe createPipelineRunTaskRuns or something?
| } | ||
|
|
||
| func randomizeTaskRunName(t *v1alpha1.Task) string { | ||
| return util.AppendRandomString(t.Name) |
There was a problem hiding this comment.
can we just use the name of the pipeline in combination with the name of the task?
There was a problem hiding this comment.
i can add pipeline-run name in combination of name of task hoping we would a unique name for each pipeline-run.
| return util.AppendRandomString(t.Name) | ||
| } | ||
|
|
||
| func (c *Reconciler) getPipelineIfExists(pr *v1alpha1.PipelineRun) (*v1alpha1.Pipeline, error) { |
There was a problem hiding this comment.
no need for IfExists in the name
pkg/util/randstring.go
Outdated
| @@ -11,7 +11,7 @@ See the License for the specific language governing permissions and | |||
| limitations under the License. | |||
| */ | |||
|
|
|||
| package test | |||
| package util | |||
| if err != nil { | ||
| return errors.NewBadRequest(fmt.Sprintf("pipeline %q is not valid due to %v", p.Name, err)) | ||
| } | ||
| for _, t := range tasks { |
There was a problem hiding this comment.
check my comment in the review, should we create all taskRun or just the first one and taskRun controller can do the rest?
|
@pivotal-nader-ziada Thanks! I completely forgot the flow. For the initial demo we were targeting linear pipeline. However, in future, it should be able to run tasks in parallel. we probably need to have a method that should check if all inputs for a given task are available and then run. |
tejal29
force-pushed
the
invoke_tasks
branch
2 times, most recently
from
d810552 to
7e3ef32
Compare
|
looks good, but has conflicts now because of #97 Just one nit: can you remove the |
| if err != nil { | ||
| return err | ||
| } | ||
| c.PipelineClientSet.PipelineV1alpha1().TaskRuns(p.Namespace).Create(tr) |
There was a problem hiding this comment.
Can we move the this line inside the createTask func, also check for any errors coming back
In this commit, we are adding createPipelineRun to PipelineRun Controller. This method will be used in PipelineRun.reconcile in future when we detect the pipelineRun does not any taskruns for tasks in the pipeline.
In previous commit, the pipeline controller creates TaskRuns for all tasks in the PipelineSpec. I changed to only create one `TaskRun`. It creates a `TaskRun` for the first `Task` for which `TaskRun` does not exist.
|
@pivotal-nader-ziada This is ready for last look. Sorry, i messed the commit history. Instead of running |
|
/lgtm |
Sync master with latest commit
This addresses a part of #61.
In this commit, we are adding createPipelineRun to PipelineRun Controller.
This method will be used in PipelineRun.reconcile in future when we detect the pipelineRun
does not have tasksruns for tasks in the pipeline.