Repository: GoogleContainerTools/kpt
Pull request #3645 (2022-10-31 19:22:44)

cli: rpkg push surface renderstatus
This PR adds support for displaying render status as part of of `kpt alpha rpkg push` command.

Here is an example output in case of render failure:

```sh
$ ~/work/bin/kpt alpha rpkg push application-blueprints-33f255 /tmp/new-pkg -n default
Package is updated, but failed to render the package.
Error: fn.render: pkg /:
        pkg.render:
        pipeline.run: error: function failure
[RUNNING] "gcr.io/kpt-fn/set-namespace:v0.4.1"
[PASS] "gcr.io/kpt-fn/set-namespace:v0.4.1"
  Results:
    [info]: all namespaces are already "example". no value changed
[RUNNING] "gcr.io/kpt-fn/starlark:v0.4.3"
[FAIL] "gcr.io/kpt-fn/starlark:v0.4.3"
  Results:
    [error]: fail: Validation always fails

```

Here is an example output in case of successful run:

```sh
$ ~/work/bin/kpt alpha rpkg push application-blueprints-33f255a /tmp/new-pkg -n default
[RUNNING] "gcr.io/kpt-fn/set-namespace:v0.4.1"
[PASS] "gcr.io/kpt-fn/set-namespace:v0.4.1"
  Results:
    [info]: all namespaces are already "example". no value changed
```

xref: https://github.com/GoogleContainerTools/kpt/issues/3635

droot: /cc @ChristopherFry `ExitCode` field behavior in each of the result item is now fixed. It will be non-zero for functions returning error (the behavior was wrong for builtin functions).

Patch:
diff --git a/commands/alpha/rpkg/push/command.go b/commands/alpha/rpkg/push/command.go
index c7be0abe..6d514a70 100644
--- a/commands/alpha/rpkg/push/command.go
+++ b/commands/alpha/rpkg/push/command.go
@@ -23,9 +23,11 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"strings"
 
 	"github.com/GoogleContainerTools/kpt/internal/docs/generated/rpkgdocs"
 	"github.com/GoogleContainerTools/kpt/internal/errors"
+	"github.com/GoogleContainerTools/kpt/internal/fnruntime"
 	"github.com/GoogleContainerTools/kpt/internal/printer"
 	"github.com/GoogleContainerTools/kpt/internal/util/porch"
 	porchapi "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
@@ -117,7 +119,7 @@ func (r *runner) runE(cmd *cobra.Command, args []string) error {
 		return errors.E(op, err)
 	}
 
-	if err := r.client.Update(r.ctx, &porchapi.PackageRevisionResources{
+	pkgResources := porchapi.PackageRevisionResources{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "PackageRevisionResources",
 			APIVersion: porchapi.SchemeGroupVersion.Identifier(),
@@ -129,12 +131,86 @@ func (r *runner) runE(cmd *cobra.Command, args []string) error {
 		Spec: porchapi.PackageRevisionResourcesSpec{
 			Resources: resources,
 		},
-	}); err != nil {
+	}
+	if err := r.client.Update(r.ctx, &pkgResources); err != nil {
 		return errors.E(op, err)
 	}
+	rs := pkgResources.Status.RenderStatus
+	if rs.Err != "" {
+		r.printer.Printf("Package is updated, but failed to render the package.\n")
+		r.printer.Printf("Error: %s\n", rs.Err)
+	}
+	if len(rs.Result.Items) > 0 {
+		for _, result := range rs.Result.Items {
+			r.printer.Printf("[RUNNING] %q \n", result.Image)
+			printOpt := printer.NewOpt()
+			if result.ExitCode != 0 {
+				r.printer.OptPrintf(printOpt, "[FAIL] %q\n", result.Image)
+			} else {
+				r.printer.OptPrintf(printOpt, "[PASS] %q\n", result.Image)
+			}
+			r.printFnResult(result, printOpt)
+		}
+	}
 	return nil
 }
 
+// printFnResult prints given function result in a user friendly
+// format on kpt CLI.
+func (r *runner) printFnResult(fnResult *porchapi.Result, opt *printer.Options) {
+	if len(fnResult.Results) > 0 {
+		// function returned structured results
+		var lines []string
+		for _, item := range fnResult.Results {
+			lines = append(lines, str(item))
+		}
+		ri := &fnruntime.MultiLineFormatter{
+			Title:          "Results",
+			Lines:          lines,
+			TruncateOutput: printer.TruncateOutput,
+		}
+		r.printer.OptPrintf(opt, "%s", ri.String())
+	}
+}
+
+// String provides a human-readable message for the result item
+func str(i porchapi.ResultItem) string {
+	identifier := i.ResourceRef
+	var idStringList []string
+	if identifier != nil {
+		if identifier.APIVersion != "" {
+			idStringList = append(idStringList, identifier.APIVersion)
+		}
+		if identifier.Kind != "" {
+			idStringList = append(idStringList, identifier.Kind)
+		}
+		if identifier.Namespace != "" {
+			idStringList = append(idStringList, identifier.Namespace)
+		}
+		if identifier.Name != "" {
+			idStringList = append(idStringList, identifier.Name)
+		}
+	}
+	formatString := "[%s]"
+	severity := i.Severity
+	// We default Severity to Info when converting a result to a message.
+	if i.Severity == "" {
+		severity = "info"
+	}
+	list := []interface{}{severity}
+	if len(idStringList) > 0 {
+		formatString += " %s"
+		list = append(list, strings.Join(idStringList, "/"))
+	}
+	if i.Field != nil {
+		formatString += " %s"
+		list = append(list, i.Field.Path)
+	}
+	formatString += ": %s"
+	list = append(list, i.Message)
+	return fmt.Sprintf(formatString, list...)
+}
+
 func readFromDir(dir string) (map[string]string, error) {
 	resources := map[string]string{}
 	if err := filepath.Walk(dir, func(path string, info fs.FileInfo, err error) error {
diff --git a/go.mod b/go.mod
index c5e57afa..dfbea9e8 100644
--- a/go.mod
+++ b/go.mod
@@ -3,7 +3,7 @@ module github.com/GoogleContainerTools/kpt
 go 1.18
 
 require (
-	github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20221027171429-622c0141f3d7
+	github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20221028161857-aa271f292cc0
 	github.com/bytecodealliance/wasmtime-go v0.39.0
 	github.com/cpuguy83/go-md2man/v2 v2.0.2
 	github.com/go-errors/errors v1.4.2
diff --git a/go.sum b/go.sum
index c3c07a1a..a509372d 100644
--- a/go.sum
+++ b/go.sum
@@ -87,6 +87,8 @@ github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym
 github.com/Djarvur/go-err113 v0.0.0-20210108212216-aea10b59be24/go.mod h1:4UJr5HIiMZrwgkSPdsjy2uOQExX/WEILpIrO9UPGuXs=
 github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20221027171429-622c0141f3d7 h1:WKfyM9TDvHaGJ0n98rO/U7qkPL8wAWPvLKQYNT7TNvY=
 github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20221027171429-622c0141f3d7/go.mod h1:ASrhnLAL4ahTuiUJyepqcpVRXIoRMJyDs8/eSxwhgZM=
+github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20221028161857-aa271f292cc0 h1:Nay/s1StXHUKyIxerpXb8o0hZUkRjrbteLO6ardI26Y=
+github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20221028161857-aa271f292cc0/go.mod h1:ASrhnLAL4ahTuiUJyepqcpVRXIoRMJyDs8/eSxwhgZM=
 github.com/MakeNowJust/heredoc v0.0.0-20170808103936-bb23615498cd h1:sjQovDkwrZp8u+gxLtPgKGjk5hCxuy2hrRejBTA9xFU=
 github.com/MakeNowJust/heredoc v0.0.0-20170808103936-bb23615498cd/go.mod h1:64YHyfSL2R96J44Nlwm39UHepQbyR5q10x7iYa1ks2E=
 github.com/Masterminds/goutils v1.1.0/go.mod h1:8cTjp+g8YejhMuvIA5y2vz3BpJxksy863GQaJW2MFNU=
diff --git a/internal/fnruntime/fnerrors.go b/internal/fnruntime/fnerrors.go
index 988c6d36..3cf6a341 100644
--- a/internal/fnruntime/fnerrors.go
+++ b/internal/fnruntime/fnerrors.go
@@ -45,7 +45,7 @@ type ExecError struct {
 func (fe *ExecError) String() string {
 	var b strings.Builder
 
-	errLines := &multiLineFormatter{
+	errLines := &MultiLineFormatter{
 		Title:          "Stderr",
 		Lines:          strings.Split(fe.Stderr, "\n"),
 		UseQuote:       true,
diff --git a/internal/fnruntime/runner.go b/internal/fnruntime/runner.go
index a3a9490a..a83db137 100644
--- a/internal/fnruntime/runner.go
+++ b/internal/fnruntime/runner.go
@@ -295,10 +295,15 @@ func (fr *FunctionRunner) do(input []*yaml.RNode) (output []*yaml.RNode, err err
 	}
 	if err != nil {
 		var execErr *ExecError
+		// set exitCode to non-zero by default in case of an error.
+		// It will be overridden with appropriate exitCode if the function runtime returns execError.
+		// builtinruntime and podEvaluator function runtime do not return execError so having
+		// a default is important.
+		fnResult.ExitCode = 1
+		fr.fnResults.ExitCode = 1
 		if goerrors.As(err, &execErr) {
 			fnResult.ExitCode = execErr.ExitCode
 			fnResult.Stderr = execErr.Stderr
-			fr.fnResults.ExitCode = 1
 		}
 		// accumulate the results
 		fr.fnResults.Items = append(fr.fnResults.Items, *fnResult)
@@ -427,7 +432,7 @@ func printFnResult(ctx context.Context, fnResult *fnresult.Result, opt *printer.
 		for _, item := range fnResult.Results {
 			lines = append(lines, item.String())
 		}
-		ri := &multiLineFormatter{
+		ri := &MultiLineFormatter{
 			Title:          "Results",
 			Lines:          lines,
 			TruncateOutput: printer.TruncateOutput,
@@ -448,7 +453,7 @@ func printFnExecErr(ctx context.Context, fnErr *ExecError) {
 func printFnStderr(ctx context.Context, stdErr string) {
 	pr := printer.FromContextOrDie(ctx)
 	if len(stdErr) > 0 {
-		errLines := &multiLineFormatter{
+		errLines := &MultiLineFormatter{
 			Title:          "Stderr",
 			Lines:          strings.Split(stdErr, "\n"),
 			UseQuote:       true,
@@ -492,9 +497,9 @@ func enforcePathInvariants(nodes []*yaml.RNode) error {
 	return nil
 }
 
-// multiLineFormatter knows how to format multiple lines in pretty format
+// MultiLineFormatter knows how to format multiple lines in pretty format
 // that can be displayed to an end user.
-type multiLineFormatter struct {
+type MultiLineFormatter struct {
 	// Title under which lines need to be printed
 	Title string
 
@@ -512,7 +517,7 @@ type multiLineFormatter struct {
 }
 
 // String returns multiline string.
-func (ri *multiLineFormatter) String() string {
+func (ri *MultiLineFormatter) String() string {
 	if ri.MaxLines == 0 {
 		ri.MaxLines = FnExecErrorTruncateLines
 	}
diff --git a/porch/api/generated/clientset/versioned/fake/register.go b/porch/api/generated/clientset/versioned/fake/register.go
index 2b1d2df7..36b9da3c 100644
--- a/porch/api/generated/clientset/versioned/fake/register.go
+++ b/porch/api/generated/clientset/versioned/fake/register.go
@@ -35,14 +35,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{
 // AddToScheme adds all types of this clientset into the given scheme. This allows composition
 // of clientsets, like in:
 //
-//	import (
-//	  "k8s.io/client-go/kubernetes"
-//	  clientsetscheme "k8s.io/client-go/kubernetes/scheme"
-//	  aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme"
-//	)
+//   import (
+//     "k8s.io/client-go/kubernetes"
+//     clientsetscheme "k8s.io/client-go/kubernetes/scheme"
+//     aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme"
+//   )
 //
-//	kclientset, _ := kubernetes.NewForConfig(c)
-//	_ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme)
+//   kclientset, _ := kubernetes.NewForConfig(c)
+//   _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme)
 //
 // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types
 // correctly.
diff --git a/porch/api/generated/clientset/versioned/scheme/register.go b/porch/api/generated/clientset/versioned/scheme/register.go
index 90fc1da2..cee00d25 100644
--- a/porch/api/generated/clientset/versioned/scheme/register.go
+++ b/porch/api/generated/clientset/versioned/scheme/register.go
@@ -35,14 +35,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{
 // AddToScheme adds all types of this clientset into the given scheme. This allows composition
 // of clientsets, like in:
 //
-//	import (
-//	  "k8s.io/client-go/kubernetes"
-//	  clientsetscheme "k8s.io/client-go/kubernetes/scheme"
-//	  aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme"
-//	)
+//   import (
+//     "k8s.io/client-go/kubernetes"
+//     clientsetscheme "k8s.io/client-go/kubernetes/scheme"
+//     aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme"
+//   )
 //
-//	kclientset, _ := kubernetes.NewForConfig(c)
-//	_ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme)
+//   kclientset, _ := kubernetes.NewForConfig(c)
+//   _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme)
 //
 // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types
 // correctly.
diff --git a/porch/api/porch/types_packagerevisions.go b/porch/api/porch/types_packagerevisions.go
index a5eab8ac..3c65483d 100644
--- a/porch/api/porch/types_packagerevisions.go
+++ b/porch/api/porch/types_packagerevisions.go
@@ -347,27 +347,6 @@ const (
 	ConditionUnknown ConditionStatus = "Unknown"
 )
 
-// Result contains the structured result from an individual function
-type Result struct {
-	// Image is the full name of the image that generates this result
-	// Image and Exec are mutually exclusive
-	Image string `json:"image,omitempty"`
-	// ExecPath is the the absolute os-specific path to the executable file
-	// If user provides an executable file with commands, ExecPath should
-	// contain the entire input string.
-	ExecPath string `json:"exec,omitempty"`
-	// TODO(droot): This is required for making structured results subpackage aware.
-	// Enable this once test harness supports filepath based assertions.
-	// Pkg is OS specific Absolute path to the package.
-	// Pkg string `yaml:"pkg,omitempty"`
-	// Stderr is the content in function stderr
-	Stderr string `json:"stderr,omitempty"`
-	// ExitCode is the exit code from running the function
-	ExitCode int `json:"exitCode"`
-	// Results is the list of results for the function
-	Results []ResultItem `json:"results,omitempty"`
-}
-
 const (
 	// Deprecated: prefer ResultListGVK
 	ResultListKind = "FunctionResultList"
@@ -380,7 +359,6 @@ const (
 )
 
 // ResultList contains aggregated results from multiple functions
-// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 type ResultList struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
@@ -390,6 +368,27 @@ type ResultList struct {
 	Items []*Result `json:"items,omitempty"`
 }
 
+// Result contains the structured result from an individual function
+type Result struct {
+	// Image is the full name of the image that generates this result
+	// Image and Exec are mutually exclusive
+	Image string `json:"image,omitempty"`
+	// ExecPath is the the absolute os-specific path to the executable file
+	// If user provides an executable file with commands, ExecPath should
+	// contain the entire input string.
+	ExecPath string `json:"exec,omitempty"`
+	// TODO(droot): This is required for making structured results subpackage aware.
+	// Enable this once test harness supports filepath based assertions.
+	// Pkg is OS specific Absolute path to the package.
+	// Pkg string `yaml:"pkg,omitempty"`
+	// Stderr is the content in function stderr
+	Stderr string `json:"stderr,omitempty"`
+	// ExitCode is the exit code from running the function
+	ExitCode int `json:"exitCode"`
+	// Results is the list of results for the function
+	Results []ResultItem `json:"results,omitempty"`
+}
+
 // ResultItem defines a validation result
 type ResultItem struct {
 	// Message is a human readable message. This field is required.
diff --git a/porch/api/porch/v1alpha1/types_packagerevisions.go b/porch/api/porch/v1alpha1/types_packagerevisions.go
index 4a7b3593..1f018d71 100644
--- a/porch/api/porch/v1alpha1/types_packagerevisions.go
+++ b/porch/api/porch/v1alpha1/types_packagerevisions.go
@@ -358,27 +358,6 @@ const (
 	ConditionUnknown ConditionStatus = "Unknown"
 )
 
-// Result contains the structured result from an individual function
-type Result struct {
-	// Image is the full name of the image that generates this result
-	// Image and Exec are mutually exclusive
-	Image string `json:"image,omitempty"`
-	// ExecPath is the the absolute os-specific path to the executable file
-	// If user provides an executable file with commands, ExecPath should
-	// contain the entire input string.
-	ExecPath string `json:"exec,omitempty"`
-	// TODO(droot): This is required for making structured results subpackage aware.
-	// Enable this once test harness supports filepath based assertions.
-	// Pkg is OS specific Absolute path to the package.
-	// Pkg string `yaml:"pkg,omitempty"`
-	// Stderr is the content in function stderr
-	Stderr string `json:"stderr,omitempty"`
-	// ExitCode is the exit code from running the function
-	ExitCode int `json:"exitCode"`
-	// Results is the list of results for the function
-	Results []ResultItem `json:"results,omitempty"`
-}
-
 const (
 	// Deprecated: prefer ResultListGVK
 	ResultListKind = "FunctionResultList"
@@ -391,7 +370,6 @@ const (
 )
 
 // ResultList contains aggregated results from multiple functions
-// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 type ResultList struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
@@ -401,6 +379,27 @@ type ResultList struct {
 	Items []*Result `json:"items,omitempty"`
 }
 
+// Result contains the structured result from an individual function
+type Result struct {
+	// Image is the full name of the image that generates this result
+	// Image and Exec are mutually exclusive
+	Image string `json:"image,omitempty"`
+	// ExecPath is the the absolute os-specific path to the executable file
+	// If user provides an executable file with commands, ExecPath should
+	// contain the entire input string.
+	ExecPath string `json:"exec,omitempty"`
+	// TODO(droot): This is required for making structured results subpackage aware.
+	// Enable this once test harness supports filepath based assertions.
+	// Pkg is OS specific Absolute path to the package.
+	// Pkg string `yaml:"pkg,omitempty"`
+	// Stderr is the content in function stderr
+	Stderr string `json:"stderr,omitempty"`
+	// ExitCode is the exit code from running the function
+	ExitCode int `json:"exitCode"`
+	// Results is the list of results for the function
+	Results []ResultItem `json:"results,omitempty"`
+}
+
 // ResultItem defines a validation result
 type ResultItem struct {
 	// Message is a human readable message. This field is required.
diff --git a/porch/api/porch/v1alpha1/zz_generated.deepcopy.go b/porch/api/porch/v1alpha1/zz_generated.deepcopy.go
index a2ad63a8..c8eecf26 100644
--- a/porch/api/porch/v1alpha1/zz_generated.deepcopy.go
+++ b/porch/api/porch/v1alpha1/zz_generated.deepcopy.go
@@ -932,14 +932,6 @@ func (in *ResultList) DeepCopy() *ResultList {
 	return out
 }
 
-// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object.
-func (in *ResultList) DeepCopyObject() runtime.Object {
-	if c := in.DeepCopy(); c != nil {
-		return c
-	}
-	return nil
-}
-
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *SecretRef) DeepCopyInto(out *SecretRef) {
 	*out = *in
diff --git a/porch/api/porch/zz_generated.deepcopy.go b/porch/api/porch/zz_generated.deepcopy.go
index f8967902..9d41fa35 100644
--- a/porch/api/porch/zz_generated.deepcopy.go
+++ b/porch/api/porch/zz_generated.deepcopy.go
@@ -932,14 +932,6 @@ func (in *ResultList) DeepCopy() *ResultList {
 	return out
 }
 
-// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object.
-func (in *ResultList) DeepCopyObject() runtime.Object {
-	if c := in.DeepCopy(); c != nil {
-		return c
-	}
-	return nil
-}
-
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *SecretRef) DeepCopyInto(out *SecretRef) {
 	*out = *in
diff --git a/porch/go.mod b/porch/go.mod
index 8867470b..a59d2a62 100644
--- a/porch/go.mod
+++ b/porch/go.mod
@@ -17,7 +17,7 @@ require (
 	github.com/GoogleContainerTools/kpt-functions-catalog/functions/go/set-namespace v0.4.1
 	github.com/GoogleContainerTools/kpt-functions-catalog/functions/go/starlark v0.4.3
 	github.com/GoogleContainerTools/kpt-functions-sdk/go/fn v0.0.0-20220506190241-f85503febd54
-	github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20221027171429-622c0141f3d7
+	github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20221028161857-aa271f292cc0
 	github.com/bluekeyes/go-gitdiff v0.6.1
 	github.com/go-git/go-billy/v5 v5.3.1
 	github.com/go-git/go-git/v5 v5.4.3-0.20220408232334-4f916225cb2f


Test patch:
diff --git a/internal/fnruntime/runner_test.go b/internal/fnruntime/runner_test.go
index 05bad6d6..8f8099cc 100644
--- a/internal/fnruntime/runner_test.go
+++ b/internal/fnruntime/runner_test.go
@@ -101,13 +101,13 @@ data: {foo: bar}
 
 func TestMultilineFormatter(t *testing.T) {
 	type testcase struct {
-		ml       *multiLineFormatter
+		ml       *MultiLineFormatter
 		expected string
 	}
 
 	testcases := map[string]testcase{
 		"multiline should format lines and truncate": {
-			ml: &multiLineFormatter{
+			ml: &MultiLineFormatter{
 				Title: "Results",
 				Lines: []string{
 					"line-1",
@@ -127,7 +127,7 @@ func TestMultilineFormatter(t *testing.T) {
 `,
 		},
 		"multiline should format without truncate": {
-			ml: &multiLineFormatter{
+			ml: &MultiLineFormatter{
 				Title: "Results",
 				Lines: []string{
 					"line-1",