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

Merged
merged 7 commits into from
Nov 6, 2024

Conversation

liamfallon
Copy link
Member

@liamfallon liamfallon commented Oct 14, 2024

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.

Copy link
Contributor

nephio-prow bot commented Oct 14, 2024

[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:

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

@nephio-prow nephio-prow bot added the approved label Oct 14, 2024
@liamfallon liamfallon added the bug Something isn't working label Oct 14, 2024
@liamfallon
Copy link
Member Author

/assign @efiacor @kispaljr @nagygergo

@liamfallon
Copy link
Member Author

/retest

1 similar comment
@liamfallon
Copy link
Member Author

/retest

@nagygergo
Copy link
Contributor

nagygergo commented Oct 15, 2024

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

@liamfallon
Copy link
Member Author

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

will do

@liamfallon
Copy link
Member Author

/retest

@liamfallon
Copy link
Member Author

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
Copy link
Member Author

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.

@@ -1016,25 +1016,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

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

@kispaljr
Copy link
Collaborator

kispaljr commented Nov 6, 2024

/lgtm

@nephio-prow nephio-prow bot added the lgtm label Nov 6, 2024
@nephio-prow nephio-prow bot merged commit 0c863bc into nephio-project:main Nov 6, 2024
5 checks passed
@liamfallon liamfallon deleted the noop-pull-zap branch November 27, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved bug Something isn't working lgtm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants