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.

Sign up for GitHub

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

Jump to bottom

Fix overwrite of package resources with empty resource map #123

Merged
merged 7 commits into from
Nov 6, 2024
+38 −22
Merged

Fix overwrite of package resources with empty resource map #123

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b75160d
Pushing a package with no changes in resources deletes all resources …
liamfallon Oct 13, 2024
a2c4950
Allow all mutations to cary on in the case of more than one
liamfallon Oct 14, 2024
4c36ecc
Remove references to DB
liamfallon Oct 14, 2024
c3e1170
Incvestigate why e2e failing on github
liamfallon Oct 16, 2024
f059489
Incvestigate why e2e failing on github
liamfallon Oct 16, 2024
f8383e5
Updated fix and test to check for empty resources
liamfallon Oct 21, 2024
23ee51c
Updated code to show how renderStatus is handled
liamfallon Nov 5, 2024
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: 17 additions & 15 deletions pkg/engine/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you using the renderStatus return value somewhere? if not, then I think marking the fact that it is ignored by naming it _ is the idiomatic way to go.

Copy link
Member Author

@liamfallon liamfallon Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renderStatus is returned by the function on lines 1043 and 1047.

Now, renderStatus was set by line 1032 before I introduced the if statement at line 1025. Looking at the applyResourceMutations function, when it is called at line 1020, it will return with a value of nil; the loop exits on the continue at line 1057 with renderStatus not set.

Another approach would be to use - in line 1021 as you sugges, but then I would have to add a line before line 1025 to declare renderStatus and set it to nil for the return statements.

I think the code here is quite confusing, especially the double call to applyResourceMutations but here, I was trying to make as few changes as possible. I think engine.go in its entirety should be among the first files to target for refactoring in Porch.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The renderStatus returned by this method will eventually be written back to the status of the PackageRevisionResources object returned by the Update() call. So far it represented the results of the render operation (a.k.a. executing the pipeline of the kpt package).
If the body of your if statement is skipped, no actual rendering is happening, so IMHO the intuitive way of handling this case would be to return with a nil renderStatus.

Now I think this is exactly what your code is doing, because the first applyResourceMutations call doesn't have a render mutation in its input (a render mutation is of type "eval"), so it will return with a nil renderStatus. So I think your code is correct.

However.... :) [sorry for the nitpicking]
IMHO it would be more readable if you would explicitly drop the renderStatus return value in the first applyResourceMutations call, by naming it _, as it was before.
And explicitly set the renderStatus to nil in the else branch of your new if statement. That would express the logic more directly: if there was a change do a render and return with its results, if there are no changes, no render is needed, and return with and empty render result.

I just want to reiterate that your code is correct as is, so you can safely choose to ignore any of the above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it, it's probably clearer all right, I'll update it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's changed in the latest update there. Thanks @kispaljr

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 {
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,
}})
}
// No lifecycle change when updating package resources; updates are done.
repoPkgRev, err := draft.Close(ctx)
if err != nil {
Expand Down
28 changes: 19 additions & 9 deletions test/e2e/e2e_test.go
View file
Open in desktop
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