From 33b52b3e52caa8587ef124e817cfa1aef2997a14 Mon Sep 17 00:00:00 2001
From: Catalin Stratulat <catalin.stratulat@ericsson.com>
Date: Tue, 26 Nov 2024 14:01:58 +0000
Subject: [PATCH] enforcing api error if pipeline render fails instead of
 ignoring error and allowing it to pass

---
 pkg/engine/engine.go                           |  4 +++-
 pkg/registry/porch/packagerevisionresources.go |  1 +
 pkg/task/generictaskhandler.go                 | 17 ++++++++++++-----
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go
index 6893e579..2ccc3fc4 100644
--- a/pkg/engine/engine.go
+++ b/pkg/engine/engine.go
@@ -504,7 +504,9 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj
 	}
 
 	renderStatus, err := cad.taskHandler.DoPRResourceMutations(ctx, pr2Update, draft, oldRes, newRes)
-
+	if err != nil {
+		return nil, renderStatus, err
+	}
 	// No lifecycle change when updating package resources; updates are done.
 	repoPkgRev, err := draft.Close(ctx, "")
 	if err != nil {
diff --git a/pkg/registry/porch/packagerevisionresources.go b/pkg/registry/porch/packagerevisionresources.go
index 10388484..0aa15826 100644
--- a/pkg/registry/porch/packagerevisionresources.go
+++ b/pkg/registry/porch/packagerevisionresources.go
@@ -183,6 +183,7 @@ func (r *packageRevisionResources) Update(ctx context.Context, name string, objI
 
 	rev, renderStatus, err := r.cad.UpdatePackageResources(ctx, &repositoryObj, oldRepoPkgRev, oldApiPkgRevResources, newObj)
 	if err != nil {
+		err := fmt.Errorf("%w%s%s%s", err, "\n\nKpt function pipeline error occurred during render.", "\nPackage has NOT been pushed to remote.", "\nAmend local package using error message above & retry.")
 		return nil, false, apierrors.NewInternalError(err)
 	}
 
diff --git a/pkg/task/generictaskhandler.go b/pkg/task/generictaskhandler.go
index e5ba8e51..9bfa37fb 100644
--- a/pkg/task/generictaskhandler.go
+++ b/pkg/task/generictaskhandler.go
@@ -225,19 +225,26 @@ func (th *genericTaskHandler) DoPRResourceMutations(ctx context.Context, pr2Upda
 
 	var renderStatus *api.RenderStatus
 	if len(appliedResources.Contents) > 0 {
+		// a porch package which fails render validation will no longer be accepted.
 		// render the package
-		// Render failure will not fail the overall API operation.
+		// Render failure WILL fail the overall API operation.
 		// The render error and result is captured as part of renderStatus above
-		// and is returned in packageresourceresources API's status field. We continue with
-		// saving the non-rendered resources to avoid losing user's changes.
-		// and supress this err.
-		_, renderStatus, _ = applyResourceMutations(ctx,
+		// and is returned in PackageRevisionResources API's status field.
+		// We do not push the package further to remote
+		// the users changes are captured on their local
+		// and can be amended using the error returned to properly validate/mutate the package before retrying
+		// we no longer suppress this err.
+		_, renderStatus, err = applyResourceMutations(ctx,
 			draft,
 			appliedResources,
 			[]mutation{&renderPackageMutation{
 				runnerOptions: runnerOptions,
 				runtime:       th.runtime,
 			}})
+		if err != nil {
+			klog.Errorf("Kpt function pipeline error occurred in render. Package has NOT been pushed to remote. Modify and Fix local package and retry.")
+			return renderStatus, err
+		}
 	} else {
 		renderStatus = nil
 	}