From b75160dc14774fcb6f419d37f50e9edc0a982362 Mon Sep 17 00:00:00 2001 From: liamfallon Date: Sun, 13 Oct 2024 21:05:15 +0100 Subject: [PATCH 1/7] Pushing a package with no changes in resources deletes all resources in package --- pkg/engine/engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 8ff622a5..f157a6df 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -1039,7 +1039,7 @@ func applyResourceMutations(ctx context.Context, draft repository.PackageDraft, updatedResources, taskResult, err := m.Apply(ctx, baseResources) if taskResult == nil && err == nil { // a nil taskResult means nothing changed - continue + return baseResources, nil, nil } var task *api.Task From a2c49501e1120324324f12faf1662b803c7bbbee Mon Sep 17 00:00:00 2001 From: liamfallon Date: Mon, 14 Oct 2024 07:46:37 -0700 Subject: [PATCH 2/7] Allow all mutations to cary on in the case of more than one --- pkg/engine/engine.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index f157a6df..a12f6034 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -1039,7 +1039,9 @@ func applyResourceMutations(ctx context.Context, draft repository.PackageDraft, updatedResources, taskResult, err := m.Apply(ctx, baseResources) if taskResult == nil && err == nil { // a nil taskResult means nothing changed - return baseResources, nil, nil + baseResources = updatedResources + applied = updatedResources + continue } var task *api.Task @@ -1092,7 +1094,7 @@ func (m *updatePackageMutation) Apply(ctx context.Context, resources repository. } targetUpstream := m.updateTask.Update.Upstream - if targetUpstream.Type == api.RepositoryTypeGit || targetUpstream.Type == api.RepositoryTypeOCI { + if targetUpstream.Type == api.RepositoryTypeGit || targetUpstream.Type == api.RepositoryTypeOCI || targetUpstream.Type == api.RepositoryTypeDB { return repository.PackageResources{}, nil, fmt.Errorf("update is not supported for non-porch upstream packages") } @@ -1157,7 +1159,7 @@ func (m *updatePackageMutation) currUpstream() (*api.PackageRevisionRef, error) return nil, fmt.Errorf("package %s does not have original upstream info", m.pkgName) } upstream := m.cloneTask.Clone.Upstream - if upstream.Type == api.RepositoryTypeGit || upstream.Type == api.RepositoryTypeOCI { + if upstream.Type == api.RepositoryTypeGit || upstream.Type == api.RepositoryTypeOCI || upstream.Type == api.RepositoryTypeDB { return nil, fmt.Errorf("upstream package must be porch native package. Found it to be %s", upstream.Type) } return upstream.UpstreamRef, nil From 4c36eccb46960d2d6acb8d2a75f05d864df102f9 Mon Sep 17 00:00:00 2001 From: liamfallon Date: Mon, 14 Oct 2024 08:07:36 -0700 Subject: [PATCH 3/7] Remove references to DB --- pkg/engine/engine.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index a12f6034..41510ba7 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -1094,7 +1094,7 @@ func (m *updatePackageMutation) Apply(ctx context.Context, resources repository. } targetUpstream := m.updateTask.Update.Upstream - if targetUpstream.Type == api.RepositoryTypeGit || targetUpstream.Type == api.RepositoryTypeOCI || targetUpstream.Type == api.RepositoryTypeDB { + if targetUpstream.Type == api.RepositoryTypeGit || targetUpstream.Type == api.RepositoryTypeOCI { return repository.PackageResources{}, nil, fmt.Errorf("update is not supported for non-porch upstream packages") } @@ -1159,7 +1159,7 @@ func (m *updatePackageMutation) currUpstream() (*api.PackageRevisionRef, error) return nil, fmt.Errorf("package %s does not have original upstream info", m.pkgName) } upstream := m.cloneTask.Clone.Upstream - if upstream.Type == api.RepositoryTypeGit || upstream.Type == api.RepositoryTypeOCI || upstream.Type == api.RepositoryTypeDB { + if upstream.Type == api.RepositoryTypeGit || upstream.Type == api.RepositoryTypeOCI { return nil, fmt.Errorf("upstream package must be porch native package. Found it to be %s", upstream.Type) } return upstream.UpstreamRef, nil From c3e1170e37345287981c008e0d8f40665c5cb1c8 Mon Sep 17 00:00:00 2001 From: liamfallon Date: Wed, 16 Oct 2024 11:05:54 -0700 Subject: [PATCH 4/7] Incvestigate why e2e failing on github --- pkg/engine/engine.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 41510ba7..7b659652 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -1039,8 +1039,8 @@ func applyResourceMutations(ctx context.Context, draft repository.PackageDraft, updatedResources, taskResult, err := m.Apply(ctx, baseResources) if taskResult == nil && err == nil { // a nil taskResult means nothing changed - baseResources = updatedResources - applied = updatedResources + // baseResources = updatedResources + // applied = updatedResources continue } From f05948903deeb28e965b38c794eeea02ff120519 Mon Sep 17 00:00:00 2001 From: liamfallon Date: Wed, 16 Oct 2024 11:22:46 -0700 Subject: [PATCH 5/7] Incvestigate why e2e failing on github --- pkg/engine/engine.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 7b659652..41510ba7 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -1039,8 +1039,8 @@ func applyResourceMutations(ctx context.Context, draft repository.PackageDraft, updatedResources, taskResult, err := m.Apply(ctx, baseResources) if taskResult == nil && err == nil { // a nil taskResult means nothing changed - // baseResources = updatedResources - // applied = updatedResources + baseResources = updatedResources + applied = updatedResources continue } From f8383e55f83d47c6987c507ee3a0606f8fcedbe4 Mon Sep 17 00:00:00 2001 From: liamfallon Date: Mon, 21 Oct 2024 17:50:43 +0100 Subject: [PATCH 6/7] Updated fix and test to check for empty resources --- pkg/engine/engine.go | 34 +++++++++++++++++----------------- test/e2e/e2e_test.go | 28 +++++++++++++++++++--------- 2 files changed, 36 insertions(+), 26 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 41510ba7..f0f74f9a 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -1003,25 +1003,27 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj resources := repository.PackageResources{ Contents: prevResources.Spec.Resources, } - appliedResources, _, err := applyResourceMutations(ctx, draft, resources, mutations) + + appliedResources, renderStatus, err := applyResourceMutations(ctx, draft, resources, mutations) if err != nil { return nil, nil, err } - // render the package - // Render failure will not 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, - draft, - appliedResources, - []mutation{&renderPackageMutation{ - runnerOptions: runnerOptions, - runtime: cad.runtime, - }}) - + if len(appliedResources.Contents) > 0 { + // render the package + // Render failure will not 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, + draft, + appliedResources, + []mutation{&renderPackageMutation{ + runnerOptions: runnerOptions, + runtime: cad.runtime, + }}) + } // No lifecycle change when updating package resources; updates are done. repoPkgRev, err := draft.Close(ctx) if err != nil { @@ -1039,8 +1041,6 @@ func applyResourceMutations(ctx context.Context, draft repository.PackageDraft, updatedResources, taskResult, err := m.Apply(ctx, baseResources) if taskResult == nil && err == nil { // a nil taskResult means nothing changed - baseResources = updatedResources - applied = updatedResources continue } diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 8cab7fc6..34f51ebf 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -814,34 +814,44 @@ func (t *PorchSuite) TestUpdateResourcesEmptyPatch(ctx context.Context) { t.CreateF(ctx, pr) // Check its task list - var newPackage porchapi.PackageRevision + var pkgBeforeUpdate porchapi.PackageRevision t.GetF(ctx, client.ObjectKey{ Namespace: t.Namespace, Name: pr.Name, - }, &newPackage) - tasksBeforeUpdate := newPackage.Spec.Tasks + }, &pkgBeforeUpdate) + tasksBeforeUpdate := pkgBeforeUpdate.Spec.Tasks assert.Equal(t, 2, len(tasksBeforeUpdate)) // Get the package resources - var newPackageResources porchapi.PackageRevisionResources + var resourcesBeforeUpdate porchapi.PackageRevisionResources t.GetF(ctx, client.ObjectKey{ Namespace: t.Namespace, Name: pr.Name, - }, &newPackageResources) + }, &resourcesBeforeUpdate) // "Update" the package resources, without changing anything - t.UpdateF(ctx, &newPackageResources) + t.UpdateF(ctx, &resourcesBeforeUpdate) // Check the task list - var newPackageUpdated porchapi.PackageRevision + var pkgAfterUpdate porchapi.PackageRevision t.GetF(ctx, client.ObjectKey{ Namespace: t.Namespace, Name: pr.Name, - }, &newPackageUpdated) - tasksAfterUpdate := newPackageUpdated.Spec.Tasks + }, &pkgAfterUpdate) + tasksAfterUpdate := pkgAfterUpdate.Spec.Tasks assert.Equal(t, 2, len(tasksAfterUpdate)) assert.True(t, reflect.DeepEqual(tasksBeforeUpdate, tasksAfterUpdate)) + + // Get the package resources + var resourcesAfterUpdate porchapi.PackageRevisionResources + t.GetF(ctx, client.ObjectKey{ + Namespace: t.Namespace, + Name: pr.Name, + }, &resourcesAfterUpdate) + + assert.Equal(t, 3, len(resourcesAfterUpdate.Spec.Resources)) + assert.True(t, reflect.DeepEqual(resourcesBeforeUpdate, resourcesAfterUpdate)) } func (t *PorchSuite) TestConcurrentResourceUpdates(ctx context.Context) { From 23ee51c8c4e8a33d76f65c27818e3bf12f93e495 Mon Sep 17 00:00:00 2001 From: liamfallon Date: Tue, 5 Nov 2024 14:48:22 +0000 Subject: [PATCH 7/7] Updated code to show how renderStatus is handled --- pkg/engine/engine.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index f0f74f9a..07bafc51 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -1004,11 +1004,12 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj Contents: prevResources.Spec.Resources, } - appliedResources, renderStatus, err := applyResourceMutations(ctx, draft, resources, mutations) + appliedResources, _, err := applyResourceMutations(ctx, draft, resources, mutations) if err != nil { return nil, nil, err } + var renderStatus *api.RenderStatus if len(appliedResources.Contents) > 0 { // render the package // Render failure will not fail the overall API operation. @@ -1023,7 +1024,10 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj runnerOptions: runnerOptions, runtime: cad.runtime, }}) + } else { + renderStatus = nil } + // No lifecycle change when updating package resources; updates are done. repoPkgRev, err := draft.Close(ctx) if err != nil {