Add user-defined compliance messages #280
Conversation
|
One thing potentially missing here is access to sprig functions like the other template fields provide. It might be helpful to export some of the information in https://github.com/stolostron/go-template-utils/blob/main/pkg/templates/sprig_wrapper.go for usage here. |
I don't think it's necessary but it'd be nice to have the same sprig template functions. |
JustinKuli
force-pushed
the
12423-custom-compliance-messages
branch
from
ed3c245 to
c251d00
Compare
| // Paranoid checks to ensure that the policy has a status of the right format | ||
| plcStatus, ok := plcMap["status"].(map[string]any) | ||
| if !ok { | ||
| goto messageTemplating |
There was a problem hiding this comment.
Oh goodness. What is this, BASIC programming??? 😆 I had no idea this existed in Go.
There was a problem hiding this comment.
It's not Go without goto 😆
I'm surprised @mprahl didn't comment on this, I think I've tried to sneak in a goto before... here it's just playing the role of a break statement for the if currentlyUsingWatch section. Maybe I set the whole thing up poorly, but when I try to re-write it without goto, it seems worse to me...
// Only add the full related object information when it can be pulled from the cache
if currentlyUsingWatch(plc) {
skip := false
// Paranoid checks to ensure that the policy has a status of the right format
plcStatus, ok := plcMap["status"].(map[string]any)
if !ok {
skip = true
}
var relObjs []any
if !skip {
relObjs, ok = plcStatus["relatedObjects"].([]any)
if !ok {
skip = true
}
}
if !skip {
for i, relObj := range plc.Status.RelatedObjects {
objNS := relObj.Object.Metadata.Namespace
objName := relObj.Object.Metadata.Name
objGVK := schema.FromAPIVersionAndKind(relObj.Object.APIVersion, relObj.Object.Kind)
fullObj, err := r.getObjectFromCache(plc, objNS, objName, objGVK)
if err == nil && fullObj != nil {
if _, ok := relObjs[i].(map[string]any); ok {
relObjs[i].(map[string]any)["object"] = fullObj.Object
}
}
}
}
}There was a problem hiding this comment.
@yiraeChristineKim , any is a recent keyword added to go, which just means interface{}, the empty interface. So any is "any" type. It's just shorter to write, which makes some of these type assertions nicer on unstructured things.
There was a problem hiding this comment.
These assertions really are just paranoia... since it's coming from a properly typed ConfigurationPolicy, I don't think it's possible they could ever fail. But, if they do fail unchecked, it's a panic, so that would be pretty bad.
There was a problem hiding this comment.
@JustinKuli I decided to let the "goto" slide even though I don't like them in general. It's prevalent in the Go standard library so I let it be an artistic decision. 😆
There was a problem hiding this comment.
I like using goto. Great first step @JustinKuli
KinD tests (latest) Flake?
|
JustinKuli
force-pushed
the
12423-custom-compliance-messages
branch
from
c251d00 to
9bc5756
Compare
| `[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`) | ||
|
|
||
| By("Verifying the ConfigurationPolicy becomes Compliant") | ||
| Eventually(func() interface{} { |
There was a problem hiding this comment.
Not important maybe string is better for consistency?
JustinKuli
force-pushed
the
12423-custom-compliance-messages
branch
from
9bc5756 to
8e3eab3
Compare
|
/lgtm /hold I like it! test cases are very detailed Thanks, Justin! I added hold for others |
| customMessage: | ||
| description: |- | ||
| CustomMessage configures the compliance messages emitted by the configuration policy, to use one | ||
| of the specified Go templates based on the current compliance. The data passed to the templates |
There was a problem hiding this comment.
the templates
Maybe is only to me...When I read this, I was confused with policy-template.
There was a problem hiding this comment.
It's not just you, policy templates, object templates, and now message templates get easily confused. I'm not sure what to do about it.
Policy authors can now define go templates to be used for compliance messages, with .DefaultMessage and .Policy fields available for getting useful information. If an error occurs with the template, those details will be appended to the default message. See the CRD description for more details. This also updates the default compliance message function to use a template, which provides an example of what is possible. The new tests also provide examples. Refs: - https://issues.redhat.com/browse/ACM-12423 Signed-off-by: Justin Kulikauskas <[email protected]>
JustinKuli
force-pushed
the
12423-custom-compliance-messages
branch
from
8e3eab3 to
089c603
Compare
KinD tests (minimum) Another (different) flake?
|
|
@mprahl @dhaiducek any more comments for this PR? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, mprahl, 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 8979505
into
open-cluster-management-io:main
Policy authors can now define go templates to be used for compliance messages, with .DefaultMessage and .Policy fields available for getting useful information. If an error occurs with the template, those details will be appended to the default message. See the CRD description for more details.
This also updates the default compliance message function to use a template, which provides an example of what is possible. The new tests also provide examples.
Refs: