porch: don't save empty patches (#3695)

porch/pkg/engine/engine.go

@@ -804,13 +804,7 @@ func convertStatusToKptfile(s api.ConditionStatus) kptfile.ConditionStatus {
 // conditionalAddRender adds a render mutation to the end of the mutations slice if the last
 // entry is not already a render mutation.
 func (cad *cadEngine) conditionalAddRender(subject client.Object, mutations []mutation) []mutation {
-	if len(mutations) == 0 {
-		return mutations
-	}
-
-	lastMutation := mutations[len(mutations)-1]
-	_, isRender := lastMutation.(*renderPackageMutation)
-	if isRender {
+	if len(mutations) == 0 || isRenderMutation(mutations[len(mutations)-1]) {
 		return mutations
 	}
 
@@ -822,6 +816,11 @@ func (cad *cadEngine) conditionalAddRender(subject client.Object, mutations []mu
 	})
 }
 
+func isRenderMutation(m mutation) bool {
+	_, isRender := m.(*renderPackageMutation)
+	return isRender
+}
+
 func (cad *cadEngine) DeletePackageRevision(ctx context.Context, repositoryObj *configapi.Repository, oldPackage *PackageRevision) error {
 	ctx, span := tracer.Start(ctx, "cadEngine::DeletePackageRevision", trace.WithAttributes())
 	defer span.End()
@@ -1018,19 +1017,31 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj
 
 // applyResourceMutations mutates the resources and returns the most recent renderResult.
 func applyResourceMutations(ctx context.Context, draft repository.PackageDraft, baseResources repository.PackageResources, mutations []mutation) (applied repository.PackageResources, renderStatus *api.RenderStatus, err error) {
+	var lastApplied mutation
 	for _, m := range mutations {
 		updatedResources, taskResult, err := m.Apply(ctx, baseResources)
+		if taskResult == nil && err == nil {
+			// a nil taskResult means nothing changed
+			continue
+		}
+
 		var task *api.Task
 		if taskResult != nil {
 			task = taskResult.Task
 		}
 		if taskResult != nil && task.Type == api.TaskTypeEval {
 			renderStatus = taskResult.RenderStatus
 		}
-
 		if err != nil {
 			return updatedResources, renderStatus, err
 		}
+
+		// if the last applied mutation was a render mutation, and so is this one, skip it
+		if lastApplied != nil && isRenderMutation(m) && isRenderMutation(lastApplied) {
+			continue
+		}
+		lastApplied = m
+
 		if err := draft.UpdateResources(ctx, &api.PackageRevisionResources{
 			Spec: api.PackageRevisionResourcesSpec{
 				Resources: updatedResources.Contents,
@@ -1246,7 +1257,9 @@ func (m *mutationReplaceResources) Apply(ctx context.Context, resources reposito
 			if err != nil {
 				return repository.PackageResources{}, nil, fmt.Errorf("error generating patch: %w", err)
 			}
-
+			if patchSpec.Contents == "" {
+				continue
+			}
 			patch.Patches = append(patch.Patches, patchSpec)
 		}
 	}
@@ -1260,12 +1273,17 @@ func (m *mutationReplaceResources) Apply(ctx context.Context, resources reposito
 			patch.Patches = append(patch.Patches, patchSpec)
 		}
 	}
-	task := &api.Task{
-		Type:  api.TaskTypePatch,
-		Patch: patch,
+	// If patch is empty, don't create a Task.
+	var taskResult *api.TaskResult
+	if len(patch.Patches) > 0 {
+		taskResult = &api.TaskResult{
+			Task: &api.Task{
+				Type:  api.TaskTypePatch,
+				Patch: patch,
+			},
+		}
 	}
-
-	return repository.PackageResources{Contents: new}, &api.TaskResult{Task: task}, nil
+	return repository.PackageResources{Contents: new}, taskResult, nil
 }
 
 func healConfig(old, new map[string]string) (map[string]string, error) {

porch/test/e2e/e2e_test.go

@@ -495,7 +495,7 @@ func (t *PorchSuite) TestUpdateResources(ctx context.Context) {
 	const (
 		repository  = "re-render-test"
 		packageName = "simple-package"
-		description = "description"
+		workspace   = "workspace"
 	)
 
 	t.registerMainGitRepositoryF(ctx, repository)
@@ -511,7 +511,7 @@ func (t *PorchSuite) TestUpdateResources(ctx context.Context) {
 		},
 		Spec: porchapi.PackageRevisionSpec{
 			PackageName:    packageName,
-			WorkspaceName:  description,
+			WorkspaceName:  workspace,
 			RepositoryName: repository,
 		},
 	}
@@ -564,6 +564,66 @@ func (t *PorchSuite) TestUpdateResources(ctx context.Context) {
 	}
 }
 
+// Test will initialize an empty package, and then make a call to update the resources
+// without actually making any changes. This test is ensuring that no additional
+// tasks get added.
+func (t *PorchSuite) TestUpdateResourcesEmptyPatch(ctx context.Context) {
+	const (
+		repository  = "empty-patch-test"
+		packageName = "simple-package"
+		workspace   = "workspace"
+	)
+
+	t.registerMainGitRepositoryF(ctx, repository)
+
+	// Create a new package (via init)
+	pr := &porchapi.PackageRevision{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "PackageRevision",
+			APIVersion: porchapi.SchemeGroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: t.namespace,
+		},
+		Spec: porchapi.PackageRevisionSpec{
+			PackageName:    packageName,
+			WorkspaceName:  workspace,
+			RepositoryName: repository,
+		},
+	}
+	t.CreateF(ctx, pr)
+
+	// Check its task list
+	var newPackage porchapi.PackageRevision
+	t.GetF(ctx, client.ObjectKey{
+		Namespace: t.namespace,
+		Name:      pr.Name,
+	}, &newPackage)
+	tasksBeforeUpdate := newPackage.Spec.Tasks
+	assert.Equal(t, 2, len(tasksBeforeUpdate))
+
+	// Get the package resources
+	var newPackageResources porchapi.PackageRevisionResources
+	t.GetF(ctx, client.ObjectKey{
+		Namespace: t.namespace,
+		Name:      pr.Name,
+	}, &newPackageResources)
+
+	// "Update" the package resources, without changing anything
+	t.UpdateF(ctx, &newPackageResources)
+
+	// Check the task list
+	var newPackageUpdated porchapi.PackageRevision
+	t.GetF(ctx, client.ObjectKey{
+		Namespace: t.namespace,
+		Name:      pr.Name,
+	}, &newPackageUpdated)
+	tasksAfterUpdate := newPackageUpdated.Spec.Tasks
+	assert.Equal(t, 2, len(tasksAfterUpdate))
+
+	assert.True(t, reflect.DeepEqual(tasksBeforeUpdate, tasksAfterUpdate))
+}
+
 func (t *PorchSuite) TestFunctionRepository(ctx context.Context) {
 	repo := &configapi.Repository{
 		ObjectMeta: metav1.ObjectMeta{
@@ -2080,7 +2140,7 @@ func (t *PorchSuite) registerMainGitRepositoryF(ctx context.Context, name string
 			Namespace: t.namespace,
 		},
 		Spec: configapi.RepositorySpec{
-			Description: "Porch Test Repository WorkspaceName",
+			Description: "Porch Test Repository Description",
 			Type:        configapi.RepositoryTypeGit,
 			Content:     configapi.RepositoryContentPackage,
 			Git: &configapi.GitRepository{