From a9e7b54d8ea7e9aee2b5b2367a5588f6abf5e188 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 | 185 ++++++--- pkg/pod/status_test.go | 365 ++++++++++++++++-- pkg/reconciler/taskrun/taskrun.go | 64 +-- pkg/reconciler/taskrun/taskrun_test.go | 272 ------------- pkg/termination/parse.go | 13 +- pkg/termination/parse_test.go | 26 +- pkg/termination/write_test.go | 2 +- 13 files changed, 540 insertions(+), 423 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..91192d3b4f0 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 ResourceRef 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 97cf2670876..f04e41d45ab 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 } @@ -1640,7 +1644,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..74afb700ccb 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,177 @@ 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 := 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: %w", s.Name, tr.Name, err) + logger.Errorf("error setting the start time of step %q in taskrun %q: %v", s.Name, tr.Name, err) + merr = multierror.Append(merr, err) + } + taskResults, pipelineResourceResults, filteredResults := filterResultsAndResources(results) + if tr.IsSuccessful() { + trs.TaskRunResults = append(trs.TaskRunResults, taskResults...) + trs.ResourcesResult = append(trs.ResourcesResult, pipelineResourceResults...) + } + msg, err = createMessageFromResults(filteredResults) + if err != nil { + 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 + } + + 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 index, result := range r { + 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..f592b51a083 100644 --- a/pkg/pod/status_test.go +++ b/pkg/pod/status_test.go @@ -33,13 +33,20 @@ import ( var ignoreVolatileTime = cmp.Comparer(func(_, _ apis.VolatileTime) bool { return true }) +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 +612,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 +994,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 +1023,27 @@ 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) + if err == nil { + t.Error("Expected error, got nil") + } - // 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 { @@ -732,9 +1054,6 @@ func TestMakeTaskRunStatus(t *testing.T) { 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 4edda8897ee..d6ab1c42844 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" @@ -189,7 +188,7 @@ 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()) + logger.Errorf("Reconcile: %v", err.Error()) } // Emit events (only when ConditionSucceeded was changed) return c.finishReconcileUpdateEmitEvents(ctx, tr, before, err) @@ -403,9 +402,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 } @@ -633,62 +631,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 d83403669ce..8a00213be26 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" @@ -2324,277 +2323,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 8b637df9cd8..51d9057a068 100644 --- a/pkg/termination/parse.go +++ b/pkg/termination/parse.go @@ -21,21 +21,32 @@ import ( "sort" 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) ([]v1beta1.PipelineResourceResult, error) { +func ParseMessage(logger *zap.SugaredLogger, msg string) ([]v1beta1.PipelineResourceResult, error) { if msg == "" { return nil, nil } + 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]v1beta1.PipelineResourceResult{} for _, rr := range r { diff --git a/pkg/termination/parse_test.go b/pkg/termination/parse_test.go index a128178fb95..dae81ac510c 100644 --- a/pkg/termination/parse_test.go +++ b/pkg/termination/parse_test.go @@ -16,11 +16,13 @@ limitations under the License. package termination import ( + "strings" "testing" "github.com/google/go-cmp/cmp" v1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/test/diff" + "knative.dev/pkg/logging" ) func TestParseMessage(t *testing.T) { @@ -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 77fbcb737ef..7d5ca1ad76d 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)) }