diff --git a/docs/tasks.md b/docs/tasks.md index 1402770cd01..6d5bb290539 100644 --- a/docs/tasks.md +++ b/docs/tasks.md @@ -330,7 +330,7 @@ steps: echo "I am supposed to sleep for 60 seconds!" sleep 60 timeout: 5s -``` +``` #### Specifying `onError` for a `step` @@ -450,7 +450,7 @@ Parameter names: For example, `foo.Is-Bar_` is a valid parameter name, but `barIsBa$` or `0banana` are not. -> NOTE: +> NOTE: > 1. Parameter names are **case insensitive**. For example, `APPLE` and `apple` will be treated as equal. If they appear in the same TaskSpec's params, it will be rejected as invalid. > 2. If a parameter name contains dots (.), it must be referenced by using the [bracket notation](#substituting-parameters-and-resources) with either single or double quotes i.e. `$(params['foo.bar'])`, `$(params["foo.bar"])`. See the following example for more information. @@ -684,6 +684,30 @@ or [at the `Pipeline` level](./pipelines.md#configuring-execution-results-at-the **Note:** The maximum size of a `Task's` results is limited by the container termination message feature of Kubernetes, as results are passed back to the controller via this mechanism. At present, the limit is ["4096 bytes"](https://github.com/kubernetes/kubernetes/blob/96e13de777a9eb57f87889072b68ac40467209ac/pkg/kubelet/container/runtime.go#L632). + +**Note:** The result type currently support `string` and `array` (`array` is alpha gated feature), you can write `array` results via JSON escaped format. In the example below, the task specifies one files in the `results` field and write `array` to the file. And `array` is currently supported in Task level not in Pipeline level. + +``` +kind: Task +apiVersion: tekton.dev/v1beta1 +metadata: + name: write-array + annotations: + description: | + A simple task that writes array +spec: + results: + - name: array-results + type: array + description: The array results + steps: + - name: write-array + image: bash:latest + script: | + #!/usr/bin/env bash + echo -n "[\"hello\",\"world\"]" | tee $(results.array-results.path) +``` + Results are written to the termination message encoded as JSON objects and Tekton uses those objects to pass additional information to the controller. As such, `Task` results are best suited for holding small amounts of data, such as commit SHAs, branch names, ephemeral namespaces, and so on. @@ -1183,10 +1207,10 @@ log into the `Pod` and add a `Step` that pauses the `Task` at the desired stage. ### Running Step Containers as a Non Root User -All steps that do not require to be run as a root user should make use of TaskRun features to -designate the container for a step runs as a user without root permissions. As a best practice, -running containers as non root should be built into the container image to avoid any possibility -of the container being run as root. However, as a further measure of enforcing this practice, +All steps that do not require to be run as a root user should make use of TaskRun features to +designate the container for a step runs as a user without root permissions. As a best practice, +running containers as non root should be built into the container image to avoid any possibility +of the container being run as root. However, as a further measure of enforcing this practice, steps can make use of a `securityContext` to specify how the container should run. An example of running Task steps as a non root user is shown below: @@ -1230,17 +1254,17 @@ spec: runAsUser: 1001 ``` -In the example above, the step `show-user-2000` specifies via a `securityContext` that the container -for the step should run as user 2000. A `securityContext` must still be specified via a TaskRun `podTemplate` -for this TaskRun to run in a Kubernetes environment that enforces running containers as non root as a requirement. +In the example above, the step `show-user-2000` specifies via a `securityContext` that the container +for the step should run as user 2000. A `securityContext` must still be specified via a TaskRun `podTemplate` +for this TaskRun to run in a Kubernetes environment that enforces running containers as non root as a requirement. -The `runAsNonRoot` property specified via the `podTemplate` above validates that steps part of this TaskRun are -running as non root users and will fail to start any step container that attempts to run as root. Only specifying -`runAsNonRoot: true` will not actually run containers as non root as the property simply validates that steps are not +The `runAsNonRoot` property specified via the `podTemplate` above validates that steps part of this TaskRun are +running as non root users and will fail to start any step container that attempts to run as root. Only specifying +`runAsNonRoot: true` will not actually run containers as non root as the property simply validates that steps are not running as root. It is the `runAsUser` property that is actually used to set the non root user ID for the container. -If a step defines its own `securityContext`, it will be applied for the step container over the `securityContext` -specified at the pod level via the TaskRun `podTemplate`. +If a step defines its own `securityContext`, it will be applied for the step container over the `securityContext` +specified at the pod level via the TaskRun `podTemplate`. More information about Pod and Container Security Contexts can be found via the [Kubernetes website](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-pod). diff --git a/examples/v1beta1/taskruns/alpha/emit-array-results.yaml b/examples/v1beta1/taskruns/alpha/emit-array-results.yaml new file mode 100644 index 00000000000..60ef713445e --- /dev/null +++ b/examples/v1beta1/taskruns/alpha/emit-array-results.yaml @@ -0,0 +1,39 @@ +kind: Task +apiVersion: tekton.dev/v1beta1 +metadata: + name: write-array + annotations: + description: | + A simple task that writes array +spec: + results: + - name: array-results + type: array + description: The array results + steps: + - name: write-array + image: bash:latest + script: | + #!/usr/bin/env bash + echo -n "[\"hello\",\"world\"]" | tee $(results.array-results.path) + - name: check-results-array + image: ubuntu + script: | + #!/bin/bash + VALUE=$(cat $(results.array-results.path)) + EXPECTED=[\"hello\",\"world\"] + diff=$(diff <(printf "%s\n" "${VALUE[@]}") <(printf "%s\n" "${EXPECTED[@]}")) + if [[ -z "$diff" ]]; then + echo "TRUE" + else + echo "FALSE" + fi +--- +kind: TaskRun +apiVersion: tekton.dev/v1beta1 +metadata: + name: write-array-tr +spec: + taskRef: + name: write-array + kind: task diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index bf89f787a11..50a0bcb6705 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -4670,15 +4670,16 @@ func schema_pkg_apis_pipeline_v1beta1_TaskRunResult(ref common.ReferenceCallback "value": { SchemaProps: spec.SchemaProps{ Description: "Value the given value of the result", - Default: "", - Type: []string{"string"}, - Format: "", + Default: map[string]interface{}{}, + Ref: ref("github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.ArrayOrString"), }, }, }, Required: []string{"name", "value"}, }, }, + Dependencies: []string{ + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.ArrayOrString"}, } } diff --git a/pkg/apis/pipeline/v1beta1/param_types.go b/pkg/apis/pipeline/v1beta1/param_types.go index 4f2c594bc3f..068278264fd 100644 --- a/pkg/apis/pipeline/v1beta1/param_types.go +++ b/pkg/apis/pipeline/v1beta1/param_types.go @@ -141,17 +141,43 @@ type ArrayOrString struct { // UnmarshalJSON implements the json.Unmarshaller interface. func (arrayOrString *ArrayOrString) UnmarshalJSON(value []byte) error { - switch value[0] { - case '[': - arrayOrString.Type = ParamTypeArray - return json.Unmarshal(value, &arrayOrString.ArrayVal) - case '{': - arrayOrString.Type = ParamTypeObject - return json.Unmarshal(value, &arrayOrString.ObjectVal) - default: + // ArrayOrString is used for Results Value as well, the results can be any kind of + // data so we need to check if it is empty. + if len(value) == 0 { arrayOrString.Type = ParamTypeString - return json.Unmarshal(value, &arrayOrString.StringVal) + return nil + } + if value[0] == '[' { + // We're trying to Unmarshal to []string, but for cases like []int or other types + // of nested array which we don't support yet, we should continue and Unmarshal + // it to String. If the Type being set doesn't match what it actually should be, + // it will be captured by validation in reconciler. + // if failed to unmarshal to array, we will convert the value to string and marshal it to string + var a []string + if err := json.Unmarshal(value, &a); err == nil { + arrayOrString.Type = ParamTypeArray + arrayOrString.ArrayVal = a + return nil + } + } + if value[0] == '{' { + // if failed to unmarshal to map, we will convert the value to string and marshal it to string + var m map[string]string + if err := json.Unmarshal(value, &m); err == nil { + arrayOrString.Type = ParamTypeObject + arrayOrString.ObjectVal = m + return nil + } + } + + // By default we unmarshal to string + arrayOrString.Type = ParamTypeString + if err := json.Unmarshal(value, &arrayOrString.StringVal); err == nil { + return nil } + arrayOrString.StringVal = string(value) + + return nil } // MarshalJSON implements the json.Marshaller interface. diff --git a/pkg/apis/pipeline/v1beta1/param_types_test.go b/pkg/apis/pipeline/v1beta1/param_types_test.go index 1ec6610cb0f..b9dedac0100 100644 --- a/pkg/apis/pipeline/v1beta1/param_types_test.go +++ b/pkg/apis/pipeline/v1beta1/param_types_test.go @@ -227,6 +227,10 @@ func TestArrayOrString_UnmarshalJSON(t *testing.T) { input map[string]interface{} result v1beta1.ArrayOrString }{ + { + input: map[string]interface{}{"val": 123}, + result: *v1beta1.NewArrayOrString("123"), + }, { input: map[string]interface{}{"val": "123"}, result: *v1beta1.NewArrayOrString("123"), @@ -282,6 +286,49 @@ func TestArrayOrString_UnmarshalJSON(t *testing.T) { } } +func TestArrayOrString_UnmarshalJSON_Directly(t *testing.T) { + cases := []struct { + desc string + input string + expected v1beta1.ArrayOrString + }{ + {desc: "empty value", input: ``, expected: *v1beta1.NewArrayOrString("")}, + {desc: "int value", input: `1`, expected: *v1beta1.NewArrayOrString("1")}, + {desc: "int array", input: `[1,2,3]`, expected: *v1beta1.NewArrayOrString("[1,2,3]")}, + {desc: "nested array", input: `[1,\"2\",3]`, expected: *v1beta1.NewArrayOrString(`[1,\"2\",3]`)}, + {desc: "string value", input: `hello`, expected: *v1beta1.NewArrayOrString("hello")}, + {desc: "array value", input: `["hello","world"]`, expected: *v1beta1.NewArrayOrString("hello", "world")}, + {desc: "object value", input: `{"hello":"world"}`, expected: *v1beta1.NewObject(map[string]string{"hello": "world"})}, + } + + for _, c := range cases { + aos := v1beta1.ArrayOrString{} + if err := aos.UnmarshalJSON([]byte(c.input)); err != nil { + t.Errorf("Failed to unmarshal input '%v': %v", c.input, err) + } + if !reflect.DeepEqual(aos, c.expected) { + t.Errorf("Failed to unmarshal input '%v': expected %+v, got %+v", c.input, c.expected, aos) + } + } +} + +func TestArrayOrString_UnmarshalJSON_Error(t *testing.T) { + cases := []struct { + desc string + input string + }{ + {desc: "empty value", input: "{\"val\": }"}, + {desc: "wrong beginning value", input: "{\"val\": @}"}, + } + + for _, c := range cases { + var result ArrayOrStringHolder + if err := json.Unmarshal([]byte(c.input), &result); err == nil { + t.Errorf("Should return err but got nil '%v'", c.input) + } + } +} + func TestArrayOrString_MarshalJSON(t *testing.T) { cases := []struct { input v1beta1.ArrayOrString diff --git a/pkg/apis/pipeline/v1beta1/result_types.go b/pkg/apis/pipeline/v1beta1/result_types.go index 2848d45949f..a4fd77b26da 100644 --- a/pkg/apis/pipeline/v1beta1/result_types.go +++ b/pkg/apis/pipeline/v1beta1/result_types.go @@ -39,7 +39,7 @@ type TaskRunResult struct { Type ResultsType `json:"type,omitempty"` // Value the given value of the result - Value string `json:"value"` + Value ArrayOrString `json:"value"` } // ResultsType indicates the type of a result; @@ -54,7 +54,9 @@ type ResultsType string // Valid ResultsType: const ( ResultsTypeString ResultsType = "string" + ResultsTypeArray ResultsType = "array" + ResultsTypeObject ResultsType = "object" ) // AllResultsTypes can be used for ResultsTypes validation. -var AllResultsTypes = []ResultsType{ResultsTypeString} +var AllResultsTypes = []ResultsType{ResultsTypeString, ResultsTypeArray, ResultsTypeObject} diff --git a/pkg/apis/pipeline/v1beta1/result_validation.go b/pkg/apis/pipeline/v1beta1/result_validation.go index 4c64b780d16..f8b31857280 100644 --- a/pkg/apis/pipeline/v1beta1/result_validation.go +++ b/pkg/apis/pipeline/v1beta1/result_validation.go @@ -17,22 +17,21 @@ import ( "context" "fmt" + "github.com/tektoncd/pipeline/pkg/apis/config" "knative.dev/pkg/apis" ) // Validate implements apis.Validatable -func (tr TaskResult) Validate(_ context.Context) *apis.FieldError { +func (tr TaskResult) Validate(ctx context.Context) (errs *apis.FieldError) { if !resultNameFormatRegex.MatchString(tr.Name) { return apis.ErrInvalidKeyName(tr.Name, "name", fmt.Sprintf("Name must consist of alphanumeric characters, '-', '_', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my-name', or 'my_name', regex used for validation is '%s')", ResultNameFormat)) } - // Validate the result type - validType := false - for _, allowedType := range AllResultsTypes { - if tr.Type == allowedType { - validType = true - } + // Array and Object is alpha feature + if tr.Type == ResultsTypeArray || tr.Type == ResultsTypeObject { + return errs.Also(ValidateEnabledAPIFields(ctx, "results type", config.AlphaAPIFields)) } - if !validType { + + if tr.Type != ResultsTypeString { return apis.ErrInvalidValue(tr.Type, "type", fmt.Sprintf("type must be string")) } diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index f218f3f2a84..6f39956b341 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -2605,8 +2605,8 @@ }, "value": { "description": "Value the given value of the result", - "type": "string", - "default": "" + "default": {}, + "$ref": "#/definitions/v1beta1.ArrayOrString" } } }, diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index a19e50f11af..c69e2604756 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -305,7 +305,7 @@ func TestTaskSpecValidate(t *testing.T) { }}, }, }, { - name: "valid result type", + name: "valid result type string", fields: fields{ Steps: []v1beta1.Step{{ Image: "my-image", @@ -317,6 +317,32 @@ func TestTaskSpecValidate(t *testing.T) { Description: "my great result", }}, }, + }, { + name: "valid result type array", + fields: fields{ + Steps: []v1beta1.Step{{ + Image: "my-image", + Args: []string{"arg"}, + }}, + Results: []v1beta1.TaskResult{{ + Name: "MY-RESULT", + Type: v1beta1.ResultsTypeArray, + Description: "my great result", + }}, + }, + }, { + name: "valid result type object", + fields: fields{ + Steps: []v1beta1.Step{{ + Image: "my-image", + Args: []string{"arg"}, + }}, + Results: []v1beta1.TaskResult{{ + Name: "MY-RESULT", + Type: v1beta1.ResultsTypeObject, + Description: "my great result", + }}, + }, }, { name: "valid task name context", fields: fields{ diff --git a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go index 43ea81e4520..c739fe1d7f2 100644 --- a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go @@ -1971,6 +1971,7 @@ func (in *TaskRunResources) DeepCopy() *TaskRunResources { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TaskRunResult) DeepCopyInto(out *TaskRunResult) { *out = *in + in.Value.DeepCopyInto(&out.Value) return } @@ -2135,7 +2136,9 @@ func (in *TaskRunStatusFields) DeepCopyInto(out *TaskRunStatusFields) { if in.TaskRunResults != nil { in, out := &in.TaskRunResults, &out.TaskRunResults *out = make([]TaskRunResult, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } if in.Sidecars != nil { in, out := &in.Sidecars, &out.Sidecars diff --git a/pkg/entrypoint/entrypointer.go b/pkg/entrypoint/entrypointer.go index d4c3877fe65..92a13219a21 100644 --- a/pkg/entrypoint/entrypointer.go +++ b/pkg/entrypoint/entrypointer.go @@ -184,7 +184,7 @@ func (e Entrypointer) Go() error { // strings.Split(..) with an empty string returns an array that contains one element, an empty string. // This creates an error when trying to open the result folder as a file. if len(e.Results) >= 1 && e.Results[0] != "" { - if err := e.readResultsFromDisk(); err != nil { + if err := e.readResultsFromDisk(pipeline.DefaultResultPath); err != nil { logger.Fatalf("Error while handling results: %s", err) } } @@ -192,13 +192,13 @@ func (e Entrypointer) Go() error { return err } -func (e Entrypointer) readResultsFromDisk() error { +func (e Entrypointer) readResultsFromDisk(resultDir string) error { output := []v1beta1.PipelineResourceResult{} for _, resultFile := range e.Results { if resultFile == "" { continue } - fileContents, err := ioutil.ReadFile(filepath.Join(pipeline.DefaultResultPath, resultFile)) + fileContents, err := ioutil.ReadFile(filepath.Join(resultDir, resultFile)) if os.IsNotExist(err) { continue } else if err != nil { diff --git a/pkg/entrypoint/entrypointer_test.go b/pkg/entrypoint/entrypointer_test.go index ec872f56e85..2cc06095d22 100644 --- a/pkg/entrypoint/entrypointer_test.go +++ b/pkg/entrypoint/entrypointer_test.go @@ -31,7 +31,10 @@ import ( "github.com/google/go-cmp/cmp" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "github.com/tektoncd/pipeline/pkg/termination" "github.com/tektoncd/pipeline/test/diff" + "knative.dev/pkg/logging" ) func TestEntrypointerFailures(t *testing.T) { @@ -249,6 +252,89 @@ func TestEntrypointer(t *testing.T) { } } +func TestReadResultsFromDisk(t *testing.T) { + for _, c := range []struct { + desc string + results []string + resultContent []v1beta1.ArrayOrString + want []v1beta1.PipelineResourceResult + }{{ + desc: "read string result file", + results: []string{"results"}, + resultContent: []v1beta1.ArrayOrString{*v1beta1.NewArrayOrString("hello world")}, + want: []v1beta1.PipelineResourceResult{ + {Value: `"hello world"`, + ResultType: 1}}, + }, { + desc: "read array result file", + results: []string{"results"}, + resultContent: []v1beta1.ArrayOrString{*v1beta1.NewArrayOrString("hello", "world")}, + want: []v1beta1.PipelineResourceResult{ + {Value: `["hello","world"]`, + ResultType: 1}}, + }, { + desc: "read string and array result files", + results: []string{"resultsArray", "resultsString"}, + resultContent: []v1beta1.ArrayOrString{*v1beta1.NewArrayOrString("hello", "world"), *v1beta1.NewArrayOrString("hello world")}, + want: []v1beta1.PipelineResourceResult{ + {Value: `["hello","world"]`, + ResultType: 1}, + {Value: `"hello world"`, + ResultType: 1}, + }, + }, + } { + t.Run(c.desc, func(t *testing.T) { + terminationPath := "termination" + if terminationFile, err := ioutil.TempFile("", "termination"); err != nil { + t.Fatalf("unexpected error creating temporary termination file: %v", err) + } else { + terminationPath = terminationFile.Name() + defer os.Remove(terminationFile.Name()) + } + resultsFilePath := []string{} + for i, r := range c.results { + if resultsFile, err := ioutil.TempFile("", r); err != nil { + t.Fatalf("unexpected error creating temporary termination file: %v", err) + } else { + resultName := resultsFile.Name() + c.want[i].Key = resultName + resultsFilePath = append(resultsFilePath, resultName) + d, err := json.Marshal(c.resultContent[i]) + if err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(resultName, d, 0777); err != nil { + t.Fatal(err) + } + defer os.Remove(resultName) + } + } + + e := Entrypointer{ + Results: resultsFilePath, + TerminationPath: terminationPath, + } + if err := e.readResultsFromDisk(""); err != nil { + t.Fatal(err) + } + msg, err := ioutil.ReadFile(terminationPath) + if err != nil { + t.Fatal(err) + } + logger, _ := logging.NewLogger("", "status") + got, _ := termination.ParseMessage(logger, string(msg)) + for _, g := range got { + aos := v1beta1.ArrayOrString{} + aos.UnmarshalJSON([]byte(g.Value)) + } + if d := cmp.Diff(got, c.want); d != "" { + t.Fatalf("Diff(-want,+got): %v", d) + } + }) + } +} + func TestEntrypointer_ReadBreakpointExitCodeFromDisk(t *testing.T) { expectedExitCode := 1 // setup test diff --git a/pkg/pod/status.go b/pkg/pod/status.go index 2a1ba103d05..5cddaffb546 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -227,9 +227,18 @@ func filterResultsAndResources(results []v1beta1.PipelineResourceResult) ([]v1be for _, r := range results { switch r.ResultType { case v1beta1.TaskRunResultType: + aos := v1beta1.ArrayOrString{} + err := aos.UnmarshalJSON([]byte(r.Value)) + if err != nil { + continue + } + // TODO(#4723): Validate that the type we inferred from aos is matching the + // TaskResult Type before setting it to the taskRunResult. + // TODO(#4723): Validate the taskrun results against taskresults for object val taskRunResult := v1beta1.TaskRunResult{ Name: r.Key, - Value: r.Value, + Type: v1beta1.ResultsType(aos.Type), + Value: aos, } taskResults = append(taskResults, taskRunResult) filteredResults = append(filteredResults, r) diff --git a/pkg/pod/status_test.go b/pkg/pod/status_test.go index 006f76ae215..4af72c0f82d 100644 --- a/pkg/pod/status_test.go +++ b/pkg/pod/status_test.go @@ -629,7 +629,8 @@ func TestMakeTaskRunStatus(t *testing.T) { }}, TaskRunResults: []v1beta1.TaskRunResult{{ Name: "resultName", - Value: "resultValue", + Type: v1beta1.ResultsTypeString, + Value: *v1beta1.NewArrayOrString("resultValue"), }}, // We don't actually care about the time, just that it's not nil CompletionTime: &metav1.Time{Time: time.Now()}, @@ -667,7 +668,8 @@ func TestMakeTaskRunStatus(t *testing.T) { }}, TaskRunResults: []v1beta1.TaskRunResult{{ Name: "resultName", - Value: "resultValue", + Type: v1beta1.ResultsTypeString, + Value: *v1beta1.NewArrayOrString("resultValue"), }}, // We don't actually care about the time, just that it's not nil CompletionTime: &metav1.Time{Time: time.Now()}, @@ -714,10 +716,12 @@ func TestMakeTaskRunStatus(t *testing.T) { Sidecars: []v1beta1.SidecarState{}, TaskRunResults: []v1beta1.TaskRunResult{{ Name: "resultNameOne", - Value: "resultValueThree", + Type: v1beta1.ResultsTypeString, + Value: *v1beta1.NewArrayOrString("resultValueThree"), }, { Name: "resultNameTwo", - Value: "resultValueTwo", + Type: v1beta1.ResultsTypeString, + Value: *v1beta1.NewArrayOrString("resultValueTwo"), }}, // We don't actually care about the time, just that it's not nil CompletionTime: &metav1.Time{Time: time.Now()}, @@ -806,7 +810,8 @@ func TestMakeTaskRunStatus(t *testing.T) { }}, TaskRunResults: []v1beta1.TaskRunResult{{ Name: "resultNameThree", - Value: "", + Type: v1beta1.ResultsTypeString, + Value: *v1beta1.NewArrayOrString(""), }}, // We don't actually care about the time, just that it's not nil CompletionTime: &metav1.Time{Time: time.Now()}, @@ -844,7 +849,8 @@ func TestMakeTaskRunStatus(t *testing.T) { }}, TaskRunResults: []v1beta1.TaskRunResult{{ Name: "resultNameThree", - Value: "", + Type: v1beta1.ResultsTypeString, + Value: *v1beta1.NewArrayOrString(""), }}, // We don't actually care about the time, just that it's not nil CompletionTime: &metav1.Time{Time: time.Now()}, @@ -1054,7 +1060,220 @@ func TestMakeTaskRunStatus(t *testing.T) { }, }, } + logger, _ := logging.NewLogger("", "status") + got, err := MakeTaskRunStatus(logger, tr, &c.pod) + if err != nil { + t.Errorf("MakeTaskRunResult: %s", err) + } + + // 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 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 TestMakeTaskRunStatusAlpha(t *testing.T) { + for _, c := range []struct { + desc string + podStatus corev1.PodStatus + pod corev1.Pod + want v1beta1.TaskRunStatus + }{{ + desc: "test empty result", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-bar", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"", "type":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: statusSuccess(), + 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":"","type":1}]`, + }}, + 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", + Type: v1beta1.ResultsTypeString, + Value: *v1beta1.NewArrayOrString(""), + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "test string result", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-bar", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"hello", "type":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: statusSuccess(), + 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":"hello","type":1}]`, + }}, + 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", + Type: v1beta1.ResultsTypeString, + Value: *v1beta1.NewArrayOrString("hello"), + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "test array result", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-bar", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"[\"hello\",\"world\"]", "type":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: statusSuccess(), + 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":"[\"hello\",\"world\"]","type":1}]`, + }}, + 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", + Type: v1beta1.ResultsTypeArray, + Value: *v1beta1.NewArrayOrString("hello", "world"), + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "test object result", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-bar", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"{\"hello\":\"world\"}", "type":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: statusSuccess(), + 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":"{\"hello\":\"world\"}","type":1}]`, + }}, + 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", + Type: v1beta1.ResultsTypeObject, + Value: *v1beta1.NewObject(map[string]string{"hello": "world"}), + }}, + // 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() + if cmp.Diff(c.pod, corev1.Pod{}) == "" { + c.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, &c.pod) if err != nil { @@ -1079,6 +1298,7 @@ func TestMakeTaskRunStatus(t *testing.T) { } }) } + } func TestMakeRunStatusJSONError(t *testing.T) { diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index 4187b915a84..089b5fc7bf8 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -182,6 +182,8 @@ func replaceParamValues(params []v1beta1.Param, stringReplacements map[string]st // and omitted from the returned slice. A nil slice is returned if no results are passed in or all // results are invalid. func ApplyTaskResultsToPipelineResults( + // TODO(#4723): Change PipelineResult Value to support array, so we can update + // PipelineResults, right now we only update the StringVal from TaskResults to PipelineResults results []v1beta1.PipelineResult, taskRunResults map[string][]v1beta1.TaskRunResult, customTaskResults map[string][]v1alpha1.RunResult) []v1beta1.PipelineRunResult { @@ -231,7 +233,7 @@ func ApplyTaskResultsToPipelineResults( func taskResultValue(taskName string, resultName string, taskResults map[string][]v1beta1.TaskRunResult) *string { for _, trResult := range taskResults[taskName] { if trResult.Name == resultName { - return &trResult.Value + return &trResult.Value.StringVal } } return nil diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index 8dd15896080..495f39b9483 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -791,7 +791,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { taskResults: map[string][]v1beta1.TaskRunResult{ "pt1": {{ Name: "foo", - Value: "bar", + Value: *v1beta1.NewArrayOrString("bar"), }}, }, expected: nil, @@ -804,7 +804,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { taskResults: map[string][]v1beta1.TaskRunResult{ "pt1": {{ Name: "foo", - Value: "bar", + Value: *v1beta1.NewArrayOrString("bar"), }}, }, expected: nil, @@ -827,7 +827,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { taskResults: map[string][]v1beta1.TaskRunResult{ "definitely-not-pt1": {{ Name: "foo", - Value: "bar", + Value: *v1beta1.NewArrayOrString("bar"), }}, }, expected: nil, @@ -840,7 +840,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { taskResults: map[string][]v1beta1.TaskRunResult{ "pt1": {{ Name: "definitely-not-foo", - Value: "bar", + Value: *v1beta1.NewArrayOrString("bar"), }}, }, expected: nil, @@ -864,7 +864,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { taskResults: map[string][]v1beta1.TaskRunResult{ "pt2": {{ Name: "bar", - Value: "rae", + Value: *v1beta1.NewArrayOrString("rae"), }}, }, expected: []v1beta1.PipelineRunResult{{ @@ -884,15 +884,15 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { "pt1": { { Name: "foo", - Value: "do", + Value: *v1beta1.NewArrayOrString("do"), }, { Name: "bar", - Value: "mi", + Value: *v1beta1.NewArrayOrString("mi"), }, }, "pt2": {{ Name: "baz", - Value: "rae", + Value: *v1beta1.NewArrayOrString("rae"), }}, }, expected: []v1beta1.PipelineRunResult{{ @@ -969,7 +969,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { taskResults: map[string][]v1beta1.TaskRunResult{ "normaltask": {{ Name: "baz", - Value: "rae", + Value: *v1beta1.NewArrayOrString("rae"), }}, }, expected: []v1beta1.PipelineRunResult{{ diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index d2551a214f8..0c38ba4af19 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go @@ -3250,7 +3250,7 @@ func TestResolvedPipelineRunTask_IsFinallySkipped(t *testing.T) { TaskRunStatusFields: v1beta1.TaskRunStatusFields{ TaskRunResults: []v1beta1.TaskRunResult{{ Name: "commit", - Value: "SHA2", + Value: *v1beta1.NewArrayOrString("SHA2"), }}, }, }, @@ -3383,7 +3383,7 @@ func TestResolvedPipelineRunTask_IsFinalTask(t *testing.T) { TaskRunStatusFields: v1beta1.TaskRunStatusFields{ TaskRunResults: []v1beta1.TaskRunResult{{ Name: "commit", - Value: "SHA2", + Value: *v1beta1.NewArrayOrString("SHA2"), }}, }, }, diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go index aa2af67463e..8bc64639168 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go @@ -2456,10 +2456,10 @@ func TestPipelineRunState_GetResultsFuncs(t *testing.T) { TaskRunStatusFields: v1beta1.TaskRunStatusFields{ TaskRunResults: []v1beta1.TaskRunResult{{ Name: "foo", - Value: "oof", + Value: *v1beta1.NewArrayOrString("oof"), }, { Name: "bar", - Value: "rab", + Value: *v1beta1.NewArrayOrString("rab"), }}, }, }, @@ -2492,7 +2492,7 @@ func TestPipelineRunState_GetResultsFuncs(t *testing.T) { TaskRunStatusFields: v1beta1.TaskRunStatusFields{ TaskRunResults: []v1beta1.TaskRunResult{{ Name: "fail-foo", - Value: "fail-oof", + Value: *v1beta1.NewArrayOrString("fail-oof"), }}, }, }, @@ -2511,7 +2511,7 @@ func TestPipelineRunState_GetResultsFuncs(t *testing.T) { TaskRunStatusFields: v1beta1.TaskRunStatusFields{ TaskRunResults: []v1beta1.TaskRunResult{{ Name: "unknown-foo", - Value: "unknown-oof", + Value: *v1beta1.NewArrayOrString("unknown-oof"), }}, }, }, @@ -2608,10 +2608,10 @@ func TestPipelineRunState_GetResultsFuncs(t *testing.T) { expectedTaskResults := map[string][]v1beta1.TaskRunResult{ "successful-task-with-results-1": {{ Name: "foo", - Value: "oof", + Value: *v1beta1.NewArrayOrString("oof"), }, { Name: "bar", - Value: "rab", + Value: *v1beta1.NewArrayOrString("rab"), }}, "successful-task-without-results-1": nil, } diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go index 641355415b6..bd82529e488 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go @@ -138,11 +138,13 @@ func resolveResultRef(pipelineState PipelineRunState, resultRef *v1beta1.ResultR return nil, resultRef.PipelineTask, fmt.Errorf("task %q referenced by result was not successful", referencedPipelineTask.PipelineTask.Name) } - var runName, taskRunName, resultValue string + var runName, runValue, taskRunName string + var resultValue v1beta1.ArrayOrString var err error if referencedPipelineTask.IsCustomTask() { runName = referencedPipelineTask.Run.Name - resultValue, err = findRunResultForParam(referencedPipelineTask.Run, resultRef) + runValue, err = findRunResultForParam(referencedPipelineTask.Run, resultRef) + resultValue = *v1beta1.NewArrayOrString(runValue) if err != nil { return nil, resultRef.PipelineTask, err } @@ -155,7 +157,7 @@ func resolveResultRef(pipelineState PipelineRunState, resultRef *v1beta1.ResultR } return &ResolvedResultRef{ - Value: *v1beta1.NewArrayOrString(resultValue), + Value: resultValue, FromTaskRun: taskRunName, FromRun: runName, ResultReference: *resultRef, @@ -172,14 +174,14 @@ func findRunResultForParam(run *v1alpha1.Run, reference *v1beta1.ResultRef) (str return "", fmt.Errorf("Could not find result with name %s for task %s", reference.Result, reference.PipelineTask) } -func findTaskResultForParam(taskRun *v1beta1.TaskRun, reference *v1beta1.ResultRef) (string, error) { +func findTaskResultForParam(taskRun *v1beta1.TaskRun, reference *v1beta1.ResultRef) (v1beta1.ArrayOrString, error) { results := taskRun.Status.TaskRunStatusFields.TaskRunResults for _, result := range results { if result.Name == reference.Result { return result.Value, nil } } - return "", fmt.Errorf("Could not find result with name %s for task %s", reference.Result, reference.PipelineTask) + return v1beta1.ArrayOrString{}, fmt.Errorf("Could not find result with name %s for task %s", reference.Result, reference.PipelineTask) } func (rs ResolvedResultRefs) getStringReplacements() map[string]string { diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go index 810ab509cde..327585cfd7f 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go @@ -43,7 +43,7 @@ var pipelineRunState = PipelineRunState{{ TaskRunStatusFields: v1beta1.TaskRunStatusFields{ TaskRunResults: []v1beta1.TaskRunResult{{ Name: "aResult", - Value: "aResultValue", + Value: *v1beta1.NewArrayOrString("aResultValue"), }}, }, }, @@ -159,7 +159,7 @@ func TestTaskParamResolver_ResolveResultRefs(t *testing.T) { TaskRunStatusFields: v1beta1.TaskRunStatusFields{ TaskRunResults: []v1beta1.TaskRunResult{{ Name: "aResult", - Value: "aResultValue", + Value: *v1beta1.NewArrayOrString("aResultValue"), }}, }, }, @@ -195,7 +195,7 @@ func TestTaskParamResolver_ResolveResultRefs(t *testing.T) { TaskRunStatusFields: v1beta1.TaskRunStatusFields{ TaskRunResults: []v1beta1.TaskRunResult{{ Name: "aResult", - Value: "aResultValue", + Value: *v1beta1.NewArrayOrString("aResultValue"), }}, }, }, @@ -215,7 +215,7 @@ func TestTaskParamResolver_ResolveResultRefs(t *testing.T) { TaskRunStatusFields: v1beta1.TaskRunStatusFields{ TaskRunResults: []v1beta1.TaskRunResult{{ Name: "bResult", - Value: "bResultValue", + Value: *v1beta1.NewArrayOrString("bResultValue"), }}, }, }, @@ -258,7 +258,7 @@ func TestTaskParamResolver_ResolveResultRefs(t *testing.T) { TaskRunStatusFields: v1beta1.TaskRunStatusFields{ TaskRunResults: []v1beta1.TaskRunResult{{ Name: "aResult", - Value: "aResultValue", + Value: *v1beta1.NewArrayOrString("aResultValue"), }}, }, }, diff --git a/pkg/termination/parse_test.go b/pkg/termination/parse_test.go index dae81ac510c..e7aa7d8f9d2 100644 --- a/pkg/termination/parse_test.go +++ b/pkg/termination/parse_test.go @@ -39,6 +39,15 @@ func TestParseMessage(t *testing.T) { Key: "foo", Value: "bar", }}, + }, { + desc: "invalid key in message", + msg: `[{"invalid": "digest","value":"hereisthedigest"},{"key":"foo","value":"bar"}]`, + want: []v1beta1.PipelineResourceResult{{ + Value: "hereisthedigest", + }, { + Key: "foo", + Value: "bar", + }}, }, { desc: "empty message", msg: "", diff --git a/test/ignore_step_error_test.go b/test/ignore_step_error_test.go index 9229643284b..bc77fdcc8f0 100644 --- a/test/ignore_step_error_test.go +++ b/test/ignore_step_error_test.go @@ -100,7 +100,7 @@ spec: } for _, r := range taskrunItem.Status.TaskRunResults { - if r.Name == "result1" && r.Value != "123" { + if r.Name == "result1" && r.Value.StringVal != "123" { t.Fatalf("task1 should have initialized a result \"result1\" to \"123\"") } }