Aggregate Pending status from replicated policies #64
Conversation
| policiesv1 "open-cluster-management.io/governance-policy-propagator/api/v1" | ||
| "open-cluster-management.io/governance-policy-propagator/controllers/common" | ||
| ) | ||
|
|
There was a problem hiding this comment.
I extracted calculatePerClusterStatus as a separate function with the intention of unit testing it, but I couldn't get the fakeclient to work with the List call that this makes. Using envtest would probably work, but would take more setup than I wanted to do here.
JustinKuli
force-pushed
the
aggregate-pending-status
branch
from
e0f452f to
863a3f5
Compare
When a replicated policy is in the Pending state, and all others for that root policy are Compliant, then the root policy should be Pending. This change extracts some other aggregation logic in a way that should be more testable, and adds unit tests for some previous existing behavior. Refs: - stolostron/backlog#26182 Signed-off-by: Justin Kulikauskas <[email protected]>
JustinKuli
force-pushed
the
aggregate-pending-status
branch
from
863a3f5 to
9018d65
Compare
| return nil, err | ||
| } | ||
|
|
||
| status := make([]*policiesv1.CompliancePerClusterStatus, 0, len(replicatedPlcList.Items)+len(failedClusters)) |
| }) | ||
| } | ||
|
|
||
| sort.Slice(status, func(i, j int) bool { |
There was a problem hiding this comment.
Is there any need to do sorting here? I know it would be looks nice but the overhead seems unnecessary for the usage of calculatePerClusterStatus.
There was a problem hiding this comment.
I think it's necessary because the list is copied into the status after this. If the order was inconsistent, the status might be updated when there aren't really any changes.
There was a problem hiding this comment.
I think it's necessary because the list is copied into the status after this. If the order was inconsistent, the status might be updated when there aren't really any changes.
Got it.
| } | ||
|
|
||
| // calculateRootCompliance uses the input per-cluster statuses to determine what a root policy's | ||
| // ComplianceState should be. General precedence is: NonCompliant > Pending > Unknown > Compliant. |
There was a problem hiding this comment.
The logic in calculateRootCompliance looks good to me.
| case2PolicyName string = "case2-test-policy" | ||
| case2PolicyYaml string = "../resources/case2_aggregation/case2-test-policy.yaml" | ||
| ) | ||
|
|
||
| var _ = Describe("Test policy status aggregation", func() { |
There was a problem hiding this comment.
Have you tested the unknown case here
There was a problem hiding this comment.
Not here, but it is tested in the unit-style test TestCalculateRootCompliance
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ChunxiAlexLuo, JustinKuli 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 |
When a replicated policy is in the Pending state, and all others for that root policy are Compliant, then the root policy should be Pending.
This change extracts some other aggregation logic in a way that should be more testable, and adds unit tests for some previous existing behavior.
Refs:
Signed-off-by: Justin Kulikauskas [email protected]