Fix overwrite of package resources with empty resource map

[liamfallon]

When changes are pushed to a package that are identical to the existing package, the updatedResources method returns the resources. However the 'if' block that starts at line 1053 does not set applied to the value of the resources, so when the loop terminates after the continue statement, the function returns with applied having a value of an empty string map. This value is set in the draft package revision, resulting in all the existing resources in the package draft being erased.

[nephio-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liamfallon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [liamfallon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[liamfallon]

/assign @efiacor @kispaljr @nagygergo

[liamfallon]

/retest

[liamfallon]

/retest

[nagygergo]

Great catch, could you write a testcase for this make sure this doesn't regress?

[liamfallon]

Great catch, could you write a testcase for this make sure this doesn't regress?

will do

[liamfallon]

/retest

[liamfallon]

Great catch, could you write a testcase for this make sure this doesn't regress?

There is an existing test but that test did not check if the before and after resources were the same. I have added this check to the test.

[liamfallon]

In the latest commit, I changed the solution for the problem.

In fact, the old implementation worked for the git repo implementation because the existing git repo implementation looks for a 'kptfile' in the resources and when it doesn't find one, it returns an error. The resource mutations are called by the ApplyResourceMutations() function at line 1030.

While it's OK to return appliedResources as null, it is not OK to pass a null resources to the repo implementation to store.

Therefore, the resource saving code call at line 1030 is surrounded by an if block that checks that there are resources to save.

[kispaljr]

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.

[liamfallon]

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.

[kispaljr]

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.

[liamfallon]

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

[liamfallon]

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

[kispaljr]

/lgtm