Skip to content

Commit

Permalink
Switch to PipelineRunStatus.TaskRuns containing pipeline task name too
Browse files Browse the repository at this point in the history
  • Loading branch information
@abayer
abayer committed Mar 1, 2019
1 parent 114470b commit 2a14d3a
Show file tree
Hide file tree
Showing 7 changed files with 87 additions and 94 deletions.
19 changes: 11 additions & 8 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,18 @@ type PipelineRunStatus struct {
// CompletionTime is the time the PipelineRun completed.
// +optional
CompletionTime *metav1.Time `json:"completionTime,omitempty"`
// map of TaskRun Status with the taskRun name as the key
// map of PipelineRunTaskRunStatus with the taskRun name as the key
// +optional
TaskRuns map[string]TaskRunStatus `json:"taskRuns,omitempty"`
// map of PipelineTask name keys to TaskRun name values
TaskRuns map[string]*PipelineRunTaskRunStatus `json:"taskRuns,omitempty"`
}

// PipelineRunTaskRunStatus contains the name of the PipelineRun+PipelineTask for this TaskRun and the TaskRun's Status
type PipelineRunTaskRunStatus struct {
// PipelineTaskName is the name of the PipelineRun+PipelineTask
PipelineTaskName string `json:"pipelineTaskName"`
// Status is the TaskRunStatus for the corresponding TaskRun
// +optional
TaskRunNames map[string]string `json:"taskRunNames,omitempty"`
Status *TaskRunStatus `json:"status,omitempty"`
}

var pipelineRunCondSet = duckv1alpha1.NewBatchConditionSet()
Expand All @@ -143,14 +149,11 @@ func (pr *PipelineRunStatus) GetCondition(t duckv1alpha1.ConditionType) *duckv1a
// InitializeConditions will set all conditions in pipelineRunCondSet to unknown for the PipelineRun
func (pr *PipelineRunStatus) InitializeConditions() {
if pr.TaskRuns == nil {
pr.TaskRuns = make(map[string]TaskRunStatus)
pr.TaskRuns = make(map[string]*PipelineRunTaskRunStatus)
}
if pr.StartTime.IsZero() {
pr.StartTime = &metav1.Time{time.Now()}
}
if pr.TaskRunNames == nil {
pr.TaskRunNames = make(map[string]string)
}
pipelineRunCondSet.Manage(pr).InitializeConditions()
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func TestInitializeConditions(t *testing.T) {
t.Fatalf("PipelineRun StartTime not initialized correctly")
}

p.Status.TaskRuns["fooTask"] = TaskRunStatus{}
p.Status.TaskRuns["fooTask"] = &PipelineRunTaskRunStatus{}

p.Status.InitializeConditions()
if len(p.Status.TaskRuns) != 1 {
Expand Down
43 changes: 32 additions & 11 deletions pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 9 additions & 4 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
}
pr.ObjectMeta.Labels[pipeline.GroupName+pipeline.PipelineLabelKey] = p.Name

pipelineState, taskRunNames, err := resources.ResolvePipelineRun(
pipelineState, err := resources.ResolvePipelineRun(
*pr,
func(name string) (v1alpha1.TaskInterface, error) {
return c.taskLister.Tasks(pr.Namespace).Get(name)
Expand Down Expand Up @@ -312,8 +312,6 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
return nil
}

pr.Status.TaskRunNames = taskRunNames

for _, rprt := range pipelineState {
err := taskrun.ValidateResolvedTaskResources(rprt.PipelineTask.Params, rprt.ResolvedTaskResources)
if err != nil {
Expand Down Expand Up @@ -377,7 +375,14 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
func updateTaskRunsStatus(pr *v1alpha1.PipelineRun, pipelineState []*resources.ResolvedPipelineRunTask) {
for _, rprt := range pipelineState {
if rprt.TaskRun != nil {
pr.Status.TaskRuns[rprt.TaskRun.Name] = rprt.TaskRun.Status
prtrs := pr.Status.TaskRuns[rprt.TaskRun.Name]
if prtrs == nil {
prtrs = &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: fmt.Sprintf("%s-%s", pr.Name, rprt.PipelineTask.Name),
}
}
prtrs.Status = &rprt.TaskRun.Status
pr.Status.TaskRuns[rprt.TaskRun.Name] = prtrs
}
}
}
Expand Down
23 changes: 13 additions & 10 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,16 +408,19 @@ func TestUpdateTaskRunsState(t *testing.T) {
tb.StepState(tb.StateTerminated(0)),
))

expectedTaskRunsStatus := make(map[string]v1alpha1.TaskRunStatus)
expectedTaskRunsStatus["test-pipeline-run-success-unit-test-1"] = v1alpha1.TaskRunStatus{
Steps: []v1alpha1.StepState{{
ContainerState: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{ExitCode: 0},
},
}},
Conditions: []duckv1alpha1.Condition{{
Type: duckv1alpha1.ConditionSucceeded,
}},
expectedTaskRunsStatus := make(map[string]*v1alpha1.PipelineRunTaskRunStatus)
expectedTaskRunsStatus["test-pipeline-run-success-unit-test-1"] = &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: "test-pipeline-run-unit-test-1",
Status: &v1alpha1.TaskRunStatus{
Steps: []v1alpha1.StepState{{
ContainerState: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{ExitCode: 0},
},
}},
Conditions: []duckv1alpha1.Condition{{
Type: duckv1alpha1.ConditionSucceeded,
}},
},
}
expectedPipelineRunStatus := v1alpha1.PipelineRunStatus{
TaskRuns: expectedTaskRunsStatus,
Expand Down
30 changes: 9 additions & 21 deletions pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package resources

import (
"fmt"
"strings"
"time"

"github.com/knative/build-pipeline/pkg/names"
Expand Down Expand Up @@ -176,21 +175,15 @@ func ResolvePipelineRun(
getResource resources.GetResource,
tasks []v1alpha1.PipelineTask,
providedResources map[string]v1alpha1.PipelineResourceRef,
) (PipelineRunState, map[string]string, error) {

taskRunNames := pipelineRun.Status.TaskRunNames
) (PipelineRunState, error) {

state := []*ResolvedPipelineRunTask{}
for i := range tasks {
pt := tasks[i]

ptName := fmt.Sprintf("%s-%s", pipelineRun.Name, pt.Name)
if taskRunNames[ptName] == "" {
taskRunNames[ptName] = getTaskRunName(taskRunNames, pipelineRun.Status.TaskRuns, ptName)
}
rprt := ResolvedPipelineRunTask{
PipelineTask: &pt,
TaskRunName: taskRunNames[ptName],
TaskRunName: getTaskRunName(pipelineRun.Status.TaskRuns, fmt.Sprintf("%s-%s", pipelineRun.Name, pt.Name)),
}

// Find the Task that this task in the Pipeline this PipelineTask is using
Expand All @@ -202,7 +195,7 @@ func ResolvePipelineRun(
t, err = getTask(pt.TaskRef.Name)
}
if err != nil {
return nil, nil, &TaskNotFoundError{
return nil, &TaskNotFoundError{
Name: pt.TaskRef.Name,
Msg: err.Error(),
}
Expand All @@ -211,20 +204,20 @@ func ResolvePipelineRun(
// Get all the resources that this task will be using, if any
inputs, outputs, err := getPipelineRunTaskResources(pt, providedResources)
if err != nil {
return nil, nil, fmt.Errorf("unexpected error which should have been caught by Pipeline webhook: %v", err)
return nil, fmt.Errorf("unexpected error which should have been caught by Pipeline webhook: %v", err)
}

spec := t.TaskSpec()
rtr, err := resources.ResolveTaskResources(&spec, t.TaskMetadata().Name, inputs, outputs, getResource)
if err != nil {
return nil, nil, &ResourceNotFoundError{Msg: err.Error()}
return nil, &ResourceNotFoundError{Msg: err.Error()}
}
rprt.ResolvedTaskResources = rtr

// Add this task to the state of the PipelineRun
state = append(state, &rprt)
}
return state, taskRunNames, nil
return state, nil
}

// ResolveTaskRuns will go through all tasks in state and check if there are existing TaskRuns
Expand All @@ -246,14 +239,9 @@ func ResolveTaskRuns(getTaskRun GetTaskRun, state PipelineRunState) error {
}

// getTaskRunName should return a unique name for a `TaskRun` if one has not already been defined, and the existing one otherwise.
func getTaskRunName(taskRunNames map[string]string, taskRunsStatus map[string]v1alpha1.TaskRunStatus, base string) string {
if taskRunNames[base] != "" {
return taskRunNames[base]
}

// This path is now deprecated, but is retained to prevent breaking existing PipelineRuns as of the addition of PipelineRunStatus.TaskRunNames
for k := range taskRunsStatus {
if strings.HasPrefix(k, base) {
func getTaskRunName(taskRunsStatus map[string]*v1alpha1.PipelineRunTaskRunStatus, base string) string {
for k, v := range taskRunsStatus {
if v.PipelineTaskName == base {
return k
}
}
Expand Down
51 changes: 12 additions & 39 deletions pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -545,25 +545,17 @@ func TestResolvePipelineRun(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "pipelinerun",
},
Status: v1alpha1.PipelineRunStatus{
TaskRunNames: map[string]string{},
},
}
// The Task "task" doesn't actually take any inputs or outputs, but validating
// that is not done as part of Run resolution
getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil }
getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return nil, nil }
getResource := func(name string) (*v1alpha1.PipelineResource, error) { return r, nil }

pipelineState, taskRunNames, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, p.Spec.Tasks, providedResources)
pipelineState, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, p.Spec.Tasks, providedResources)
if err != nil {
t.Fatalf("Error getting tasks for fake pipeline %s: %s", p.ObjectMeta.Name, err)
}
expectedTaskRunNames := map[string]string{
"pipelinerun-mytask1": "pipelinerun-mytask1-9l9zj",
"pipelinerun-mytask2": "pipelinerun-mytask2-mz4c7",
"pipelinerun-mytask3": "pipelinerun-mytask3-mssqb",
}
expectedState := PipelineRunState{{
PipelineTask: &p.Spec.Tasks[0],
TaskRunName: "pipelinerun-mytask1-9l9zj",
Expand Down Expand Up @@ -605,9 +597,6 @@ func TestResolvePipelineRun(t *testing.T) {
if d := cmp.Diff(pipelineState, expectedState, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" {
t.Errorf("Expected to get current pipeline state %v, but actual differed: %s", expectedState, d)
}
if d := cmp.Diff(taskRunNames, expectedTaskRunNames); d != "" {
t.Errorf("Expected to get TaskRunNames %v, but actual differed: %s", expectedTaskRunNames, d)
}
}

func TestResolvePipelineRun_PipelineTaskHasNoResources(t *testing.T) {
Expand All @@ -630,11 +619,8 @@ func TestResolvePipelineRun_PipelineTaskHasNoResources(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "pipelinerun",
},
Status: v1alpha1.PipelineRunStatus{
TaskRunNames: map[string]string{},
},
}
pipelineState, _, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, pts, providedResources)
pipelineState, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, pts, providedResources)
if err != nil {
t.Fatalf("Did not expect error when resolving PipelineRun without Resources: %v", err)
}
Expand Down Expand Up @@ -674,11 +660,8 @@ func TestResolvePipelineRun_TaskDoesntExist(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "pipelinerun",
},
Status: v1alpha1.PipelineRunStatus{
TaskRunNames: map[string]string{},
},
}
_, _, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, pts, providedResources)
_, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, pts, providedResources)
switch err := err.(type) {
case nil:
t.Fatalf("Expected error getting non-existent Tasks for Pipeline %s but got none", p.Name)
Expand Down Expand Up @@ -723,11 +706,8 @@ func TestResolvePipelineRun_ResourceBindingsDontExist(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "pipelinerun",
},
Status: v1alpha1.PipelineRunStatus{
TaskRunNames: map[string]string{},
},
}
_, _, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, tt.p.Spec.Tasks, providedResources)
_, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, tt.p.Spec.Tasks, providedResources)
if err == nil {
t.Fatalf("Expected error when bindings are in incorrect state for Pipeline %s but got none", p.Name)
}
Expand Down Expand Up @@ -775,11 +755,8 @@ func TestResolvePipelineRun_ResourcesDontExist(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "pipelinerun",
},
Status: v1alpha1.PipelineRunStatus{
TaskRunNames: map[string]string{},
},
}
_, _, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, tt.p.Spec.Tasks, providedResources)
_, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, tt.p.Spec.Tasks, providedResources)
switch err := err.(type) {
case nil:
t.Fatalf("Expected error getting non-existent Resources for Pipeline %s but got none", p.Name)
Expand Down Expand Up @@ -1076,16 +1053,18 @@ func TestResolvePipelineRun_withExistingTaskRuns(t *testing.T) {
Type: v1alpha1.PipelineResourceTypeGit,
},
}
taskrunStatus := map[string]v1alpha1.TaskRunStatus{}
taskrunStatus["pipelinerun-mytask-with-a-really-long-name-to-trigger-tru-9l9zj"] = v1alpha1.TaskRunStatus{}
taskrunStatus := map[string]*v1alpha1.PipelineRunTaskRunStatus{}
taskrunStatus["pipelinerun-mytask-with-a-really-long-name-to-trigger-tru-9l9zj"] = &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: "pipelinerun-mytask-with-a-really-long-name-to-trigger-truncation",
Status: &v1alpha1.TaskRunStatus{},
}

pr := v1alpha1.PipelineRun{
ObjectMeta: metav1.ObjectMeta{
Name: "pipelinerun",
},
Status: v1alpha1.PipelineRunStatus{
TaskRuns: taskrunStatus,
TaskRunNames: map[string]string{},
TaskRuns: taskrunStatus,
},
}

Expand All @@ -1095,13 +1074,10 @@ func TestResolvePipelineRun_withExistingTaskRuns(t *testing.T) {
getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return nil, nil }
getResource := func(name string) (*v1alpha1.PipelineResource, error) { return r, nil }

pipelineState, taskRunNames, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, p.Spec.Tasks, providedResources)
pipelineState, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, p.Spec.Tasks, providedResources)
if err != nil {
t.Fatalf("Error getting tasks for fake pipeline %s: %s", p.ObjectMeta.Name, err)
}
expectedTaskRunNames := map[string]string{
"pipelinerun-mytask-with-a-really-long-name-to-trigger-truncation": "pipelinerun-mytask-with-a-really-long-name-to-trigger-tru-9l9zj",
}
expectedState := PipelineRunState{{
PipelineTask: &p.Spec.Tasks[0],
TaskRunName: "pipelinerun-mytask-with-a-really-long-name-to-trigger-tru-9l9zj",
Expand All @@ -1119,7 +1095,4 @@ func TestResolvePipelineRun_withExistingTaskRuns(t *testing.T) {
if d := cmp.Diff(pipelineState, expectedState, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" {
t.Fatalf("Expected to get current pipeline state %v, but actual differed: %s", expectedState, d)
}
if d := cmp.Diff(taskRunNames, expectedTaskRunNames); d != "" {
t.Errorf("Expected to get TaskRunNames %v, but actual differed: %s", expectedTaskRunNames, d)
}
}

0 comments on commit 2a14d3a

Please sign in to comment.