Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix overwrite of package resources with empty resource map #123

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 19 additions & 13 deletions pkg/engine/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -1003,24 +1003,30 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj
resources := repository.PackageResources{
Contents: prevResources.Spec.Resources,
}

appliedResources, _, 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,
}})
var renderStatus *api.RenderStatus
if len(appliedResources.Contents) > 0 {
liamfallon marked this conversation as resolved.
Show resolved Hide resolved
// 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,
}})
} else {
renderStatus = nil
}

// No lifecycle change when updating package resources; updates are done.
repoPkgRev, err := draft.Close(ctx)
Expand Down
28 changes: 19 additions & 9 deletions test/e2e/e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading