Consolidate compliance messages #323
Conversation
yiraeChristineKim
force-pushed
the
ACM-15773
branch
2 times, most recently
from
89ba6b1 to
9bfe0f2
Compare
|
/cc @JustinKuli |
| for j, nsName := range objectNameStrsToNsNames[namesStr] { | ||
| if j != 0 { | ||
| compliancyDetailsMsg = setCompliancyDetailsMsgEnd(compliancyDetailsMsg) | ||
| } |
There was a problem hiding this comment.
If j == 0, then the current message would be empty, right? So then setCompliancyDetailsMsgEnd would not add a semicolon?
I think checking the index and the message in setCompliancyDetailsMsgEnd is redundant. I'd prefer to just use the index, and add the semicolon directly:
| for j, nsName := range objectNameStrsToNsNames[namesStr] { | |
| if j != 0 { | |
| compliancyDetailsMsg = setCompliancyDetailsMsgEnd(compliancyDetailsMsg) | |
| } | |
| for j, nsName := range objectNameStrsToNsNames[namesStr] { | |
| if j != 0 { | |
| compliancyDetailsMsg += "; " | |
| } |
| for _, msgTemplate := range msgTemplates { | ||
| names := msgMap[msgTemplate] | ||
| slices.Sort(names) // Sort names for consistent output | ||
|
|
||
| compliancyDetailsMsg = setCompliancyDetailsMsgEnd(compliancyDetailsMsg) | ||
|
|
||
| joinedNames := strings.Join(names, ", ") | ||
| if joinedNames != "" { | ||
| joinedNames = fmt.Sprintf(" [%s]", joinedNames) | ||
| } | ||
|
|
||
| compliancyDetailsMsg += fmt.Sprintf(msgTemplate, resourceName, joinedNames) | ||
| } |
There was a problem hiding this comment.
Assuming the index approach works, I think that would be more clear here as well:
| for _, msgTemplate := range msgTemplates { | |
| names := msgMap[msgTemplate] | |
| slices.Sort(names) // Sort names for consistent output | |
| compliancyDetailsMsg = setCompliancyDetailsMsgEnd(compliancyDetailsMsg) | |
| joinedNames := strings.Join(names, ", ") | |
| if joinedNames != "" { | |
| joinedNames = fmt.Sprintf(" [%s]", joinedNames) | |
| } | |
| compliancyDetailsMsg += fmt.Sprintf(msgTemplate, resourceName, joinedNames) | |
| } | |
| for j, msgTemplate := range msgTemplates { | |
| names := msgMap[msgTemplate] | |
| slices.Sort(names) // Sort names for consistent output | |
| if j != 0 { | |
| compliancyDetailsMsg += "; " | |
| } | |
| joinedNames := strings.Join(names, ", ") | |
| if joinedNames != "" { | |
| joinedNames = fmt.Sprintf(" [%s]", joinedNames) | |
| } | |
| compliancyDetailsMsg += fmt.Sprintf(msgTemplate, resourceName, joinedNames) | |
| } |
|
|
||
| // This prevents repeating the same reason for each unique object name list. | ||
| // Process the object name strings in order to ensure a deterministic reason and message. |
There was a problem hiding this comment.
Nit: since this function returns a map (and lookups from maps in go can be in an inconsistent order), it is the getCombinedCompliancyDetailsMsg and createStatus functions which ensures they are in a deterministic order.
Also, I just want to thank you for bringing this into a separate function! It really helps readability of createStatus 🥇
There was a problem hiding this comment.
You are right, how about move "// Process the object name strings in order to ensure a deterministic reason and message." above "getCombinedCompliancyDetailsMsg" as function comment
policy.yaml
Outdated
| # - complianceType: musthave | ||
| # namespaceSelector: | ||
| # matchExpressions: | ||
| # - key: yikim | ||
| # operator: Exists | ||
| # objectSelector: | ||
| # matchExpressions: | ||
| # - key: yikim | ||
| # operator: Exists | ||
| # objectDefinition: | ||
| # apiVersion: config-policy-controller.io/v1 | ||
| # kind: FakeAPI | ||
| # metadata: | ||
| # labels: | ||
| # selected: "true" | ||
| # namespace: case42-e2e-2 |
There was a problem hiding this comment.
Are these commented bits necessary?
There was a problem hiding this comment.
OH my god this should be deleted
| "configmaps [rex, woody] found but not as specified in namespace toy-story1; " + | ||
| "configmaps [buzz, potato] found but not as specified in namespace toy-story2", | ||
| }, |
There was a problem hiding this comment.
I love when the GH review begins with an easy-to-read test! This looks great
Ref: https://issues.redhat.com/browse/ACM-15773 Signed-off-by: yiraeChristineKim <[email protected]>
yiraeChristineKim
force-pushed
the
ACM-15773
branch
from
9bfe0f2 to
4ea4275
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, yiraeChristineKim 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 |
openshift-merge-bot
bot
merged commit facb1a8
into
open-cluster-management-io:main
Currently the compliance message is listed out for each object from the object selector. Ideally these messages would be consolidated for better consumption and readability.
Definition of Done for Engineering Story Owner (Checklist)
Status messages from the objectSelector are consolidated rather than listed out individually for each name.
Only combine messages for the same object-template (when it uses a resource selector, there can be multiple objects)
example of change:
configmaps [rex, woody] found but not as specified in namespace toy-story1; configmaps [buzz, potato] found but not as specified in namespace toy-story2Ref: https://issues.redhat.com/browse/ACM-15773