diff --git a/porch/pkg/engine/engine.go b/porch/pkg/engine/engine.go index 51b5a8aaa5..46ec7f94da 100644 --- a/porch/pkg/engine/engine.go +++ b/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,8 +1017,14 @@ 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 @@ -1027,10 +1032,16 @@ func applyResourceMutations(ctx context.Context, draft repository.PackageDraft, 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) { diff --git a/porch/test/e2e/e2e_test.go b/porch/test/e2e/e2e_test.go index 2ef5924e57..67979f4c09 100644 --- a/porch/test/e2e/e2e_test.go +++ b/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{