Faster approvals for the auto-approve controller

[johnbelamaric]

There is now no longer a default delay; it only delays if you specifically give a delay annotation.

The minimum delay is now 0.

Re-ordered the checks so that the delay will not come into play unless all other checks are already met.

Added some tests.

[johnbelamaric]

@henderiw

For the packages that go through specialization (in free5gc-packages), there is currently no delay annotation defined.

I checked my e2e system, and it looks like the initial clone & function pipeline eval of the package revision that porch does will add the specialize condition to the readiness gates. This means we should be OK; we should not need any delay anymore, because with the Porch fixes to manage resourceVersion and the PV readiness check and the readinessGate check, approval should not kick in until it's all done.

I will test it out on Monday.

That said, I suspect there may still be some concurrency issues in Porch, so this may not work immediately. Worst case we add some (shorter) delay back in until we fix that.

[johnbelamaric]

Ok, so I had a few minutes to try this out and was curious.

Times (in seconds) from the last e2e that ran (with @electrocucaracha's latest changes), compared to times with this PR and the related changes to get rid of the delay annotations:

Script	Existing	Updated	Difference
001.sh	253	142	-111
002.sh	638	533	-105
003.sh	122	96	-26
004.sh	161	155	-6
005.sh	404	242	-162
006.sh	446	321	-125
007.sh	406	297	-109
008.sh	287	292	5
009.sh	25	15	-10

For a total difference of -649 seconds, or almost 11 minutes faster.

[electrocucaracha]

My understanding is that a delay will be applied only if it has the annotation, right? Regarding the rest of the code looks good, my suggestion is to use the constants for referring to the annotations.

[johnbelamaric]

My understanding is that a delay will be applied only if it has the annotation, right? Regarding the rest of the code looks good, my suggestion is to use the constants for referring to the annotations.

Thanks Victor. I thought about that as well, but decided to go with the explicit values. The reason is that the annotations are effectively an API for the approval controller. So, I wouldn't want those inadvertently changed. I would rather the tests fail if someone changes the annotation name constant values, because those reflect real failures that we would see in the field if we released the software.

And yes - the delay will only be applied now if the annotation exists.

[electrocucaracha]

/lgtm

[johnbelamaric]

/approve

[nephio-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric

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 [johnbelamaric]

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