From 1d38854a25dd9e735388646acaf873fe744cb6c7 Mon Sep 17 00:00:00 2001 From: Alexander Misdorp <6093240+Peaorl@users.noreply.github.com> Date: Tue, 25 Aug 2020 18:59:44 +0200 Subject: [PATCH] Introducing InternalTektonResultType as a ResultType In light of #3087 the need for a ResultType that is not exposed as a TaskRunResult or PipelineResourceResult arises. In #3087, a Step can emit a result indicating a Step timeout has occurred. This is a result that should not be exposed hence the need for a new ResultType called InternalTektonResultType. This commit ensures results of this type are filtered out. Introducing an InternalTektonResultType ensures a future proof solution to internal results that should not be exposed. Aside from the example in #3087, a present candidate is the result written out by a Step containing a "StartedAt" key. Currently this result is filtered out with a specific function. Marking it as an InternalTektonResultType now allows for this result to automatically be filtered out. Additionally this commit brings about refactoring (and sometimes renaming) of functions related to converting pod statuses to taskrun statuses from pkg/reconciler/taskrun/taskrun.go to pkg/pod/status/status.go. This is accompanied with moving unit test cases from taskrun_test.go to status_test.go. --- cmd/git-init/main.go | 4 +- cmd/imagedigestexporter/main.go | 4 +- pkg/apis/pipeline/v1beta1/resource_types.go | 6 +- pkg/apis/pipeline/v1beta1/task_types.go | 2 + .../pipeline/v1beta1/zz_generated.deepcopy.go | 10 +- pkg/entrypoint/entrypointer.go | 10 +- pkg/pod/status.go | 189 ++++++--- pkg/pod/status_test.go | 368 ++++++++++++++++-- pkg/reconciler/taskrun/taskrun.go | 66 +--- pkg/reconciler/taskrun/taskrun_test.go | 272 ------------- pkg/termination/parse.go | 21 +- pkg/termination/parse_test.go | 36 +- pkg/termination/write_test.go | 2 +- 13 files changed, 558 insertions(+), 432 deletions(-) diff --git a/cmd/git-init/main.go b/cmd/git-init/main.go index 9bc81dd7bc3..afbb43ab81b 100644 --- a/cmd/git-init/main.go +++ b/cmd/git-init/main.go @@ -62,7 +62,7 @@ func main() { { Key: "commit", Value: commit, - ResourceRef: v1beta1.PipelineResourceRef{ + ResourceRef: &v1beta1.PipelineResourceRef{ Name: resourceName, }, ResourceName: resourceName, @@ -70,7 +70,7 @@ func main() { { Key: "url", Value: fetchSpec.URL, - ResourceRef: v1beta1.PipelineResourceRef{ + ResourceRef: &v1beta1.PipelineResourceRef{ Name: resourceName, }, ResourceName: resourceName, diff --git a/cmd/imagedigestexporter/main.go b/cmd/imagedigestexporter/main.go index a33fd28f678..53db04d9a6a 100644 --- a/cmd/imagedigestexporter/main.go +++ b/cmd/imagedigestexporter/main.go @@ -67,7 +67,7 @@ func main() { Key: "digest", Value: digest.String(), ResourceName: imageResource.Name, - ResourceRef: v1beta1.PipelineResourceRef{ + ResourceRef: &v1beta1.PipelineResourceRef{ Name: imageResource.Name, }, }) @@ -75,7 +75,7 @@ func main() { Key: "url", Value: imageResource.URL, ResourceName: imageResource.Name, - ResourceRef: v1beta1.PipelineResourceRef{ + ResourceRef: &v1beta1.PipelineResourceRef{ Name: imageResource.Name, }, }) diff --git a/pkg/apis/pipeline/v1beta1/resource_types.go b/pkg/apis/pipeline/v1beta1/resource_types.go index 94225ebc66f..d63ab891d38 100644 --- a/pkg/apis/pipeline/v1beta1/resource_types.go +++ b/pkg/apis/pipeline/v1beta1/resource_types.go @@ -123,10 +123,10 @@ type PipelineResourceResult struct { Key string `json:"key"` Value string `json:"value"` ResourceName string `json:"resourceName,omitempty"` - // This field should be deprecated and removed in the next API version. + // The field ResourceName should be deprecated and removed in the next API version. // See https://github.com/tektoncd/pipeline/issues/2694 for more information. - ResourceRef PipelineResourceRef `json:"resourceRef,omitempty"` - ResultType ResultType `json:"type,omitempty"` + ResourceRef *PipelineResourceRef `json:"resourceRef,omitempty"` + ResultType ResultType `json:"type,omitempty"` } // ResultType used to find out whether a PipelineResourceResult is from a task result or not diff --git a/pkg/apis/pipeline/v1beta1/task_types.go b/pkg/apis/pipeline/v1beta1/task_types.go index 34fe2c3bb87..405527f8b32 100644 --- a/pkg/apis/pipeline/v1beta1/task_types.go +++ b/pkg/apis/pipeline/v1beta1/task_types.go @@ -26,6 +26,8 @@ const ( TaskRunResultType ResultType = "TaskRunResult" // PipelineResourceResultType default pipeline result value PipelineResourceResultType ResultType = "PipelineResourceResult" + // InternalTektonResultType default internal tekton result value + InternalTektonResultType ResultType = "InternalTektonResult" // UnknownResultType default unknown result type value UnknownResultType ResultType = "" ) diff --git a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go index 37733e74976..0f31575dc46 100644 --- a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go @@ -459,7 +459,11 @@ func (in *PipelineResourceRef) DeepCopy() *PipelineResourceRef { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PipelineResourceResult) DeepCopyInto(out *PipelineResourceResult) { *out = *in - out.ResourceRef = in.ResourceRef + if in.ResourceRef != nil { + in, out := &in.ResourceRef, &out.ResourceRef + *out = new(PipelineResourceRef) + **out = **in + } return } @@ -1612,7 +1616,9 @@ func (in *TaskRunStatusFields) DeepCopyInto(out *TaskRunStatusFields) { if in.ResourcesResult != nil { in, out := &in.ResourcesResult, &out.ResourcesResult *out = make([]PipelineResourceResult, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } if in.TaskRunResults != nil { in, out := &in.TaskRunResults, &out.TaskRunResults diff --git a/pkg/entrypoint/entrypointer.go b/pkg/entrypoint/entrypointer.go index 188344a8fe7..6c9ed4edbc4 100644 --- a/pkg/entrypoint/entrypointer.go +++ b/pkg/entrypoint/entrypointer.go @@ -102,8 +102,9 @@ func (e Entrypointer) Go() error { // *but* we write postfile to make next steps bail too. e.WritePostFile(e.PostFile, err) output = append(output, v1beta1.PipelineResourceResult{ - Key: "StartedAt", - Value: time.Now().Format(timeFormat), + Key: "StartedAt", + Value: time.Now().Format(timeFormat), + ResultType: v1beta1.InternalTektonResultType, }) return err @@ -114,8 +115,9 @@ func (e Entrypointer) Go() error { e.Args = append([]string{e.Entrypoint}, e.Args...) } output = append(output, v1beta1.PipelineResourceResult{ - Key: "StartedAt", - Value: time.Now().Format(timeFormat), + Key: "StartedAt", + Value: time.Now().Format(timeFormat), + ResultType: v1beta1.InternalTektonResultType, }) err := e.Runner.Run(e.Args...) diff --git a/pkg/pod/status.go b/pkg/pod/status.go index f7d9ad66625..b949348d46d 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -23,6 +23,7 @@ import ( "strings" "time" + "github.com/hashicorp/go-multierror" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/names" "github.com/tektoncd/pipeline/pkg/termination" @@ -97,89 +98,181 @@ func SidecarsReady(podStatus corev1.PodStatus) bool { } // MakeTaskRunStatus returns a TaskRunStatus based on the Pod's status. -func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev1.Pod, taskSpec v1beta1.TaskSpec) v1beta1.TaskRunStatus { +func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev1.Pod, taskSpec v1beta1.TaskSpec) (v1beta1.TaskRunStatus, error) { trs := &tr.Status if trs.GetCondition(apis.ConditionSucceeded) == nil || trs.GetCondition(apis.ConditionSucceeded).Status == corev1.ConditionUnknown { // If the taskRunStatus doesn't exist yet, it's because we just started running MarkStatusRunning(trs, v1beta1.TaskRunReasonRunning.String(), "Not all Steps in the Task have finished executing") } + // Complete if we did not find a step that is not complete, or the pod is in a definitely complete phase + complete := areStepsComplete(pod) || pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed + + if complete { + updateCompletedTaskRun(trs, pod) + } else { + updateIncompleteTaskRun(trs, pod) + } + trs.PodName = pod.Name trs.Steps = []v1beta1.StepState{} trs.Sidecars = []v1beta1.SidecarState{} + var stepStatuses []corev1.ContainerStatus + var sidecarStatuses []corev1.ContainerStatus for _, s := range pod.Status.ContainerStatuses { if IsContainerStep(s.Name) { - if s.State.Terminated != nil && len(s.State.Terminated.Message) != 0 { - message, time, err := removeStartInfoFromTerminationMessage(s) + stepStatuses = append(stepStatuses, s) + } else if isContainerSidecar(s.Name) { + sidecarStatuses = append(sidecarStatuses, s) + } + } + + var merr *multierror.Error + if err := setTaskRunStatusBasedOnStepStatus(logger, stepStatuses, &tr); err != nil { + merr = multierror.Append(merr, err) + } + + setTaskRunStatusBasedOnSidecarStatus(sidecarStatuses, trs) + + trs.TaskRunResults = removeDuplicateResults(trs.TaskRunResults) + + // Sort step states according to the order specified in the TaskRun spec's steps. + trs.Steps = sortTaskRunStepOrder(trs.Steps, taskSpec.Steps) + + return *trs, merr.ErrorOrNil() +} + +func setTaskRunStatusBasedOnStepStatus(logger *zap.SugaredLogger, stepStatuses []corev1.ContainerStatus, tr *v1beta1.TaskRun) *multierror.Error { + + trs := &tr.Status + var merr *multierror.Error + + for _, s := range stepStatuses { + if s.State.Terminated != nil && len(s.State.Terminated.Message) != 0 { + msg := s.State.Terminated.Message + + results, err := termination.ParseMessage(logger, msg) + if err != nil { + logger.Errorf("termination message could not be parsed as JSON: %v", err) + merr = multierror.Append(merr, err) + } else { + time, err := extractStartedAtTimeFromResults(results) + if err != nil { + logger.Errorf("error setting the start time of step %q in taskrun %q: %v", s.Name, tr.Name, err) + merr = multierror.Append(merr, err) + } + logger.Errorf("%s: %d", s.Name, len(results)) + taskResults, pipelineResourceResults, filteredResults := filterResultsAndResources(results) + if tr.IsSuccessful() { + trs.TaskRunResults = append(trs.TaskRunResults, taskResults...) + trs.ResourcesResult = append(trs.ResourcesResult, pipelineResourceResults...) + } + msg, err = createMessageFromResults(filteredResults) + logger.Errorf("%s: %d", s.Name, len(filteredResults)) if err != nil { - logger.Errorf("error setting the start time of step %q in taskrun %q: %w", s.Name, tr.Name, err) + logger.Errorf("%v", err) + err = multierror.Append(merr, err) + } else { + s.State.Terminated.Message = msg } if time != nil { s.State.Terminated.StartedAt = *time - s.State.Terminated.Message = message } } - trs.Steps = append(trs.Steps, v1beta1.StepState{ - ContainerState: *s.State.DeepCopy(), - Name: trimStepPrefix(s.Name), - ContainerName: s.Name, - ImageID: s.ImageID, - }) - } else if isContainerSidecar(s.Name) { - trs.Sidecars = append(trs.Sidecars, v1beta1.SidecarState{ - ContainerState: *s.State.DeepCopy(), - Name: TrimSidecarPrefix(s.Name), - ContainerName: s.Name, - ImageID: s.ImageID, - }) } + trs.Steps = append(trs.Steps, v1beta1.StepState{ + ContainerState: *s.State.DeepCopy(), + Name: trimStepPrefix(s.Name), + ContainerName: s.Name, + ImageID: s.ImageID, + }) } - // Complete if we did not find a step that is not complete, or the pod is in a definitely complete phase - complete := areStepsComplete(pod) || pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed + return merr - if complete { - updateCompletedTaskRun(trs, pod) - } else { - updateIncompleteTaskRun(trs, pod) +} + +func setTaskRunStatusBasedOnSidecarStatus(sidecarStatuses []corev1.ContainerStatus, trs *v1beta1.TaskRunStatus) { + for _, s := range sidecarStatuses { + trs.Sidecars = append(trs.Sidecars, v1beta1.SidecarState{ + ContainerState: *s.State.DeepCopy(), + Name: TrimSidecarPrefix(s.Name), + ContainerName: s.Name, + ImageID: s.ImageID, + }) } +} - // Sort step states according to the order specified in the TaskRun spec's steps. - trs.Steps = sortTaskRunStepOrder(trs.Steps, taskSpec.Steps) +func createMessageFromResults(results []v1beta1.PipelineResourceResult) (string, error) { + if len(results) == 0 { + return "", nil + } + bytes, err := json.Marshal(results) + if err != nil { + return "", fmt.Errorf("error marshalling remaining results back into termination message: %w", err) + } + return string(bytes), nil +} - return *trs +func filterResultsAndResources(results []v1beta1.PipelineResourceResult) ([]v1beta1.TaskRunResult, []v1beta1.PipelineResourceResult, []v1beta1.PipelineResourceResult) { + var taskResults []v1beta1.TaskRunResult + var pipelineResourceResults []v1beta1.PipelineResourceResult + var filteredResults []v1beta1.PipelineResourceResult + for _, r := range results { + switch r.ResultType { + case v1beta1.TaskRunResultType: + taskRunResult := v1beta1.TaskRunResult{ + Name: r.Key, + Value: r.Value, + } + taskResults = append(taskResults, taskRunResult) + filteredResults = append(filteredResults, r) + case v1beta1.InternalTektonResultType: + // Internal messages are ignored because they're not used as external result + continue + case v1beta1.PipelineResourceResultType: + fallthrough + default: + pipelineResourceResults = append(pipelineResourceResults, r) + filteredResults = append(filteredResults, r) + } + } + + return taskResults, pipelineResourceResults, filteredResults } -// removeStartInfoFromTerminationMessage searches for a result called "StartedAt" in the JSON-formatted -// termination message of a step and returns the values to use for sets State.Terminated if it's -// found. The "StartedAt" result is also removed from the list of results in the container status. -func removeStartInfoFromTerminationMessage(s corev1.ContainerStatus) (string, *metav1.Time, error) { - r, err := termination.ParseMessage(s.State.Terminated.Message) - if err != nil { - return "", nil, fmt.Errorf("termination message could not be parsed as JSON: %w", err) +func removeDuplicateResults(taskRunResult []v1beta1.TaskRunResult) []v1beta1.TaskRunResult { + if len(taskRunResult) == 0 { + return nil } - for index, result := range r { + + uniq := make([]v1beta1.TaskRunResult, 0) + latest := make(map[string]v1beta1.TaskRunResult, 0) + for _, res := range taskRunResult { + if _, seen := latest[res.Name]; !seen { + uniq = append(uniq, res) + } + latest[res.Name] = res + } + for i, res := range uniq { + uniq[i] = latest[res.Name] + } + return uniq +} + +func extractStartedAtTimeFromResults(results []v1beta1.PipelineResourceResult) (*metav1.Time, error) { + for _, result := range results { if result.Key == "StartedAt" { t, err := time.Parse(timeFormat, result.Value) if err != nil { - return "", nil, fmt.Errorf("could not parse time value %q in StartedAt field: %w", result.Value, err) + return nil, fmt.Errorf("could not parse time value %q in StartedAt field: %w", result.Value, err) } - message := "" startedAt := metav1.NewTime(t) - // remove the entry for the starting time - r = append(r[:index], r[index+1:]...) - if len(r) == 0 { - message = "" - } else if bytes, err := json.Marshal(r); err != nil { - return "", nil, fmt.Errorf("error marshalling remaining results back into termination message: %w", err) - } else { - message = string(bytes) - } - return message, &startedAt, nil + return &startedAt, nil } } - return "", nil, nil + return nil, nil } func updateCompletedTaskRun(trs *v1beta1.TaskRunStatus, pod *corev1.Pod) { diff --git a/pkg/pod/status_test.go b/pkg/pod/status_test.go index 11a6913315d..c34d8543798 100644 --- a/pkg/pod/status_test.go +++ b/pkg/pod/status_test.go @@ -33,13 +33,23 @@ import ( var ignoreVolatileTime = cmp.Comparer(func(_, _ apis.VolatileTime) bool { return true }) +var dummyTime, _ = time.Parse(timeFormat, "2012-11-01T22:08:41.000Z") +var dummyMetaTime = metav1.Time{dummyTime} + +var conditionRunning apis.Condition = apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionUnknown, + Reason: v1beta1.TaskRunReasonRunning.String(), + Message: "Not all Steps in the Task have finished executing", +} +var conditionSucceeded apis.Condition = apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + Reason: v1beta1.TaskRunReasonSuccessful.String(), + Message: "All Steps have completed executing", +} + func TestMakeTaskRunStatus(t *testing.T) { - conditionRunning := apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionUnknown, - Reason: v1beta1.TaskRunReasonRunning.String(), - Message: "Not all Steps in the Task have finished executing", - } for _, c := range []struct { desc string podStatus corev1.PodStatus @@ -605,6 +615,329 @@ func TestMakeTaskRunStatus(t *testing.T) { }, }, }, { + desc: "image resource updated", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-foo", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"digest","value":"sha256:12345","resourceRef":{"name":"source-image"}}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"digest","value":"sha256:12345","resourceRef":{"name":"source-image"}}]`, + }}, + Name: "foo", + ContainerName: "step-foo", + }}, + Sidecars: []v1beta1.SidecarState{}, + ResourcesResult: []v1beta1.PipelineResourceResult{{ + Key: "digest", + Value: "sha256:12345", + ResourceRef: &v1beta1.PipelineResourceRef{Name: "source-image"}, + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "test result with pipeline result", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-bar", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}},{"key":"resultName","value":"resultValue","type":"TaskRunResult"}]`, + }}, + Name: "bar", + ContainerName: "step-bar", + }}, + Sidecars: []v1beta1.SidecarState{}, + ResourcesResult: []v1beta1.PipelineResourceResult{{ + Key: "digest", + Value: "sha256:1234", + ResourceRef: &v1beta1.PipelineResourceRef{Name: "source-image"}, + }}, + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "resultName", + Value: "resultValue", + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "test result with pipeline result - no result type", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-banana", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}},{"key":"resultName","value":"resultValue","type":"TaskRunResult"}]`, + }}, + Name: "banana", + ContainerName: "step-banana", + }}, + Sidecars: []v1beta1.SidecarState{}, + ResourcesResult: []v1beta1.PipelineResourceResult{{ + Key: "digest", + Value: "sha256:1234", + ResourceRef: &v1beta1.PipelineResourceRef{Name: "source-image"}, + }}, + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "resultName", + Value: "resultValue", + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "two test results", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-one", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"resultValueOne", "type": "TaskRunResult"}, {"key":"resultNameTwo","value":"resultValueTwo", "type": "TaskRunResult"}]`, + }, + }, + }, { + Name: "step-two", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"resultValueThree","type":"TaskRunResult"},{"key":"resultNameTwo","value":"resultValueTwo","type":"TaskRunResult"}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"resultValueOne","type":"TaskRunResult"},{"key":"resultNameTwo","value":"resultValueTwo","type":"TaskRunResult"}]`, + }}, + Name: "one", + ContainerName: "step-one", + }, { + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"resultValueThree","type":"TaskRunResult"},{"key":"resultNameTwo","value":"resultValueTwo","type":"TaskRunResult"}]`, + }}, + Name: "two", + ContainerName: "step-two", + }}, + Sidecars: []v1beta1.SidecarState{}, + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "resultNameOne", + Value: "resultValueThree", + }, { + Name: "resultNameTwo", + Value: "resultValueTwo", + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "taskrun status set to failed if task fails", + podStatus: corev1.PodStatus{ + Phase: corev1.PodFailed, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-mango", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{Conditions: []apis.Condition{{ + Reason: "Failed", + Message: "build failed for unspecified reasons.", + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + }}}, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}}, + Name: "mango", + ContainerName: "step-mango", + }}, + Sidecars: []v1beta1.SidecarState{}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "termination message not adhering to pipelineresourceresult format is filtered from taskrun termination message", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-pineapple", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"invalid":"resultName","invalid":"resultValue"}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}}, + Name: "pineapple", + ContainerName: "step-pineapple", + }}, + Sidecars: []v1beta1.SidecarState{}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "filter internaltektonresult", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-pear", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"","type":"PipelineResourceResult"}, {"key":"resultNameTwo","value":"","type":"InternalTektonResult"}, {"key":"resultNameThree","value":"","type":"TaskRunResult"}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"","type":"PipelineResourceResult"},{"key":"resultNameThree","value":"","type":"TaskRunResult"}]`, + }}, + Name: "pear", + ContainerName: "step-pear", + }}, + Sidecars: []v1beta1.SidecarState{}, + ResourcesResult: []v1beta1.PipelineResourceResult{{ + Key: "resultNameOne", + Value: "", + ResultType: "PipelineResourceResult", + }}, + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "resultNameThree", + Value: "", + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }} { + t.Run(c.desc, func(t *testing.T) { + now := metav1.Now() + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + Namespace: "foo", + CreationTimestamp: now, + }, + Status: c.podStatus, + } + startTime := time.Date(2010, 1, 1, 1, 1, 1, 1, time.UTC) + tr := v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "task-run", + Namespace: "foo", + }, + Status: v1beta1.TaskRunStatus{ + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + StartTime: &metav1.Time{Time: startTime}, + }, + }, + } + + logger, _ := logging.NewLogger("", "status") + got, err := MakeTaskRunStatus(logger, tr, pod, c.taskSpec) + + // Common traits, set for test case brevity. + c.want.PodName = "pod" + c.want.StartTime = &metav1.Time{Time: startTime} + + ensureTimeNotNil := cmp.Comparer(func(x, y *metav1.Time) bool { + if x == nil { + return y == nil + } + return y != nil + }) + if err != nil { + t.Errorf("MakeTaskRunResult: %s", err) + } + if d := cmp.Diff(c.want, got, ignoreVolatileTime, ensureTimeNotNil); d != "" { + t.Errorf("Diff %s", diff.PrintWantGot(d)) + } + if tr.Status.StartTime.Time != c.want.StartTime.Time { + t.Errorf("Expected TaskRun startTime to be unchanged but was %s", tr.Status.StartTime) + } + }) + } +} + +func TestMakeRunStatusErrors(t *testing.T) { + for _, c := range []struct { + desc string + podStatus corev1.PodStatus + taskSpec v1beta1.TaskSpec + want v1beta1.TaskRunStatus + }{{ desc: "non-json-termination-message-with-steps-afterwards-shouldnt-panic", taskSpec: v1beta1.TaskSpec{ Steps: []v1beta1.Step{{Container: corev1.Container{ @@ -664,7 +997,6 @@ func TestMakeTaskRunStatus(t *testing.T) { ExitCode: 1, Message: "this is a non-json termination message. dont panic!", }}, - Name: "non-json", ContainerName: "step-non-json", ImageID: "image", @@ -694,34 +1026,24 @@ func TestMakeTaskRunStatus(t *testing.T) { }, }} { t.Run(c.desc, func(t *testing.T) { - now := metav1.Now() pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: "pod", - Namespace: "foo", - CreationTimestamp: now, + Name: "pod", + Namespace: "foo", }, Status: c.podStatus, } - startTime := time.Date(2010, 1, 1, 1, 1, 1, 1, time.UTC) tr := v1beta1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ Name: "task-run", Namespace: "foo", }, - Status: v1beta1.TaskRunStatus{ - TaskRunStatusFields: v1beta1.TaskRunStatusFields{ - StartTime: &metav1.Time{Time: startTime}, - }, - }, } logger, _ := logging.NewLogger("", "status") - got := MakeTaskRunStatus(logger, tr, pod, c.taskSpec) + got, err := MakeTaskRunStatus(logger, tr, pod, c.taskSpec) - // Common traits, set for test case brevity. c.want.PodName = "pod" - c.want.StartTime = &metav1.Time{Time: startTime} ensureTimeNotNil := cmp.Comparer(func(x, y *metav1.Time) bool { if x == nil { @@ -729,12 +1051,12 @@ func TestMakeTaskRunStatus(t *testing.T) { } return y != nil }) + if err == nil { + t.Error("Expected error, got nil") + } if d := cmp.Diff(c.want, got, ignoreVolatileTime, ensureTimeNotNil); d != "" { t.Errorf("Diff %s", diff.PrintWantGot(d)) } - if tr.Status.StartTime.Time != c.want.StartTime.Time { - t.Errorf("Expected TaskRun startTime to be unchanged but was %s", tr.Status.StartTime) - } }) } } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 24ff8fefbbb..fcc2736f01e 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -41,7 +41,6 @@ import ( "github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" - "github.com/tektoncd/pipeline/pkg/termination" "github.com/tektoncd/pipeline/pkg/timeout" "github.com/tektoncd/pipeline/pkg/workspace" corev1 "k8s.io/api/core/v1" @@ -185,7 +184,9 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg // Reconcile this copy of the task run and then write back any status // updates regardless of whether the reconciliation errored out. if err = c.reconcile(ctx, tr, taskSpec, rtr); err != nil { - logger.Errorf("Reconcile error: %v", err.Error()) + if merr, ok := err.(*multierror.Error); ok { + logger.Errorf("Reconcile: %v", merr.Error()) + } } // Emit events (only when ConditionSucceeded was changed) return c.finishReconcileUpdateEmitEvents(ctx, tr, before, err) @@ -397,9 +398,8 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, } // Convert the Pod's status to the equivalent TaskRun Status. - tr.Status = podconvert.MakeTaskRunStatus(logger, *tr, pod, *taskSpec) - - if err := updateTaskRunResourceResult(tr, *pod); err != nil { + tr.Status, err = podconvert.MakeTaskRunStatus(logger, *tr, pod, *taskSpec) + if err != nil { return err } @@ -627,62 +627,6 @@ func (c *Reconciler) createPod(ctx context.Context, tr *v1beta1.TaskRun, rtr *re type DeletePod func(podName string, options *metav1.DeleteOptions) error -func updateTaskRunResourceResult(taskRun *v1beta1.TaskRun, pod corev1.Pod) error { - podconvert.SortContainerStatuses(&pod) - - if taskRun.IsSuccessful() { - for idx, cs := range pod.Status.ContainerStatuses { - if cs.State.Terminated != nil { - msg := cs.State.Terminated.Message - r, err := termination.ParseMessage(msg) - if err != nil { - return fmt.Errorf("parsing message for container status %d: %v", idx, err) - } - taskResults, pipelineResourceResults := getResults(r) - taskRun.Status.TaskRunResults = append(taskRun.Status.TaskRunResults, taskResults...) - taskRun.Status.ResourcesResult = append(taskRun.Status.ResourcesResult, pipelineResourceResults...) - } - } - taskRun.Status.TaskRunResults = removeDuplicateResults(taskRun.Status.TaskRunResults) - } - return nil -} - -func getResults(results []v1beta1.PipelineResourceResult) ([]v1beta1.TaskRunResult, []v1beta1.PipelineResourceResult) { - var taskResults []v1beta1.TaskRunResult - var pipelineResourceResults []v1beta1.PipelineResourceResult - for _, r := range results { - switch r.ResultType { - case v1beta1.TaskRunResultType: - taskRunResult := v1beta1.TaskRunResult{ - Name: r.Key, - Value: r.Value, - } - taskResults = append(taskResults, taskRunResult) - case v1beta1.PipelineResourceResultType: - fallthrough - default: - pipelineResourceResults = append(pipelineResourceResults, r) - } - } - return taskResults, pipelineResourceResults -} - -func removeDuplicateResults(taskRunResult []v1beta1.TaskRunResult) []v1beta1.TaskRunResult { - uniq := make([]v1beta1.TaskRunResult, 0) - latest := make(map[string]v1beta1.TaskRunResult, 0) - for _, res := range taskRunResult { - if _, seen := latest[res.Name]; !seen { - uniq = append(uniq, res) - } - latest[res.Name] = res - } - for i, res := range uniq { - uniq[i] = latest[res.Name] - } - return uniq -} - func isExceededResourceQuotaError(err error) bool { return err != nil && k8serrors.IsForbidden(err) && strings.Contains(err.Error(), "exceeded quota") } diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 9f6897fd2c2..0ca7dc3e1ca 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -53,7 +53,6 @@ import ( ktesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/record" "knative.dev/pkg/apis" - duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1" "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/logging" @@ -2318,277 +2317,6 @@ func TestReconcileCloudEvents(t *testing.T) { } } -func TestUpdateTaskRunResourceResult(t *testing.T) { - for _, c := range []struct { - desc string - pod corev1.Pod - taskRunStatus *v1beta1.TaskRunStatus - want []resourcev1alpha1.PipelineResourceResult - }{{ - desc: "image resource updated", - pod: corev1.Pod{ - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, - }, - }, - }}, - }, - }, - want: []resourcev1alpha1.PipelineResourceResult{{ - Key: "digest", - Value: "sha256:1234", - ResourceRef: resourcev1alpha1.PipelineResourceRef{Name: "source-image"}, - }}, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - tr := &v1beta1.TaskRun{} - tr.Status.SetCondition(&apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - }) - if err := updateTaskRunResourceResult(tr, c.pod); err != nil { - t.Errorf("updateTaskRunResourceResult: %s", err) - } - if d := cmp.Diff(c.want, tr.Status.ResourcesResult); d != "" { - t.Errorf("updateTaskRunResourceResult %s", diff.PrintWantGot(d)) - } - }) - } -} - -func TestUpdateTaskRunResult(t *testing.T) { - for _, c := range []struct { - desc string - pod corev1.Pod - taskRunStatus *v1beta1.TaskRunStatus - wantResults []v1beta1.TaskRunResult - want []resourcev1alpha1.PipelineResourceResult - }{{ - desc: "test result with pipeline result", - pod: corev1.Pod{ - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}, "type": "PipelineResourceResult"}]`, - }, - }, - }}, - }, - }, - wantResults: []v1beta1.TaskRunResult{{ - Name: "resultName", - Value: "resultValue", - }}, - want: []resourcev1alpha1.PipelineResourceResult{{ - Key: "digest", - Value: "sha256:1234", - ResourceRef: resourcev1alpha1.PipelineResourceRef{Name: "source-image"}, - ResultType: "PipelineResourceResult", - }}, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - tr := &v1beta1.TaskRun{} - tr.Status.SetCondition(&apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - }) - if err := updateTaskRunResourceResult(tr, c.pod); err != nil { - t.Errorf("updateTaskRunResourceResult: %s", err) - } - if d := cmp.Diff(c.wantResults, tr.Status.TaskRunResults); d != "" { - t.Errorf("updateTaskRunResourceResult TaskRunResults %s", diff.PrintWantGot(d)) - } - if d := cmp.Diff(c.want, tr.Status.ResourcesResult); d != "" { - t.Errorf("updateTaskRunResourceResult ResourcesResult %s", diff.PrintWantGot(d)) - } - }) - } -} - -func TestUpdateTaskRunResult2(t *testing.T) { - for _, c := range []struct { - desc string - pod corev1.Pod - taskRunStatus *v1beta1.TaskRunStatus - wantResults []v1beta1.TaskRunResult - want []resourcev1alpha1.PipelineResourceResult - }{{ - desc: "test result with pipeline result - no result type", - pod: corev1.Pod{ - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, - }, - }, - }}, - }, - }, - wantResults: []v1beta1.TaskRunResult{{ - Name: "resultName", - Value: "resultValue", - }}, - want: []resourcev1alpha1.PipelineResourceResult{{ - Key: "digest", - Value: "sha256:1234", - ResourceRef: resourcev1alpha1.PipelineResourceRef{Name: "source-image"}, - }}, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - tr := &v1beta1.TaskRun{} - tr.Status.SetCondition(&apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - }) - if err := updateTaskRunResourceResult(tr, c.pod); err != nil { - t.Errorf("updateTaskRunResourceResult: %s", err) - } - if d := cmp.Diff(c.wantResults, tr.Status.TaskRunResults); d != "" { - t.Errorf("updateTaskRunResourceResult %s", diff.PrintWantGot(d)) - } - if d := cmp.Diff(c.want, tr.Status.ResourcesResult); d != "" { - t.Errorf("updateTaskRunResourceResult %s", diff.PrintWantGot(d)) - } - }) - } -} - -func TestUpdateTaskRunResultTwoResults(t *testing.T) { - for _, c := range []struct { - desc string - pod corev1.Pod - taskRunStatus *v1beta1.TaskRunStatus - want []v1beta1.TaskRunResult - }{{ - desc: "two test results", - pod: corev1.Pod{ - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultNameOne","value":"resultValueOne", "type": "TaskRunResult"},{"key":"resultNameTwo","value":"resultValueTwo", "type": "TaskRunResult"}]`, - }, - }, - }, { - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultNameOne","value":"resultValueThree", "type": "TaskRunResult"},{"key":"resultNameTwo","value":"resultValueTwo", "type": "TaskRunResult"}]`, - }, - }, - }}, - }, - }, - want: []v1beta1.TaskRunResult{{ - Name: "resultNameOne", - Value: "resultValueThree", - }, { - Name: "resultNameTwo", - Value: "resultValueTwo", - }}, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - tr := &v1beta1.TaskRun{} - tr.Status.SetCondition(&apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - }) - if err := updateTaskRunResourceResult(tr, c.pod); err != nil { - t.Errorf("updateTaskRunResourceResult: %s", err) - } - if d := cmp.Diff(c.want, tr.Status.TaskRunResults); d != "" { - t.Errorf("updateTaskRunResourceResult %s", diff.PrintWantGot(d)) - } - }) - } -} - -func TestUpdateTaskRunResultWhenTaskFailed(t *testing.T) { - for _, c := range []struct { - desc string - podStatus corev1.PodStatus - taskRunStatus *v1beta1.TaskRunStatus - wantResults []v1beta1.TaskRunResult - want []resourcev1alpha1.PipelineResourceResult - }{{ - desc: "update task results when task fails", - podStatus: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"name":"source-image","digest":"sha256:1234"}]`, - }, - }, - }}, - }, - taskRunStatus: &v1beta1.TaskRunStatus{ - Status: duckv1beta1.Status{Conditions: []apis.Condition{{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionFalse, - }}}, - }, - wantResults: nil, - want: nil, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - if d := cmp.Diff(c.want, c.taskRunStatus.ResourcesResult); d != "" { - t.Errorf("updateTaskRunResourceResult resources %s", diff.PrintWantGot(d)) - } - if d := cmp.Diff(c.wantResults, c.taskRunStatus.TaskRunResults); d != "" { - t.Errorf("updateTaskRunResourceResult results %s", diff.PrintWantGot(d)) - } - }) - } -} - -func TestUpdateTaskRunResourceResult_Errors(t *testing.T) { - for _, c := range []struct { - desc string - pod corev1.Pod - taskRunStatus *v1beta1.TaskRunStatus - want []resourcev1alpha1.PipelineResourceResult - }{{ - desc: "image resource exporter with malformed json output", - pod: corev1.Pod{ - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `MALFORMED JSON{"digest":"sha256:1234"}`, - }, - }, - }}, - }, - }, - taskRunStatus: &v1beta1.TaskRunStatus{ - Status: duckv1beta1.Status{Conditions: []apis.Condition{{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - }}}, - }, - want: nil, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - if err := updateTaskRunResourceResult(&v1beta1.TaskRun{Status: *c.taskRunStatus}, c.pod); err == nil { - t.Error("Expected error, got nil") - } - if d := cmp.Diff(c.want, c.taskRunStatus.ResourcesResult); d != "" { - t.Errorf("updateTaskRunResourceResult %s", diff.PrintWantGot(d)) - } - }) - } -} - func TestReconcile_Single_SidecarState(t *testing.T) { runningState := corev1.ContainerStateRunning{StartedAt: metav1.Time{Time: time.Now()}} taskRun := tb.TaskRun("test-taskrun-sidecars", diff --git a/pkg/termination/parse.go b/pkg/termination/parse.go index a23b7804a0d..51d9057a068 100644 --- a/pkg/termination/parse.go +++ b/pkg/termination/parse.go @@ -20,28 +20,39 @@ import ( "fmt" "sort" - v1alpha1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + v1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "go.uber.org/zap" ) // ParseMessage parses a termination message as results. // // If more than one item has the same key, only the latest is returned. Items // are sorted by their key. -func ParseMessage(msg string) ([]v1alpha1.PipelineResourceResult, error) { +func ParseMessage(logger *zap.SugaredLogger, msg string) ([]v1beta1.PipelineResourceResult, error) { if msg == "" { return nil, nil } - var r []v1alpha1.PipelineResourceResult + + var r []v1beta1.PipelineResourceResult if err := json.Unmarshal([]byte(msg), &r); err != nil { return nil, fmt.Errorf("parsing message json: %v", err) } + for i, rr := range r { + if rr == (v1beta1.PipelineResourceResult{}) { + //Erase incorrect result + r[i] = r[len(r)-1] + r = r[:len(r)-1] + logger.Errorf("termination message contains non taskrun or pipelineresource result keys") + } + } + // Remove duplicates (last one wins) and sort by key. - m := map[string]v1alpha1.PipelineResourceResult{} + m := map[string]v1beta1.PipelineResourceResult{} for _, rr := range r { m[rr.Key] = rr } - var r2 []v1alpha1.PipelineResourceResult + var r2 []v1beta1.PipelineResourceResult for _, v := range m { r2 = append(r2, v) } diff --git a/pkg/termination/parse_test.go b/pkg/termination/parse_test.go index 5e577841f36..dae81ac510c 100644 --- a/pkg/termination/parse_test.go +++ b/pkg/termination/parse_test.go @@ -16,21 +16,23 @@ limitations under the License. package termination import ( + "strings" "testing" "github.com/google/go-cmp/cmp" - v1alpha1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + v1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/test/diff" + "knative.dev/pkg/logging" ) func TestParseMessage(t *testing.T) { for _, c := range []struct { desc, msg string - want []v1alpha1.PipelineResourceResult + want []v1beta1.PipelineResourceResult }{{ desc: "valid message", msg: `[{"key": "digest","value":"hereisthedigest"},{"key":"foo","value":"bar"}]`, - want: []v1alpha1.PipelineResourceResult{{ + want: []v1beta1.PipelineResourceResult{{ Key: "digest", Value: "hereisthedigest", }, { @@ -47,7 +49,7 @@ func TestParseMessage(t *testing.T) { {"key":"foo","value":"first"}, {"key":"foo","value":"middle"}, {"key":"foo","value":"last"}]`, - want: []v1alpha1.PipelineResourceResult{{ + want: []v1beta1.PipelineResourceResult{{ Key: "foo", Value: "last", }}, @@ -57,7 +59,7 @@ func TestParseMessage(t *testing.T) { {"key":"zzz","value":"last"}, {"key":"ddd","value":"middle"}, {"key":"aaa","value":"first"}]`, - want: []v1alpha1.PipelineResourceResult{{ + want: []v1beta1.PipelineResourceResult{{ Key: "aaa", Value: "first", }, { @@ -69,7 +71,8 @@ func TestParseMessage(t *testing.T) { }}, }} { t.Run(c.desc, func(t *testing.T) { - got, err := ParseMessage(c.msg) + logger, _ := logging.NewLogger("", "status") + got, err := ParseMessage(logger, c.msg) if err != nil { t.Fatalf("ParseMessage: %v", err) } @@ -80,8 +83,23 @@ func TestParseMessage(t *testing.T) { } } -func TestParseMessage_Invalid(t *testing.T) { - if _, err := ParseMessage("INVALID NOT JSON"); err == nil { - t.Error("Expected error parsing invalid JSON, got nil") +func TestParseMessageInvalidMessage(t *testing.T) { + for _, c := range []struct { + desc, msg, wantError string + }{{ + desc: "invalid JSON", + msg: "invalid JSON", + wantError: "parsing message json", + }} { + t.Run(c.desc, func(t *testing.T) { + logger, _ := logging.NewLogger("", "status") + _, err := ParseMessage(logger, c.msg) + if err == nil { + t.Errorf("Expected error parsing incorrect termination message, got nil") + } + if !strings.HasPrefix(err.Error(), c.wantError) { + t.Errorf("Expected different error: %s", c.wantError) + } + }) } } diff --git a/pkg/termination/write_test.go b/pkg/termination/write_test.go index b23c2a63f40..e9084534128 100644 --- a/pkg/termination/write_test.go +++ b/pkg/termination/write_test.go @@ -62,7 +62,7 @@ func TestExistingFile(t *testing.T) { if fileContents, err := ioutil.ReadFile(tmpFile.Name()); err != nil { logger.Fatalf("Unexpected error reading %v: %v", tmpFile.Name(), err) } else { - want := `[{"key":"key1","value":"hello","resourceRef":{}},{"key":"key2","value":"world","resourceRef":{}}]` + want := `[{"key":"key1","value":"hello"},{"key":"key2","value":"world"}]` if d := cmp.Diff(want, string(fileContents)); d != "" { t.Fatalf("Diff %s", diff.PrintWantGot(d)) }