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

Render status cleanup through "PorchFile" implementation #108

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Catalin-Stratulat-Ericsson
Copy link
Contributor

@Catalin-Stratulat-Ericsson Catalin-Stratulat-Ericsson commented Sep 10, 2024

This PR is an amalgamation of 3 separate issues, these being
nephio-project/nephio#699,
nephio-project/nephio#603,
nephio-project/nephio#783
The first 2 mainly address the render status of a porch package, at the moment the render status is not propagating down to the PackageRevisionResources and always returns an exit code 0 status even when a rendering issue was encountered (#603).
An Improvement in the returned render status data as per (#603 & #699). It achieves this propagating metadata such as the latest render status to a "PorchFile" suggested by @kispaljr in (#783).
This solution does "work" in the sense that it is able to accomplish what I've mentioned above however there are 2 main problems and as the discussion in (#783) is still ongoing i wish to open this up to suggestions/opinions.

Copy link
Contributor

nephio-prow bot commented Sep 10, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Catalin-Stratulat-Ericsson
Once this PR has been reviewed and has the lgtm label, please assign efiacor for approval by writing /assign @efiacor in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@Catalin-Stratulat-Ericsson
Copy link
Contributor Author

1st Problem (Not Efficient):
The current implementation requires a second patch after the render to inject the porch file(containing said render status). this is obviously highly inefficient as every single package would now need to on update run a patch, render, and now another patch(containing the porch file). Any suggestion on how to improve this would be greatly welcomed.

@Catalin-Stratulat-Ericsson
Copy link
Contributor Author

2nd Problem (Failing Tests):
The second issue relates to the first in a way and it relates to the tests. the TestUpdateResources & TestUpdateResourcesEmptyPatch in particular, ive gone through these tests and the second call of the patch seems to cause conflicts on this line https://github.com/Nordix/porch/blob/330b9d3ce0b5a7fa84e26b64a0c17fa764a37848/pkg/engine/engine.go#L1082.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant