Allow informonly configuration policies #75
Conversation
| return | ||
| } | ||
| // or when a policy is informonly | ||
| if tObjectUnstructured.GroupVersionKind().Group != utils.GConstraint && |
There was a problem hiding this comment.
Nit: Right now this conditional block looks like:
if (!A && !B) {
...
} else if A {
...
} else if B {
...
}Especially if the blocks for A and B (gatekeeper things) could be sure to return at the end, it could be rewritten like:
if A {
...
return
} else if B {
...
return
}
// logic for !A && !BWhich would save an indent - especially nice here because things are getting pretty nested with other checks.
| if spec, ok := tObjectUnstructured.Object["spec"]; ok { | ||
| specObject, ok := spec.(map[string]interface{}) | ||
| if ok { | ||
| if remediationAction, ok := specObject["remediationAction"]; ok { | ||
| _, ok := remediationAction.(string) | ||
| if ok { |
There was a problem hiding this comment.
All of the logic looks right. But it's not as easy to read as I would prefer. My personal preference is to reduce indentation when possible.
Since this is a lot of checks, one trick would be to consider what should be done when a check fails: in this case, it should just return without changing any settings. So the first two checks could be re-written like this:
spec, ok := tObjectUnstructured.Object["spec"]
if !ok {
return // template didn't have a spec
}
specObject, ok := spec.(map[string]interface{})
if !ok {
return // template's spec wasn't a map
}
...Maybe in general this function should return an error (those comments indicate what might go into the error message), but it hasn't historically, so I don't think there's a need to add that now.
There was a problem hiding this comment.
Alternately, you could save lines by using the if ___; ok { thing every time, at least to be consistent.
| if spec, ok := tObjectUnstructured.Object["spec"]; ok { | |
| specObject, ok := spec.(map[string]interface{}) | |
| if ok { | |
| if remediationAction, ok := specObject["remediationAction"]; ok { | |
| _, ok := remediationAction.(string) | |
| if ok { | |
| if spec, ok := tObjectUnstructured.Object["spec"]; ok { | |
| if specObject, ok := spec.(map[string]interface{}); ok { | |
| if remediationAction, ok := specObject["remediationAction"]; ok { | |
| if remAction, ok := remediationAction.(string); ok { |
There was a problem hiding this comment.
With regards to this, I was trying to be consistent in the way the assertions were already being made in the code. Should I change all the comma-ok assertions in this function so that they are each evaluated in one line?
There was a problem hiding this comment.
Ah, now I see that the existing case had the one-liner for the lookup, but had it separated for the type assertion. So this was consistently following the inconsistency 😅
Honestly, I'll accept any form you're happy with (as long as it's not much crazier than what is already in the PR). As I said, the logic looks right. De-tangling the test cases was my main reason for requesting changes on the PR, otherwise I would've approved it with these as optional suggestions.
| apiVersion: policy.open-cluster-management.io/v1 | ||
| kind: ConfigurationPolicy | ||
| metadata: | ||
| name: create-configmap |
There was a problem hiding this comment.
With my other suggestion around cleanup, I think the two ConfigurationPolicies would need distinct names.
Signed-off-by: Jason Zhang <jaszhang@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, zyjjay 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-robot
merged commit c3f2764
into
open-cluster-management-io:main
Overrides the parent policy's remediationAction when 'informonly' is provided as the remediationAction for ConfigurationPolicy