Allow creating/updating draft package revisions with a failing pipeline

[kispaljr]

See related discussion here: nephio-project/nephio#876

Right now the creation of a new draft PackageRevision fails if the pipeline in the kpt package fails. Isn't only proposal/approval should be rejected in this case? Similarly how we allow to create draft packages with not-ready readiness gates, and only check readiness at proposal/approval time.

This makes it very hard to debug cases, when an algorithm tries to create a new package (like a PackageVariant, or any other higher level controller). This means that the content of the newly created package is calculated, it doesn't exists on the developers machine, so she cannot try to fix the package by locally running kpt fn render or similar.
On the other hand if we allow the creation of a half-ready draft, it would allow the developer to download the faulty package and try to troubleshoot the problem. I believe the purpose of draft revisions is to allow the preparation of the final version of a package anyway.

[nephio-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kispaljr

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [kispaljr]

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

[nagygergo]

Could you move the vendoring of the kpt function sdk libraries into a separate PR? I'm fine with it, just so it's not bundled with this one.

[Catalin-Stratulat-Ericsson]

• i agree also with what Istvan & Gergely are saying however i don't believe we should allow render to pass as you are describing UNTIL the render status is properly propagated through either in the packagerevision status section or somewhere easily ascertained by someone using porch. This was introduced as a temporary measure until that issue is handled,
• if we remove the blocker at draft phase right now "faulty" rendered packages can be pushed through and no mention is logged regarding its faults and this can mistakenly be pushed through later in the system and even if this block gets pushed to the approved phase the user will get a return saying their package has some issue but when they inspect no mention will be made as to what this issue is from my understanding.
• I believe the render status needs to be handled first then naturally this change can follow right behind it.

[Catalin-Stratulat-Ericsson]

1. after looking at the code a bit i see you are adding the render status in the kpt file itself, im not quite sure from first glance where you are storing this? in the conditions? or are you adding another section in the api for it.
2. i believe we discussed about this before that if were making api changes on kpt on porch then a kpt "render" and a porch "render" will no longer be interchangeable.
3. Is that something we are willing to live with? or am i misreading things somewhere?

[lapentad]

/retest

[liamfallon]

@kispaljr we have tweaked the Sonarcloud checks but i think you may need to rebase to pick up the tweaked config file.

[kispaljr]

Could you move the vendoring of the kpt function sdk libraries into a separate PR? I'm fine with it, just so it's not bundled with this one.

@nagygergo Good idea, I did that: #212 . Unfortunately until that PR is merged all the changes related to the vendoring are also showing up in this change. I will update the original description soon to point to the important parts of the change.

[sonarqubecloud]

Quality Gate failed

Failed conditions
35.4% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[kispaljr]

• i agree also with what Istvan & Gergely are saying however i don't believe we should allow render to pass as you are describing UNTIL the render status is properly propagated through either in the packagerevision status section or somewhere easily ascertained by someone using porch. This was introduced as a temporary measure until that issue is handled,

This PR adds a condition to the status of the Kptfile, that indicates the success or failure of the latest render.

Proposal and approval is rejected if this condition is false.

This condition is easily observed by any porch client either in the status of the PackageRevision object, or as part of the Kptfile.

[kispaljr]

if we remove the blocker at draft phase right now "faulty" rendered packages can be pushed through and no mention is logged regarding its faults and this can mistakenly be pushed through later in the system and even if this block gets pushed to the approved phase the user will get a return saying their package has some issue but when they inspect no mention will be made as to what this issue is from my understanding.

The error that user gets when propose/approve is rejected, states clearly that the pipeline execution has failed, and the Message of the Render condition will even contain the full render status in JSON format

[kispaljr]

I believe the render status needs to be handled first then naturally this change can follow right behind it.

This change only handles the binary result (failed or succeeded ) of the render operation. I believe this is enough for this particular use case.

Making the full RenderStatus always available is still a good functionality to add, but it can come independently from this.

[kispaljr]

after looking at the code a bit i see you are adding the render status in the kpt file itself, im not quite sure from first glance where you are storing this? in the conditions? or are you adding another section in the api for it.

Yes, I store a condition that stores the binary result of the last render. Since conditions are already a part of Kptfile, this doesn't require any API change

[kispaljr]

i believe we discussed about this before that if were making api changes on kpt on porch then a kpt "render" and a porch "render" will no longer be interchangeable.

Since conditions are already a part of Kptfile, this doesn't require any API change. That leaves kpt render and porch render as compatible as it is now.

[efiacor]

Hi @kispaljr ,
Is this PR still blocked or?

[kispaljr]

This code is so old that is not meaningful anymore.